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