On Mon, 2015-01-26 at 15:24 +0200, Erez Shitrit wrote: > >>>> The main cause is the concept that was broken for the send-only join, > >>>> when you treat the sendonly like a regular mcg and add it to the mc list > >>>> and to the mc_task etc. > >>> I'm looking at 3.18 right now, and 3.18 adds sendonly groups to the mcg > >>> just like my current code does. The only difference, and I do mean > >>> *only*, is that it calls sendonly_join directly instead of via the > >>> mcast_task. > >> Yes, and i already wrote that it is more than just "only", it changed > >> the concept of the sendonly mc packet. > > Be more specific please. What do you mean by "concept"? And just so we > > are clear, this all started because the existing multicast code was > > super easy to break and was racy, so if the "concept" you are referring > > to is what made the original code easy to break and racy, I'm not going > > to care one whit that I changed that concept. > > > > I agree that you fixed many bugs in your patches to 3.18, where the mc > flow was easy to break, no argue about that. > The only issue that i disagree is about the way now sendonly is handled > (and i think that this is the reason for the regression we see now).
It's not. The patch I sent you off list should be sufficient at this point to finally put your issues to rest. > In general, IMHO, the sendonly join is part of the TX flow and not part > of the ipoib_set_mcast_list flow. I disagree. I'll get into that below. > The original meaning of the ipoib_set_mcast_list task that restart the > mc_task is to be used for the kernel in order to add one or more new > mcg's macs to the driver/HW (ndo_set_rx_mode), the sendonly mc is not > such object, its mac should not be part of the "mac" list of the driver > (in IB wards, no qp_attach for it) This part I agree with. > and from the kernel point of view > whenever it sends packet from sendonly mcg type no need to do the join, > it's a regular send, the only reason we have the sendonly join is the IB > enforcement for such mcg. This is where you and I differ. There is a requirement on us from the IB spec that we have to join a sendonly MC group in order to do what the kernel would think of as a normal send if we were on Ethernet. We aren't on Ethernet though, so acting like we are to the above layer is fine, but in our own layer we should be coding to the realities of our layer. And that means treating a sendonly MC group like a regular MC group. > The reason the driver keeps the sendonly mcg in its mc_list is from > others reasons, the first is to handle the case when the kernel decides > to move a mcg from sendonly membership to full-member, one more other > reason is to do the leave operation when needed and not for being > handled as a full-member mcg. It's been a long time since I first started working on this issue, so some of the details are fuzzy in my memory. I think my first 8 patch patchset is now about 6 months old. But, I think probably the most important thing you left off this list, and the one that trumps all the others, is that whether a sendonly MC group has a QP attached or not, we still have to account for these groups, we have to track them, and on shutdown we have to reliably leave them without oopsing. Putting the join of the sendonly groups directly in mcast_send opens us up to problems with synchronizing our mcast list (either due to a dev flush, a mcast restart task, or something else). I can't say for certain that the change to how we record the return value from ib_sa_join_multicast in sendonly_join is enough to keep the sendonly join code from racing with the flush code and causing problems. For that reason, I am highly reluctant to put the sendonly join back directly into the tx path because that could have been part of the original problem in the first place, it's just that my memory of back then is too fuzzy to say that with 100% certainty either way. -- Doug Ledford <[email protected]> GPG KeyID: 0E572FDD
signature.asc
Description: This is a digitally signed message part
