| Message ID | 20221113010823.6436-2-guoqing.jiang@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Misc changes for rtrs | expand |
Hi Guoqing, Thx for the patch, see comments below. On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > The ib_dev_count is supposed to track the number of added ib devices > which is only used in rtrs_srv_{add,remove}_one. > > However we only trigger rtrs_srv_add_one from rnbd_srv_init_module > -> rtrs_srv_open -> ib_register_client -> client->add which should > happen only once. client->add is call per ib_device, eg: jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_* lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_0 -> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0 lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_1 -> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1 rtrs will be call twice for mlx5_0 and mlx5_1 devices So this one is NACK And so is rtrs_srv_close since it is only called > by unload rnbd-server or failure case when load rnbd-server. > > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > --- > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 16 ---------------- > drivers/infiniband/ulp/rtrs/rtrs-srv.h | 1 - > 2 files changed, 17 deletions(-) > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > index 22d7ba05e9fe..79504aaef0cc 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > @@ -2097,8 +2097,6 @@ static int rtrs_srv_add_one(struct ib_device *device) > int ret = 0; > > mutex_lock(&ib_ctx.ib_dev_mutex); > - if (ib_ctx.ib_dev_count) > - goto out; > > /* > * Since our CM IDs are NOT bound to any ib device we will create them > @@ -2108,21 +2106,12 @@ static int rtrs_srv_add_one(struct ib_device *device) > ret = rtrs_srv_rdma_init(ctx, ib_ctx.port); > if (ret) { > /* > - * We errored out here. > * According to the ib code, if we encounter an error here then the > * error code is ignored, and no more calls to our ops are made. > */ > pr_err("Failed to initialize RDMA connection"); > - goto err_out; > } > > -out: > - /* > - * Keep a track on the number of ib devices added > - */ > - ib_ctx.ib_dev_count++; > - > -err_out: > mutex_unlock(&ib_ctx.ib_dev_mutex); > return ret; > } > @@ -2132,10 +2121,6 @@ static void rtrs_srv_remove_one(struct ib_device *device, void *client_data) > struct rtrs_srv_ctx *ctx; > > mutex_lock(&ib_ctx.ib_dev_mutex); > - ib_ctx.ib_dev_count--; > - > - if (ib_ctx.ib_dev_count) > - goto out; > > /* > * Since our CM IDs are NOT bound to any ib device we will remove them > @@ -2145,7 +2130,6 @@ static void rtrs_srv_remove_one(struct ib_device *device, void *client_data) > rdma_destroy_id(ctx->cm_id_ip); > rdma_destroy_id(ctx->cm_id_ib); > > -out: > mutex_unlock(&ib_ctx.ib_dev_mutex); > } > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h > index 2f8a638e36fa..eccc432b0715 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h > @@ -126,7 +126,6 @@ struct rtrs_srv_ib_ctx { > struct rtrs_srv_ctx *srv_ctx; > u16 port; > struct mutex ib_dev_mutex; > - int ib_dev_count; > }; > > extern struct class *rtrs_dev_class; > -- > 2.31.1 >
Hi Jinpu, On 11/14/22 3:39 PM, Jinpu Wang wrote: > Hi Guoqing, > > Thx for the patch, see comments below. > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: >> The ib_dev_count is supposed to track the number of added ib devices >> which is only used in rtrs_srv_{add,remove}_one. >> >> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module >> -> rtrs_srv_open -> ib_register_client -> client->add which should >> happen only once. > client->add is call per ib_device, eg: > jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_* > lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_0 -> > ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0 > lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_1 -> > ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1 > rtrs will be call twice for mlx5_0 and mlx5_1 devices Ah, yes. But still we can only load/unload module once, I guess it was used to avoid racy condition (concurrent loading/unloading module?), could you elaborate why it is needed? Thanks, Guoqing
On Mon, Nov 14, 2022 at 9:00 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > Hi Jinpu, > > On 11/14/22 3:39 PM, Jinpu Wang wrote: > > Hi Guoqing, > > > > Thx for the patch, see comments below. > > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > >> The ib_dev_count is supposed to track the number of added ib devices > >> which is only used in rtrs_srv_{add,remove}_one. > >> > >> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module > >> -> rtrs_srv_open -> ib_register_client -> client->add which should > >> happen only once. > > client->add is call per ib_device, eg: > > jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_* > > lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_0 -> > > ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0 > > lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_1 -> > > ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1 > > rtrs will be call twice for mlx5_0 and mlx5_1 devices > > Ah, yes. > > But still we can only load/unload module once, I guess it was used to avoid > racy condition (concurrent loading/unloading module?), could you elaborate > why it is needed? The change was introduced due to a bug report, you can follow the discussion here for the history and reason: https://lore.kernel.org/linux-rdma/20200617103732.10356-1-haris.iqbal@cloud.ionos.com/ > > Thanks, > Guoqing Thx!
On 11/14/22 4:24 PM, Jinpu Wang wrote: > On Mon, Nov 14, 2022 at 9:00 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: >> Hi Jinpu, >> >> On 11/14/22 3:39 PM, Jinpu Wang wrote: >>> Hi Guoqing, >>> >>> Thx for the patch, see comments below. >>> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: >>>> The ib_dev_count is supposed to track the number of added ib devices >>>> which is only used in rtrs_srv_{add,remove}_one. >>>> >>>> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module >>>> -> rtrs_srv_open -> ib_register_client -> client->add which should >>>> happen only once. >>> client->add is call per ib_device, eg: >>> jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_* >>> lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_0 -> >>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0 >>> lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_1 -> >>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1 >>> rtrs will be call twice for mlx5_0 and mlx5_1 devices >> Ah, yes. >> >> But still we can only load/unload module once, I guess it was used to avoid >> racy condition (concurrent loading/unloading module?), could you elaborate >> why it is needed? > The change was introduced due to a bug report, you can follow the > discussion here for the history and reason: > https://lore.kernel.org/linux-rdma/20200617103732.10356-1-haris.iqbal@cloud.ionos.com/ Thanks for the link. I probably missed something but I don't know how rnbd_server module can be initialized before cma module since we have the dependency chain as follows. INFINIBAND_RTRS_SERVER depends on INFINIBAND_ADDR_TRANS BLK_DEV_RNBD_SERVER depends on INFINIBAND_RTRS_SERVER But commit 558d52b2976b ("RDMA/rtrs-srv: Incorporate ib_register_client into rtrs server init") did mention this. "and if the rnbd_server module is initialized before RDMA cma module, a null ptr dereference occurs during the RDMA bind operation." Thanks, Guoqing
On Mon, Nov 14, 2022 at 9:45 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > On 11/14/22 4:24 PM, Jinpu Wang wrote: > > On Mon, Nov 14, 2022 at 9:00 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > >> Hi Jinpu, > >> > >> On 11/14/22 3:39 PM, Jinpu Wang wrote: > >>> Hi Guoqing, > >>> > >>> Thx for the patch, see comments below. > >>> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > >>>> The ib_dev_count is supposed to track the number of added ib devices > >>>> which is only used in rtrs_srv_{add,remove}_one. > >>>> > >>>> However we only trigger rtrs_srv_add_one from rnbd_srv_init_module > >>>> -> rtrs_srv_open -> ib_register_client -> client->add which should > >>>> happen only once. > >>> client->add is call per ib_device, eg: > >>> jwang@ps404a-1.stg:~$ ls -l /sys/class/infiniband/mlx5_* > >>> lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_0 -> > >>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.0/infiniband/mlx5_0 > >>> lrwxrwxrwx 1 root root 0 Nov 8 13:49 /sys/class/infiniband/mlx5_1 -> > >>> ../../devices/pci0000:ae/0000:ae:00.0/0000:af:00.1/infiniband/mlx5_1 > >>> rtrs will be call twice for mlx5_0 and mlx5_1 devices > >> Ah, yes. > >> > >> But still we can only load/unload module once, I guess it was used to avoid > >> racy condition (concurrent loading/unloading module?), could you elaborate > >> why it is needed? > > The change was introduced due to a bug report, you can follow the > > discussion here for the history and reason: > > https://lore.kernel.org/linux-rdma/20200617103732.10356-1-haris.iqbal@cloud.ionos.com/ > > Thanks for the link. > > I probably missed something but I don't know how rnbd_server module can be > initialized before cma module since we have the dependency chain as > follows. Hi Guoqing, One of the ways this was happening was, when one builds the RNBD/RTRS module into the kernel bzImage and then boots up the kernel. Depending on the module init sequence, if the RNBD/RTRS modules are picked before the RDMA one, then this issue would hit. With the changes, RNBD/RTRS just register itself to ib. If any devices are present at that moment, for example when the module is modprob'ed later into the kernel, then .add gets called through ib_register_client. If no devices are present, for example in case if the module is built into the bzImage, and is inserted before RDMA module, then ib_register_client simply registers RTRS, and returns without calling .add Now, when RDMA gets initialized, and it detects devices, then .add is called for each device, which will land into rtrs_srv_add_one > > INFINIBAND_RTRS_SERVER depends on INFINIBAND_ADDR_TRANS > BLK_DEV_RNBD_SERVER depends on INFINIBAND_RTRS_SERVER > > But commit 558d52b2976b ("RDMA/rtrs-srv: Incorporate ib_register_client into > rtrs server init") did mention this. > > "and if the rnbd_server module is initialized before RDMA cma module, a > null ptr > dereference occurs during the RDMA bind operation." > > Thanks, > Guoqing
Hi Haris, On 11/14/22 9:36 PM, Haris Iqbal wrote: > >>> The change was introduced due to a bug report, you can follow the >>> discussion here for the history and reason: >>> https://lore.kernel.org/linux-rdma/20200617103732.10356-1-haris.iqbal@cloud.ionos.com/ >> Thanks for the link. >> >> I probably missed something but I don't know how rnbd_server module can be >> initialized before cma module since we have the dependency chain as >> follows. > Hi Guoqing, > > One of the ways this was happening was, when one builds the RNBD/RTRS > module into the kernel bzImage and then boots up the kernel. Depending > on the module init sequence, if the RNBD/RTRS modules are picked > before the RDMA one, then this issue would hit. This is what I missed since I usually build RNBD with M. > With the changes, RNBD/RTRS just register itself to ib. If any devices > are present at that moment, for example when the module is modprob'ed > later into the kernel, then .add gets called through > ib_register_client. If no devices are present, for example in case if > the module is built into the bzImage, and is inserted before RDMA > module, then ib_register_client simply registers RTRS, and returns > without calling .add > Now, when RDMA gets initialized, and it detects devices, then .add is > called for each device, which will land into rtrs_srv_add_one Thanks for the details! It explains well why ib_register_client is added, and rtrs_srv_rdma_init is guaranteed to be called once too. Will drop this one. Thanks, Guoqings
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c index 22d7ba05e9fe..79504aaef0cc 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c @@ -2097,8 +2097,6 @@ static int rtrs_srv_add_one(struct ib_device *device) int ret = 0; mutex_lock(&ib_ctx.ib_dev_mutex); - if (ib_ctx.ib_dev_count) - goto out; /* * Since our CM IDs are NOT bound to any ib device we will create them @@ -2108,21 +2106,12 @@ static int rtrs_srv_add_one(struct ib_device *device) ret = rtrs_srv_rdma_init(ctx, ib_ctx.port); if (ret) { /* - * We errored out here. * According to the ib code, if we encounter an error here then the * error code is ignored, and no more calls to our ops are made. */ pr_err("Failed to initialize RDMA connection"); - goto err_out; } -out: - /* - * Keep a track on the number of ib devices added - */ - ib_ctx.ib_dev_count++; - -err_out: mutex_unlock(&ib_ctx.ib_dev_mutex); return ret; } @@ -2132,10 +2121,6 @@ static void rtrs_srv_remove_one(struct ib_device *device, void *client_data) struct rtrs_srv_ctx *ctx; mutex_lock(&ib_ctx.ib_dev_mutex); - ib_ctx.ib_dev_count--; - - if (ib_ctx.ib_dev_count) - goto out; /* * Since our CM IDs are NOT bound to any ib device we will remove them @@ -2145,7 +2130,6 @@ static void rtrs_srv_remove_one(struct ib_device *device, void *client_data) rdma_destroy_id(ctx->cm_id_ip); rdma_destroy_id(ctx->cm_id_ib); -out: mutex_unlock(&ib_ctx.ib_dev_mutex); } diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.h b/drivers/infiniband/ulp/rtrs/rtrs-srv.h index 2f8a638e36fa..eccc432b0715 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.h +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.h @@ -126,7 +126,6 @@ struct rtrs_srv_ib_ctx { struct rtrs_srv_ctx *srv_ctx; u16 port; struct mutex ib_dev_mutex; - int ib_dev_count; }; extern struct class *rtrs_dev_class;
The ib_dev_count is supposed to track the number of added ib devices which is only used in rtrs_srv_{add,remove}_one. However we only trigger rtrs_srv_add_one from rnbd_srv_init_module -> rtrs_srv_open -> ib_register_client -> client->add which should happen only once. And so is rtrs_srv_close since it is only called by unload rnbd-server or failure case when load rnbd-server. Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> --- drivers/infiniband/ulp/rtrs/rtrs-srv.c | 16 ---------------- drivers/infiniband/ulp/rtrs/rtrs-srv.h | 1 - 2 files changed, 17 deletions(-)