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 <leonro@nvidia.com>]

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