[ofa-general] IPoIB kernel Oops -- race condition

[Yossi Etigin]

Jack Morgenstein wrote:
> Is the patch below what you mean?  I think this will work.
> (It is compiled, but not yet tested).
> I've opened Bugzilla 1666 on this issue.

Yes, this is what exactly I've meant.
However please see some remarks below.

> --- is4_kernel.orig/drivers/infiniband/ulp/ipoib/ipoib.h	2009-06-30 09:44:48.434443000 +0300
> +++ is4_kernel/drivers/infiniband/ulp/ipoib/ipoib.h	2009-06-30 09:44:49.004011000 +0300
> @@ -288,10 +288,12 @@ struct ipoib_dev_priv {
>  
>  	struct ipoib_mcast *broadcast;
>  	struct list_head multicast_list;
> +	struct list_head mcast_remove_list;
>  	struct rb_root multicast_tree;
>  
>  	struct delayed_work pkey_poll_task;
>  	struct delayed_work mcast_task;
> +	struct work_struct mcast_remove_task;
>  	struct work_struct carrier_on_task;
>  	struct work_struct flush_light;
>  	struct work_struct flush_normal;

What do you think about renaming the mcast_task to mcast_join_task 
and multicast_list to mcast_join_list? It will make the purpose and
the analogy between the two more obvious.

>  static void __exit ipoib_cleanup_module(void)
>  {
>  	ib_unregister_client(&ipoib_client);
> +	flush_workqueue(ipoib_workqueue);
>  	ib_sa_unregister_client(&ipoib_sa_client);
>  	ipoib_unregister_debugfs();
>  	destroy_workqueue(ipoib_workqueue);

This is already done in ipoib_remove_one().

> --- is4_kernel.orig/drivers/infiniband/ulp/ipoib/ipoib_multicast.c	2009-06-30 09:44:48.955963000 +0300
> +++ is4_kernel/drivers/infiniband/ulp/ipoib/ipoib_multicast.c	2009-06-30 09:45:44.990941000 +0300
> @@ -657,6 +657,31 @@ static int ipoib_mcast_leave(struct net_
>  	return 0;
>  }
>  
> +void ipoib_mcast_remove_task(struct work_struct *work)
> +{
> +	struct ipoib_dev_priv *priv =
> +		container_of(work, struct ipoib_dev_priv, mcast_remove_task);
> +	struct net_device *dev = priv->dev;
> +
> +	LIST_HEAD(remove_list);
> +	struct ipoib_mcast *mcast, *tmcast;
> +	unsigned long flags;
> +
> +	ipoib_dbg_mcast(priv, "flushing multicast list\n");
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	list_for_each_entry_safe(mcast, tmcast, &priv->mcast_remove_list, list) {
> +		list_del(&mcast->list);
> +		list_add_tail(&mcast->list, &remove_list);
> +	}
> +	spin_unlock_irqrestore(&priv->lock, flags);

I think you could use list_splice() here.

> @@ -864,11 +884,7 @@ void ipoib_mcast_restart_task(struct wor
>  	netif_addr_unlock(dev);
>  	local_irq_restore(flags);
>  
> -	/* We have to cancel outside of the spinlock */
> -	list_for_each_entry_safe(mcast, tmcast, &remove_list, list) {
> -		ipoib_mcast_leave(mcast->dev, mcast);
> -		ipoib_mcast_free(mcast);
> -	}
> +	queue_work(ipoib_workqueue, &priv->mcast_remove_task);
>  
>  	if (test_bit(IPOIB_FLAG_ADMIN_UP, &priv->flags))
>  		ipoib_mcast_start_thread(dev);

Modifying ipoib_mcast_restart_task() is not really needed since it runs on the WQ
anyway. On the other hand, for compatibility (?) with other changes it may be a good
thing to do.

--Yossi