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

[Moni Shoua]

Michael S. Tsirkin wrote:
> 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?
> 
> 
I still see a difference between removing all devices and unloading a module.
In the first case, the function ipoib_neigh_destructor is still accessible but not in the second.
I can't always set the neighbour destructor to NULL 
because it is shared among other neighbours. I can do it only when I don't want the 
destructor to be called ever (which is the case of module unloading)


>> @@ -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?
I think you're right. Maybe I need to attack the issue in a different way.
I need to do some rethinking.

> 
>> +			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();