<br><font size=2 face="sans-serif">Hi Micheal,</font>
<br>
<br><font size=2 face="sans-serif">Is there a reason to have the atomic
set_bit() within a lock (even for </font>
<br><font size=2 face="sans-serif">a race condition of stop vs send, it
doesn't seem to be required) ?</font>
<br><font size=2 face="sans-serif">Which means the test_bit() can also
be put before the existing lock...</font>
<br>
<br><font size=2 face="sans-serif">Thanks,</font>
<br>
<br><font size=2 face="sans-serif">- KK</font>
<br>
<br><font size=2><tt>openib-general-bounces@openib.org wrote on 12/09/2005
12:04:06 AM:<br>
<br>
> Quoting Michael S. Tsirkin <mst@mellanox.co.il>:<br>
> > Subject: [PATCH] ipoib_multicast/ipoib_mcast_send race<br>
> > <br>
> > Hello, Roland!<br>
> > Here's another race scenario.<br>
> > <br>
> > ---<br>
> > <br>
> > Fix the following race scenario:<br>
> > device is up.<br>
> > port event or set mcast list triggers ipoib_mcast_stop_thread,
<br>
> > This cancels the query and waits on mcast "done" completion.<br>
> > completion is called and "done" is set.<br>
> > Meanwhile, ipoib_mcast_send arrives and starts a new query,<br>
> > re-initializing "done".<br>
> > <br>
> > Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il><br>
> <br>
> The patch I posted previously leaked an skb when a multicast<br>
> send arrived while the mcast thread is stopped.<br>
> <br>
> Further, there's an additional issue that I saw in testing:<br>
> ipoib_mcast_send may get called when priv->broadcast is NULL<br>
> (e.g. if the device was downed and then upped internally because<br>
> of a port event).<br>
> If this happends and the sendonly join request gets completed before<br>
> priv->broadcast is set, we get an oops that I posted previously.<br>
> <br>
> Here's a better patch to address these two problems.<br>
> It has been running fine here for a while now.<br>
> <br>
> Please note that this replaces the ipoib_multicast/ipoib_mcast_send
patch,<br>
> but not the ADMIN_UP patch that I posted previously.<br>
> <br>
> ---<br>
> <br>
> Do not send multicasts if mcast thread is stopped or if<br>
> priv->broadcast is not set.<br>
> <br>
> Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il><br>
> <br>
> Index: openib/drivers/infiniband/ulp/ipoib/ipoib_multicast.c<br>
> ===================================================================<br>
> --- openib/drivers/infiniband/ulp/ipoib/ipoib_multicast.c   (revision
4222)<br>
> +++ openib/drivers/infiniband/ulp/ipoib/ipoib_multicast.c   (working
copy)<br>
> @@ -582,6 +582,10 @@ int ipoib_mcast_start_thread(struct net_<br>
>        queue_work(ipoib_workqueue, &priv->mcast_task);<br>
>     up(&mcast_mutex);<br>
>  <br>
> +   spin_lock_irq(&priv->lock);<br>
> +   set_bit(IPOIB_MCAST_STARTED, &priv->flags);<br>
> +   spin_unlock_irq(&priv->lock);<br>
> +<br>
>     return 0;<br>
>  }<br>
>  <br>
> @@ -592,6 +596,10 @@ int ipoib_mcast_stop_thread(struct net_d<br>
>  <br>
>     ipoib_dbg_mcast(priv, "stopping multicast thread\n");<br>
>  <br>
> +   spin_lock_irq(&priv->lock);<br>
> +   clear_bit(IPOIB_MCAST_STARTED, &priv->flags);<br>
> +   spin_unlock_irq(&priv->lock);<br>
> +<br>
>     down(&mcast_mutex);<br>
>     clear_bit(IPOIB_MCAST_RUN, &priv->flags);<br>
>     cancel_delayed_work(&priv->mcast_task);<br>
> @@ -674,6 +682,11 @@ void ipoib_mcast_send(struct net_device <br>
>      */<br>
>     spin_lock(&priv->lock);<br>
>  <br>
> +   if (!test_bit(IPOIB_MCAST_STARTED, &priv->flags) ||
!priv->broadcast) {<br>
> +      dev_kfree_skb_any(skb);<br>
> +      goto unlock;<br>
> +   }<br>
> +<br>
>     mcast = __ipoib_mcast_find(dev, mgid);<br>
>     if (!mcast) {<br>
>        /* Let's create a new send only group now
*/<br>
> @@ -732,6 +745,7 @@ out:<br>
>        ipoib_send(dev, skb, mcast->ah, IB_MULTICAST_QPN);<br>
>     }<br>
>  <br>
> +unlock:<br>
>     spin_unlock(&priv->lock);<br>
>  }<br>
>  <br>
> Index: openib/drivers/infiniband/ulp/ipoib/ipoib.h<br>
> ===================================================================<br>
> --- openib/drivers/infiniband/ulp/ipoib/ipoib.h   (revision 4222)<br>
> +++ openib/drivers/infiniband/ulp/ipoib/ipoib.h   (working copy)<br>
> @@ -78,6 +78,7 @@ enum {<br>
>     IPOIB_FLAG_SUBINTERFACE   = 4,<br>
>     IPOIB_MCAST_RUN      = 5,<br>
>     IPOIB_STOP_REAPER         = 6,<br>
> +   IPOIB_MCAST_STARTED       = 7,<br>
>  <br>
>     IPOIB_MAX_BACKOFF_SECONDS = 16,<br>
>  <br>
> <br>
> -- <br>
> MST<br>
> _______________________________________________<br>
> openib-general mailing list<br>
> openib-general@openib.org<br>
> http://openib.org/mailman/listinfo/openib-general<br>
> <br>
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general<br>
</tt></font>