Re: [PATCH 09/22] IB/ipoib: fix IPOIB_MCAST_RUN flag usage

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]

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 <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >   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 <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]