[PATCH v5 0/4] Handle ARPHRD_NONE devices for siw

[PATCH v5 0/4] Handle ARPHRD_NONE devices for siw

[Chuck Lever]

Here's a series that implements support for siw on tunnel devices,
based on suggestions from Jason Gunthorpe and Tom Talpey.

This series does not address a similar issue with rxe because RoCE
GID resolution behaves differently than it does for iWARP devices.
An independent solution is likely to be required for rxe.

Changes since v4:
- Address review comments from Tom Talpey

Changes since v3:
- Clean up RCU dereference in cma_validate_port()

Changes since v2:
- Split into multiple patches
- Pre-initialize gid_attr::ndev for iWARP devices

---

Chuck Lever (4):
      RDMA/siw: Fabricate a GID on tun and loopback devices
      RDMA/core: Set gid_attr.ndev for iWARP devices
      RDMA/cma: Deduplicate error flow in cma_validate_port()
      RDMA/cma: Avoid GID lookups on iWARP devices


 drivers/infiniband/core/cache.c       | 11 +++++++++++
 drivers/infiniband/core/cma.c         | 26 +++++++++++++++++++++-----
 drivers/infiniband/sw/siw/siw.h       |  1 +
 drivers/infiniband/sw/siw/siw_main.c  | 22 ++++++++--------------
 drivers/infiniband/sw/siw/siw_verbs.c |  4 ++--
 5 files changed, 43 insertions(+), 21 deletions(-)

--
Chuck Lever

[PATCH v5 1/4] RDMA/siw: Fabricate a GID on tun and loopback devices

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

LOOPBACK and NONE (tunnel) devices have all-zero MAC addresses.
Currently, siw_device_create() falls back to copying the IB device's
name in those cases, because an all-zero MAC address breaks the RDMA
core address resolution mechanism.

However, at the point when siw_device_create() constructs a GID, the
ib_device::name field is uninitialized, leaving the MAC address to
remain in an all-zero state.

Fabricate a random artificial GID for such devices, and ensure this
artificial GID is returned for all device query operations.

Reported-by: Tom Talpey <tom@talpey.com>
Link: https://lore.kernel.org/linux-rdma/SA0PR15MB391986C07C4D41E107E79659994FA@SA0PR15MB3919.namprd15.prod.outlook.com/T/#t
Fixes: a2d36b02c15d ("RDMA/siw: Enable siw on tunnel devices")
Reviewed-by: Bernard Metzler <bmt@zurich.ibm.com>
Reviewed-by: Tom Talpey <tom@talpey.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 drivers/infiniband/sw/siw/siw.h       |    1 +
 drivers/infiniband/sw/siw/siw_main.c  |   22 ++++++++--------------
 drivers/infiniband/sw/siw/siw_verbs.c |    4 ++--
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/sw/siw/siw.h b/drivers/infiniband/sw/siw/siw.h
index 2f3a9cda3850..8b4a710b82bc 100644
--- a/drivers/infiniband/sw/siw/siw.h
+++ b/drivers/infiniband/sw/siw/siw.h
@@ -74,6 +74,7 @@ struct siw_device {
 
 	u32 vendor_part_id;
 	int numa_node;
+	char raw_gid[ETH_ALEN];
 
 	/* physical port state (only one port per device) */
 	enum ib_port_state state;
diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c
index 65b5cda5457b..f45600d169ae 100644
--- a/drivers/infiniband/sw/siw/siw_main.c
+++ b/drivers/infiniband/sw/siw/siw_main.c
@@ -75,8 +75,7 @@ static int siw_device_register(struct siw_device *sdev, const char *name)
 		return rv;
 	}
 
-	siw_dbg(base_dev, "HWaddr=%pM\n", sdev->netdev->dev_addr);
-
+	siw_dbg(base_dev, "HWaddr=%pM\n", sdev->raw_gid);
 	return 0;
 }
 
@@ -313,24 +312,19 @@ static struct siw_device *siw_device_create(struct net_device *netdev)
 		return NULL;
 
 	base_dev = &sdev->base_dev;
-
 	sdev->netdev = netdev;
 
-	if (netdev->type != ARPHRD_LOOPBACK && netdev->type != ARPHRD_NONE) {
-		addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
-				    netdev->dev_addr);
+	if (netdev->addr_len) {
+		memcpy(sdev->raw_gid, netdev->dev_addr,
+		       min_t(unsigned int, netdev->addr_len, ETH_ALEN));
 	} else {
 		/*
-		 * This device does not have a HW address,
-		 * but connection mangagement lib expects gid != 0
+		 * This device does not have a HW address, but
+		 * connection mangagement requires a unique gid.
 		 */
-		size_t len = min_t(size_t, strlen(base_dev->name), 6);
-		char addr[6] = { };
-
-		memcpy(addr, base_dev->name, len);
-		addrconf_addr_eui48((unsigned char *)&base_dev->node_guid,
-				    addr);
+		eth_random_addr(sdev->raw_gid);
 	}
+	addrconf_addr_eui48((u8 *)&base_dev->node_guid, sdev->raw_gid);
 
 	base_dev->uverbs_cmd_mask |= BIT_ULL(IB_USER_VERBS_CMD_POST_SEND);
 
diff --git a/drivers/infiniband/sw/siw/siw_verbs.c b/drivers/infiniband/sw/siw/siw_verbs.c
index 398ec13db624..32b0befd25e2 100644
--- a/drivers/infiniband/sw/siw/siw_verbs.c
+++ b/drivers/infiniband/sw/siw/siw_verbs.c
@@ -157,7 +157,7 @@ int siw_query_device(struct ib_device *base_dev, struct ib_device_attr *attr,
 	attr->vendor_part_id = sdev->vendor_part_id;
 
 	addrconf_addr_eui48((u8 *)&attr->sys_image_guid,
-			    sdev->netdev->dev_addr);
+			    sdev->raw_gid);
 
 	return 0;
 }
@@ -218,7 +218,7 @@ int siw_query_gid(struct ib_device *base_dev, u32 port, int idx,
 
 	/* subnet_prefix == interface_id == 0; */
 	memset(gid, 0, sizeof(*gid));
-	memcpy(&gid->raw[0], sdev->netdev->dev_addr, 6);
+	memcpy(gid->raw, sdev->raw_gid, ETH_ALEN);
 
 	return 0;
 }

[PATCH v5 2/4] RDMA/core: Set gid_attr.ndev for iWARP devices

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Have the iwarp side properly set the ndev in the device's sgid_attrs
so that address resolution can treat it more like a RoCE device.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Tom Talpey <tom@talpey.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 drivers/infiniband/core/cache.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 2e91d8879326..33f9d02f9b60 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -1457,6 +1457,17 @@ static int config_non_roce_gid_cache(struct ib_device *device,
 				 i);
 			goto err;
 		}
+
+		if (rdma_protocol_iwarp(device, port)) {
+			struct net_device *ndev;
+
+			ndev = ib_device_get_netdev(device, port);
+			if (!ndev)
+				continue;
+			RCU_INIT_POINTER(gid_attr.ndev, ndev);
+			dev_put(ndev);
+		}
+
 		gid_attr.index = i;
 		tprops->subnet_prefix =
 			be64_to_cpu(gid_attr.gid.global.subnet_prefix);

[PATCH v5 3/4] RDMA/cma: Deduplicate error flow in cma_validate_port()

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up to prepare for the addition of new logic.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 drivers/infiniband/core/cma.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 6b3f4384e46a..889b3e4ea980 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -686,30 +686,31 @@ cma_validate_port(struct ib_device *device, u32 port,
 		  struct rdma_id_private *id_priv)
 {
 	struct rdma_dev_addr *dev_addr = &id_priv->id.route.addr.dev_addr;
+	const struct ib_gid_attr *sgid_attr = ERR_PTR(-ENODEV);
 	int bound_if_index = dev_addr->bound_dev_if;
-	const struct ib_gid_attr *sgid_attr;
 	int dev_type = dev_addr->dev_type;
 	struct net_device *ndev = NULL;
 
 	if (!rdma_dev_access_netns(device, id_priv->id.route.addr.dev_addr.net))
-		return ERR_PTR(-ENODEV);
+		goto out;
 
 	if ((dev_type == ARPHRD_INFINIBAND) && !rdma_protocol_ib(device, port))
-		return ERR_PTR(-ENODEV);
+		goto out;
 
 	if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
-		return ERR_PTR(-ENODEV);
+		goto out;
 
 	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) {
 		ndev = dev_get_by_index(dev_addr->net, bound_if_index);
 		if (!ndev)
-			return ERR_PTR(-ENODEV);
+			goto out;
 	} else {
 		gid_type = IB_GID_TYPE_IB;
 	}
 
 	sgid_attr = rdma_find_gid_by_port(device, gid, gid_type, port, ndev);
 	dev_put(ndev);
+out:
 	return sgid_attr;
 }
 

[PATCH v5 4/4] RDMA/cma: Avoid GID lookups on iWARP devices

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

We would like to enable the use of siw on top of a VPN that is
constructed and managed via a tun device. That hasn't worked up
until now because ARPHRD_NONE devices (such as tun devices) have
no GID for the RDMA/core to look up.

But it turns out that the egress device has already been picked for
us -- no GID is necessary. addr_handler() just has to do the right
thing with it.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 drivers/infiniband/core/cma.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 889b3e4ea980..07bb5ac4019d 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -700,6 +700,21 @@ cma_validate_port(struct ib_device *device, u32 port,
 	if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
 		goto out;
 
+	/* Linux iWARP devices have but one port */
+	if (rdma_protocol_iwarp(device, port)) {
+		sgid_attr = rdma_get_gid_attr(device, port, 0);
+		if (IS_ERR(sgid_attr))
+			goto out;
+
+		rcu_read_lock();
+		ndev = rcu_dereference(sgid_attr->ndev);
+		if (!net_eq(dev_net(ndev), dev_addr->net) ||
+		    ndev->ifindex != bound_if_index)
+			sgid_attr = ERR_PTR(-ENODEV);
+		rcu_read_unlock();
+		goto out;
+	}
+
 	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) {
 		ndev = dev_get_by_index(dev_addr->net, bound_if_index);
 		if (!ndev)

Re: [PATCH v5 0/4] Handle ARPHRD_NONE devices for siw

[Zhu Yanjun]

在 2023/6/29 23:16, Chuck Lever 写道:
> Here's a series that implements support for siw on tunnel devices,
> based on suggestions from Jason Gunthorpe and Tom Talpey.
>
> This series does not address a similar issue with rxe because RoCE
> GID resolution behaves differently than it does for iWARP devices.
> An independent solution is likely to be required for rxe.

Thanks for letting me know this.

Zhu Yanjun

>
> Changes since v4:
> - Address review comments from Tom Talpey
>
> Changes since v3:
> - Clean up RCU dereference in cma_validate_port()
>
> Changes since v2:
> - Split into multiple patches
> - Pre-initialize gid_attr::ndev for iWARP devices
>
> ---
>
> Chuck Lever (4):
>        RDMA/siw: Fabricate a GID on tun and loopback devices
>        RDMA/core: Set gid_attr.ndev for iWARP devices
>        RDMA/cma: Deduplicate error flow in cma_validate_port()
>        RDMA/cma: Avoid GID lookups on iWARP devices
>
>
>   drivers/infiniband/core/cache.c       | 11 +++++++++++
>   drivers/infiniband/core/cma.c         | 26 +++++++++++++++++++++-----
>   drivers/infiniband/sw/siw/siw.h       |  1 +
>   drivers/infiniband/sw/siw/siw_main.c  | 22 ++++++++--------------
>   drivers/infiniband/sw/siw/siw_verbs.c |  4 ++--
>   5 files changed, 43 insertions(+), 21 deletions(-)
>
> --
> Chuck Lever
>

Re: [PATCH v5 4/4] RDMA/cma: Avoid GID lookups on iWARP devices

[Tom Talpey]

On 6/29/2023 11:16 AM, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> We would like to enable the use of siw on top of a VPN that is
> constructed and managed via a tun device. That hasn't worked up
> until now because ARPHRD_NONE devices (such as tun devices) have
> no GID for the RDMA/core to look up.
> 
> But it turns out that the egress device has already been picked for
> us -- no GID is necessary. addr_handler() just has to do the right
> thing with it.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>   drivers/infiniband/core/cma.c |   15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 889b3e4ea980..07bb5ac4019d 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -700,6 +700,21 @@ cma_validate_port(struct ib_device *device, u32 port,
>   	if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
>   		goto out;
>   
> +	/* Linux iWARP devices have but one port */

I don't believe this comment is correct, or necessary. In-tree drivers
exist for several multi-port iWARP devices, and the port bnumber passed
to rdma_protocol_iwarp() and rdma_get_gid_attr() will follow, no?

The code looks correct otherwise...

Tom.

> +	if (rdma_protocol_iwarp(device, port)) {
> +		sgid_attr = rdma_get_gid_attr(device, port, 0);
> +		if (IS_ERR(sgid_attr))
> +			goto out;
> +
> +		rcu_read_lock();
> +		ndev = rcu_dereference(sgid_attr->ndev);
> +		if (!net_eq(dev_net(ndev), dev_addr->net) ||
> +		    ndev->ifindex != bound_if_index)
> +			sgid_attr = ERR_PTR(-ENODEV);
> +		rcu_read_unlock();
> +		goto out;
> +	}
> +
>   	if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) {
>   		ndev = dev_get_by_index(dev_addr->net, bound_if_index);
>   		if (!ndev)
> 
> 
> 

Re: [PATCH v5 4/4] RDMA/cma: Avoid GID lookups on iWARP devices

[Chuck Lever III]

> On Jul 1, 2023, at 12:24 PM, Tom Talpey <tom@talpey.com> wrote:
> 
> On 6/29/2023 11:16 AM, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>> We would like to enable the use of siw on top of a VPN that is
>> constructed and managed via a tun device. That hasn't worked up
>> until now because ARPHRD_NONE devices (such as tun devices) have
>> no GID for the RDMA/core to look up.
>> But it turns out that the egress device has already been picked for
>> us -- no GID is necessary. addr_handler() just has to do the right
>> thing with it.
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>  drivers/infiniband/core/cma.c |   15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>> index 889b3e4ea980..07bb5ac4019d 100644
>> --- a/drivers/infiniband/core/cma.c
>> +++ b/drivers/infiniband/core/cma.c
>> @@ -700,6 +700,21 @@ cma_validate_port(struct ib_device *device, u32 port,
>>   if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
>>   goto out;
>>  + /* Linux iWARP devices have but one port */
> 
> I don't believe this comment is correct, or necessary. In-tree drivers
> exist for several multi-port iWARP devices, and the port bnumber passed
> to rdma_protocol_iwarp() and rdma_get_gid_attr() will follow, no?

Then I must have misunderstood what Jason said about the reason
for the rdma_protocol_iwarp() check. He said that we are able to
do this kind of GID lookup because iWARP devices have only a
single port.

Jason?


> The code looks correct otherwise...
> 
> Tom.
> 
>> + if (rdma_protocol_iwarp(device, port)) {
>> + sgid_attr = rdma_get_gid_attr(device, port, 0);
>> + if (IS_ERR(sgid_attr))
>> + goto out;
>> +
>> + rcu_read_lock();
>> + ndev = rcu_dereference(sgid_attr->ndev);
>> + if (!net_eq(dev_net(ndev), dev_addr->net) ||
>> +     ndev->ifindex != bound_if_index)
>> + sgid_attr = ERR_PTR(-ENODEV);
>> + rcu_read_unlock();
>> + goto out;
>> + }
>> +
>>   if (dev_type == ARPHRD_ETHER && rdma_protocol_roce(device, port)) {
>>   ndev = dev_get_by_index(dev_addr->net, bound_if_index);
>>   if (!ndev)


--
Chuck Lever

Re: [PATCH v5 4/4] RDMA/cma: Avoid GID lookups on iWARP devices

[Jason Gunthorpe]

On Sat, Jul 01, 2023 at 04:27:23PM +0000, Chuck Lever III wrote:
> 
> 
> > On Jul 1, 2023, at 12:24 PM, Tom Talpey <tom@talpey.com> wrote:
> > 
> > On 6/29/2023 11:16 AM, Chuck Lever wrote:
> >> From: Chuck Lever <chuck.lever@oracle.com>
> >> We would like to enable the use of siw on top of a VPN that is
> >> constructed and managed via a tun device. That hasn't worked up
> >> until now because ARPHRD_NONE devices (such as tun devices) have
> >> no GID for the RDMA/core to look up.
> >> But it turns out that the egress device has already been picked for
> >> us -- no GID is necessary. addr_handler() just has to do the right
> >> thing with it.
> >> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >> ---
> >>  drivers/infiniband/core/cma.c |   15 +++++++++++++++
> >>  1 file changed, 15 insertions(+)
> >> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> >> index 889b3e4ea980..07bb5ac4019d 100644
> >> --- a/drivers/infiniband/core/cma.c
> >> +++ b/drivers/infiniband/core/cma.c
> >> @@ -700,6 +700,21 @@ cma_validate_port(struct ib_device *device, u32 port,
> >>   if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
> >>   goto out;
> >>  + /* Linux iWARP devices have but one port */
> > 
> > I don't believe this comment is correct, or necessary. In-tree drivers
> > exist for several multi-port iWARP devices, and the port bnumber passed
> > to rdma_protocol_iwarp() and rdma_get_gid_attr() will follow, no?
> 
> Then I must have misunderstood what Jason said about the reason
> for the rdma_protocol_iwarp() check. He said that we are able to
> do this kind of GID lookup because iWARP devices have only a
> single port.
> 
> Jason?

I don't know alot about iwarp - tom does iwarp really have multiported
*struct ib_device* models? This is different from multiport hw.

If it is multiport how do the gid tables work across the ports?

Jason

Re: [PATCH v5 4/4] RDMA/cma: Avoid GID lookups on iWARP devices

[Tom Talpey]

On 7/3/2023 5:07 PM, Jason Gunthorpe wrote:
> On Sat, Jul 01, 2023 at 04:27:23PM +0000, Chuck Lever III wrote:
>>
>>
>>> On Jul 1, 2023, at 12:24 PM, Tom Talpey <tom@talpey.com> wrote:
>>>
>>> On 6/29/2023 11:16 AM, Chuck Lever wrote:
>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>> We would like to enable the use of siw on top of a VPN that is
>>>> constructed and managed via a tun device. That hasn't worked up
>>>> until now because ARPHRD_NONE devices (such as tun devices) have
>>>> no GID for the RDMA/core to look up.
>>>> But it turns out that the egress device has already been picked for
>>>> us -- no GID is necessary. addr_handler() just has to do the right
>>>> thing with it.
>>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>   drivers/infiniband/core/cma.c |   15 +++++++++++++++
>>>>   1 file changed, 15 insertions(+)
>>>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>>>> index 889b3e4ea980..07bb5ac4019d 100644
>>>> --- a/drivers/infiniband/core/cma.c
>>>> +++ b/drivers/infiniband/core/cma.c
>>>> @@ -700,6 +700,21 @@ cma_validate_port(struct ib_device *device, u32 port,
>>>>    if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
>>>>    goto out;
>>>>   + /* Linux iWARP devices have but one port */
>>>
>>> I don't believe this comment is correct, or necessary. In-tree drivers
>>> exist for several multi-port iWARP devices, and the port bnumber passed
>>> to rdma_protocol_iwarp() and rdma_get_gid_attr() will follow, no?
>>
>> Then I must have misunderstood what Jason said about the reason
>> for the rdma_protocol_iwarp() check. He said that we are able to
>> do this kind of GID lookup because iWARP devices have only a
>> single port.
>>
>> Jason?
> 
> I don't know alot about iwarp - tom does iwarp really have multiported
> *struct ib_device* models? This is different from multiport hw.

I don't see how the iWARP protocol impacts this, but I believe the
cxgb4 provider implements multiport. It sets the ibdev.phys_port_cnt
anyway. Perhaps this is incorrect.

> If it is multiport how do the gid tables work across the ports?

Again, not sure how to respond. iWARP doesn't express the gid as a
protocol element. And the iw_cm really doesn't either, although it
does implement a gid-type API I guess. That's local behavior though,
not something that goes on the wire directly.

Maybe I should ask... what does the "Linux iWARP devices have but one
port" actually mean in the comment? Would the code below it not work
if that were not the case? All I'm saying is that the comment seems
to be unnecessary, and confusing.

Tom.

Re: [PATCH v5 4/4] RDMA/cma: Avoid GID lookups on iWARP devices

[Chuck Lever III]

> On Jul 4, 2023, at 10:23 AM, Tom Talpey <tom@talpey.com> wrote:
> 
> On 7/3/2023 5:07 PM, Jason Gunthorpe wrote:
>> On Sat, Jul 01, 2023 at 04:27:23PM +0000, Chuck Lever III wrote:
>>> 
>>> 
>>>> On Jul 1, 2023, at 12:24 PM, Tom Talpey <tom@talpey.com> wrote:
>>>> 
>>>> On 6/29/2023 11:16 AM, Chuck Lever wrote:
>>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>> We would like to enable the use of siw on top of a VPN that is
>>>>> constructed and managed via a tun device. That hasn't worked up
>>>>> until now because ARPHRD_NONE devices (such as tun devices) have
>>>>> no GID for the RDMA/core to look up.
>>>>> But it turns out that the egress device has already been picked for
>>>>> us -- no GID is necessary. addr_handler() just has to do the right
>>>>> thing with it.
>>>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>> ---
>>>>>  drivers/infiniband/core/cma.c |   15 +++++++++++++++
>>>>>  1 file changed, 15 insertions(+)
>>>>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>>>>> index 889b3e4ea980..07bb5ac4019d 100644
>>>>> --- a/drivers/infiniband/core/cma.c
>>>>> +++ b/drivers/infiniband/core/cma.c
>>>>> @@ -700,6 +700,21 @@ cma_validate_port(struct ib_device *device, u32 port,
>>>>>   if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
>>>>>   goto out;
>>>>>  + /* Linux iWARP devices have but one port */
>>>> 
>>>> I don't believe this comment is correct, or necessary. In-tree drivers
>>>> exist for several multi-port iWARP devices, and the port bnumber passed
>>>> to rdma_protocol_iwarp() and rdma_get_gid_attr() will follow, no?
>>> 
>>> Then I must have misunderstood what Jason said about the reason
>>> for the rdma_protocol_iwarp() check. He said that we are able to
>>> do this kind of GID lookup because iWARP devices have only a
>>> single port.
>>> 
>>> Jason?
>> I don't know alot about iwarp - tom does iwarp really have multiported
>> *struct ib_device* models? This is different from multiport hw.
> 
> I don't see how the iWARP protocol impacts this, but I believe the
> cxgb4 provider implements multiport. It sets the ibdev.phys_port_cnt
> anyway. Perhaps this is incorrect.
> 
>> If it is multiport how do the gid tables work across the ports?
> 
> Again, not sure how to respond. iWARP doesn't express the gid as a
> protocol element. And the iw_cm really doesn't either, although it
> does implement a gid-type API I guess. That's local behavior though,
> not something that goes on the wire directly.
> 
> Maybe I should ask... what does the "Linux iWARP devices have but one
> port" actually mean in the comment? Would the code below it not work
> if that were not the case? All I'm saying is that the comment seems
> to be unnecessary, and confusing.

It replaces a code comment you complained about in an earlier review
regarding the use of "if (rdma_protocol_iwarp())". As far as I
understand, /in Linux/ each iWARP endpoint gets its own ib_device
and that device has exactly one port.

So for example, a physical device that has two ports would appear
as two ib_devices each with a single port. Is that not how it
works? It's certainly possible I've misunderstood something.

Again, this is not about the protocol, it's about how Linux
implements it. I'm open to specific suggestions to improve the
comment, or I can remove it if the code is sufficiently clear.


--
Chuck Lever

Re: [PATCH v5 4/4] RDMA/cma: Avoid GID lookups on iWARP devices

[Jason Gunthorpe]

On Tue, Jul 04, 2023 at 02:54:59PM +0000, Chuck Lever III wrote:
> 
> 
> > On Jul 4, 2023, at 10:23 AM, Tom Talpey <tom@talpey.com> wrote:
> > 
> > On 7/3/2023 5:07 PM, Jason Gunthorpe wrote:
> >> On Sat, Jul 01, 2023 at 04:27:23PM +0000, Chuck Lever III wrote:
> >>> 
> >>> 
> >>>> On Jul 1, 2023, at 12:24 PM, Tom Talpey <tom@talpey.com> wrote:
> >>>> 
> >>>> On 6/29/2023 11:16 AM, Chuck Lever wrote:
> >>>>> From: Chuck Lever <chuck.lever@oracle.com>
> >>>>> We would like to enable the use of siw on top of a VPN that is
> >>>>> constructed and managed via a tun device. That hasn't worked up
> >>>>> until now because ARPHRD_NONE devices (such as tun devices) have
> >>>>> no GID for the RDMA/core to look up.
> >>>>> But it turns out that the egress device has already been picked for
> >>>>> us -- no GID is necessary. addr_handler() just has to do the right
> >>>>> thing with it.
> >>>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> >>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>>> ---
> >>>>>  drivers/infiniband/core/cma.c |   15 +++++++++++++++
> >>>>>  1 file changed, 15 insertions(+)
> >>>>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> >>>>> index 889b3e4ea980..07bb5ac4019d 100644
> >>>>> --- a/drivers/infiniband/core/cma.c
> >>>>> +++ b/drivers/infiniband/core/cma.c
> >>>>> @@ -700,6 +700,21 @@ cma_validate_port(struct ib_device *device, u32 port,
> >>>>>   if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
> >>>>>   goto out;
> >>>>>  + /* Linux iWARP devices have but one port */
> >>>> 
> >>>> I don't believe this comment is correct, or necessary. In-tree drivers
> >>>> exist for several multi-port iWARP devices, and the port bnumber passed
> >>>> to rdma_protocol_iwarp() and rdma_get_gid_attr() will follow, no?
> >>> 
> >>> Then I must have misunderstood what Jason said about the reason
> >>> for the rdma_protocol_iwarp() check. He said that we are able to
> >>> do this kind of GID lookup because iWARP devices have only a
> >>> single port.
> >>> 
> >>> Jason?
> >> I don't know alot about iwarp - tom does iwarp really have multiported
> >> *struct ib_device* models? This is different from multiport hw.
> > 
> > I don't see how the iWARP protocol impacts this, but I believe the
> > cxgb4 provider implements multiport. It sets the ibdev.phys_port_cnt
> > anyway. Perhaps this is incorrect.
> > 
> >> If it is multiport how do the gid tables work across the ports?
> > 
> > Again, not sure how to respond. iWARP doesn't express the gid as a
> > protocol element. And the iw_cm really doesn't either, although it
> > does implement a gid-type API I guess. That's local behavior though,
> > not something that goes on the wire directly.
> > 
> > Maybe I should ask... what does the "Linux iWARP devices have but one
> > port" actually mean in the comment? Would the code below it not work
> > if that were not the case? All I'm saying is that the comment seems
> > to be unnecessary, and confusing.
> 
> It replaces a code comment you complained about in an earlier review
> regarding the use of "if (rdma_protocol_iwarp())". As far as I
> understand, /in Linux/ each iWARP endpoint gets its own ib_device
> and that device has exactly one port.
>
> So for example, a physical device that has two ports would appear
> as two ib_devices each with a single port. Is that not how it
> works? It's certainly possible I've misunderstood something.

That is how I would expect it to work. Multi-port ib_device is really
only something that exists to support IB's APM, and iWarp doesn't have
that.

Otherwise verbs says a QP is bound to a single IB device's port and a
single GID of that port. It should not float around between multiple
ports.

So, I don't know what the iwarp drivers did here.

As for rthe comment, I don't think it is quite right, this code
already knows what ib_device port it is supposed to be using somehow,
so it doesn't matter.

I think it should be more like:

 In iWarp mode we need to have a sgid entry to be able to locate the
 netdevice. iWarp drivers are not allowed to associate more than one
 net device with their gid tables, so returning the first entry is
 sufficient. iWarp will ignore the GID entries actual GID, and the
 passed in GID may not even be present in the GID table for tunnels
 and other non-ethernet netdevices.

Jason

Re: [PATCH v5 4/4] RDMA/cma: Avoid GID lookups on iWARP devices

[Chuck Lever III]

> On Jul 10, 2023, at 1:06 PM, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> On Tue, Jul 04, 2023 at 02:54:59PM +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Jul 4, 2023, at 10:23 AM, Tom Talpey <tom@talpey.com> wrote:
>>> 
>>> On 7/3/2023 5:07 PM, Jason Gunthorpe wrote:
>>>> On Sat, Jul 01, 2023 at 04:27:23PM +0000, Chuck Lever III wrote:
>>>>> 
>>>>> 
>>>>>> On Jul 1, 2023, at 12:24 PM, Tom Talpey <tom@talpey.com> wrote:
>>>>>> 
>>>>>> On 6/29/2023 11:16 AM, Chuck Lever wrote:
>>>>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>>>> We would like to enable the use of siw on top of a VPN that is
>>>>>>> constructed and managed via a tun device. That hasn't worked up
>>>>>>> until now because ARPHRD_NONE devices (such as tun devices) have
>>>>>>> no GID for the RDMA/core to look up.
>>>>>>> But it turns out that the egress device has already been picked for
>>>>>>> us -- no GID is necessary. addr_handler() just has to do the right
>>>>>>> thing with it.
>>>>>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>>> ---
>>>>>>> drivers/infiniband/core/cma.c |   15 +++++++++++++++
>>>>>>> 1 file changed, 15 insertions(+)
>>>>>>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>>>>>>> index 889b3e4ea980..07bb5ac4019d 100644
>>>>>>> --- a/drivers/infiniband/core/cma.c
>>>>>>> +++ b/drivers/infiniband/core/cma.c
>>>>>>> @@ -700,6 +700,21 @@ cma_validate_port(struct ib_device *device, u32 port,
>>>>>>>  if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
>>>>>>>  goto out;
>>>>>>> + /* Linux iWARP devices have but one port */
>>>>>> 
>>>>>> I don't believe this comment is correct, or necessary. In-tree drivers
>>>>>> exist for several multi-port iWARP devices, and the port bnumber passed
>>>>>> to rdma_protocol_iwarp() and rdma_get_gid_attr() will follow, no?
>>>>> 
>>>>> Then I must have misunderstood what Jason said about the reason
>>>>> for the rdma_protocol_iwarp() check. He said that we are able to
>>>>> do this kind of GID lookup because iWARP devices have only a
>>>>> single port.
>>>>> 
>>>>> Jason?
>>>> I don't know alot about iwarp - tom does iwarp really have multiported
>>>> *struct ib_device* models? This is different from multiport hw.
>>> 
>>> I don't see how the iWARP protocol impacts this, but I believe the
>>> cxgb4 provider implements multiport. It sets the ibdev.phys_port_cnt
>>> anyway. Perhaps this is incorrect.
>>> 
>>>> If it is multiport how do the gid tables work across the ports?
>>> 
>>> Again, not sure how to respond. iWARP doesn't express the gid as a
>>> protocol element. And the iw_cm really doesn't either, although it
>>> does implement a gid-type API I guess. That's local behavior though,
>>> not something that goes on the wire directly.
>>> 
>>> Maybe I should ask... what does the "Linux iWARP devices have but one
>>> port" actually mean in the comment? Would the code below it not work
>>> if that were not the case? All I'm saying is that the comment seems
>>> to be unnecessary, and confusing.
>> 
>> It replaces a code comment you complained about in an earlier review
>> regarding the use of "if (rdma_protocol_iwarp())". As far as I
>> understand, /in Linux/ each iWARP endpoint gets its own ib_device
>> and that device has exactly one port.
>> 
>> So for example, a physical device that has two ports would appear
>> as two ib_devices each with a single port. Is that not how it
>> works? It's certainly possible I've misunderstood something.
> 
> That is how I would expect it to work. Multi-port ib_device is really
> only something that exists to support IB's APM, and iWarp doesn't have
> that.
> 
> Otherwise verbs says a QP is bound to a single IB device's port and a
> single GID of that port. It should not float around between multiple
> ports.
> 
> So, I don't know what the iwarp drivers did here.
> 
> As for rthe comment, I don't think it is quite right, this code
> already knows what ib_device port it is supposed to be using somehow,
> so it doesn't matter.
> 
> I think it should be more like:
> 
> In iWarp mode we need to have a sgid entry to be able to locate the
> netdevice. iWarp drivers are not allowed to associate more than one
> net device with their gid tables, so returning the first entry is
> sufficient. iWarp will ignore the GID entries actual GID, and the
> passed in GID may not even be present in the GID table for tunnels
> and other non-ethernet netdevices.

I can make that change and post a refresh. I'd like
to hear from Tom first.



--
Chuck Lever

Re: [PATCH v5 4/4] RDMA/cma: Avoid GID lookups on iWARP devices

[Tom Talpey]

On 7/11/2023 6:49 PM, Chuck Lever III wrote:
> 
> 
>> On Jul 10, 2023, at 1:06 PM, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>
>> On Tue, Jul 04, 2023 at 02:54:59PM +0000, Chuck Lever III wrote:
>>>
>>>
>>>> On Jul 4, 2023, at 10:23 AM, Tom Talpey <tom@talpey.com> wrote:
>>>>
>>>> On 7/3/2023 5:07 PM, Jason Gunthorpe wrote:
>>>>> On Sat, Jul 01, 2023 at 04:27:23PM +0000, Chuck Lever III wrote:
>>>>>>
>>>>>>
>>>>>>> On Jul 1, 2023, at 12:24 PM, Tom Talpey <tom@talpey.com> wrote:
>>>>>>>
>>>>>>> On 6/29/2023 11:16 AM, Chuck Lever wrote:
>>>>>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>>>>> We would like to enable the use of siw on top of a VPN that is
>>>>>>>> constructed and managed via a tun device. That hasn't worked up
>>>>>>>> until now because ARPHRD_NONE devices (such as tun devices) have
>>>>>>>> no GID for the RDMA/core to look up.
>>>>>>>> But it turns out that the egress device has already been picked for
>>>>>>>> us -- no GID is necessary. addr_handler() just has to do the right
>>>>>>>> thing with it.
>>>>>>>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>>>>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>>>>> ---
>>>>>>>> drivers/infiniband/core/cma.c |   15 +++++++++++++++
>>>>>>>> 1 file changed, 15 insertions(+)
>>>>>>>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>>>>>>>> index 889b3e4ea980..07bb5ac4019d 100644
>>>>>>>> --- a/drivers/infiniband/core/cma.c
>>>>>>>> +++ b/drivers/infiniband/core/cma.c
>>>>>>>> @@ -700,6 +700,21 @@ cma_validate_port(struct ib_device *device, u32 port,
>>>>>>>>   if ((dev_type != ARPHRD_INFINIBAND) && rdma_protocol_ib(device, port))
>>>>>>>>   goto out;
>>>>>>>> + /* Linux iWARP devices have but one port */
>>>>>>>
>>>>>>> I don't believe this comment is correct, or necessary. In-tree drivers
>>>>>>> exist for several multi-port iWARP devices, and the port bnumber passed
>>>>>>> to rdma_protocol_iwarp() and rdma_get_gid_attr() will follow, no?
>>>>>>
>>>>>> Then I must have misunderstood what Jason said about the reason
>>>>>> for the rdma_protocol_iwarp() check. He said that we are able to
>>>>>> do this kind of GID lookup because iWARP devices have only a
>>>>>> single port.
>>>>>>
>>>>>> Jason?
>>>>> I don't know alot about iwarp - tom does iwarp really have multiported
>>>>> *struct ib_device* models? This is different from multiport hw.
>>>>
>>>> I don't see how the iWARP protocol impacts this, but I believe the
>>>> cxgb4 provider implements multiport. It sets the ibdev.phys_port_cnt
>>>> anyway. Perhaps this is incorrect.
>>>>
>>>>> If it is multiport how do the gid tables work across the ports?
>>>>
>>>> Again, not sure how to respond. iWARP doesn't express the gid as a
>>>> protocol element. And the iw_cm really doesn't either, although it
>>>> does implement a gid-type API I guess. That's local behavior though,
>>>> not something that goes on the wire directly.
>>>>
>>>> Maybe I should ask... what does the "Linux iWARP devices have but one
>>>> port" actually mean in the comment? Would the code below it not work
>>>> if that were not the case? All I'm saying is that the comment seems
>>>> to be unnecessary, and confusing.
>>>
>>> It replaces a code comment you complained about in an earlier review
>>> regarding the use of "if (rdma_protocol_iwarp())". As far as I
>>> understand, /in Linux/ each iWARP endpoint gets its own ib_device
>>> and that device has exactly one port.
>>>
>>> So for example, a physical device that has two ports would appear
>>> as two ib_devices each with a single port. Is that not how it
>>> works? It's certainly possible I've misunderstood something.
>>
>> That is how I would expect it to work. Multi-port ib_device is really
>> only something that exists to support IB's APM, and iWarp doesn't have
>> that.
>>
>> Otherwise verbs says a QP is bound to a single IB device's port and a
>> single GID of that port. It should not float around between multiple
>> ports.
>>
>> So, I don't know what the iwarp drivers did here.
>>
>> As for rthe comment, I don't think it is quite right, this code
>> already knows what ib_device port it is supposed to be using somehow,
>> so it doesn't matter.
>>
>> I think it should be more like:
>>
>> In iWarp mode we need to have a sgid entry to be able to locate the
>> netdevice. iWarp drivers are not allowed to associate more than one
>> net device with their gid tables, so returning the first entry is
>> sufficient. iWarp will ignore the GID entries actual GID, and the
>> passed in GID may not even be present in the GID table for tunnels
>> and other non-ethernet netdevices.
> 
> I can make that change and post a refresh. I'd like
> to hear from Tom first.

The extra detail is helpful, but I'm still queasy about tying this to
iWARP. The situation seems much more general. But the breadcrumbs are
here so if the situation arises in another transport, there's a trail
for others to follow.

Tom.