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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
GPG KeyID: 0E572FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
