| Message ID | 20210503114818.288896-5-gi-oh.kim@ionos.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Misc update for rtrs | expand |
On Mon, May 03, 2021 at 01:48:02PM +0200, Gioh Kim wrote: > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > It was difficult to find out why it failed to establish RDMA > connection. This patch adds some messages to show which function > has failed why. > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> > --- > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > index 3d09d01e34b4..df17dd4c1e28 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx, > * If this request is not the first connection request from the > * client for this session then fail and return error. > */ > - if (!first_conn) > + if (!first_conn) { > + pr_err("Error: Not the first connection request for this session\n"); > return ERR_PTR(-ENXIO); > + } > > /* need to allocate a new srv */ > srv = kzalloc(sizeof(*srv), GFP_KERNEL); > @@ -1812,6 +1814,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > if (IS_ERR(srv)) { > err = PTR_ERR(srv); > + pr_err("get_or_create_srv(), error %d\n", err); > goto reject_w_err; > } > mutex_lock(&srv->paths_mutex); > @@ -1850,11 +1853,13 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > mutex_unlock(&srv->paths_mutex); > put_srv(srv); > err = PTR_ERR(sess); > + pr_err("RTRS server session allocation failed: %d\n", err); > goto reject_w_err; > } > } > err = create_con(sess, cm_id, cid); > if (err) { > + rtrs_err((&sess->s), "create_con(), error %d\n", err); > (void)rtrs_rdma_do_reject(cm_id, err); Unrelated to this change, but this (void) casting should be go. Thanks > /* > * Since session has other connections we follow normal way > @@ -1865,6 +1870,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > } > err = rtrs_rdma_do_accept(sess, cm_id); > if (err) { > + rtrs_err((&sess->s), "rtrs_rdma_do_accept(), error %d\n", err); > (void)rtrs_rdma_do_reject(cm_id, err); > /* > * Since current connection was successfully added to the > -- > 2.25.1 >
On Sun, May 9, 2021 at 1:27 PM Leon Romanovsky <leon@kernel.org> wrote: > > On Mon, May 03, 2021 at 01:48:02PM +0200, Gioh Kim wrote: > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > > > It was difficult to find out why it failed to establish RDMA > > connection. This patch adds some messages to show which function > > has failed why. > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> > > --- > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > index 3d09d01e34b4..df17dd4c1e28 100644 > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx, > > * If this request is not the first connection request from the > > * client for this session then fail and return error. > > */ > > - if (!first_conn) > > + if (!first_conn) { > > + pr_err("Error: Not the first connection request for this session\n"); > > return ERR_PTR(-ENXIO); > > + } > > > > /* need to allocate a new srv */ > > srv = kzalloc(sizeof(*srv), GFP_KERNEL); > > @@ -1812,6 +1814,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > > if (IS_ERR(srv)) { > > err = PTR_ERR(srv); > > + pr_err("get_or_create_srv(), error %d\n", err); > > goto reject_w_err; > > } > > mutex_lock(&srv->paths_mutex); > > @@ -1850,11 +1853,13 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > mutex_unlock(&srv->paths_mutex); > > put_srv(srv); > > err = PTR_ERR(sess); > > + pr_err("RTRS server session allocation failed: %d\n", err); > > goto reject_w_err; > > } > > } > > err = create_con(sess, cm_id, cid); > > if (err) { > > + rtrs_err((&sess->s), "create_con(), error %d\n", err); > > (void)rtrs_rdma_do_reject(cm_id, err); > > Unrelated to this change, but this (void) casting should be go. Hi Leon We wanted to explicitly signal that the code is ignoring the return value of the function. Is there a strong reason for the casting to be removed? > > Thanks > > > /* > > * Since session has other connections we follow normal way > > @@ -1865,6 +1870,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > } > > err = rtrs_rdma_do_accept(sess, cm_id); > > if (err) { > > + rtrs_err((&sess->s), "rtrs_rdma_do_accept(), error %d\n", err); > > (void)rtrs_rdma_do_reject(cm_id, err); > > /* > > * Since current connection was successfully added to the > > -- > > 2.25.1 > >
On Mon, May 10, 2021 at 12:55:42PM +0200, Haris Iqbal wrote: > On Sun, May 9, 2021 at 1:27 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Mon, May 03, 2021 at 01:48:02PM +0200, Gioh Kim wrote: > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > > > > > It was difficult to find out why it failed to establish RDMA > > > connection. This patch adds some messages to show which function > > > has failed why. > > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> > > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > > > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> > > > --- > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > index 3d09d01e34b4..df17dd4c1e28 100644 > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx, > > > * If this request is not the first connection request from the > > > * client for this session then fail and return error. > > > */ > > > - if (!first_conn) > > > + if (!first_conn) { > > > + pr_err("Error: Not the first connection request for this session\n"); > > > return ERR_PTR(-ENXIO); > > > + } > > > > > > /* need to allocate a new srv */ > > > srv = kzalloc(sizeof(*srv), GFP_KERNEL); > > > @@ -1812,6 +1814,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > > > if (IS_ERR(srv)) { > > > err = PTR_ERR(srv); > > > + pr_err("get_or_create_srv(), error %d\n", err); > > > goto reject_w_err; > > > } > > > mutex_lock(&srv->paths_mutex); > > > @@ -1850,11 +1853,13 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > > mutex_unlock(&srv->paths_mutex); > > > put_srv(srv); > > > err = PTR_ERR(sess); > > > + pr_err("RTRS server session allocation failed: %d\n", err); > > > goto reject_w_err; > > > } > > > } > > > err = create_con(sess, cm_id, cid); > > > if (err) { > > > + rtrs_err((&sess->s), "create_con(), error %d\n", err); > > > (void)rtrs_rdma_do_reject(cm_id, err); > > > > Unrelated to this change, but this (void) casting should be go. > > Hi Leon > > We wanted to explicitly signal that the code is ignoring the return > value of the function. Is there a strong reason for the casting to be > removed? "Don't write useless code and don't assume that the kernel developers are first year college students" - is this strong enough reason for you? Your (void) casting doesn't give anything extra, just churn and even more suspicious review over such code. Thanks
On Mon, May 10, 2021 at 2:03 PM Leon Romanovsky <leon@kernel.org> wrote: > > On Mon, May 10, 2021 at 12:55:42PM +0200, Haris Iqbal wrote: > > On Sun, May 9, 2021 at 1:27 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > On Mon, May 03, 2021 at 01:48:02PM +0200, Gioh Kim wrote: > > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > > > > > > > It was difficult to find out why it failed to establish RDMA > > > > connection. This patch adds some messages to show which function > > > > has failed why. > > > > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> > > > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > > > > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> > > > > --- > > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++- > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > > index 3d09d01e34b4..df17dd4c1e28 100644 > > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > > @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx, > > > > * If this request is not the first connection request from the > > > > * client for this session then fail and return error. > > > > */ > > > > - if (!first_conn) > > > > + if (!first_conn) { > > > > + pr_err("Error: Not the first connection request for this session\n"); > > > > return ERR_PTR(-ENXIO); > > > > + } > > > > > > > > /* need to allocate a new srv */ > > > > srv = kzalloc(sizeof(*srv), GFP_KERNEL); > > > > @@ -1812,6 +1814,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > > > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > > > > if (IS_ERR(srv)) { > > > > err = PTR_ERR(srv); > > > > + pr_err("get_or_create_srv(), error %d\n", err); > > > > goto reject_w_err; > > > > } > > > > mutex_lock(&srv->paths_mutex); > > > > @@ -1850,11 +1853,13 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > > > mutex_unlock(&srv->paths_mutex); > > > > put_srv(srv); > > > > err = PTR_ERR(sess); > > > > + pr_err("RTRS server session allocation failed: %d\n", err); > > > > goto reject_w_err; > > > > } > > > > } > > > > err = create_con(sess, cm_id, cid); > > > > if (err) { > > > > + rtrs_err((&sess->s), "create_con(), error %d\n", err); > > > > (void)rtrs_rdma_do_reject(cm_id, err); > > > > > > Unrelated to this change, but this (void) casting should be go. > > > > Hi Leon > > > > We wanted to explicitly signal that the code is ignoring the return > > value of the function. Is there a strong reason for the casting to be > > removed? > > "Don't write useless code and don't assume that the kernel developers > are first year college students" - is this strong enough reason for you? I am all for simple and minimum code; but this casting does have a decent reason. Anyway if the RDMA code style calls for this to be removed; will remove them in a separate patch and send out with the next batch of patches. > > Your (void) casting doesn't give anything extra, just churn and even > more suspicious review over such code. > > Thanks
On Mon, May 10, 2021 at 02:16:02PM +0200, Haris Iqbal wrote: > On Mon, May 10, 2021 at 2:03 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Mon, May 10, 2021 at 12:55:42PM +0200, Haris Iqbal wrote: > > > On Sun, May 9, 2021 at 1:27 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > On Mon, May 03, 2021 at 01:48:02PM +0200, Gioh Kim wrote: > > > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com> > > > > > > > > > > It was difficult to find out why it failed to establish RDMA > > > > > connection. This patch adds some messages to show which function > > > > > has failed why. > > > > > > > > > > Signed-off-by: Md Haris Iqbal <haris.iqbal@ionos.com> > > > > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com> > > > > > Signed-off-by: Gioh Kim <gi-oh.kim@ionos.com> > > > > > --- > > > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 +++++++- > > > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > > > index 3d09d01e34b4..df17dd4c1e28 100644 > > > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > > > > @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx, > > > > > * If this request is not the first connection request from the > > > > > * client for this session then fail and return error. > > > > > */ > > > > > - if (!first_conn) > > > > > + if (!first_conn) { > > > > > + pr_err("Error: Not the first connection request for this session\n"); > > > > > return ERR_PTR(-ENXIO); > > > > > + } > > > > > > > > > > /* need to allocate a new srv */ > > > > > srv = kzalloc(sizeof(*srv), GFP_KERNEL); > > > > > @@ -1812,6 +1814,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > > > > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > > > > > if (IS_ERR(srv)) { > > > > > err = PTR_ERR(srv); > > > > > + pr_err("get_or_create_srv(), error %d\n", err); > > > > > goto reject_w_err; > > > > > } > > > > > mutex_lock(&srv->paths_mutex); > > > > > @@ -1850,11 +1853,13 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > > > > mutex_unlock(&srv->paths_mutex); > > > > > put_srv(srv); > > > > > err = PTR_ERR(sess); > > > > > + pr_err("RTRS server session allocation failed: %d\n", err); > > > > > goto reject_w_err; > > > > > } > > > > > } > > > > > err = create_con(sess, cm_id, cid); > > > > > if (err) { > > > > > + rtrs_err((&sess->s), "create_con(), error %d\n", err); > > > > > (void)rtrs_rdma_do_reject(cm_id, err); > > > > > > > > Unrelated to this change, but this (void) casting should be go. > > > > > > Hi Leon > > > > > > We wanted to explicitly signal that the code is ignoring the return > > > value of the function. Is there a strong reason for the casting to be > > > removed? > > > > "Don't write useless code and don't assume that the kernel developers > > are first year college students" - is this strong enough reason for you? > > I am all for simple and minimum code; but this casting does have a > decent reason. > > Anyway if the RDMA code style calls for this to be removed; will > remove them in a separate patch and send out with the next batch of > patches. Yes, please, we prefer proper APIs that don't require any special comments to use them. Thanks > > > > > Your (void) casting doesn't give anything extra, just churn and even > > more suspicious review over such code. > > > > Thanks
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c index 3d09d01e34b4..df17dd4c1e28 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c @@ -1356,8 +1356,10 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx, * If this request is not the first connection request from the * client for this session then fail and return error. */ - if (!first_conn) + if (!first_conn) { + pr_err("Error: Not the first connection request for this session\n"); return ERR_PTR(-ENXIO); + } /* need to allocate a new srv */ srv = kzalloc(sizeof(*srv), GFP_KERNEL); @@ -1812,6 +1814,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); if (IS_ERR(srv)) { err = PTR_ERR(srv); + pr_err("get_or_create_srv(), error %d\n", err); goto reject_w_err; } mutex_lock(&srv->paths_mutex); @@ -1850,11 +1853,13 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, mutex_unlock(&srv->paths_mutex); put_srv(srv); err = PTR_ERR(sess); + pr_err("RTRS server session allocation failed: %d\n", err); goto reject_w_err; } } err = create_con(sess, cm_id, cid); if (err) { + rtrs_err((&sess->s), "create_con(), error %d\n", err); (void)rtrs_rdma_do_reject(cm_id, err); /* * Since session has other connections we follow normal way @@ -1865,6 +1870,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, } err = rtrs_rdma_do_accept(sess, cm_id); if (err) { + rtrs_err((&sess->s), "rtrs_rdma_do_accept(), error %d\n", err); (void)rtrs_rdma_do_reject(cm_id, err); /* * Since current connection was successfully added to the