Re: [PATCH v3 for-next 05/13] IB/cm: Reference count ib_cm_ids
On 20/05/2015 03:49, Hefty, Sean wrote: I wonder if the existing ib_cm interface is even what we want. Currently, the rdma_cm pushes the private data (i.e. IP address) comparison into the ib_cm. This is only used by the rdma_cm. Should that instead be moved out of the ib_cm and handled in the rdma_cm? And then update the ib_cm to support multiple listens on the same service id. Well, that really is the problem here, the private data compare doesn't really do enough because rdma cm wildcard's the IP address and does it's own check anyhow. So I see all of this as different proposals on how to fix that. rdma_cm basically already does most of the private data compare itself. Maybe the first thing to fix is this. Since the private data compare is no longer sufficient, remove it and update the rdma_cm to handle the change. Regardless of how the code ends up structured, the namespace support should then easily fit into that solution. Patch 8 in this series (IB/cma: Add compare_data checks to the RDMA CM module) basically does that (change rdma_cm so that it doesn't rely on private_data checks in ib_cm). Indeed, it is positioned in the series before adding the rdma_cm namespace support. It remains to clean up ib_cm's ib_cm_listen interface now that compare_data isn't used, but I'm not sure this belongs in this series. Haggai -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 for-next 05/13] IB/cm: Reference count ib_cm_ids
It remains to clean up ib_cm's ib_cm_listen interface now that compare_data isn't used, but I'm not sure this belongs in this series. This patch series is changing the behavior that the compare data solves. Currently, the ib_cm handles all of the multiplexing for the rdma_cm -- that's the reason for the compare data. This series changes that such that the ib_cm would handle half the multiplexing, with the other half handled by the rdma_cm. We should not insert that sort of split. So, I disagree that this isn't part of this series. Either do all of the multiplexing in the ib_cm -- without exposing it to inet (add port/pkey filtering if that works) -- or move all of it out. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 for-next 05/13] IB/cm: Reference count ib_cm_ids
I find Haggai's argument compelling, it is a very small amount of code and data to add a sharing count, and a very large amount to duplicate the whole service id map into cma.c. I get wanting to share the listen list, but we end up with this: drivers/infiniband/core/cm.c | 129 +- that's more code than what the ib_cm module uses to track listens, and the result is a solution that ends up being split in a weird fashion between the ib_cm, iw_cm (TBD), and rdma_cm. I wonder if the existing ib_cm interface is even what we want. Currently, the rdma_cm pushes the private data (i.e. IP address) comparison into the ib_cm. This is only used by the rdma_cm. Should that instead be moved out of the ib_cm and handled in the rdma_cm? And then update the ib_cm to support multiple listens on the same service id. For example, the ib_cm could simply queue all listen requests on the same service id. When a REQ arrives, it just calls each listener back until one 'claims' the REQ. The destruction of a listener could then be synced with the callback. From what I can tell, the current proposal requires that the ib_cm user be prepared to receive a REQ callback for a listen that it has already destroyed. If needed, a flag can be added to the ib_cm_listen to indicate if the service id is shared/exclusive. - Sean -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
