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
_______________________________________________
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
