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