RDMA fixes and refactoring for 4.15

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

-----Original Message-----
From: Jason Gunthorpe [mailto:jgg-uk2M96/[email protected]]
Sent: Tuesday, November 14, 2017 6:18 PM
To: Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; [email protected]; Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Subject: Re: [PATCH rdma-next 01/31] IB/core: Fix do not add RoCEv2 default
GID when IPv6 is disabled

On Tue, Nov 14, 2017 at 02:51:48PM +0200, Leon Romanovsky wrote:

drivers/infiniband/core/cache.c void ib_cache_gid_set_default_gid(struct ib_device *ib_dev, u8 port, · line 680

Quoted 8 lines

drivers/infiniband/core/cache.c +8 -0

+	/* Default GID is created using unique GUID and local subnet prefix,
+	 * as described in section 4.1.1 and 3.5.10 in IB spec 1.3.
+	 * Therefore don't create RoCEv2 default GID based on it that
+	 * resembles as IPv6 GID based on link local address when IPv6 is
+	 * disabled in kernel.
+	 */
+	if (!IS_ENABLED(CONFIG_IPV6))
+		gid_type_mask &= ~BIT(IB_GID_TYPE_ROCE_UDP_ENCAP);

There is also a sysctl to globally disable ipv6, I would think it would be better to check than then a CONFIG symbol?

Currently in rest of the IB stack in cma.c, addr.c we check for IS_ENABLED(CONFIG_IPV6). May be when all of them are changed, this can also shift to honoring sysctl.

-----Original Message-----
From: Jason Gunthorpe [mailto:jgg-uk2M96/[email protected]]
Sent: Tuesday, November 14, 2017 10:41 PM
To: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc: Leon Romanovsky <[email protected]>; Doug Ledford <[email protected]>; [email protected]
Subject: Re: [PATCH rdma-next 01/31] IB/core: Fix do not add RoCEv2 default
GID when IPv6 is disabled

On Wed, Nov 15, 2017 at 03:59:26AM +0000, Parav Pandit wrote:

Quoted 3 lines

Currently in rest of the IB stack in cma.c, addr.c we check for IS_ENABLED(CONFIG_IPV6). May be when all of them are changed, this can also shift to honoring sysctl.

I think most of the rest of the stack is guarding things that would cause linking problems when ipv6 is compiled out, those cannot be changed to sysctl.

Since this clearly should be sysctl based it should start out that way.

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Currently two RoCE default GIDs are added whenever HCA supports RoCEv1 and RoCEv2. Default GIDs are created using unique GUID and well known prefix as defined by the IB spec 1.3 section 4.1.1 and 3.5.10.

For few HCAs GUID is derived from the MAC address of the netdev. Due to this scheme, default RoCEv2 GID resemebles link-local IPv6 based GID. Even when IPv6 is disabled at compile time in kernel, default RoCEv2 GID can get added. When such RoCEv2 GID is used, offload HCA starts using IPv6 packets for QP.
This leads to sending IPv6 packets in network from a host which has IPv6 disabled kernel.

This can be seen with below simple test.
ib_send_bw -x 1 -d mlx5_1
ib_send_bw -x 1 -d mlx5_1 <ip_v4_addr>

It may not be a good idea to send/receive IPv6 packets on kernel which has IPv6 disabled.

Therefore lets honor networking stack's IPv6 disabled setting. This patch avoids adding RoCEv2 based default GID when IPv6 is disabled in kernel.

ibv_devinfo -v output can be seen as below on IPv6 disabled kernel as below with one IPv4 address assign to netdevice.

GID[  0]: fe80:0000:0000:0000:268a:07ff:fe55:4661
GID[  2]: 0000:0000:0000:0000:0000:ffff:c3a8:0124
GID[  3]: 0000:0000:0000:0000:0000:ffff:c3a8:0124

This also ensures that any RoCE specific legacy application who has assumed GID indexes for IP addresses start from 2, still continue to have such IP based GIDs assigned from start index of 2.

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

Quoted 3 lines · Leon Romanovsky

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

BTW, Leon when you send these patches please make sure the Reviewer's are CC'd as they should see and have a chance to respond to review comments.. I added Eli manually in this case..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

drivers/infiniband/core/verbs.c +15 -0

+static int ib_resolve_unicast_gid_dmac(struct ib_device *device,
+				       struct rdma_ah_attr *ah_attr)
+{
+	struct ib_gid_attr sgid_attr;
+	struct ib_global_route *grh;
+	int hop_limit = 0xff;
+	union ib_gid sgid;
+	int ret;
+
+	grh = rdma_ah_retrieve_grh(ah_attr);
+
+	ret = ib_query_gid(device,
+			   rdma_ah_get_port_num(ah_attr),
+			   grh->sgid_index,
+			   &sgid, &sgid_attr);

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Currently ib_resolve_eth_dmac() bypasses IPv6 stack for resolving link local destination mac address and translates the IPv6 link local GID to MAC address, which is not always correct.
Static link local neighbour entry can be setup using
ip -6 neigh add <link_local_addr> lladdr <mac_addr> dev eth0 Current code bypasses such administratively set entries and IPV6 stack.

-----Original Message-----
From: Jason Gunthorpe [mailto:jgg-uk2M96/[email protected]]
Sent: Tuesday, November 14, 2017 6:33 PM
To: Leon Romanovsky <[email protected]>
Cc: Doug Ledford <[email protected]>; [email protected]; Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Subject: Re: [PATCH rdma-next 03/31] IB/core: Depend on IPv6 stack to resolve
link local address

Quoted 8 lines · Leon Romanovsky

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Currently ib_resolve_eth_dmac() bypasses IPv6 stack for resolving link local destination mac address and translates the IPv6 link local GID to MAC address, which is not always correct.
Static link local neighbour entry can be setup using ip -6 neigh add <link_local_addr> lladdr <mac_addr> dev eth0 Current code bypasses such administratively set entries and IPV6 stack.

At the very least this description makes little sense.

The code is inspecting a DGID, and when in rocev1 mode, the DGID is

*NOT* an IPv6 address and the correct thing to do is what it is doing now.

In rocev2 mode the DGID becomes and IPv6 address and you should do ND.

Has this distinction been captured in the patch?

Jason

Quoted 1 line

Has this distinction been captured in the patch?

Yes. When SGID type is RoCEv2, it performs ARP/ND.

This is described in commit log - it uses
rdma_addr_find_l2_eth_by_grh() which performs ND.

-----Original Message-----
From: Jason Gunthorpe [mailto:jgg-uk2M96/[email protected]]
Sent: Tuesday, November 14, 2017 10:41 PM
To: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc: Leon Romanovsky <[email protected]>; Doug Ledford <[email protected]>; [email protected]
Subject: Re: [PATCH rdma-next 03/31] IB/core: Depend on IPv6 stack to resolve
link local address

Quoted 1 line · Parav Pandit

Quoted 1 line

Has this distinction been captured in the patch?

Quoted 1 line

Yes. When SGID type is RoCEv2, it performs ARP/ND.

Good then..

Quoted 2 lines

This is described in commit log - it uses
rdma_addr_find_l2_eth_by_grh() which performs ND.

I find your commit messages very hard to understand.

From: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

If from some reason the SM responses to path query request with response that doesn't contain the exact SGID, ipoib should warn and change that part of the response before push it to the path record DB. Otherwise, new record will be added to the path record DB with no access to them from the ipoib.

Demonstration of the bug is as the follow:

Quoted 8 lines · Leon Romanovsky

From: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

If from some reason the SM responses to path query request with response that doesn't contain the exact SGID, ipoib should warn and change that part of the response before push it to the path record DB. Otherwise, new record will be added to the path record DB with no access to them from the ipoib.

Quoted 1 line

Demonstration of the bug is as the follow:

this series contains bunch of fixes, any reason to direct them to -next (4.16) and not -rc (4.15)

Also, in most of the cases ppl didn't provide a Fixes: line with the offending patch, any reason to skip that?

From: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

If from some reason the SM responses to path query request with response that doesn't contain the exact SGID, ipoib should warn and change that part of the response before push it to the path record DB. Otherwise, new record will be added to the path record DB with no access to them from the ipoib.

Demonstration of the bug is as the follow:
ipoib wants to send to GID fe80:0000:0000:0000:0002:c903:00ef:5ee2, it creates new record in the DB with that gid as a key, and issues a new request to the sm.
Now, the SM from some reason returns path-record with other SGID (for example, fe80:0000:0000:0001:0002:c903:00ef:5ee2 that contains the local subnet prefix) now ipoib will overwrite the current entry with the new one, and if new request to the original GID arrives ipoib will not find it in the DB (was overwritten) and will create new record that in its turn will also be overwritten by the response from the SM, and so on till the driver eats all the device memory.

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

 drivers/infiniband/ulp/ipoib/ipoib_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/infiniband/ulp/ipoib/ipoib_main.c +1 -0

index 12b7f911f0e5..b173d618c59c 100644
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c

@@ -775,6 +775,16 @@ static void path_rec_completion(int status,
 	spin_lock_irqsave(&priv->lock, flags);

Quoted 31 lines · Leon Romanovsky

From: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

If from some reason the SM responses to path query request with response that doesn't contain the exact SGID, ipoib should warn and change that part of the response before push it to the path record DB. Otherwise, new record will be added to the path record DB with no access to them from the ipoib.

Demonstration of the bug is as the follow:
ipoib wants to send to GID fe80:0000:0000:0000:0002:c903:00ef:5ee2, it creates new record in the DB with that gid as a key, and issues a new request to the sm.
Now, the SM from some reason returns path-record with other SGID (for example, fe80:0000:0000:0001:0002:c903:00ef:5ee2 that contains the local subnet prefix) now ipoib will overwrite the current entry with the new one, and if new request to the original GID arrives ipoib will not find it in the DB (was overwritten) and will create new record that in its turn will also be overwritten by the response from the SM, and so on till the driver eats all the device memory.

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

 drivers/infiniband/ulp/ipoib/ipoib_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/infiniband/ulp/ipoib/ipoib_main.c +1 -0

index 12b7f911f0e5..b173d618c59c 100644
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c

@@ -775,6 +775,16 @@ static void path_rec_completion(int status,
      spin_lock_irqsave(&priv->lock, flags);
 

drivers/infiniband/ulp/ipoib/ipoib_main.c static void path_rec_completion(int status, · line 785

Quoted 9 lines

drivers/infiniband/ulp/ipoib/ipoib_main.c +8 -0

      if (!IS_ERR_OR_NULL(ah)) {
+             /* check there is no mismatch from the request */
+             if (memcmp(pathrec->dgid.raw, path->pathrec.dgid.raw,
+                        sizeof(union ib_gid))) {
+                     pr_warn("%s got PathRec for gid %pI6 while asked for %pI6\n",
+                             dev->name, pathrec->dgid.raw, path->pathrec.dgid.raw);
+                     /* overwrite the response from the sm  before copy to the db */
+                     memcpy(pathrec->dgid.raw, path->pathrec.dgid.raw,
+                            sizeof(union ib_gid));

This doesn't seem like it should be a warning, and replacing the good DGID from the SA with local garbage seems really wrong.

The SA should be free to return a correct DGID, for instance by replacing a old or link local subnet prefix with the current value of the subnet prefix, or by returning a different gid index for the destination depending on mulitpath requirements.

So the right thing to do here is to somehow make ipoib use the data from the SA.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines

Quoted 9 lines

      if (!IS_ERR_OR_NULL(ah)) {

drivers/infiniband/ulp/ipoib/ipoib_main.c +8 -0

+             /* check there is no mismatch from the request */
+             if (memcmp(pathrec->dgid.raw, path->pathrec.dgid.raw,
+                        sizeof(union ib_gid))) {
+                     pr_warn("%s got PathRec for gid %pI6 while asked for %pI6\n",
+                             dev->name, pathrec->dgid.raw, path->pathrec.dgid.raw);
+                     /* overwrite the response from the sm  before copy to the db */
+                     memcpy(pathrec->dgid.raw, path->pathrec.dgid.raw,
+                            sizeof(union ib_gid));

Quoted 2 lines

This doesn't seem like it should be a warning, and replacing the good DGID from the SA with local garbage seems really wrong.

IPoIB is L2 driver and should act as a pipe and cannot add its opinion about what is a good or a garbage GID.

If the host is asking for specific mac address, IPoIB should return the response for the original request/mac and should not decide what is good for the host.

Quoted 10 lines · Erez Shitrit

Quoted 10 lines

Quoted 9 lines

      if (!IS_ERR_OR_NULL(ah)) {

(quoted diff) +8 -0

+             /* check there is no mismatch from the request */
+             if (memcmp(pathrec->dgid.raw, path->pathrec.dgid.raw,
+                        sizeof(union ib_gid))) {
+                     pr_warn("%s got PathRec for gid %pI6 while asked for %pI6\n",
+                             dev->name, pathrec->dgid.raw, path->pathrec.dgid.raw);
+                     /* overwrite the response from the sm  before copy to the db */
+                     memcpy(pathrec->dgid.raw, path->pathrec.dgid.raw,
+                            sizeof(union ib_gid));

Quoted 5 lines

Quoted 2 lines

This doesn't seem like it should be a warning, and replacing the good DGID from the SA with local garbage seems really wrong.

IPoIB is L2 driver and should act as a pipe and cannot add its opinion about what is a good or a garbage GID.

Which is why ipoib should not override the result from the SA.

It needs to work in a way where it looks up a DGID from the LLADDR and gets back a wire GRH and LRH which come from SA.

Quoted 4 lines

If the host is asking for specific mac address, IPoIB should return the response for the original request/mac and should not decide what is good for the host.

No, it is up to the SA to return the GRH to put in the packets, it should never be changed in any way by a ULP.

Jason

Quoted 17 lines · Jason Gunthorpe

Quoted 15 lines · Erez Shitrit

Quoted 10 lines

Review

Quoted 9 lines

     if (!IS_ERR_OR_NULL(ah)) {

(quoted diff) +8 -0

+             /* check there is no mismatch from the request */
+             if (memcmp(pathrec->dgid.raw, path->pathrec.dgid.raw,
+                        sizeof(union ib_gid))) {
+                     pr_warn("%s got PathRec for gid %pI6 while asked for %pI6\n",
+                             dev->name, pathrec->dgid.raw, path->pathrec.dgid.raw);
+                     /* overwrite the response from the sm  before copy to the db */
+                     memcpy(pathrec->dgid.raw, path->pathrec.dgid.raw,
+                            sizeof(union ib_gid));

Quoted 2 lines

This doesn't seem like it should be a warning, and replacing the good DGID from the SA with local garbage seems really wrong.

IPoIB is L2 driver and should act as a pipe and cannot add its opinion about what is a good or a garbage GID.

Quoted 1 line

Which is why ipoib should not override the result from the SA.

It override only the DGID in the record, and that for IPoIB uses only, not the ah which is the important.
otherwise if the host will re-ask again for that DGID it will not be found in IPoIB database, and new record will be created and so on till the end of all the device memory.

Quoted 4 lines

It needs to work in a way where it looks up a DGID from the LLADDR and gets back a wire GRH and LRH which come from SA.

For that there is the ah object, this object is used as it came from the SA.

Quoted 7 lines

Quoted 4 lines

If the host is asking for specific mac address, IPoIB should return the response for the original request/mac and should not decide what is good for the host.

No, it is up to the SA to return the GRH to put in the packets, it should never be changed in any way by a ULP.

The change is only for the ULP, the user/host has no idea of that, and it gets the same ah that came from the SA.

Quoted 1 line

Quoted 1 line

Jason

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

It override only the DGID in the record, and that for IPoIB uses only, not the ah which is the important. otherwise if the host will re-ask again for that DGID it will not be found in IPoIB database, and new record will be created and so on till the end of all the device memory.

Isn’t the correct solution to mask away the 16-bit subnet prefix when looking up a unicast DGID in the database?

Quoted 2 lines

It needs to work in a way where it looks up a DGID from the LLADDR and gets back a wire GRH and LRH which come from SA.

For that there is the ah object, this object is used as it came from the SA.

From: Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

If from some reason the SM responses to path query request with response that doesn't contain the exact SGID, ipoib should warn and change that part of the response before push it to the path record DB. Otherwise, new record will be added to the path record DB with no access to them from the ipoib.

Demonstration of the bug is as the follow:
ipoib wants to send to GID fe80:0000:0000:0000:0002:c903:00ef:5ee2, it creates new record in the DB with that gid as a key, and issues a new request to the sm.
Now, the SM from some reason returns path-record with other SGID (for example, fe80:0000:0000:0001:0002:c903:00ef:5ee2 that contains the local subnet prefix) now ipoib will overwrite the current entry with the new one, and if new request to the original GID arrives ipoib will not find it in the DB (was overwritten) and will create new record that in its turn will also be overwritten by the response from the SM, and so on till the driver eats all the device memory.

Signed-off-by Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

 drivers/infiniband/ulp/ipoib/ipoib_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/infiniband/ulp/ipoib/ipoib_main.c +2 -0

index 12b7f911f0e5..b173d618c59c 100644
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c

@@ -775,6 +775,16 @@ static void path_rec_completion(int status,
 	spin_lock_irqsave(&priv->lock, flags);
 
 	if (!IS_ERR_OR_NULL(ah)) {
+		/* check there is no mismatch from the request */

Quoted 13 lines · Leon Romanovsky

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

While copying destination address in statistics, consider size of destination address.

Fixes ce117ffac2e9 ("RDMA/cma: Export AF_IB statistics")

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/infiniband/core/cma.c static int cma_get_id_stats(struct sk_buff *skb, struct netlink_callback *cb)

Quoted 5 lines

drivers/infiniband/core/cma.c +0 -0

index 1fdb473b5df7..9841e7ca5c06 100644
@@ -4431,7 +4431,7 @@ static int cma_get_id_stats(struct sk_buff *skb, struct netlink_callback *cb)

Leon, isn't that the code you want to remove in your other patch?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/[email protected]>

The rq/sq->psn is 24 bits as defined in the IB spec,therefore we mask out the 8 most significant bits to avoid overflow in modify qp.

Signed-off-by Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/verbs.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

drivers/infiniband/core/verbs.c +6 -0

index 75ebd74f8bbd..584b6029cb44 100644
@@ -1321,6 +1321,7 @@ static int ib_resolve_eth_dmac(struct ib_device *device,
 int ib_modify_qp_with_udata(struct ib_qp *qp, struct ib_qp_attr *attr,
                            int attr_mask, struct ib_udata *udata)
 {
+       u8 port = attr_mask & IB_QP_PORT ? attr->port_num : qp->port;
        int ret;
 
        if (attr_mask & IB_QP_AV) {

@@ -1328,6 +1329,21 @@ int ib_modify_qp_with_udata(struct ib_qp *qp, struct ib_qp_attr *attr,
                if (ret)
                        return ret;
        }
+
+       if (rdma_ib_or_roce(qp->device, port)) {
+               if (attr_mask & IB_QP_RQ_PSN && attr->rq_psn & ~0xffffff) {
+                       pr_warn("%s: %s rq_psn overflow, masking to 24 bits\n",
+                               __func__, qp->device->name);

drivers/infiniband/core/verbs.c +0 -1

        ret = ib_security_modify_qp(qp, attr, attr_mask, udata);
        if (!ret && (attr_mask & IB_QP_PORT))
                qp->port = attr->port_num;
--

2.15.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 5 lines

drivers/infiniband/core/verbs.c +5 -0

+
+       if (rdma_ib_or_roce(qp->device, port)) {
+               if (attr_mask & IB_QP_RQ_PSN && attr->rq_psn & ~0xffffff) {
+                       pr_warn("%s: %s rq_psn overflow, masking to 24 bits\n",
+                               __func__, qp->device->name);

drivers/infiniband/core/verbs.c int ib_modify_qp_with_udata(struct ib_qp *qp, struct ib_qp_attr *attr, · line 1346

Quoted 10 lines

drivers/infiniband/core/verbs.c +10 -0

+                       attr->rq_psn &= 0xffffff;
+               }
+
+               if (attr_mask & IB_QP_SQ_PSN && attr->sq_psn & ~0xffffff) {
+                       pr_warn("%s: %s sq_psn overflow, masking to 24 bits\n",
+                               __func__, qp->device->name);
+                       attr->sq_psn &= 0xffffff;
+               }
+       }
+

Isn't this what we tried to push in the past and Jason/Sean rejected [1]?

[1] https://marc.info/?l=linux-rdma&m=142309249413868&w=2

From: Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/[email protected]>

The rq/sq->psn is 24 bits as defined in the IB spec,therefore we mask out the 8 most significant bits to avoid overflow in modify qp.

Signed-off-by Majd Dibbiny <majd-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

 drivers/infiniband/core/verbs.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

In cm_req_handler error flows, sometimes cm_id_priv->timewait_info isn't free'd.

From: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Add debug prints to the error paths in the connection manager control flows, to help debug connection management problems.

Signed-off-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cm.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

drivers/infiniband/core/cm.c +51 -1

index fe7a5e0d79b0..cbb385e02947 100644
@@ -1841,6 +1841,8 @@ static int cm_req_handler(struct cm_work *work)
 
        listen_cm_id_priv = cm_match_req(work, cm_id_priv);
        if (!listen_cm_id_priv) {
+               pr_debug("%s: local_id %d, no listen_cm_id_priv\n", __func__,
+                        be32_to_cpu(cm_id->local_id));
                ret = -EINVAL;
                goto free_timeinfo;
        }

@@ -1998,6 +2000,8 @@ int ib_send_cm_rep(struct ib_cm_id *cm_id,
        spin_lock_irqsave(&cm_id_priv->lock, flags);
        if (cm_id->state != IB_CM_REQ_RCVD &&
            cm_id->state != IB_CM_MRA_REQ_SENT) {
+               pr_debug("%s: local_comm_id %d, cm_id->state: %d\n", __func__,
+                        be32_to_cpu(cm_id_priv->id.local_id), cm_id->state);
                ret = -EINVAL;
                goto out;
        }

@@ -2064,6 +2068,8 @@ int ib_send_cm_rtu(struct ib_cm_id *cm_id,
        spin_lock_irqsave(&cm_id_priv->lock, flags);
        if (cm_id->state != IB_CM_REP_RCVD &&
            cm_id->state != IB_CM_MRA_REP_SENT) {
+               pr_debug("%s: local_id %d, cm_id->state %d\n", __func__,
+                        be32_to_cpu(cm_id->local_id), cm_id->state);
                ret = -EINVAL;
                goto error;
        }

@@ -2171,6 +2177,8 @@ static int cm_rep_handler(struct cm_work *work)
        cm_id_priv = cm_acquire_id(rep_msg->remote_comm_id, 0);
        if (!cm_id_priv) {
                cm_dup_rep_handler(work);
+               pr_debug("%s: remote_comm_id %d, no cm_id_priv\n", __func__,
+                        be32_to_cpu(rep_msg->remote_comm_id));
                return -EINVAL;
        }
 

@@ -2184,6 +2192,10 @@ static int cm_rep_handler(struct cm_work *work)
        default:
                spin_unlock_irq(&cm_id_priv->lock);
                ret = -EINVAL;
+               pr_debug("%s: cm_id_priv->id.state: %d, local_comm_id %d, remote_comm_id %d\n",
+                        __func__, cm_id_priv->id.state,
+                        be32_to_cpu(rep_msg->local_comm_id),
+                        be32_to_cpu(rep_msg->remote_comm_id));
                goto error;
        }
 

@@ -2197,6 +2209,8 @@ static int cm_rep_handler(struct cm_work *work)
                spin_unlock(&cm.lock);
                spin_unlock_irq(&cm_id_priv->lock);
                ret = -EINVAL;
+               pr_debug("%s: Failed to insert remote id %d\n", __func__,
+                        be32_to_cpu(rep_msg->remote_comm_id));
                goto error;
        }
        /* Check for a stale connection. */

@@ -2214,6 +2228,10 @@ static int cm_rep_handler(struct cm_work *work)
                             IB_CM_REJ_STALE_CONN, CM_MSG_RESPONSE_REP,
                             NULL, 0);
                ret = -EINVAL;
+               pr_debug("%s: Stale connection. local_comm_id %d, remote_comm_id %d\n",
+                        __func__, be32_to_cpu(rep_msg->local_comm_id),
+                        be32_to_cpu(rep_msg->remote_comm_id));
+
                if (cur_cm_id_priv) {
                        cm_id = &cur_cm_id_priv->id;
                        ib_send_cm_dreq(cm_id, NULL, 0);

@@ -2360,6 +2378,8 @@ int ib_send_cm_dreq(struct ib_cm_id *cm_id,
        cm_id_priv = container_of(cm_id, struct cm_id_private, id);
        spin_lock_irqsave(&cm_id_priv->lock, flags);
        if (cm_id->state != IB_CM_ESTABLISHED) {
+               pr_debug("%s: local_id %d, cm_id->state: %d\n", __func__,
+                        be32_to_cpu(cm_id->local_id), cm_id->state);
                ret = -EINVAL;
                goto out;
        }

@@ -2429,6 +2449,8 @@ int ib_send_cm_drep(struct ib_cm_id *cm_id,
        if (cm_id->state != IB_CM_DREQ_RCVD) {
                spin_unlock_irqrestore(&cm_id_priv->lock, flags);
                kfree(data);
+               pr_debug("%s: local_id %d, cm_idcm_id->state(%d) != IB_CM_DREQ_RCVD\n",
+                        __func__, be32_to_cpu(cm_id->local_id), cm_id->state);
                return -EINVAL;
        }
 

@@ -2494,6 +2516,9 @@ static int cm_dreq_handler(struct cm_work *work)
                atomic_long_inc(&work->port->counter_group[CM_RECV_DUPLICATES].
                                counter[CM_DREQ_COUNTER]);
                cm_issue_drep(work->port, work->mad_recv_wc);
+               pr_debug("%s: no cm_id_priv, local_comm_id %d, remote_comm_id %d\n",
+                        __func__, be32_to_cpu(dreq_msg->local_comm_id),
+                        be32_to_cpu(dreq_msg->remote_comm_id));
                return -EINVAL;
        }
 

@@ -2536,6 +2561,9 @@ static int cm_dreq_handler(struct cm_work *work)
                                counter[CM_DREQ_COUNTER]);
                goto unlock;
        default:
+               pr_debug("%s: local_id %d, cm_id_priv->id.state: %d\n",
+                        __func__, be32_to_cpu(cm_id_priv->id.local_id),
+                        cm_id_priv->id.state);
                goto unlock;
        }
        cm_id_priv->id.state = IB_CM_DREQ_RCVD;

@@ -2639,6 +2667,8 @@ int ib_send_cm_rej(struct ib_cm_id *cm_id,
                cm_enter_timewait(cm_id_priv);
                break;
        default:
+               pr_debug("%s: local_id %d, cm_id->state: %d\n", __func__,
+                        be32_to_cpu(cm_id_priv->id.local_id), cm_id->state);
                ret = -EINVAL;
                goto out;
        }

@@ -2749,6 +2779,9 @@ static int cm_rej_handler(struct cm_work *work)
                /* fall through */
        default:
                spin_unlock_irq(&cm_id_priv->lock);
+               pr_debug("%s: local_id %d, cm_id_priv->id.state: %d\n",
+                        __func__, be32_to_cpu(cm_id_priv->id.local_id),
+                        cm_id_priv->id.state);
                ret = -EINVAL;
                goto out;
        }

@@ -2812,6 +2845,9 @@ int ib_send_cm_mra(struct ib_cm_id *cm_id,
                }
                /* fall through */
        default:
+               pr_debug("%s: local_id %d, cm_id_priv->id.state: %d\n",
+                        __func__, be32_to_cpu(cm_id_priv->id.local_id),
+                        cm_id_priv->id.state);
                ret = -EINVAL;
                goto error1;
        }

@@ -2913,6 +2949,9 @@ static int cm_mra_handler(struct cm_work *work)
                                counter[CM_MRA_COUNTER]);
                /* fall through */
        default:
+               pr_debug("%s local_id %d, cm_id_priv->id.state: %d\n",
+                        __func__, be32_to_cpu(cm_id_priv->id.local_id),
+                        cm_id_priv->id.state);
                goto out;
        }
 

@@ -3693,6 +3732,7 @@ static void cm_work_handler(struct work_struct *_work)
                ret = cm_timewait_handler(work);
                break;
        default:
+               pr_debug("cm_event.event: 0x%x\n", work->cm_event.event);
                ret = -EINVAL;
                break;
        }

@@ -3728,6 +3768,8 @@ static int cm_establish(struct ib_cm_id *cm_id)
                ret = -EISCONN;
                break;
        default:
+               pr_debug("%s: local_id %d, cm_id->state: %d\n", __func__,
+                        be32_to_cpu(cm_id->local_id), cm_id->state);
                ret = -EINVAL;
                break;
        }

@@ -3925,6 +3967,9 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
                ret = 0;
                break;
        default:
+               pr_debug("%s: local_id %d, cm_id_priv->id.state: %d\n",
+                        __func__, be32_to_cpu(cm_id_priv->id.local_id),
+                        cm_id_priv->id.state);
                ret = -EINVAL;
                break;
        }

@@ -3972,6 +4017,9 @@ static int cm_init_qp_rtr_attr(struct cm_id_private *cm_id_priv,
                ret = 0;
                break;
        default:
+               pr_debug("%s: local_id %d, cm_id_priv->id.state: %d\n",
+                        __func__, be32_to_cpu(cm_id_priv->id.local_id),
+                        cm_id_priv->id.state);
                ret = -EINVAL;
                break;
        }

@@ -4031,6 +4079,9 @@ static int cm_init_qp_rts_attr(struct cm_id_private *cm_id_priv,
                ret = 0;
                break;
        default:
+               pr_debug("%s: local_id %d, cm_id_priv->id.state: %d\n",
+                        __func__, be32_to_cpu(cm_id_priv->id.local_id),
+                        cm_id_priv->id.state);
                ret = -EINVAL;
                break;
        }
--

2.15.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Current code checks for NULL ndev twice where 2nd check is always invalid given the fact that during route resolving stage, device address must be bound to netdevice interface.

This patch simplifies such check.

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

drivers/infiniband/core/cma.c +3 -1

index e63a51b68ed5..168f1230756b 100644
@@ -2527,8 +2527,10 @@ static int cma_resolve_iboe_route(struct rdma_id_private
 *id_priv)
 	struct rdma_addr *addr = &route->addr;
 	struct cma_work *work;
 	int ret;
-	struct net_device *ndev = NULL;
+	struct net_device *ndev;
 	enum ib_gid_type gid_type = IB_GID_TYPE_ROCE;
+	unsigned long supported_gids;
+
 	u8 default_roce_tos = id_priv->cma_dev->default_roce_tos[id_priv->id.port_num -
 					rdma_start_port(id_priv->cma_dev->device)];

drivers/infiniband/core/cma.c +13 -18

 	u8 tos = id_priv->tos_set ? id_priv->tos : default_roce_tos;

@@ -2549,30 +2551,25 @@ static int cma_resolve_iboe_route(struct rdma_id_private
 *id_priv)
 
 	route->num_paths = 1;
 
-	if (addr->dev_addr.bound_dev_if) {
-		unsigned long supported_gids;
-
-		ndev = dev_get_by_index(&init_net, addr->dev_addr.bound_dev_if);
-		if (!ndev) {
-			ret = -ENODEV;
-			goto err2;
-		}
-
-		supported_gids = roce_gid_type_mask_support(id_priv->id.device,
-							    id_priv->id.port_num);
-		gid_type = cma_route_gid_type(addr->dev_addr.network,
-					      supported_gids,
-					      id_priv->gid_type);
-		route->path_rec->rec_type =
-			sa_conv_gid_to_pathrec_type(gid_type);
-		sa_path_set_ndev(route->path_rec, &init_net);
-		sa_path_set_ifindex(route->path_rec, ndev->ifindex);
+	if (!addr->dev_addr.bound_dev_if) {
+		ret = -ENODEV;
+		goto err2;
 	}
+
+	ndev = dev_get_by_index(&init_net, addr->dev_addr.bound_dev_if);
 	if (!ndev) {
 		ret = -ENODEV;
 		goto err2;
 	}
 
+	supported_gids = roce_gid_type_mask_support(id_priv->id.device,
+						    id_priv->id.port_num);
+	gid_type = cma_route_gid_type(addr->dev_addr.network,
+				      supported_gids,
+				      id_priv->gid_type);
+	route->path_rec->rec_type = sa_conv_gid_to_pathrec_type(gid_type);
+	sa_path_set_ndev(route->path_rec, &init_net);
+	sa_path_set_ifindex(route->path_rec, ndev->ifindex);
 	sa_path_set_dmac(route->path_rec, addr->dev_addr.dst_dev_addr);
 

rdma_ip2gid((struct sockaddr *)&id_priv->id.route.addr.src_addr,

Quoted 1 line

rdma_ip2gid((struct sockaddr *)&id_priv->id.route.addr.src_addr,

Reviewed-by Knut Omang <[email protected]>

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Avoid setting path record type twice for RoCE.

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/cma.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/infiniband/core/cma.c +4 -0

index 168f1230756b..f986597201fa 100644
@@ -2567,7 +2567,11 @@ static int cma_resolve_iboe_route(struct rdma_id_private
 *id_priv)
 	gid_type = cma_route_gid_type(addr->dev_addr.network,
 				      supported_gids,
 				      id_priv->gid_type);
+	/* Use the hint from IP Stack to select GID Type */
+	if (gid_type < ib_network_to_gid_type(addr->dev_addr.network))
+		gid_type = ib_network_to_gid_type(addr->dev_addr.network);
 	route->path_rec->rec_type = sa_conv_gid_to_pathrec_type(gid_type);
+
 	sa_path_set_ndev(route->path_rec, &init_net);
 	sa_path_set_ifindex(route->path_rec, ndev->ifindex);

drivers/infiniband/core/cma.c +0 -5

 	sa_path_set_dmac(route->path_rec, addr->dev_addr.dst_dev_addr);

@@ -2577,11 +2581,6 @@ static int cma_resolve_iboe_route(struct rdma_id_private
 *id_priv)
 	rdma_ip2gid((struct sockaddr *)&id_priv->id.route.addr.dst_addr,
 		    &route->path_rec->dgid);
 
-	/* Use the hint from IP Stack to select GID Type */
-	if (gid_type < ib_network_to_gid_type(addr->dev_addr.network))
-		gid_type = ib_network_to_gid_type(addr->dev_addr.network);
-	route->path_rec->rec_type = sa_conv_gid_to_pathrec_type(gid_type);
-
 	if (((struct sockaddr *)&id_priv->id.route.addr.dst_addr)->sa_family != AF_IB)
 		/* TODO: get the hoplimit from the inet/inet6 device */

route->path_rec->hop_limit = addr->dev_addr.hoplimit;

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

cm_init_av_by_path depends on ib_init_ah_from_path to initialize ah attribute and ib_init_ah_from_path() can fail, such error should not be ignored.

This patch honors ah attribute init error.

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Quoted 3 lines

From: Or Gerlitz [mailto:[email protected]]

any reason not to point out the commit that introduced the bug?

No particular reason. I am not sure few years back it was bug or not. In current code it appear so. So I continued without Fixes commit line.

Quoted 6 lines

Quoted 4 lines · Parav Pandit

Quoted 3 lines

From: Or Gerlitz [mailto:[email protected]]

any reason not to point out the commit that introduced the bug?

No particular reason. I am not sure few years back it was bug or not. In current

Quoted 8 lines

code it appear so.

Quoted 1 line

So I continued without Fixes commit line.

The idea is that you do the git blaming and such work to get the Fixes line, and later, the maintainer of the sub-system and maintainers of stable kernel use that to decide if/where they want this to go beyond the -rc kernel you are fixing

Yes, I understand. But would that be a bug in initial commit, which transport was it handling.. without error code check what could have gone wrong.. I do not have answer for those questions. So I omitted the Fixes line. Wherever I knew for sure in this or other series, I have added.

Quoted 14 lines · Parav Pandit

Quoted 14 lines

Quoted 4 lines · Parav Pandit

Quoted 3 lines

From: Or Gerlitz [mailto:[email protected]]

any reason not to point out the commit that introduced the bug?

No particular reason. I am not sure few years back it was bug or not. In current

code it appear so.

Quoted 1 line

So I continued without Fixes commit line.

The idea is that you do the git blaming and such work to get the Fixes line, and later, the maintainer of the sub-system and maintainers of stable kernel use that to decide if/where they want this to go beyond the -rc kernel you are fixing

Quoted 5 lines

Yes, I understand. But would that be a bug in initial commit, which transport was it handling.. without error code check what could have gone wrong.. I do not have answer for those questions. So I omitted the Fixes line. Wherever I knew for sure in this or other series, I have added.

While the fixes line is nice, the more important question is if any of these patches should have a 'Cc: [email protected]' ?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 23 lines · Jason Gunthorpe

Quoted 20 lines · Parav Pandit

Quoted 6 lines

Quoted 4 lines · Parav Pandit

Quoted 3 lines

From: Or Gerlitz [mailto:[email protected]]

any reason not to point out the commit that introduced the bug?

No particular reason. I am not sure few years back it was bug or not. In current

Quoted 8 lines

code it appear so.

Quoted 1 line

So I continued without Fixes commit line.

The idea is that you do the git blaming and such work to get the Fixes line, and later, the maintainer of the sub-system and maintainers of stable kernel use that to decide if/where they want this to go beyond the -rc kernel you are fixing

Yes, I understand. But would that be a bug in initial commit, which transport was it handling.. without error code check what could have gone wrong.. I do not have answer for those questions. So I omitted the Fixes line. Wherever I knew for sure in this or other series, I have added.

Quoted 2 lines

While the fixes line is nice, the more important question is if any of these patches should have a 'Cc: [email protected]' ?

No, this is why I didn't ask to add Fixes line, because stable@ takes patches in semi-automatic mode by looking after Fixes line.

Quoted 24 lines · Leon Romanovsky

Quoted 23 lines · Jason Gunthorpe

Quoted 20 lines · Parav Pandit

Quoted 6 lines

Quoted 4 lines · Parav Pandit

Quoted 3 lines

From: Or Gerlitz [mailto:[email protected]]

any reason not to point out the commit that introduced the bug?

No particular reason. I am not sure few years back it was bug or not. In current

Quoted 8 lines

code it appear so.

Quoted 1 line

So I continued without Fixes commit line.

The idea is that you do the git blaming and such work to get the Fixes line, and later, the maintainer of the sub-system and maintainers of stable kernel use that to decide if/where they want this to go beyond the -rc kernel you are fixing

Yes, I understand. But would that be a bug in initial commit, which transport was it handling.. without error code check what could have gone wrong.. I do not have answer for those questions. So I omitted the Fixes line. Wherever I knew for sure in this or other series, I have added.

Quoted 5 lines

Quoted 2 lines

While the fixes line is nice, the more important question is if any of these patches should have a 'Cc: [email protected]' ?

No, this is why I didn't ask to add Fixes line, because stable@ takes patches in semi-automatic mode by looking after Fixes line.

And what's wrong with that? IMHO the right thing to do for fixes is

(1) putng the Fixes line
(2) cc the author of the offending patch

This way you can get them a chance to review the fix and point out to you problems in the patch that may e.g introduce a 2nd problem or a side effect you don't envision. This is what we do on netdev, practically always.

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

In case of LAP are used for RoCE

Quoted 5 lines

From: Or Gerlitz [mailto:[email protected]]
Subject: Re: [PATCH rdma-next 28/31] IB/cm: Fix avoid sleep while spin lock is

Quoted 3 lines

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

In case of LAP are used for RoCE

Quoted 2 lines

AFAIK APM is not supported with RoCE, isn't that? if yes, on what HW/RoCE mode and how do you test that?

This particular patch in this series is not tested on RoCE.
APM is not supported but injecting such frame in network is not impossible.
We don’t have particular checks to reject such frame for RoCE.
So I prefer to fix the issue before getting such hang/oops report when issue is evident in code.

Quoted 13 lines

From: Or Gerlitz [mailto:[email protected]]

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

In case of LAP are used for RoCE

AFAIK APM is not supported with RoCE, isn't that? if yes, on what HW/RoCE mode and how do you test that?

This particular patch in this series is not tested on RoCE.
APM is not supported but injecting such frame in network is not impossible. We don’t have particular checks to reject such frame for RoCE.
So I prefer to fix the issue before getting such hang/oops report when issue is

evident in code.

Quoted 5 lines

mentioning testing, in this series you did bunch of fixes for the IB core, CM and CMA.

Would be good to add a section in the cover letter mentioning under which type of testing the modified code has gone through.

It's been tested for RoCEv1, v2, IPoIB with ConnectX4, ConnectX3 HCA with perftest, rping, krping tools. In few individual patch, it's been mentioned how is this tested.

Quoted 35 lines

-----Original Message-----
From: Or Gerlitz [mailto:[email protected]]
Sent: Wednesday, November 15, 2017 1:53 AM
To: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc: Doug Ledford <[email protected]>; [email protected]; Hefty, Sean <[email protected]>; Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
Subject: Re: [PATCH rdma-next 28/31] IB/cm: Fix avoid sleep while spin lock is
held

Quoted 14 lines · Parav Pandit

Quoted 14 lines

From: Or Gerlitz [mailto:[email protected]]

Quoted 10 lines

Quoted 4 lines

Quoted 1 line

In case of LAP are used for RoCE

AFAIK APM is not supported with RoCE, isn't that? if yes, on what HW/RoCE mode and how do you test that?

This particular patch in this series is not tested on RoCE.
APM is not supported but injecting such frame in network is not impossible. We don’t have particular checks to reject such frame for RoCE. So I prefer to fix the issue before getting such hang/oops report when issue is

evident in code.

Quoted 7 lines

Quoted 5 lines

mentioning testing, in this series you did bunch of fixes for the IB core, CM and CMA.

Would be good to add a section in the cover letter mentioning under which type of testing the modified code has gone through.

It's been tested for RoCEv1, v2, IPoIB with ConnectX4, ConnectX3 HCA with

perftest, rping, krping tools.

Quoted 1 line

In few individual patch, it's been mentioned how is this tested.

Quoted 2 lines

Please also add more info (and in the cover letter please) on IPv4 vs IPv6, unicast vs multicast, etc.

Is there a template that you can share that linux-rdma follows that I can use to mention what tests are done? Any example past cover letters that I can refer as reference?

Depending on how much information you expect in cover letter, this patchset can have many revisions just for cover letter. So it would be best for you to share the template that I can fill in for v1.

Quoted 3 lines

You touched the CMA and addr modules quite heavily on all these areas, need to cover them in testing.

Which are those tests that I should execute apart from above summary?

Quoted 3 lines

did you test APM? I don't think it would be correct to change the CM APM code without proper testing.

As I already told this particular patch is not tested. All others are needless to say tested. I do not have APM setup to test this fix.
I am ok to drop this patch and provide fix when crash occurs later on.

Quoted 4 lines

From: Or Gerlitz [mailto:[email protected]]

Please also add more info (and in the cover letter please) on IPv4 vs IPv6, unicast vs multicast, etc.

Is there a template that you can share that linux-rdma follows that I can use to mention what tests are done? Any example past cover letters that I can refer as reference?

Depending on how much information you expect in cover letter, this patchset can have many revisions just for cover letter. So it would be best for you to share the template that I can fill in for v1.

Quoted 2 lines

You touched the CMA and addr modules quite heavily on all these areas, need to cover them in testing.

Which are those tests that I should execute apart from above summary?

Quoted 2 lines

did you test APM? I don't think it would be correct to change the CM APM code without proper testing.

As I already told this particular patch is not tested. All others are needless to say tested. I do not have APM setup to test this fix.
I am ok to drop this patch and provide fix when crash occurs later on.

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

In case of LAP are used for RoCE, it can lead to a problem of sleeping a context while spin lock is held in below flow.

cm_lap_handler

	->spin_lock
	-> <..switch_case..>
	-> cm_init_av_for_response
		-> ib_init_ah_from_wc
			-> rdma_addr_find_l2_eth_by_grh
				wait_for_completion()

Therefore ah attribute initialization is done for incoming lap requests outside of the lock context.

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

Quoted 13 lines

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

In case of LAP are used for RoCE, it can lead to a problem of sleeping a context while spin lock is held in below flow.

cm_lap_handler

	->spin_lock
	-> <..switch_case..>
	-> cm_init_av_for_response
		-> ib_init_ah_from_wc
			-> rdma_addr_find_l2_eth_by_grh
				wait_for_completion()

Quoted 6 lines

Therefore ah attribute initialization is done for incoming lap requests outside of the lock context.

Signed-off-by Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

Should you add a "Fixes:..."?

Pardon my ignorance, but what is LAP?

Thanks,
Ram

Quoted 1 line · Ram

Pardon my ignorance, but what is LAP?

It should be LAG.