[PATCH rdma-next 0/3] IB/cm refactors

[PATCH rdma-next 0/3] IB/cm refactors

[Mark Zhang]

Hi,

This series provides 2 IB/cm refactors:
- The first 2 patches remove the usage of service_mask, as it is
  always -1 so it doesn't help;
- The third one refines cm_insert_listen() and cm_find_listen().

Link: https://lore.kernel.org/lkml/20220624201733.GA284068@nvidia.com/t/

Thanks.

Mark Zhang (3):
  IB/cm: Remove the service_mask parameter from ib_cm_listen()
  IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter
    of cm_init_listen()
  IB/cm: Refactor cm_insert_listen() and cm_find_listen()

 drivers/infiniband/core/cm.c            | 65 +++++++++----------------
 drivers/infiniband/ulp/ipoib/ipoib_cm.c |  4 +-
 drivers/infiniband/ulp/srpt/ib_srpt.c   |  2 +-
 include/rdma/ib_cm.h                    |  8 +--
 4 files changed, 26 insertions(+), 53 deletions(-)

-- 
2.26.3

[PATCH rdma-next 1/3] IB/cm: Remove the service_mask parameter from ib_cm_listen()

[Mark Zhang]

Remove the service_mask parameter of ib_cm_listen(), as all callers
use 0.

Signed-off-by: Mark Zhang <markzhang@nvidia.com>
---
 drivers/infiniband/core/cm.c            | 8 ++------
 drivers/infiniband/ulp/ipoib/ipoib_cm.c | 4 ++--
 drivers/infiniband/ulp/srpt/ib_srpt.c   | 2 +-
 include/rdma/ib_cm.h                    | 7 +------
 4 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index b985e0d9bc05..b59f864b3d79 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -1185,12 +1185,8 @@ static int cm_init_listen(struct cm_id_private *cm_id_priv, __be64 service_id,
  *   and service ID resolution requests.  The service ID should be specified
  *   network-byte order.  If set to IB_CM_ASSIGN_SERVICE_ID, the CM will
  *   assign a service ID to the caller.
- * @service_mask: Mask applied to service ID used to listen across a
- *   range of service IDs.  If set to 0, the service ID is matched
- *   exactly.  This parameter is ignored if %service_id is set to
- *   IB_CM_ASSIGN_SERVICE_ID.
  */
-int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask)
+int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id)
 {
 	struct cm_id_private *cm_id_priv =
 		container_of(cm_id, struct cm_id_private, id);
@@ -1203,7 +1199,7 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id, __be64 service_mask)
 		goto out;
 	}
 
-	ret = cm_init_listen(cm_id_priv, service_id, service_mask);
+	ret = cm_init_listen(cm_id_priv, service_id, 0);
 	if (ret)
 		goto out;
 
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
index fd9d7f2c4d64..ebb35b809f26 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
@@ -884,8 +884,8 @@ int ipoib_cm_dev_open(struct net_device *dev)
 		goto err_cm;
 	}
 
-	ret = ib_cm_listen(priv->cm.id, cpu_to_be64(IPOIB_CM_IETF_ID | priv->qp->qp_num),
-			   0);
+	ret = ib_cm_listen(priv->cm.id,
+			   cpu_to_be64(IPOIB_CM_IETF_ID | priv->qp->qp_num));
 	if (ret) {
 		pr_warn("%s: failed to listen on ID 0x%llx\n", priv->ca->name,
 			IPOIB_CM_IETF_ID | priv->qp->qp_num);
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 21cbe30d526f..31c60aee00af 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -3191,7 +3191,7 @@ static int srpt_add_one(struct ib_device *device)
 	 * if this HCA is gone bad and replaced by different HCA
 	 */
 	ret = sdev->cm_id ?
-		ib_cm_listen(sdev->cm_id, cpu_to_be64(srpt_service_guid), 0) :
+		ib_cm_listen(sdev->cm_id, cpu_to_be64(srpt_service_guid)) :
 		0;
 	if (ret < 0) {
 		pr_err("ib_cm_listen() failed: %d (cm_id state = %d)\n", ret,
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index e23eb357b761..fbf260c1b1df 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -340,13 +340,8 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id);
  *   and service ID resolution requests.  The service ID should be specified
  *   network-byte order.  If set to IB_CM_ASSIGN_SERVICE_ID, the CM will
  *   assign a service ID to the caller.
- * @service_mask: Mask applied to service ID used to listen across a
- *   range of service IDs.  If set to 0, the service ID is matched
- *   exactly.  This parameter is ignored if %service_id is set to
- *   IB_CM_ASSIGN_SERVICE_ID.
  */
-int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id,
-		 __be64 service_mask);
+int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id);
 
 struct ib_cm_id *ib_cm_insert_listen(struct ib_device *device,
 				     ib_cm_handler cm_handler,
-- 
2.26.3

[PATCH rdma-next 2/3] IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter of cm_init_listen()

[Mark Zhang]

The service_mask is always ~cpu_to_be64(0), so the result is always
a NOP when it is &'d with a service_id. Remove it for simplicity.

Signed-off-by: Mark Zhang <markzhang@nvidia.com>
---
 drivers/infiniband/core/cm.c | 28 ++++++++--------------------
 include/rdma/ib_cm.h         |  1 -
 2 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index b59f864b3d79..84bb10799467 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -617,7 +617,6 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv,
 	struct rb_node *parent = NULL;
 	struct cm_id_private *cur_cm_id_priv;
 	__be64 service_id = cm_id_priv->id.service_id;
-	__be64 service_mask = cm_id_priv->id.service_mask;
 	unsigned long flags;
 
 	spin_lock_irqsave(&cm.lock, flags);
@@ -625,8 +624,7 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv,
 		parent = *link;
 		cur_cm_id_priv = rb_entry(parent, struct cm_id_private,
 					  service_node);
-		if ((cur_cm_id_priv->id.service_mask & service_id) ==
-		    (service_mask & cur_cm_id_priv->id.service_id) &&
+		if ((service_id == cur_cm_id_priv->id.service_id) &&
 		    (cm_id_priv->id.device == cur_cm_id_priv->id.device)) {
 			/*
 			 * Sharing an ib_cm_id with different handlers is not
@@ -670,8 +668,7 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device,
 
 	while (node) {
 		cm_id_priv = rb_entry(node, struct cm_id_private, service_node);
-		if ((cm_id_priv->id.service_mask & service_id) ==
-		     cm_id_priv->id.service_id &&
+		if ((service_id == cm_id_priv->id.service_id) &&
 		    (cm_id_priv->id.device == device)) {
 			refcount_inc(&cm_id_priv->refcount);
 			return cm_id_priv;
@@ -1158,22 +1155,17 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id)
 }
 EXPORT_SYMBOL(ib_destroy_cm_id);
 
-static int cm_init_listen(struct cm_id_private *cm_id_priv, __be64 service_id,
-			  __be64 service_mask)
+static int cm_init_listen(struct cm_id_private *cm_id_priv, __be64 service_id)
 {
-	service_mask = service_mask ? service_mask : ~cpu_to_be64(0);
-	service_id &= service_mask;
 	if ((service_id & IB_SERVICE_ID_AGN_MASK) == IB_CM_ASSIGN_SERVICE_ID &&
 	    (service_id != IB_CM_ASSIGN_SERVICE_ID))
 		return -EINVAL;
 
-	if (service_id == IB_CM_ASSIGN_SERVICE_ID) {
+	if (service_id == IB_CM_ASSIGN_SERVICE_ID)
 		cm_id_priv->id.service_id = cpu_to_be64(cm.listen_service_id++);
-		cm_id_priv->id.service_mask = ~cpu_to_be64(0);
-	} else {
+	else
 		cm_id_priv->id.service_id = service_id;
-		cm_id_priv->id.service_mask = service_mask;
-	}
+
 	return 0;
 }
 
@@ -1199,7 +1191,7 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id)
 		goto out;
 	}
 
-	ret = cm_init_listen(cm_id_priv, service_id, 0);
+	ret = cm_init_listen(cm_id_priv, service_id);
 	if (ret)
 		goto out;
 
@@ -1247,7 +1239,7 @@ struct ib_cm_id *ib_cm_insert_listen(struct ib_device *device,
 	if (IS_ERR(cm_id_priv))
 		return ERR_CAST(cm_id_priv);
 
-	err = cm_init_listen(cm_id_priv, service_id, 0);
+	err = cm_init_listen(cm_id_priv, service_id);
 	if (err) {
 		ib_destroy_cm_id(&cm_id_priv->id);
 		return ERR_PTR(err);
@@ -1518,7 +1510,6 @@ int ib_send_cm_req(struct ib_cm_id *cm_id,
 		}
 	}
 	cm_id->service_id = param->service_id;
-	cm_id->service_mask = ~cpu_to_be64(0);
 	cm_id_priv->timeout_ms = cm_convert_to_ms(
 				    param->primary_path->packet_life_time) * 2 +
 				 cm_convert_to_ms(
@@ -2075,7 +2066,6 @@ static int cm_req_handler(struct cm_work *work)
 		cpu_to_be32(IBA_GET(CM_REQ_LOCAL_COMM_ID, req_msg));
 	cm_id_priv->id.service_id =
 		cpu_to_be64(IBA_GET(CM_REQ_SERVICE_ID, req_msg));
-	cm_id_priv->id.service_mask = ~cpu_to_be64(0);
 	cm_id_priv->tid = req_msg->hdr.tid;
 	cm_id_priv->timeout_ms = cm_convert_to_ms(
 		IBA_GET(CM_REQ_LOCAL_CM_RESPONSE_TIMEOUT, req_msg));
@@ -3482,7 +3472,6 @@ int ib_send_cm_sidr_req(struct ib_cm_id *cm_id,
 	spin_lock_irqsave(&cm_id_priv->lock, flags);
 	cm_move_av_from_path(&cm_id_priv->av, &av);
 	cm_id->service_id = param->service_id;
-	cm_id->service_mask = ~cpu_to_be64(0);
 	cm_id_priv->timeout_ms = param->timeout_ms;
 	cm_id_priv->max_cm_retries = param->max_cm_retries;
 	if (cm_id->state != IB_CM_IDLE) {
@@ -3557,7 +3546,6 @@ static int cm_sidr_req_handler(struct cm_work *work)
 		cpu_to_be32(IBA_GET(CM_SIDR_REQ_REQUESTID, sidr_req_msg));
 	cm_id_priv->id.service_id =
 		cpu_to_be64(IBA_GET(CM_SIDR_REQ_SERVICEID, sidr_req_msg));
-	cm_id_priv->id.service_mask = ~cpu_to_be64(0);
 	cm_id_priv->tid = sidr_req_msg->hdr.tid;
 
 	wc = work->mad_recv_wc->wc;
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index fbf260c1b1df..8dae5847020a 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -294,7 +294,6 @@ struct ib_cm_id {
 	void			*context;
 	struct ib_device	*device;
 	__be64			service_id;
-	__be64			service_mask;
 	enum ib_cm_state	state;		/* internal CM/debug use */
 	enum ib_cm_lap_state	lap_state;	/* internal CM/debug use */
 	__be32			local_id;
-- 
2.26.3

[PATCH rdma-next 3/3] IB/cm: Refactor cm_insert_listen() and cm_find_listen()

[Mark Zhang]

Move the device and service_id match code at the top of
cm_insert_listen() and cm_find_listen() into the final else branch.

Signed-off-by: Mark Zhang <markzhang@nvidia.com>
---
 drivers/infiniband/core/cm.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 84bb10799467..d7410ee2ade7 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -624,8 +624,16 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv,
 		parent = *link;
 		cur_cm_id_priv = rb_entry(parent, struct cm_id_private,
 					  service_node);
-		if ((service_id == cur_cm_id_priv->id.service_id) &&
-		    (cm_id_priv->id.device == cur_cm_id_priv->id.device)) {
+
+		if (cm_id_priv->id.device < cur_cm_id_priv->id.device)
+			link = &(*link)->rb_left;
+		else if (cm_id_priv->id.device > cur_cm_id_priv->id.device)
+			link = &(*link)->rb_right;
+		else if (be64_lt(service_id, cur_cm_id_priv->id.service_id))
+			link = &(*link)->rb_left;
+		else if (be64_gt(service_id, cur_cm_id_priv->id.service_id))
+			link = &(*link)->rb_right;
+		else {
 			/*
 			 * Sharing an ib_cm_id with different handlers is not
 			 * supported
@@ -641,17 +649,6 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv,
 			spin_unlock_irqrestore(&cm.lock, flags);
 			return cur_cm_id_priv;
 		}
-
-		if (cm_id_priv->id.device < cur_cm_id_priv->id.device)
-			link = &(*link)->rb_left;
-		else if (cm_id_priv->id.device > cur_cm_id_priv->id.device)
-			link = &(*link)->rb_right;
-		else if (be64_lt(service_id, cur_cm_id_priv->id.service_id))
-			link = &(*link)->rb_left;
-		else if (be64_gt(service_id, cur_cm_id_priv->id.service_id))
-			link = &(*link)->rb_right;
-		else
-			link = &(*link)->rb_right;
 	}
 	cm_id_priv->listen_sharecount++;
 	rb_link_node(&cm_id_priv->service_node, parent, link);
@@ -668,11 +665,7 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device,
 
 	while (node) {
 		cm_id_priv = rb_entry(node, struct cm_id_private, service_node);
-		if ((service_id == cm_id_priv->id.service_id) &&
-		    (cm_id_priv->id.device == device)) {
-			refcount_inc(&cm_id_priv->refcount);
-			return cm_id_priv;
-		}
+
 		if (device < cm_id_priv->id.device)
 			node = node->rb_left;
 		else if (device > cm_id_priv->id.device)
@@ -681,8 +674,10 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device,
 			node = node->rb_left;
 		else if (be64_gt(service_id, cm_id_priv->id.service_id))
 			node = node->rb_right;
-		else
-			node = node->rb_right;
+		else {
+			refcount_inc(&cm_id_priv->refcount);
+			return cm_id_priv;
+		}
 	}
 	return NULL;
 }
-- 
2.26.3

Re: [PATCH rdma-next 2/3] IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter of cm_init_listen()

[Leon Romanovsky]

On Fri, Aug 19, 2022 at 12:08:58PM +0300, Mark Zhang wrote:
> The service_mask is always ~cpu_to_be64(0), so the result is always
> a NOP when it is &'d with a service_id. Remove it for simplicity.
> 
> Signed-off-by: Mark Zhang <markzhang@nvidia.com>
> ---
>  drivers/infiniband/core/cm.c | 28 ++++++++--------------------
>  include/rdma/ib_cm.h         |  1 -
>  2 files changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index b59f864b3d79..84bb10799467 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -617,7 +617,6 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv,
>  	struct rb_node *parent = NULL;
>  	struct cm_id_private *cur_cm_id_priv;
>  	__be64 service_id = cm_id_priv->id.service_id;
> -	__be64 service_mask = cm_id_priv->id.service_mask;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&cm.lock, flags);
> @@ -625,8 +624,7 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv,
>  		parent = *link;
>  		cur_cm_id_priv = rb_entry(parent, struct cm_id_private,
>  					  service_node);
> -		if ((cur_cm_id_priv->id.service_mask & service_id) ==
> -		    (service_mask & cur_cm_id_priv->id.service_id) &&
> +		if ((service_id == cur_cm_id_priv->id.service_id) &&
>  		    (cm_id_priv->id.device == cur_cm_id_priv->id.device)) {
>  			/*
>  			 * Sharing an ib_cm_id with different handlers is not
> @@ -670,8 +668,7 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device,
>  
>  	while (node) {
>  		cm_id_priv = rb_entry(node, struct cm_id_private, service_node);
> -		if ((cm_id_priv->id.service_mask & service_id) ==
> -		     cm_id_priv->id.service_id &&
> +		if ((service_id == cm_id_priv->id.service_id) &&
>  		    (cm_id_priv->id.device == device)) {
>  			refcount_inc(&cm_id_priv->refcount);
>  			return cm_id_priv;
> @@ -1158,22 +1155,17 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id)
>  }
>  EXPORT_SYMBOL(ib_destroy_cm_id);
>  
> -static int cm_init_listen(struct cm_id_private *cm_id_priv, __be64 service_id,
> -			  __be64 service_mask)
> +static int cm_init_listen(struct cm_id_private *cm_id_priv, __be64 service_id)
>  {
> -	service_mask = service_mask ? service_mask : ~cpu_to_be64(0);
> -	service_id &= service_mask;
>  	if ((service_id & IB_SERVICE_ID_AGN_MASK) == IB_CM_ASSIGN_SERVICE_ID &&
>  	    (service_id != IB_CM_ASSIGN_SERVICE_ID))
>  		return -EINVAL;
>  
> -	if (service_id == IB_CM_ASSIGN_SERVICE_ID) {
> +	if (service_id == IB_CM_ASSIGN_SERVICE_ID)
>  		cm_id_priv->id.service_id = cpu_to_be64(cm.listen_service_id++);
> -		cm_id_priv->id.service_mask = ~cpu_to_be64(0);
> -	} else {
> +	else
>  		cm_id_priv->id.service_id = service_id;
> -		cm_id_priv->id.service_mask = service_mask;
> -	}

If service_id != IB_CM_ASSIGN_SERVICE_ID, we had zero as service_mask
and not FFF... like you wrote. It puts in question all
cm_id_priv->id.service_mask & service_id => service_id conversions in
this patch.

Thanks

> +
>  	return 0;
>  }
>  
> @@ -1199,7 +1191,7 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id)
>  		goto out;
>  	}
>  
> -	ret = cm_init_listen(cm_id_priv, service_id, 0);
> +	ret = cm_init_listen(cm_id_priv, service_id);
>  	if (ret)
>  		goto out;
>  
> @@ -1247,7 +1239,7 @@ struct ib_cm_id *ib_cm_insert_listen(struct ib_device *device,
>  	if (IS_ERR(cm_id_priv))
>  		return ERR_CAST(cm_id_priv);
>  
> -	err = cm_init_listen(cm_id_priv, service_id, 0);
> +	err = cm_init_listen(cm_id_priv, service_id);
>  	if (err) {
>  		ib_destroy_cm_id(&cm_id_priv->id);
>  		return ERR_PTR(err);
> @@ -1518,7 +1510,6 @@ int ib_send_cm_req(struct ib_cm_id *cm_id,
>  		}
>  	}
>  	cm_id->service_id = param->service_id;
> -	cm_id->service_mask = ~cpu_to_be64(0);
>  	cm_id_priv->timeout_ms = cm_convert_to_ms(
>  				    param->primary_path->packet_life_time) * 2 +
>  				 cm_convert_to_ms(
> @@ -2075,7 +2066,6 @@ static int cm_req_handler(struct cm_work *work)
>  		cpu_to_be32(IBA_GET(CM_REQ_LOCAL_COMM_ID, req_msg));
>  	cm_id_priv->id.service_id =
>  		cpu_to_be64(IBA_GET(CM_REQ_SERVICE_ID, req_msg));
> -	cm_id_priv->id.service_mask = ~cpu_to_be64(0);
>  	cm_id_priv->tid = req_msg->hdr.tid;
>  	cm_id_priv->timeout_ms = cm_convert_to_ms(
>  		IBA_GET(CM_REQ_LOCAL_CM_RESPONSE_TIMEOUT, req_msg));
> @@ -3482,7 +3472,6 @@ int ib_send_cm_sidr_req(struct ib_cm_id *cm_id,
>  	spin_lock_irqsave(&cm_id_priv->lock, flags);
>  	cm_move_av_from_path(&cm_id_priv->av, &av);
>  	cm_id->service_id = param->service_id;
> -	cm_id->service_mask = ~cpu_to_be64(0);
>  	cm_id_priv->timeout_ms = param->timeout_ms;
>  	cm_id_priv->max_cm_retries = param->max_cm_retries;
>  	if (cm_id->state != IB_CM_IDLE) {
> @@ -3557,7 +3546,6 @@ static int cm_sidr_req_handler(struct cm_work *work)
>  		cpu_to_be32(IBA_GET(CM_SIDR_REQ_REQUESTID, sidr_req_msg));
>  	cm_id_priv->id.service_id =
>  		cpu_to_be64(IBA_GET(CM_SIDR_REQ_SERVICEID, sidr_req_msg));
> -	cm_id_priv->id.service_mask = ~cpu_to_be64(0);
>  	cm_id_priv->tid = sidr_req_msg->hdr.tid;
>  
>  	wc = work->mad_recv_wc->wc;
> diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
> index fbf260c1b1df..8dae5847020a 100644
> --- a/include/rdma/ib_cm.h
> +++ b/include/rdma/ib_cm.h
> @@ -294,7 +294,6 @@ struct ib_cm_id {
>  	void			*context;
>  	struct ib_device	*device;
>  	__be64			service_id;
> -	__be64			service_mask;
>  	enum ib_cm_state	state;		/* internal CM/debug use */
>  	enum ib_cm_lap_state	lap_state;	/* internal CM/debug use */
>  	__be32			local_id;
> -- 
> 2.26.3
> 

Re: [PATCH rdma-next 2/3] IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter of cm_init_listen()

[Mark Zhang]

On 8/21/2022 7:34 PM, Leon Romanovsky wrote:
> On Fri, Aug 19, 2022 at 12:08:58PM +0300, Mark Zhang wrote:
>> The service_mask is always ~cpu_to_be64(0), so the result is always
>> a NOP when it is &'d with a service_id. Remove it for simplicity.
>>
>> Signed-off-by: Mark Zhang <markzhang@nvidia.com>
>> ---
>>   drivers/infiniband/core/cm.c | 28 ++++++++--------------------
>>   include/rdma/ib_cm.h         |  1 -
>>   2 files changed, 8 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>> index b59f864b3d79..84bb10799467 100644
>> --- a/drivers/infiniband/core/cm.c
>> +++ b/drivers/infiniband/core/cm.c
>> @@ -617,7 +617,6 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv,
>>   	struct rb_node *parent = NULL;
>>   	struct cm_id_private *cur_cm_id_priv;
>>   	__be64 service_id = cm_id_priv->id.service_id;
>> -	__be64 service_mask = cm_id_priv->id.service_mask;
>>   	unsigned long flags;
>>   
>>   	spin_lock_irqsave(&cm.lock, flags);
>> @@ -625,8 +624,7 @@ static struct cm_id_private *cm_insert_listen(struct cm_id_private *cm_id_priv,
>>   		parent = *link;
>>   		cur_cm_id_priv = rb_entry(parent, struct cm_id_private,
>>   					  service_node);
>> -		if ((cur_cm_id_priv->id.service_mask & service_id) ==
>> -		    (service_mask & cur_cm_id_priv->id.service_id) &&
>> +		if ((service_id == cur_cm_id_priv->id.service_id) &&
>>   		    (cm_id_priv->id.device == cur_cm_id_priv->id.device)) {
>>   			/*
>>   			 * Sharing an ib_cm_id with different handlers is not
>> @@ -670,8 +668,7 @@ static struct cm_id_private *cm_find_listen(struct ib_device *device,
>>   
>>   	while (node) {
>>   		cm_id_priv = rb_entry(node, struct cm_id_private, service_node);
>> -		if ((cm_id_priv->id.service_mask & service_id) ==
>> -		     cm_id_priv->id.service_id &&
>> +		if ((service_id == cm_id_priv->id.service_id) &&
>>   		    (cm_id_priv->id.device == device)) {
>>   			refcount_inc(&cm_id_priv->refcount);
>>   			return cm_id_priv;
>> @@ -1158,22 +1155,17 @@ void ib_destroy_cm_id(struct ib_cm_id *cm_id)
>>   }
>>   EXPORT_SYMBOL(ib_destroy_cm_id);
>>   
>> -static int cm_init_listen(struct cm_id_private *cm_id_priv, __be64 service_id,
>> -			  __be64 service_mask)
>> +static int cm_init_listen(struct cm_id_private *cm_id_priv, __be64 service_id)
>>   {
>> -	service_mask = service_mask ? service_mask : ~cpu_to_be64(0);
>> -	service_id &= service_mask;
>>   	if ((service_id & IB_SERVICE_ID_AGN_MASK) == IB_CM_ASSIGN_SERVICE_ID &&
>>   	    (service_id != IB_CM_ASSIGN_SERVICE_ID))
>>   		return -EINVAL;
>>   
>> -	if (service_id == IB_CM_ASSIGN_SERVICE_ID) {
>> +	if (service_id == IB_CM_ASSIGN_SERVICE_ID)
>>   		cm_id_priv->id.service_id = cpu_to_be64(cm.listen_service_id++);
>> -		cm_id_priv->id.service_mask = ~cpu_to_be64(0);
>> -	} else {
>> +	else
>>   		cm_id_priv->id.service_id = service_id;
>> -		cm_id_priv->id.service_mask = service_mask;
>> -	}
> 
> If service_id != IB_CM_ASSIGN_SERVICE_ID, we had zero as service_mask
> and not FFF... like you wrote. It puts in question all
> cm_id_priv->id.service_mask & service_id => service_id conversions in
> this patch.

The id.service_mask field is removed in this patch, check the change in 
include/rdma/ib_cm.h

> 
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1199,7 +1191,7 @@ int ib_cm_listen(struct ib_cm_id *cm_id, __be64 service_id)
>>   		goto out;
>>   	}
>>   
>> -	ret = cm_init_listen(cm_id_priv, service_id, 0);
>> +	ret = cm_init_listen(cm_id_priv, service_id);
>>   	if (ret)
>>   		goto out;
>>   
>> @@ -1247,7 +1239,7 @@ struct ib_cm_id *ib_cm_insert_listen(struct ib_device *device,
>>   	if (IS_ERR(cm_id_priv))
>>   		return ERR_CAST(cm_id_priv);
>>   
>> -	err = cm_init_listen(cm_id_priv, service_id, 0);
>> +	err = cm_init_listen(cm_id_priv, service_id);
>>   	if (err) {
>>   		ib_destroy_cm_id(&cm_id_priv->id);
>>   		return ERR_PTR(err);
>> @@ -1518,7 +1510,6 @@ int ib_send_cm_req(struct ib_cm_id *cm_id,
>>   		}
>>   	}
>>   	cm_id->service_id = param->service_id;
>> -	cm_id->service_mask = ~cpu_to_be64(0);
>>   	cm_id_priv->timeout_ms = cm_convert_to_ms(
>>   				    param->primary_path->packet_life_time) * 2 +
>>   				 cm_convert_to_ms(
>> @@ -2075,7 +2066,6 @@ static int cm_req_handler(struct cm_work *work)
>>   		cpu_to_be32(IBA_GET(CM_REQ_LOCAL_COMM_ID, req_msg));
>>   	cm_id_priv->id.service_id =
>>   		cpu_to_be64(IBA_GET(CM_REQ_SERVICE_ID, req_msg));
>> -	cm_id_priv->id.service_mask = ~cpu_to_be64(0);
>>   	cm_id_priv->tid = req_msg->hdr.tid;
>>   	cm_id_priv->timeout_ms = cm_convert_to_ms(
>>   		IBA_GET(CM_REQ_LOCAL_CM_RESPONSE_TIMEOUT, req_msg));
>> @@ -3482,7 +3472,6 @@ int ib_send_cm_sidr_req(struct ib_cm_id *cm_id,
>>   	spin_lock_irqsave(&cm_id_priv->lock, flags);
>>   	cm_move_av_from_path(&cm_id_priv->av, &av);
>>   	cm_id->service_id = param->service_id;
>> -	cm_id->service_mask = ~cpu_to_be64(0);
>>   	cm_id_priv->timeout_ms = param->timeout_ms;
>>   	cm_id_priv->max_cm_retries = param->max_cm_retries;
>>   	if (cm_id->state != IB_CM_IDLE) {
>> @@ -3557,7 +3546,6 @@ static int cm_sidr_req_handler(struct cm_work *work)
>>   		cpu_to_be32(IBA_GET(CM_SIDR_REQ_REQUESTID, sidr_req_msg));
>>   	cm_id_priv->id.service_id =
>>   		cpu_to_be64(IBA_GET(CM_SIDR_REQ_SERVICEID, sidr_req_msg));
>> -	cm_id_priv->id.service_mask = ~cpu_to_be64(0);
>>   	cm_id_priv->tid = sidr_req_msg->hdr.tid;
>>   
>>   	wc = work->mad_recv_wc->wc;
>> diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
>> index fbf260c1b1df..8dae5847020a 100644
>> --- a/include/rdma/ib_cm.h
>> +++ b/include/rdma/ib_cm.h
>> @@ -294,7 +294,6 @@ struct ib_cm_id {
>>   	void			*context;
>>   	struct ib_device	*device;
>>   	__be64			service_id;
>> -	__be64			service_mask;
>>   	enum ib_cm_state	state;		/* internal CM/debug use */
>>   	enum ib_cm_lap_state	lap_state;	/* internal CM/debug use */
>>   	__be32			local_id;
>> -- 
>> 2.26.3
>>

Re: [PATCH rdma-next 2/3] IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter of cm_init_listen()

[Leon Romanovsky]

On Sun, Aug 21, 2022 at 08:50:08PM +0800, Mark Zhang wrote:
> On 8/21/2022 7:34 PM, Leon Romanovsky wrote:
> > On Fri, Aug 19, 2022 at 12:08:58PM +0300, Mark Zhang wrote:
> > > The service_mask is always ~cpu_to_be64(0), so the result is always
> > > a NOP when it is &'d with a service_id. Remove it for simplicity.
> > > 
> > > Signed-off-by: Mark Zhang <markzhang@nvidia.com>
> > > ---
> > >   drivers/infiniband/core/cm.c | 28 ++++++++--------------------
> > >   include/rdma/ib_cm.h         |  1 -
> > >   2 files changed, 8 insertions(+), 21 deletions(-)

<...>

> > > -	if (service_id == IB_CM_ASSIGN_SERVICE_ID) {
> > > +	if (service_id == IB_CM_ASSIGN_SERVICE_ID)
> > >   		cm_id_priv->id.service_id = cpu_to_be64(cm.listen_service_id++);
> > > -		cm_id_priv->id.service_mask = ~cpu_to_be64(0);
> > > -	} else {
> > > +	else
> > >   		cm_id_priv->id.service_id = service_id;
> > > -		cm_id_priv->id.service_mask = service_mask;
> > > -	}
> > 
> > If service_id != IB_CM_ASSIGN_SERVICE_ID, we had zero as service_mask
> > and not FFF... like you wrote. It puts in question all
> > cm_id_priv->id.service_mask & service_id => service_id conversions in
> > this patch.
> 
> The id.service_mask field is removed in this patch, check the change in
> include/rdma/ib_cm.h

Right, you removed service_mask and described it "is always ~cpu_to_be64(0)".
As far as I can tell, this is not true and in this "if (service_id == IB_CM_ASSIGN_SERVICE_ID)"
sometimes we set cm_id_priv->id.service_mask to be 0 and sometimes 0xFF....

Why is it correct to remove cm_id_priv->id.service_mask in this hunk?

Thanks

Re: [PATCH rdma-next 2/3] IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter of cm_init_listen()

[Mark Zhang]

On 8/21/2022 9:47 PM, Leon Romanovsky wrote:
> On Sun, Aug 21, 2022 at 08:50:08PM +0800, Mark Zhang wrote:
>> On 8/21/2022 7:34 PM, Leon Romanovsky wrote:
>>> On Fri, Aug 19, 2022 at 12:08:58PM +0300, Mark Zhang wrote:
>>>> The service_mask is always ~cpu_to_be64(0), so the result is always
>>>> a NOP when it is &'d with a service_id. Remove it for simplicity.
>>>>
>>>> Signed-off-by: Mark Zhang <markzhang@nvidia.com>
>>>> ---
>>>>    drivers/infiniband/core/cm.c | 28 ++++++++--------------------
>>>>    include/rdma/ib_cm.h         |  1 -
>>>>    2 files changed, 8 insertions(+), 21 deletions(-)
> 
> <...>
> 
>>>> -	if (service_id == IB_CM_ASSIGN_SERVICE_ID) {
>>>> +	if (service_id == IB_CM_ASSIGN_SERVICE_ID)
>>>>    		cm_id_priv->id.service_id = cpu_to_be64(cm.listen_service_id++);
>>>> -		cm_id_priv->id.service_mask = ~cpu_to_be64(0);
>>>> -	} else {
>>>> +	else
>>>>    		cm_id_priv->id.service_id = service_id;
>>>> -		cm_id_priv->id.service_mask = service_mask;
>>>> -	}
>>>
>>> If service_id != IB_CM_ASSIGN_SERVICE_ID, we had zero as service_mask
>>> and not FFF... like you wrote. It puts in question all
>>> cm_id_priv->id.service_mask & service_id => service_id conversions in
>>> this patch.
>>
>> The id.service_mask field is removed in this patch, check the change in
>> include/rdma/ib_cm.h
> 
> Right, you removed service_mask and described it "is always ~cpu_to_be64(0)".
> As far as I can tell, this is not true and in this "if (service_id == IB_CM_ASSIGN_SERVICE_ID)"
> sometimes we set cm_id_priv->id.service_mask to be 0 and sometimes 0xFF....
> 
> Why is it correct to remove cm_id_priv->id.service_mask in this hunk?

1. service_id == IB_CM_ASSIGN_SERVICE_ID:
   cm_id_priv->id.service_mask = ~cpu_to_be64(0);

2. service_id != IB_CM_ASSIGN_SERVICE_ID:
      cm_id_priv->id.service_mask = service_mask;
    It's also ~cpu_to_be64(0), because cm_init_listen() is always called
    with service_mask = 0:
      ib_cm_listen(..., 0) -> cm_init_listen(..., 0)
      ib_cm_insert_listen() -> cm_init_listen(..., 0)

So it's always ~cpu_to_be64(0)..

Thanks

Re: [PATCH rdma-next 2/3] IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter of cm_init_listen()

[Leon Romanovsky]

On Sun, Aug 21, 2022 at 10:08:54PM +0800, Mark Zhang wrote:
> On 8/21/2022 9:47 PM, Leon Romanovsky wrote:
> > On Sun, Aug 21, 2022 at 08:50:08PM +0800, Mark Zhang wrote:
> > > On 8/21/2022 7:34 PM, Leon Romanovsky wrote:
> > > > On Fri, Aug 19, 2022 at 12:08:58PM +0300, Mark Zhang wrote:
> > > > > The service_mask is always ~cpu_to_be64(0), so the result is always
> > > > > a NOP when it is &'d with a service_id. Remove it for simplicity.
> > > > > 
> > > > > Signed-off-by: Mark Zhang <markzhang@nvidia.com>
> > > > > ---
> > > > >    drivers/infiniband/core/cm.c | 28 ++++++++--------------------
> > > > >    include/rdma/ib_cm.h         |  1 -
> > > > >    2 files changed, 8 insertions(+), 21 deletions(-)
> > 
> > <...>
> > 
> > > > > -	if (service_id == IB_CM_ASSIGN_SERVICE_ID) {
> > > > > +	if (service_id == IB_CM_ASSIGN_SERVICE_ID)
> > > > >    		cm_id_priv->id.service_id = cpu_to_be64(cm.listen_service_id++);
> > > > > -		cm_id_priv->id.service_mask = ~cpu_to_be64(0);
> > > > > -	} else {
> > > > > +	else
> > > > >    		cm_id_priv->id.service_id = service_id;
> > > > > -		cm_id_priv->id.service_mask = service_mask;
> > > > > -	}
> > > > 
> > > > If service_id != IB_CM_ASSIGN_SERVICE_ID, we had zero as service_mask
> > > > and not FFF... like you wrote. It puts in question all
> > > > cm_id_priv->id.service_mask & service_id => service_id conversions in
> > > > this patch.
> > > 
> > > The id.service_mask field is removed in this patch, check the change in
> > > include/rdma/ib_cm.h
> > 
> > Right, you removed service_mask and described it "is always ~cpu_to_be64(0)".
> > As far as I can tell, this is not true and in this "if (service_id == IB_CM_ASSIGN_SERVICE_ID)"
> > sometimes we set cm_id_priv->id.service_mask to be 0 and sometimes 0xFF....
> > 
> > Why is it correct to remove cm_id_priv->id.service_mask in this hunk?
> 
> 1. service_id == IB_CM_ASSIGN_SERVICE_ID:
>   cm_id_priv->id.service_mask = ~cpu_to_be64(0);
> 
> 2. service_id != IB_CM_ASSIGN_SERVICE_ID:
>      cm_id_priv->id.service_mask = service_mask;
>    It's also ~cpu_to_be64(0), because cm_init_listen() is always called
>    with service_mask = 0:
>      ib_cm_listen(..., 0) -> cm_init_listen(..., 0)
>      ib_cm_insert_listen() -> cm_init_listen(..., 0)
> 
> So it's always ~cpu_to_be64(0)..

I see it now, thanks.

> 
> Thanks
> 
> 
> 
> 
> 

Re: [PATCH rdma-next 0/3] IB/cm refactors

[Leon Romanovsky]

On Fri, Aug 19, 2022 at 12:08:56PM +0300, Mark Zhang wrote:
> Hi,
> 
> This series provides 2 IB/cm refactors:
> - The first 2 patches remove the usage of service_mask, as it is
>   always -1 so it doesn't help;
> - The third one refines cm_insert_listen() and cm_find_listen().
> 
> Link: https://lore.kernel.org/lkml/20220624201733.GA284068@nvidia.com/t/
> 
> Thanks.
> 
> Mark Zhang (3):
>   IB/cm: Remove the service_mask parameter from ib_cm_listen()
>   IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter
>     of cm_init_listen()
>   IB/cm: Refactor cm_insert_listen() and cm_find_listen()
> 

Thanks, applied.