From: Leon Romanovsky <leonro@nvidia.com>
To: Mark Zhang <markzhang@nvidia.com>
Cc: <jgg@nvidia.com>, <dledford@redhat.com>,
<jiapeng.chong@linux.alibaba.com>, <cgel.zte@gmail.com>,
<linux-rdma@vger.kernel.org>
Subject: Re: [PATCH rdma-next 2/3] IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter of cm_init_listen()
Date: Sun, 21 Aug 2022 14:34:52 +0300 [thread overview]
Message-ID: <YwIYXI9xTSpw4Vtj@unreal> (raw)
In-Reply-To: <20220819090859.957943-3-markzhang@nvidia.com>
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
>
next prev parent reply other threads:[~2022-08-21 11:35 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-19 9:08 [PATCH rdma-next 0/3] IB/cm refactors Mark Zhang
2022-08-19 9:08 ` [PATCH rdma-next 1/3] IB/cm: Remove the service_mask parameter from ib_cm_listen() Mark Zhang
2022-08-19 9:08 ` [PATCH rdma-next 2/3] IB/cm: remove cm_id_priv->id.service_mask and service_mask parameter of cm_init_listen() Mark Zhang
2022-08-21 11:34 ` Leon Romanovsky [this message]
2022-08-21 12:50 ` Mark Zhang
2022-08-21 13:47 ` Leon Romanovsky
2022-08-21 14:08 ` Mark Zhang
2022-08-21 14:14 ` Leon Romanovsky
2022-08-19 9:08 ` [PATCH rdma-next 3/3] IB/cm: Refactor cm_insert_listen() and cm_find_listen() Mark Zhang
2022-08-29 8:21 ` [PATCH rdma-next 0/3] IB/cm refactors Leon Romanovsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YwIYXI9xTSpw4Vtj@unreal \
--to=leonro@nvidia.com \
--cc=cgel.zte@gmail.com \
--cc=dledford@redhat.com \
--cc=jgg@nvidia.com \
--cc=jiapeng.chong@linux.alibaba.com \
--cc=linux-rdma@vger.kernel.org \
--cc=markzhang@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox