[ofa-general] Re: [openib-general] [RFC] [PATCH v2] IB/ipoib: Add bonding support to IPoIB

[Michael S. Tsirkin]

Some more issues:

> This version of the patch tracks the allocs and releases of ipoib_neigh and
>  keeps a list of them.  Before IPoIB net device unregisters the list is passed
> to destroy ipoib_neighs that ride on on a bond neighbour.
> 
> This is a replacement to the method of scanning the arp and ndisc
> tables.
> 
> 
> Index: linux-2.6/drivers/infiniband/ulp/ipoib/ipoib.h
> ===================================================================
> --- linux-2.6.orig/drivers/infiniband/ulp/ipoib/ipoib.h	2007-03-04 12:20:54.749932751 +0200
> +++ linux-2.6/drivers/infiniband/ulp/ipoib/ipoib.h	2007-03-04 12:21:58.547593677 +0200
> @@ -218,6 +218,7 @@ struct ipoib_neigh {
>  	struct neighbour   *neighbour;
>  	struct net_device *dev;
>  
> +	struct list_head    all_neigh_list;
>  	struct list_head    list;
>  };
>  
> Index: linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_main.c
> ===================================================================
> --- linux-2.6.orig/drivers/infiniband/ulp/ipoib/ipoib_main.c	2007-03-04 12:21:52.720629356 +0200
> +++ linux-2.6/drivers/infiniband/ulp/ipoib/ipoib_main.c	2007-03-04 12:21:58.548593499 +0200
> @@ -66,6 +66,7 @@ MODULE_PARM_DESC(recv_queue_size, "Numbe
>  #ifdef CONFIG_INFINIBAND_IPOIB_DEBUG
>  int ipoib_debug_level;
>  
> +static int ipoib_at_exit = 0;
>  module_param_named(debug_level, ipoib_debug_level, int, 0644);
>  MODULE_PARM_DESC(debug_level, "Enable debug tracing if > 0");
>  #endif

This at_exit trick looks ugly. Ideally, hotplug removing all devices and module
removal should act identically. The fact that they do not is suspicious.
Consider hotplug removing all devices. It seems no code will test
ipoib_at_exit then. Is that right?


> @@ -85,6 +86,9 @@ struct workqueue_struct *ipoib_workqueue
>  
>  struct ib_sa_client ipoib_sa_client;
>  
> +static DEFINE_SPINLOCK(ipoib_all_neigh_list_lock);
> +static LIST_HEAD(ipoib_all_neigh_list);
> +
>  static void ipoib_add_one(struct ib_device *device);
>  static void ipoib_remove_one(struct ib_device *device);
>  
> @@ -792,6 +796,24 @@ static void ipoib_neigh_destructor(struc
>  		ipoib_put_ah(ah);
>  }
>  
> +static void ipoib_neigh_cleanup_bond(struct net_device* master,
> +				     struct net_device* slave)
> +{
> +	struct ipoib_neigh *nn, *tn;
> +
> +	spin_lock(&ipoib_all_neigh_list_lock);
> +	list_for_each_entry_safe(nn, tn, &ipoib_all_neigh_list, all_neigh_list){
> +		if ((nn->neighbour->dev == master) && (nn->dev == slave)) {

Extra ()'s not really needed here: logic ops have lower precedence than math
(IIRC, only comma and assignments have lower precedence than logic).

> +			if (ipoib_at_exit)
> +				nn->neighbour->parms->neigh_destructor = NULL;

Is it safe to do this without locking?
Could the destructor be in progress when we do this?

> +			spin_unlock(&ipoib_all_neigh_list_lock);
> +			ipoib_neigh_destructor(nn->neighbour);
> +			spin_lock(&ipoib_all_neigh_list_lock);
> +		}
> +	}
> +	spin_unlock(&ipoib_all_neigh_list_lock);
> +}
> +
>  struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neighbour,
>  				      struct net_device *dev)
>  {
> @@ -806,6 +828,9 @@ struct ipoib_neigh *ipoib_neigh_alloc(st
>  	*to_ipoib_neigh(neighbour) = neigh;
>  	skb_queue_head_init(&neigh->queue);
>  
> +	spin_lock(&ipoib_all_neigh_list_lock);
> +	list_add_tail(&neigh->all_neigh_list, &ipoib_all_neigh_list);
> +	spin_unlock(&ipoib_all_neigh_list_lock);
>  	return neigh;
>  }
>  
> @@ -818,6 +843,9 @@ void ipoib_neigh_free(struct net_device 
>  		++priv->stats.tx_dropped;
>  		dev_kfree_skb_any(skb);
>  	}
> +	spin_lock(&ipoib_all_neigh_list_lock);
> +	list_del(&neigh->all_neigh_list);
> +	spin_unlock(&ipoib_all_neigh_list_lock);
>  	kfree(neigh);
>  }
>  
> @@ -874,6 +902,8 @@ void ipoib_dev_cleanup(struct net_device
>  
>  	/* Delete any child interfaces first */
>  	list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list) {
> +		if (cpriv->dev->master)
> +			ipoib_neigh_cleanup_bond(cpriv->dev->master,priv->dev);

whitespace broken here.

>  		unregister_netdev(cpriv->dev);
>  		ipoib_dev_cleanup(cpriv->dev);
>  		free_netdev(cpriv->dev);
> @@ -1158,6 +1188,8 @@ static void ipoib_remove_one(struct ib_d
>  	list_for_each_entry_safe(priv, tmp, dev_list, list) {
>  		ib_unregister_event_handler(&priv->event_handler);
>  		flush_scheduled_work();
> +		if (priv->dev->master)
> +			ipoib_neigh_cleanup_bond(priv->dev->master,priv->dev);

and here.

>  
>  		unregister_netdev(priv->dev);
>  		ipoib_dev_cleanup(priv->dev);
> @@ -1217,6 +1249,7 @@ err_fs:
>  
>  static void __exit ipoib_cleanup_module(void)
>  {
> +	ipoib_at_exit = 1;
>  	ib_unregister_client(&ipoib_client);
>  	ib_sa_unregister_client(&ipoib_sa_client);
>  	ipoib_unregister_debugfs();
-- 
MST