<br><font size=2 face="sans-serif">Hi Micheal,</font>
<br>
<br><font size=2 face="sans-serif">But the lock doesn't help that case.
The only difference with having the</font>
<br><font size=2 face="sans-serif">lock is that in case of a race, the
mcast_send() will complete *before*</font>
<br><font size=2 face="sans-serif">the flag is set, while without the lock
the mcast_send() could be in the</font>
<br><font size=2 face="sans-serif">*middle* of execution when the flag
is set. But in both cases, the packet</font>
<br><font size=2 face="sans-serif">would be sent out. In case of reverse
race (that means the stop_thread</font>
<br><font size=2 face="sans-serif">executes before the mcast_send(), in
both cases, the packet would not</font>
<br><font size=2 face="sans-serif">be sent out, hence the lock is not helping
both cases.</font>
<br>
<br><font size=2 face="sans-serif">I feel the new code looks fine without
the 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>"Michael S. Tsirkin" <mst@mellanox.co.il>
wrote on 12/09/2005 06:41:51 PM:<br>
<br>
> The lock around clear_bit is there to ensure that ipoib_mcast_send
isnt running<br>
> already when we stop the thread.<br>
> Thats why test_bit has to be instide the lock, too.<br>
> <br>
> <br>
> Quoting r. Krishna Kumar2 <krkumar2@in.ibm.com>:<br>
> > Subject: Re: [openib-general] [PATCH fixed] was Re: [PATCH]?<br>
> ipoib_multicast/ipoib_mcast_send race<br>
> > <br>
> > <br>
> > Hi Micheal, <br>
> > <br>
> > Is there a reason to have the atomic set_bit() within a lock
(even for <br>
> > a race condition of stop vs send, it doesn't seem to be required)
? <br>
> > Which means the test_bit() can also be put before the existing
lock... <br>
> > <br>
> > Thanks, <br>
> > <br>
> > - KK <br>
> > <br>
> > 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<br>
> > 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<br>
> > 4222)<br>
> > > +++ openib/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
  (working<br>
> > 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)
||<br>
> > !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<br>
> > http://openib.org/mailman/listinfo/openib-general<br>
> > <br>
> <br>
> -- <br>
> MST<br>
</tt></font>