On Fri, 2015-02-13 at 10:50 +0200, Erez Shitrit wrote: > On 2/12/2015 3:43 AM, Doug Ledford wrote: > > The usage of IPOIB_MCAST_RUN as a flag is inconsistent. In some places > > it is used to mean "our device is administratively allowed to send > > multicast joins/leaves/packets" and in other places it means "our > > multicast join task thread is currently running and will process your > > request if you put it on the queue". However, this latter meaning is in > > fact flawed as there is a race condition between the join task testing > > the mcast list and finding it empty of remaining work, dropping the > > mcast mutex and also the priv->lock spinlock, and clearing the > > IPOIB_MCAST_RUN flag. Further, there are numerous locations that use > > the flag in the former fashion, and when all tasks complete and the task > > thread clears the RUN flag, all of those other locations will fail to > > ever again queue any work. This results in the interface coming up fine > > initially, but having problems adding new multicast groups after the > > first round of groups have all been added and the RUN flag is cleared by > > the join task thread when it thinks it is done. To resolve this issue, > > convert all locations in the code to treat the RUN flag as an indicator > > that the multicast portion of this interface is in fact administratively > > up and joins/leaves/sends can be performed. There is no harm (other > > than a slight performance penalty) to never clearing this flag and using > > it in this fashion as it simply means that a few places that used to > > micro-optimize how often this task was queued on a work queue will now > > queue the task a few extra times. We can address that suboptimal > > behavior in future patches. > > > > Signed-off-by: Doug Ledford <dledf...@redhat.com> > > --- > > drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c > > b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c > > index bc50dd0d0e4..91b8fe118ec 100644 > > --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c > > +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c > > @@ -630,8 +630,6 @@ void ipoib_mcast_join_task(struct work_struct *work) > > } > > > > ipoib_dbg_mcast(priv, "successfully joined all multicast groups\n"); > > - > > - clear_bit(IPOIB_MCAST_RUN, &priv->flags); > > } > > > > int ipoib_mcast_start_thread(struct net_device *dev) > > @@ -641,8 +639,8 @@ int ipoib_mcast_start_thread(struct net_device *dev) > > ipoib_dbg_mcast(priv, "starting multicast thread\n"); > > > > mutex_lock(&mcast_mutex); > > - if (!test_and_set_bit(IPOIB_MCAST_RUN, &priv->flags)) > > - queue_delayed_work(priv->wq, &priv->mcast_task, 0); > > + set_bit(IPOIB_MCAST_RUN, &priv->flags); > > Hi Doug, > Can you use IPOIB_FLAG_OPER_UP instead of IPOIB_MCAST_RUN, in all these > places and get rid of that RUN bit?
Yes, I think that should be easily doable. -- Doug Ledford <dledf...@redhat.com> GPG KeyID: 0E572FDD
signature.asc
Description: This is a digitally signed message part