| Message ID | 20210216073958.13957-1-jinpu.wang@cloud.ionos.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/rtrs-srv: suppress warnings passing zero to 'PTR_ERR' | expand |
On Tue, Feb 16, 2021 at 08:39:58AM +0100, Jack Wang wrote: > smatch warnings: > drivers/infiniband/ulp/rtrs/rtrs-srv.c:1805 rtrs_rdma_connect() warn: passing zero to 'PTR_ERR' > > Smatch seems confused by the refcount_read condition, so just > treat it seperately. > > Fixes: f0751419d3a1 ("RDMA/rtrs: Only allow addition of path to an already established session") > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com> > --- > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > index eb17c3a08810..2f6d665bea90 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > @@ -1799,12 +1799,16 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > } > recon_cnt = le16_to_cpu(msg->recon_cnt); > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > + if (IS_ERR(srv)) { > + err = PTR_ERR(srv); > + goto reject_w_err; > + } > /* > * "refcount == 0" happens if a previous thread calls get_or_create_srv > * allocate srv, but chunks of srv are not allocated yet. > */ > - if (IS_ERR(srv) || refcount_read(&srv->refcount) == 0) { > - err = PTR_ERR(srv); > + if (refcount_read(&srv->refcount) == 0) { I would say that "list_add(&srv->ctx_list, &ctx->srv_list);" line in the get_or_create_srv() is performed too early, relying on zero in the refcount doesn't look great. Thanks
Hi Leon, On Tue, Feb 16, 2021 at 8:50 AM Leon Romanovsky <leon@kernel.org> wrote: > > On Tue, Feb 16, 2021 at 08:39:58AM +0100, Jack Wang wrote: > > smatch warnings: > > drivers/infiniband/ulp/rtrs/rtrs-srv.c:1805 rtrs_rdma_connect() warn: passing zero to 'PTR_ERR' > > > > Smatch seems confused by the refcount_read condition, so just > > treat it seperately. > > > > Fixes: f0751419d3a1 ("RDMA/rtrs: Only allow addition of path to an already established session") > > Reported-by: kernel test robot <lkp@intel.com> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com> > > --- > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > index eb17c3a08810..2f6d665bea90 100644 > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > @@ -1799,12 +1799,16 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > } > > recon_cnt = le16_to_cpu(msg->recon_cnt); > > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > > + if (IS_ERR(srv)) { > > + err = PTR_ERR(srv); > > + goto reject_w_err; > > + } > > /* > > * "refcount == 0" happens if a previous thread calls get_or_create_srv > > * allocate srv, but chunks of srv are not allocated yet. > > */ > > - if (IS_ERR(srv) || refcount_read(&srv->refcount) == 0) { > > - err = PTR_ERR(srv); > > + if (refcount_read(&srv->refcount) == 0) { > > I would say that "list_add(&srv->ctx_list, &ctx->srv_list);" line in the > get_or_create_srv() is performed too early, Moving list_add down to the end was the initial code, but we noticed the memory allocation could take quite some time when system under memory pressure, hence we changed it in d715ff8acbd5 ("RDMA/rtrs-srv: Don't guard the whole __alloc_srv with srv_mutex") Thanks for the comment.
On Tue, Feb 16, 2021 at 09:02:03AM +0100, Jinpu Wang wrote: > Hi Leon, > On Tue, Feb 16, 2021 at 8:50 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Tue, Feb 16, 2021 at 08:39:58AM +0100, Jack Wang wrote: > > > smatch warnings: > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c:1805 rtrs_rdma_connect() warn: passing zero to 'PTR_ERR' > > > > > > Smatch seems confused by the refcount_read condition, so just > > > treat it seperately. > > > > > > Fixes: f0751419d3a1 ("RDMA/rtrs: Only allow addition of path to an already established session") > > > Reported-by: kernel test robot <lkp@intel.com> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com> > > > --- > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > index eb17c3a08810..2f6d665bea90 100644 > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > @@ -1799,12 +1799,16 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > > } > > > recon_cnt = le16_to_cpu(msg->recon_cnt); > > > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > > > + if (IS_ERR(srv)) { > > > + err = PTR_ERR(srv); > > > + goto reject_w_err; > > > + } > > > /* > > > * "refcount == 0" happens if a previous thread calls get_or_create_srv > > > * allocate srv, but chunks of srv are not allocated yet. > > > */ > > > - if (IS_ERR(srv) || refcount_read(&srv->refcount) == 0) { > > > - err = PTR_ERR(srv); > > > + if (refcount_read(&srv->refcount) == 0) { > > > > I would say that "list_add(&srv->ctx_list, &ctx->srv_list);" line in the > > get_or_create_srv() is performed too early, > Moving list_add down to the end was the initial code, but we noticed > the memory allocation could take quite > some time when system under memory pressure, hence we changed it in > d715ff8acbd5 ("RDMA/rtrs-srv: Don't guard the whole __alloc_srv with > srv_mutex") You don't need to hold lock during allocation, only during search in the list. Thanks > > Thanks for the comment.
On Tue, Feb 16, 2021 at 9:17 AM Leon Romanovsky <leon@kernel.org> wrote: > > On Tue, Feb 16, 2021 at 09:02:03AM +0100, Jinpu Wang wrote: > > Hi Leon, > > On Tue, Feb 16, 2021 at 8:50 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > On Tue, Feb 16, 2021 at 08:39:58AM +0100, Jack Wang wrote: > > > > smatch warnings: > > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c:1805 rtrs_rdma_connect() warn: passing zero to 'PTR_ERR' > > > > > > > > Smatch seems confused by the refcount_read condition, so just > > > > treat it seperately. > > > > > > > > Fixes: f0751419d3a1 ("RDMA/rtrs: Only allow addition of path to an already established session") > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com> > > > > --- > > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 ++++++-- > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > > index eb17c3a08810..2f6d665bea90 100644 > > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > > @@ -1799,12 +1799,16 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > > > } > > > > recon_cnt = le16_to_cpu(msg->recon_cnt); > > > > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > > > > + if (IS_ERR(srv)) { > > > > + err = PTR_ERR(srv); > > > > + goto reject_w_err; > > > > + } > > > > /* > > > > * "refcount == 0" happens if a previous thread calls get_or_create_srv > > > > * allocate srv, but chunks of srv are not allocated yet. > > > > */ > > > > - if (IS_ERR(srv) || refcount_read(&srv->refcount) == 0) { > > > > - err = PTR_ERR(srv); > > > > + if (refcount_read(&srv->refcount) == 0) { > > > > > > I would say that "list_add(&srv->ctx_list, &ctx->srv_list);" line in the > > > get_or_create_srv() is performed too early, > > Moving list_add down to the end was the initial code, but we noticed > > the memory allocation could take quite > > some time when system under memory pressure, hence we changed it in > > d715ff8acbd5 ("RDMA/rtrs-srv: Don't guard the whole __alloc_srv with > > srv_mutex") > > You don't need to hold lock during allocation, only during search in the list. Thanks, I will try and test. > > Thanks > > > > > Thanks for the comment.
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c index eb17c3a08810..2f6d665bea90 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c @@ -1799,12 +1799,16 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, } recon_cnt = le16_to_cpu(msg->recon_cnt); srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); + if (IS_ERR(srv)) { + err = PTR_ERR(srv); + goto reject_w_err; + } /* * "refcount == 0" happens if a previous thread calls get_or_create_srv * allocate srv, but chunks of srv are not allocated yet. */ - if (IS_ERR(srv) || refcount_read(&srv->refcount) == 0) { - err = PTR_ERR(srv); + if (refcount_read(&srv->refcount) == 0) { + err = -EBUSY; goto reject_w_err; } mutex_lock(&srv->paths_mutex);
smatch warnings: drivers/infiniband/ulp/rtrs/rtrs-srv.c:1805 rtrs_rdma_connect() warn: passing zero to 'PTR_ERR' Smatch seems confused by the refcount_read condition, so just treat it seperately. Fixes: f0751419d3a1 ("RDMA/rtrs: Only allow addition of path to an already established session") Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com> --- drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)