| Message ID | 20221113010823.6436-11-guoqing.jiang@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Misc changes for rtrs | expand |
On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > The paths_num is only increased by rtrs_rdma_connect -> __alloc_path > which is only one time thing, so is the decreasing of it given only > rtrs_srv_close_work -> del_path_from_srv, which means paths_num should > always be 1. It would actually go up to the number of paths a session will have in a multipath setup. It is the exact counter part of paths_num in the structure rtrs_clt_sess. But whereas on the client side, the number is used to access the path list for making decisions in multipathing IO like round-robin, etc. On the server side, I don't see the use of it. Maybe just for sanity checks. @Jinpu Any thoughts? > > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > --- > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 -------- > drivers/infiniband/ulp/rtrs/rtrs-srv.h | 1 - > 2 files changed, 9 deletions(-) > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > index e2ea09a8def7..400cf8ae34a3 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > @@ -1437,8 +1437,6 @@ static void __add_path_to_srv(struct rtrs_srv_sess *srv, > struct rtrs_srv_path *srv_path) > { > list_add_tail(&srv_path->s.entry, &srv->paths_list); > - srv->paths_num++; > - WARN_ON(srv->paths_num >= MAX_PATHS_NUM); > } > > static void del_path_from_srv(struct rtrs_srv_path *srv_path) > @@ -1450,8 +1448,6 @@ static void del_path_from_srv(struct rtrs_srv_path *srv_path) > > mutex_lock(&srv->paths_mutex); > list_del(&srv_path->s.entry); > - WARN_ON(!srv->paths_num); > - srv->paths_num--; > mutex_unlock(&srv->paths_mutex); > } > > @@ -1719,10 +1715,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv, > char str[NAME_MAX]; > struct rtrs_addr path; > > - if (srv->paths_num >= MAX_PATHS_NUM) { > - err = -ECONNRESET; > - goto err; > - } > if (__is_path_w_addr_exists(srv, &cm_id->route.addr)) { > err = -EEXIST; > pr_err("Path with same addr exists\n"); > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h > index eccc432b0715..8e4fcb578f49 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h > @@ -100,7 +100,6 @@ struct rtrs_srv_sess { > struct list_head paths_list; > int paths_up; > struct mutex paths_ev_mutex; > - size_t paths_num; > struct mutex paths_mutex; > uuid_t paths_uuid; > refcount_t refcount; > -- > 2.31.1 >
On Tue, Nov 15, 2022 at 10:49 AM Haris Iqbal <haris.iqbal@ionos.com> wrote: > > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > The paths_num is only increased by rtrs_rdma_connect -> __alloc_path > > which is only one time thing, so is the decreasing of it given only > > rtrs_srv_close_work -> del_path_from_srv, which means paths_num should > > always be 1. > > It would actually go up to the number of paths a session will have in > a multipath setup. It is the exact counter part of paths_num in the > structure rtrs_clt_sess. But whereas on the client side, the number is > used to access the path list for making decisions in multipathing IO > like round-robin, etc. On the server side, I don't see the use of it. > Maybe just for sanity checks. > > @Jinpu Any thoughts? Yes, the idea is RTRS can have many paths, not only one, and you can add/remove path at run time, so not a one time thing. > > > > > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > > --- > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 -------- > > drivers/infiniband/ulp/rtrs/rtrs-srv.h | 1 - > > 2 files changed, 9 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > index e2ea09a8def7..400cf8ae34a3 100644 > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > > @@ -1437,8 +1437,6 @@ static void __add_path_to_srv(struct rtrs_srv_sess *srv, > > struct rtrs_srv_path *srv_path) > > { > > list_add_tail(&srv_path->s.entry, &srv->paths_list); > > - srv->paths_num++; > > - WARN_ON(srv->paths_num >= MAX_PATHS_NUM); > > } > > > > static void del_path_from_srv(struct rtrs_srv_path *srv_path) > > @@ -1450,8 +1448,6 @@ static void del_path_from_srv(struct rtrs_srv_path *srv_path) > > > > mutex_lock(&srv->paths_mutex); > > list_del(&srv_path->s.entry); > > - WARN_ON(!srv->paths_num); > > - srv->paths_num--; > > mutex_unlock(&srv->paths_mutex); > > } > > > > @@ -1719,10 +1715,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv, > > char str[NAME_MAX]; > > struct rtrs_addr path; > > > > - if (srv->paths_num >= MAX_PATHS_NUM) { > > - err = -ECONNRESET; > > - goto err; > > - } > > if (__is_path_w_addr_exists(srv, &cm_id->route.addr)) { > > err = -EEXIST; > > pr_err("Path with same addr exists\n"); > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h > > index eccc432b0715..8e4fcb578f49 100644 > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h > > @@ -100,7 +100,6 @@ struct rtrs_srv_sess { > > struct list_head paths_list; > > int paths_up; > > struct mutex paths_ev_mutex; > > - size_t paths_num; > > struct mutex paths_mutex; > > uuid_t paths_uuid; > > refcount_t refcount; > > -- > > 2.31.1 > >
On 11/15/22 6:01 PM, Jinpu Wang wrote: > On Tue, Nov 15, 2022 at 10:49 AM Haris Iqbal <haris.iqbal@ionos.com> wrote: >> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: >>> The paths_num is only increased by rtrs_rdma_connect -> __alloc_path >>> which is only one time thing, so is the decreasing of it given only >>> rtrs_srv_close_work -> del_path_from_srv, which means paths_num should >>> always be 1. >> It would actually go up to the number of paths a session will have in >> a multipath setup. It is the exact counter part of paths_num in the >> structure rtrs_clt_sess. But whereas on the client side, the number is >> used to access the path list for making decisions in multipathing IO >> like round-robin, etc. On the server side, I don't see the use of it. >> Maybe just for sanity checks. >> >> @Jinpu Any thoughts? > Yes, the idea is RTRS can have many paths, not only one, and you can > add/remove path at run time, > so not a one time thing. Got it, thanks for your review. Guoqing
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c index e2ea09a8def7..400cf8ae34a3 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c @@ -1437,8 +1437,6 @@ static void __add_path_to_srv(struct rtrs_srv_sess *srv, struct rtrs_srv_path *srv_path) { list_add_tail(&srv_path->s.entry, &srv->paths_list); - srv->paths_num++; - WARN_ON(srv->paths_num >= MAX_PATHS_NUM); } static void del_path_from_srv(struct rtrs_srv_path *srv_path) @@ -1450,8 +1448,6 @@ static void del_path_from_srv(struct rtrs_srv_path *srv_path) mutex_lock(&srv->paths_mutex); list_del(&srv_path->s.entry); - WARN_ON(!srv->paths_num); - srv->paths_num--; mutex_unlock(&srv->paths_mutex); } @@ -1719,10 +1715,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv, char str[NAME_MAX]; struct rtrs_addr path; - if (srv->paths_num >= MAX_PATHS_NUM) { - err = -ECONNRESET; - goto err; - } if (__is_path_w_addr_exists(srv, &cm_id->route.addr)) { err = -EEXIST; pr_err("Path with same addr exists\n"); diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h index eccc432b0715..8e4fcb578f49 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h @@ -100,7 +100,6 @@ struct rtrs_srv_sess { struct list_head paths_list; int paths_up; struct mutex paths_ev_mutex; - size_t paths_num; struct mutex paths_mutex; uuid_t paths_uuid; refcount_t refcount;
The paths_num is only increased by rtrs_rdma_connect -> __alloc_path which is only one time thing, so is the decreasing of it given only rtrs_srv_close_work -> del_path_from_srv, which means paths_num should always be 1. Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> --- drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 -------- drivers/infiniband/ulp/rtrs/rtrs-srv.h | 1 - 2 files changed, 9 deletions(-)