Re: [PATCH v2 for-next 3/6] RDMA/rtrs: Fix warning when use poll mode

[Leon Romanovsky <leon@kernel.org>]

On Sun, Aug 08, 2021 at 01:07:29PM +0200, Jack Wang wrote:
> Leon Romanovsky <leon@kernel.org>于2021年8月8日 周日11:05写道：
> 
> > On Fri, Aug 06, 2021 at 01:21:09PM +0200, Md Haris Iqbal wrote:
> > > From: Jack Wang <jinpu.wang@ionos.com>
> > >
> > > when test with poll mode, it will fail and lead to warning below:
> > > echo "sessname=bla path=gid:fe80::2:c903:4e:d0b3@gid:fe80::2:c903:8:ca17
> > > device_path=/dev/nullb2 nr_poll_queues=-1" |
> > > sudo tee /sys/devices/virtual/rnbd-client/ctl/map_device
> > >
> > > rnbd_client L597: Mapping device /dev/nullb2 on session bla,
> > > (access_mode: rw, nr_poll_queues: 8)
> > > WARNING: CPU: 3 PID: 9886 at drivers/infiniband/core/cq.c:447
> > ib_cq_pool_get+0x26f/0x2a0 [ib_core]
> > >
> > > The problem is, when poll_queues are used, there will be more
> > connections than
> > > number of cpus; and those extra connections will have ib poll context
> > set to
> > > IB_POLL_DIRECT, which is not allowed to be used for shared CQs.
> > >
> > > So, in case those extra connections when poll queues are used, use
> > > ib_alloc_cq/ib_free_cq.
> > >
> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > Reviewed-by: Gioh Kim <gi-oh.kim@ionos.com>
> > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> > > ---
> > >  drivers/infiniband/ulp/rtrs/rtrs-srv.c |  1 +
> > >  drivers/infiniband/ulp/rtrs/rtrs.c     | 17 ++++++++++++++---
> > >  2 files changed, 15 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > index cd9a4ccf4c28..47775987f91a 100644
> > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > @@ -1768,6 +1768,7 @@ static struct rtrs_srv_sess *__alloc_sess(struct
> > rtrs_srv *srv,
> > >       strscpy(sess->s.sessname, str, sizeof(sess->s.sessname));
> > >
> > >       sess->s.con_num = con_num;
> > > +     sess->s.irq_con_num = con_num;
> > >       sess->s.recon_cnt = recon_cnt;
> > >       uuid_copy(&sess->s.uuid, uuid);
gTgT> > >       spin_lock_init(&sess->state_lock);
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs.c
> > b/drivers/infiniband/ulp/rtrs/rtrs.c
> > > index ca542e477d38..9bc323490ce3 100644
> > > --- a/drivers/infiniband/ulp/rtrs/rtrs.c
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs.c
> > > @@ -228,7 +228,12 @@ static int create_cq(struct rtrs_con *con, int
> > cq_vector, int nr_cqe,
> > >       struct rdma_cm_id *cm_id = con->cm_id;
> > >       struct ib_cq *cq;
> > >
> > > -     cq = ib_cq_pool_get(cm_id->device, nr_cqe, cq_vector, poll_ctx);
> > > +     if (con->cid >= con->sess->irq_con_num)
> > > +             cq = ib_alloc_cq(cm_id->device, con, nr_cqe, cq_vector,
> > > +                              poll_ctx);
> > > +     else
> > > +             cq = ib_cq_pool_get(cm_id->device, nr_cqe, cq_vector,
> > poll_ctx);
> >
> > I see same "if (con->c.cid >= sess->s.irq_con_num)" checks when calling
> > to rtrs_cq_qp_create() that will take poll_ctx and convey it to
> > create_cq().
> >
> > Please take a look on nvme_rdma_create_cq() which does the same without
> > passing poll_ctx.
> >
> > Thanks
> 
> Hi,
> 
> The reason rtrs needs to have poll_ctx is rtrs-clt and rtrs-srv use
> different poll_ctx, and they all call into rtrs_cq_qp_create, while
> nvme_rdma_create_cq is only for host (client) side.
> 
> I guess you wanted to convert the same if
> (con->c.cid>=sess->s.irq_con_num), this can be done in a new patch.

I don't want to see code that does something like that:
 if (a_cond)
   f(...)
      if (a_cond)
        do_X
       else
        do_Y

The ideal flow should have minimal number of ifs to make the code
more clear and readable, and definitely shouldn't have same if()
checks in various places of the callstack.

In your case, rtrs-srv uses IB_POLL_WORKQUEUE as poll_ctx which doesn't
look related to the issue stated in the commit message.

Thanks

> 
> Thanks
> 
> >
> >