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();
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
