| Message ID | 20230918142700.12745-1-vitaly@enfabrica.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route() | expand |
On Mon, Sep 18, 2023 at 02:27:00PM +0000, Vitaly Mayatskikh wrote: > rdma_resolve_route checks for the full rdma_protocol_iwarp support > before calling cma_resolve_iw_route, while in fact rdma_cap_iw_cm is > sufficient. This makes it possible to use IW CM for device > implementing IW Connection Management only, but not the whole iWarp. > > Signed-off-by: Vitaly Mayatskikh <vitaly@enfabrica.net> > --- > drivers/infiniband/core/cma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index c343edf2f664..356da8e625aa 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -3378,7 +3378,7 @@ int rdma_resolve_route(struct rdma_cm_id *id, unsigned long timeout_ms) > if (!ret) > cma_add_id_to_tree(id_priv); > } > - else if (rdma_protocol_iwarp(id->device, id->port_num)) > + else if (rdma_cap_iw_cm(id->device, id->port_num)) I see that rdma_protocol_iwarp() is used in other places in cma.c too, Don't they need to be updated too? Also I see that we have check for protocol RoCE in else before the changed line, shouldn't all cma.c be changed to rdma_cap_*_cm() calls? 3376 else if (rdma_protocol_roce(id->device, id->port_num)) { Thanks > ret = cma_resolve_iw_route(id_priv); > else > ret = -ENOSYS; > -- > 2.34.1 >
On Tue, Sep 19, 2023 at 4:06 PM Vitaly Mayatskikh <vitaly@enfabrica.net> wrote: > > On Tue, Sep 19, 2023 at 3:21 AM Leon Romanovsky <leon@kernel.org> wrote: > > > I see that rdma_protocol_iwarp() is used in other places in cma.c too, > > Don't they need to be updated too? > > > > Also I see that we have check for protocol RoCE in else before the > > changed line, shouldn't all cma.c be changed to rdma_cap_*_cm() calls? > > > > 3376 else if (rdma_protocol_roce(id->device, id->port_num)) { > > I can't really judge, but looking around in the code it seems that > some if not all of > those cma.c functions that are checking for the protocol - they only > called from the > drivers that actually use the protocol. For example, iSER. > > Our driver does not support iWarp, but implements IW_CM callbacks. The patch has > the only fix that was needed to make it work w/o a full blown iWarp. Ugh, adding everyone back...
On Tue, Sep 19, 2023 at 04:08:38PM -0400, Vitaly Mayatskikh wrote: > On Tue, Sep 19, 2023 at 4:06 PM Vitaly Mayatskikh <vitaly@enfabrica.net> wrote: > > > > On Tue, Sep 19, 2023 at 3:21 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > I see that rdma_protocol_iwarp() is used in other places in cma.c too, > > > Don't they need to be updated too? > > > > > > Also I see that we have check for protocol RoCE in else before the > > > changed line, shouldn't all cma.c be changed to rdma_cap_*_cm() calls? > > > > > > 3376 else if (rdma_protocol_roce(id->device, id->port_num)) { > > > > I can't really judge, but looking around in the code it seems that > > some if not all of > > those cma.c functions that are checking for the protocol - they only > > called from the > > drivers that actually use the protocol. For example, iSER. Just to make sure that we are using correct terminology - iSER is ULP (upper layer protocol) and not driver. > > > > Our driver does not support iWarp, but implements IW_CM callbacks. The patch has > > the only fix that was needed to make it work w/o a full blown iWarp. It is hard to say without having driver in-tree and seeing the result of ib_device_check_mandatory() in regards of kverbs_provider variable. Does any existing in-tree driver require the proposed change in rdma-cm? Thanks > > Ugh, adding everyone back...
On Wed, Sep 20, 2023 at 08:44:52AM +0300, Leon Romanovsky wrote: > On Tue, Sep 19, 2023 at 04:08:38PM -0400, Vitaly Mayatskikh wrote: > > On Tue, Sep 19, 2023 at 4:06 PM Vitaly Mayatskikh <vitaly@enfabrica.net> wrote: > > > > > > On Tue, Sep 19, 2023 at 3:21 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > I see that rdma_protocol_iwarp() is used in other places in cma.c too, > > > > Don't they need to be updated too? > > > > > > > > Also I see that we have check for protocol RoCE in else before the > > > > changed line, shouldn't all cma.c be changed to rdma_cap_*_cm() calls? > > > > > > > > 3376 else if (rdma_protocol_roce(id->device, id->port_num)) { > > > > > > I can't really judge, but looking around in the code it seems that > > > some if not all of > > > those cma.c functions that are checking for the protocol - they only > > > called from the > > > drivers that actually use the protocol. For example, iSER. > > Just to make sure that we are using correct terminology - iSER is ULP > (upper layer protocol) and not driver. > > > > > > > Our driver does not support iWarp, but implements IW_CM callbacks. The patch has > > > the only fix that was needed to make it work w/o a full blown iWarp. > > It is hard to say without having driver in-tree and seeing the result of > ib_device_check_mandatory() in regards of kverbs_provider variable. > > Does any existing in-tree driver require the proposed change in > rdma-cm? Yes, lets see a driver first please. iWarp CM is tightly tied to iWarp, I have a hard time understanding how you could have the CM without supporting iWarp too. Jason
On Wed, Sep 20, 2023 at 8:55 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > Our driver does not support iWarp, but implements IW_CM callbacks. The patch has > > > > the only fix that was needed to make it work w/o a full blown iWarp. > > > > It is hard to say without having driver in-tree and seeing the result of > > ib_device_check_mandatory() in regards of kverbs_provider variable. > > > > Does any existing in-tree driver require the proposed change in > > rdma-cm? > > Yes, lets see a driver first please. > > iWarp CM is tightly tied to iWarp, I have a hard time understanding > how you could have the CM without supporting iWarp too. Yes, it is coming. Not sure about the timeline, but the intent is to opensource the drivers. I'll defer the patch till then. The IB driver implements ib_device_ops->iw_* ops and cq/qp/mr-related kverbs. This is sufficient for CM to work with the device in principle. Thanks.
On Wed, Sep 20, 2023 at 09:07:44AM -0400, Vitaly Mayatskikh wrote: > On Wed, Sep 20, 2023 at 8:55 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > > Our driver does not support iWarp, but implements IW_CM callbacks. The patch has > > > > > the only fix that was needed to make it work w/o a full blown iWarp. > > > > > > It is hard to say without having driver in-tree and seeing the result of > > > ib_device_check_mandatory() in regards of kverbs_provider variable. > > > > > > Does any existing in-tree driver require the proposed change in > > > rdma-cm? > > > > Yes, lets see a driver first please. > > > > iWarp CM is tightly tied to iWarp, I have a hard time understanding > > how you could have the CM without supporting iWarp too. > > Yes, it is coming. Not sure about the timeline, but the intent is to > opensource the drivers. I'll defer the patch till then. > > The IB driver implements ib_device_ops->iw_* ops and cq/qp/mr-related > kverbs. This is sufficient for CM to work with the device in > principle. But is it iWarp? I'm not keen on seeing people abuse iwarp stuff for some non-standards based thing. iwarp is already in a disused state, there isn't enough community energy there to police something non-standards based. Jason
On Wed, Sep 20, 2023 at 9:10 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > But is it iWarp? > > I'm not keen on seeing people abuse iwarp stuff for some non-standards > based thing. iwarp is already in a disused state, there isn't enough > community energy there to police something non-standards based. No, it is IP-based, but not iWarp. Using CM implementation for IP-network (IW_CM) was a logical decision. In fact, IW_CM can be rebranded as IP_CM as it is not tightly coupled with iWarp per se and can serve any IP-based protocols. Thanks.
On Wed, Sep 20, 2023 at 09:53:00AM -0400, Vitaly Mayatskikh wrote: > On Wed, Sep 20, 2023 at 9:10 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > > But is it iWarp? > > > > I'm not keen on seeing people abuse iwarp stuff for some non-standards > > based thing. iwarp is already in a disused state, there isn't enough > > community energy there to police something non-standards based. > > No, it is IP-based, but not iWarp. Using CM implementation for > IP-network (IW_CM) was a logical decision. In fact, IW_CM can be > rebranded as IP_CM as it is not tightly coupled with iWarp per se and > can serve any IP-based protocols. And what happens if one of your non-standard nodes points its IWCM at an IP that is actually running iWarp? Jason
On Wed, Sep 20, 2023 at 9:55 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > And what happens if one of your non-standard nodes points its IWCM at > an IP that is actually running iWarp? IWCM takes an IP and a port, creates a socket and listens or connects to that endpoint. It's not like we are hijacking, say, a dedicated RoCE UDP/4791 port and using it for something else. While the user may try to connect his app to an arbitrary iWarp listener, it would fail to negotiate. But that could happen even without us. Similarly our protocol would fail a negotiation if something else tries to connect to our listening endpoint. I think the name causes all this confusion. Thanks.
On Wed, Sep 20, 2023 at 10:24:00AM -0400, Vitaly Mayatskikh wrote: > On Wed, Sep 20, 2023 at 9:55 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > > And what happens if one of your non-standard nodes points its IWCM at > > an IP that is actually running iWarp? > > IWCM takes an IP and a port, creates a socket and listens or connects > to that endpoint. It is a bit more going on, but yes it does that But also you don't need iwcm to do that, just open a socket in your userspace. Jason
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index c343edf2f664..356da8e625aa 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -3378,7 +3378,7 @@ int rdma_resolve_route(struct rdma_cm_id *id, unsigned long timeout_ms) if (!ret) cma_add_id_to_tree(id_priv); } - else if (rdma_protocol_iwarp(id->device, id->port_num)) + else if (rdma_cap_iw_cm(id->device, id->port_num)) ret = cma_resolve_iw_route(id_priv); else ret = -ENOSYS;
rdma_resolve_route checks for the full rdma_protocol_iwarp support before calling cma_resolve_iw_route, while in fact rdma_cap_iw_cm is sufficient. This makes it possible to use IW CM for device implementing IW Connection Management only, but not the whole iWarp. Signed-off-by: Vitaly Mayatskikh <vitaly@enfabrica.net> --- drivers/infiniband/core/cma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)