Re: [PATCH FIX for-3.19] IB/ipoib: Fix failed multicast joins/sends

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

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

On Wed, 2015-01-14 at 11:54 +0200, Or Gerlitz wrote:
> On 1/14/2015 8:38 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" [...]
> what? please tell which of these upstream hits (prior to any fix) has 
> this semantics?

Actually, almost all of them.  I don't think they were originally
intended to be that way, but as a result of the mechanics of how
multicast joins are done, the net result is what it is.  But even more
importantly, this semantic is what we *want*.  For example, in
join_finish() where it tests IPOIB_MCAST_RUN before queueing delayed
work on a join failure, we *want* this to queue work whether the current
task thinks it is done or not as this is an async failure case and the
thread could have thought it was finished and cleared the flag before we
got our failure.  Treating this flag with the old semantic leaves this
situation with a dangling error condition that doesn't get retried until
something else restarts the mcast thread.  In fact, in almost all of the
places that this flag is checked, we really do want to run the thread
again unless we are in the middle of a mcast flush or we are downing the
dev.  In all other cases, we want to requeue the task.  So the semantic
I'm applying here, even if not what you or some other people had in
mind, is in fact closer to the truth of how the flag is used, and is
more in line with how we *want* the flag used.

> # git grep -n IPOIB_MCAST_RUN drivers/infiniband/ulp/ipoib
> drivers/infiniband/ulp/ipoib/ipoib.h:90: IPOIB_MCAST_RUN           = 6,
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:434: if 
> (test_bit(IPOIB_MCAST_RUN, &priv->flags))
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:466:     if (status && 
> test_bit(IPOIB_MCAST_RUN, &priv->flags))
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:536: if 
> (test_bit(IPOIB_MCAST_RUN, &priv->flags))
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:550:     if 
> (!test_bit(IPOIB_MCAST_RUN, &priv->flags))
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:576: if 
> (test_bit(IPOIB_MCAST_RUN, &priv->flags))
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:634: 
> clear_bit(IPOIB_MCAST_RUN, &priv->flags);
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:644:     if 
> (!test_and_set_bit(IPOIB_MCAST_RUN, &priv->flags))
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:658: 
> clear_bit(IPOIB_MCAST_RUN, &priv->flags);
> drivers/infiniband/ulp/ipoib/ipoib_multicast.c:728: if 
> (!test_and_set_bit(IPOIB_MCAST_RUN, &priv->flags))
> 


-- 
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 --]