[PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Vitaly Mayatskikh]

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))
 		ret = cma_resolve_iw_route(id_priv);
 	else
 		ret = -ENOSYS;
-- 
2.34.1

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Leon Romanovsky]

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
> 

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Vitaly Mayatskikh]

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

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Leon Romanovsky]

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

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Jason Gunthorpe]

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

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Vitaly Mayatskikh]

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.

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Jason Gunthorpe]

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

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Vitaly Mayatskikh]

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.

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Jason Gunthorpe]

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

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Vitaly Mayatskikh]

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.

Re: [PATCH] RDMA/core: use rdma_cap_iw_cm() in rdma_resolve_route()

[Jason Gunthorpe]

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