| Message ID | 20230201150815.409582-8-urezki@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c86b2b032bd1a5bd8136fdc31ee9c9d452d062f0 |
| Headers | show |
| Series | Rename k[v]free_rcu() single argument to k[v]free_rcu_mightsleep() | expand |
On Wed, Feb 01, 2023 at 04:08:13PM +0100, Uladzislau Rezki (Sony) wrote: > The kfree_rcu()'s single argument name is deprecated therefore > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly > underline that it is for sleepable contexts. > > Please check the RXE driver in a way that a single argument can > be used. Briefly looking at it and rcu_head should be embed to > free an obj over RCU-core. The context might be atomic. > > Cc: Bob Pearson <rpearsonhpe@gmail.com> > Cc: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > --- > drivers/infiniband/sw/rxe/rxe_pool.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Could you please add you reviwed-by or Acked-by tags so we can bring our series with renaming for the next merge window? Thanks! -- Uladzislau Rezki
> On Wed, Feb 01, 2023 at 04:08:13PM +0100, Uladzislau Rezki (Sony) wrote: > > The kfree_rcu()'s single argument name is deprecated therefore > > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly > > underline that it is for sleepable contexts. > > > > Please check the RXE driver in a way that a single argument can > > be used. Briefly looking at it and rcu_head should be embed to > > free an obj over RCU-core. The context might be atomic. > > > > Cc: Bob Pearson <rpearsonhpe@gmail.com> > > Cc: Jason Gunthorpe <jgg@nvidia.com> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > --- > > drivers/infiniband/sw/rxe/rxe_pool.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > Could you please add you reviwed-by or Acked-by tags so we can bring > our series with renaming for the next merge window? > > Thanks! > __rxe_cleanup() can be called in two contexts, sleepable and not. Therefore usage of a single argument of the kvfree_rcu() is not correct here. Could you please fix and check your driver? If my above statement is not correct, please provide Acked-by or Reviwed-by tags to the path that is in question. Otherwise please add an rcu_head in your data to free objects over kvfree_rcu() using double argument API. Could you please support? -- Uladzislau Rezki
On Thu, Mar 09, 2023 at 03:13:08PM +0100, Uladzislau Rezki wrote: > > On Wed, Feb 01, 2023 at 04:08:13PM +0100, Uladzislau Rezki (Sony) wrote: > > > The kfree_rcu()'s single argument name is deprecated therefore > > > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly > > > underline that it is for sleepable contexts. > > > > > > Please check the RXE driver in a way that a single argument can > > > be used. Briefly looking at it and rcu_head should be embed to > > > free an obj over RCU-core. The context might be atomic. > > > > > > Cc: Bob Pearson <rpearsonhpe@gmail.com> > > > Cc: Jason Gunthorpe <jgg@nvidia.com> > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > > > --- > > > drivers/infiniband/sw/rxe/rxe_pool.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > Could you please add you reviwed-by or Acked-by tags so we can bring > > our series with renaming for the next merge window? > > > > Thanks! > > > __rxe_cleanup() can be called in two contexts, sleepable and not. > Therefore usage of a single argument of the kvfree_rcu() is not correct > here. > > Could you please fix and check your driver? If my above statement > is not correct, please provide Acked-by or Reviwed-by tags to the > path that is in question. > > Otherwise please add an rcu_head in your data to free objects over > kvfree_rcu() using double argument API. > > Could you please support? Also this one needs renaming? It came in because of the commit in 6.3-rc1: 72a03627443d ("RDMA/rxe: Remove rxe_alloc()") It could be squashed into this patch itself since it is infiniband related. Paul noticed that this breaks dropping the old API on -next, so it is blocking the renaming. ---8<----------------------- diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c index b10aa1580a64..ae3a100e18fb 100644 --- a/drivers/infiniband/sw/rxe/rxe_mr.c +++ b/drivers/infiniband/sw/rxe/rxe_mr.c @@ -731,7 +731,7 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata) return -EINVAL; rxe_cleanup(mr); - kfree_rcu(mr); + kfree_rcu_mightsleep(mr); return 0; }
On 3/9/23 18:55, Joel Fernandes wrote: > On Thu, Mar 09, 2023 at 03:13:08PM +0100, Uladzislau Rezki wrote: >>> On Wed, Feb 01, 2023 at 04:08:13PM +0100, Uladzislau Rezki (Sony) wrote: >>>> The kfree_rcu()'s single argument name is deprecated therefore >>>> rename it to kfree_rcu_mightsleep() variant. The goal is explicitly >>>> underline that it is for sleepable contexts. >>>> >>>> Please check the RXE driver in a way that a single argument can >>>> be used. Briefly looking at it and rcu_head should be embed to >>>> free an obj over RCU-core. The context might be atomic. >>>> >>>> Cc: Bob Pearson <rpearsonhpe@gmail.com> >>>> Cc: Jason Gunthorpe <jgg@nvidia.com> >>>> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> >>>> --- >>>> drivers/infiniband/sw/rxe/rxe_pool.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>> Could you please add you reviwed-by or Acked-by tags so we can bring >>> our series with renaming for the next merge window? >>> >>> Thanks! >>> >> __rxe_cleanup() can be called in two contexts, sleepable and not. >> Therefore usage of a single argument of the kvfree_rcu() is not correct >> here. >> >> Could you please fix and check your driver? If my above statement >> is not correct, please provide Acked-by or Reviwed-by tags to the >> path that is in question. >> >> Otherwise please add an rcu_head in your data to free objects over >> kvfree_rcu() using double argument API. >> >> Could you please support? > > Also this one needs renaming? It came in because of the commit in 6.3-rc1: > 72a03627443d ("RDMA/rxe: Remove rxe_alloc()") > > It could be squashed into this patch itself since it is infiniband related. > > Paul noticed that this breaks dropping the old API on -next, so it is > blocking the renaming. > > ---8<----------------------- > > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c > index b10aa1580a64..ae3a100e18fb 100644 > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > @@ -731,7 +731,7 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata) > return -EINVAL; > > rxe_cleanup(mr); > - kfree_rcu(mr); > + kfree_rcu_mightsleep(mr); > return 0; > } > I just got back from a 1 week vacation and missed all this. The "RDMA/rxe: Remove rxe_alloc()" patch just moved the memory allocation for MR (verbs) objects outside of the rxe_pool code since it only applied to MRs and not the other verbs objects (AH, QP, CQ, ...). That code has to handle a unique situation for AH objects which can be created or destroyed by connection manager code in atomic context while all the other ones including MRs are always created/destroyed in process context. All objects other than MR's are created/destroyed in the rdma-core code (drivers/infiniband/core). The rxe driver keeps xarray's of pointers to the various objects which are protected by rcu locking and so it made sense to use kfree_rcu to delete the object with a delay. In the MR case ..._mightsleep seems harmless and should not be an issue. However on reflection, all the references to the MR objects are ref counted and they have been dropped before reaching the kfree and so there really never was a good reason to use kfree_rcu in the first place. So a better solution would be to replace kfree_rcu with kfree. There is a timeout in completion_done() that triggers a WARN_ON() and this is only seen if the driver is broken for some reason but that is equivalent to getting a seg fault so no reason to further delay the kfree. Reviewed-by: Bob Pearson <rpearsonhpe@gmail.com>
On Thu, Mar 9, 2023 at 10:13 PM Uladzislau Rezki <urezki@gmail.com> wrote: > > > On Wed, Feb 01, 2023 at 04:08:13PM +0100, Uladzislau Rezki (Sony) wrote: > > > The kfree_rcu()'s single argument name is deprecated therefore > > > rename it to kfree_rcu_mightsleep() variant. The goal is explicitly > > > underline that it is for sleepable contexts. > > > > > > Please check the RXE driver in a way that a single argument can > > > be used. Briefly looking at it and rcu_head should be embed to > > > free an obj over RCU-core. The context might be atomic. > > > > > > Cc: Bob Pearson <rpearsonhpe@gmail.com> > > > Cc: Jason Gunthorpe <jgg@nvidia.com> > > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> Thanks. Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com> Zhu Yanjun > > > --- > > > drivers/infiniband/sw/rxe/rxe_pool.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > Could you please add you reviwed-by or Acked-by tags so we can bring > > our series with renaming for the next merge window? > > > > Thanks! > > > __rxe_cleanup() can be called in two contexts, sleepable and not. > Therefore usage of a single argument of the kvfree_rcu() is not correct > here. > > Could you please fix and check your driver? If my above statement > is not correct, please provide Acked-by or Reviwed-by tags to the > path that is in question. > > Otherwise please add an rcu_head in your data to free objects over > kvfree_rcu() using double argument API. > > Could you please support? > > -- > Uladzislau Rezki
On Mon, Mar 13, 2023 at 02:43:43PM -0500, Bob Pearson wrote: > On 3/9/23 18:55, Joel Fernandes wrote: > > On Thu, Mar 09, 2023 at 03:13:08PM +0100, Uladzislau Rezki wrote: > >>> On Wed, Feb 01, 2023 at 04:08:13PM +0100, Uladzislau Rezki (Sony) wrote: > >>>> The kfree_rcu()'s single argument name is deprecated therefore > >>>> rename it to kfree_rcu_mightsleep() variant. The goal is explicitly > >>>> underline that it is for sleepable contexts. > >>>> > >>>> Please check the RXE driver in a way that a single argument can > >>>> be used. Briefly looking at it and rcu_head should be embed to > >>>> free an obj over RCU-core. The context might be atomic. > >>>> > >>>> Cc: Bob Pearson <rpearsonhpe@gmail.com> > >>>> Cc: Jason Gunthorpe <jgg@nvidia.com> > >>>> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > >>>> --- > >>>> drivers/infiniband/sw/rxe/rxe_pool.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>> Could you please add you reviwed-by or Acked-by tags so we can bring > >>> our series with renaming for the next merge window? > >>> > >>> Thanks! > >>> > >> __rxe_cleanup() can be called in two contexts, sleepable and not. > >> Therefore usage of a single argument of the kvfree_rcu() is not correct > >> here. > >> > >> Could you please fix and check your driver? If my above statement > >> is not correct, please provide Acked-by or Reviwed-by tags to the > >> path that is in question. > >> > >> Otherwise please add an rcu_head in your data to free objects over > >> kvfree_rcu() using double argument API. > >> > >> Could you please support? > > > > Also this one needs renaming? It came in because of the commit in 6.3-rc1: > > 72a03627443d ("RDMA/rxe: Remove rxe_alloc()") > > > > It could be squashed into this patch itself since it is infiniband related. > > > > Paul noticed that this breaks dropping the old API on -next, so it is > > blocking the renaming. > > > > ---8<----------------------- > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c > > index b10aa1580a64..ae3a100e18fb 100644 > > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > > @@ -731,7 +731,7 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata) > > return -EINVAL; > > > > rxe_cleanup(mr); > > - kfree_rcu(mr); > > + kfree_rcu_mightsleep(mr); > > return 0; > > } > > > I just got back from a 1 week vacation and missed all this. > > The "RDMA/rxe: Remove rxe_alloc()" patch just moved the memory allocation > for MR (verbs) objects outside of the rxe_pool code since it only applied > to MRs and not the other verbs objects (AH, QP, CQ, ...). That code has to > handle a unique situation for AH objects which can be created or destroyed > by connection manager code in atomic context while all the other ones > including MRs are always created/destroyed in process context. All objects > other than MR's are created/destroyed in the rdma-core code > (drivers/infiniband/core). > > The rxe driver keeps xarray's of pointers to the various objects which are > protected by rcu locking and so it made sense to use kfree_rcu to delete > the object with a delay. In the MR case ..._mightsleep seems harmless and > should not be an issue. > > However on reflection, all the references to the MR objects are ref counted > and they have been dropped before reaching the kfree and so there really > never was a good reason to use kfree_rcu in the first place. So a better > solution would be to replace kfree_rcu with kfree. There is a timeout in > completion_done() that triggers a WARN_ON() and this is only seen if the > driver is broken for some reason but that is equivalent to getting a seg > fault so no reason to further delay the kfree. > > Reviewed-by: Bob Pearson <rpearsonhpe@gmail.com> Thanks, I am planning to send the following patch for 6.4 consideration, please let me know if you disagree. Still testing it. ----8<--- From: Joel Fernandes (Google) <joel@joelfernandes.org> Subject: [PATCH] RDMA/rxe: Rename kfree_rcu() to kvfree_rcu_mightsleep() The k[v]free_rcu() macro's single-argument form is deprecated. Therefore switch to the new k[v]free_rcu_mightsleep() variant. The goal is to avoid accidental use of the single-argument forms, which can introduce functionality bugs in atomic contexts and latency bugs in non-atomic contexts. There is no functionality change with this patch. Link: https://lore.kernel.org/rcu/20230201150815.409582-1-urezki@gmail.com Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com> Reviewed-by: Bob Pearson <rpearsonhpe@gmail.com> Reviewed-by: Paul E. McKenney <paulmck@kernel.org> Fixes: 72a03627443d ("RDMA/rxe: Remove rxe_alloc()") Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- drivers/infiniband/sw/rxe/rxe_mr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c index b10aa1580a64..ae3a100e18fb 100644 --- a/drivers/infiniband/sw/rxe/rxe_mr.c +++ b/drivers/infiniband/sw/rxe/rxe_mr.c @@ -731,7 +731,7 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata) return -EINVAL; rxe_cleanup(mr); - kfree_rcu(mr); + kfree_rcu_mightsleep(mr); return 0; }
On 3/15/23 06:50, Joel Fernandes wrote: > On Mon, Mar 13, 2023 at 02:43:43PM -0500, Bob Pearson wrote: >> On 3/9/23 18:55, Joel Fernandes wrote: >>> On Thu, Mar 09, 2023 at 03:13:08PM +0100, Uladzislau Rezki wrote: >>>>> On Wed, Feb 01, 2023 at 04:08:13PM +0100, Uladzislau Rezki (Sony) wrote: >>>>>> The kfree_rcu()'s single argument name is deprecated therefore >>>>>> rename it to kfree_rcu_mightsleep() variant. The goal is explicitly >>>>>> underline that it is for sleepable contexts. >>>>>> >>>>>> Please check the RXE driver in a way that a single argument can >>>>>> be used. Briefly looking at it and rcu_head should be embed to >>>>>> free an obj over RCU-core. The context might be atomic. >>>>>> >>>>>> Cc: Bob Pearson <rpearsonhpe@gmail.com> >>>>>> Cc: Jason Gunthorpe <jgg@nvidia.com> >>>>>> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> >>>>>> --- >>>>>> drivers/infiniband/sw/rxe/rxe_pool.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>> Could you please add you reviwed-by or Acked-by tags so we can bring >>>>> our series with renaming for the next merge window? >>>>> >>>>> Thanks! >>>>> >>>> __rxe_cleanup() can be called in two contexts, sleepable and not. >>>> Therefore usage of a single argument of the kvfree_rcu() is not correct >>>> here. >>>> >>>> Could you please fix and check your driver? If my above statement >>>> is not correct, please provide Acked-by or Reviwed-by tags to the >>>> path that is in question. >>>> >>>> Otherwise please add an rcu_head in your data to free objects over >>>> kvfree_rcu() using double argument API. >>>> >>>> Could you please support? >>> >>> Also this one needs renaming? It came in because of the commit in 6.3-rc1: >>> 72a03627443d ("RDMA/rxe: Remove rxe_alloc()") >>> >>> It could be squashed into this patch itself since it is infiniband related. >>> >>> Paul noticed that this breaks dropping the old API on -next, so it is >>> blocking the renaming. >>> >>> ---8<----------------------- >>> >>> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c >>> index b10aa1580a64..ae3a100e18fb 100644 >>> --- a/drivers/infiniband/sw/rxe/rxe_mr.c >>> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c >>> @@ -731,7 +731,7 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata) >>> return -EINVAL; >>> >>> rxe_cleanup(mr); >>> - kfree_rcu(mr); >>> + kfree_rcu_mightsleep(mr); >>> return 0; >>> } >>> >> I just got back from a 1 week vacation and missed all this. >> >> The "RDMA/rxe: Remove rxe_alloc()" patch just moved the memory allocation >> for MR (verbs) objects outside of the rxe_pool code since it only applied >> to MRs and not the other verbs objects (AH, QP, CQ, ...). That code has to >> handle a unique situation for AH objects which can be created or destroyed >> by connection manager code in atomic context while all the other ones >> including MRs are always created/destroyed in process context. All objects >> other than MR's are created/destroyed in the rdma-core code >> (drivers/infiniband/core). >> >> The rxe driver keeps xarray's of pointers to the various objects which are >> protected by rcu locking and so it made sense to use kfree_rcu to delete >> the object with a delay. In the MR case ..._mightsleep seems harmless and >> should not be an issue. >> >> However on reflection, all the references to the MR objects are ref counted >> and they have been dropped before reaching the kfree and so there really >> never was a good reason to use kfree_rcu in the first place. So a better >> solution would be to replace kfree_rcu with kfree. There is a timeout in >> completion_done() that triggers a WARN_ON() and this is only seen if the >> driver is broken for some reason but that is equivalent to getting a seg >> fault so no reason to further delay the kfree. >> >> Reviewed-by: Bob Pearson <rpearsonhpe@gmail.com> > > Thanks, I am planning to send the following patch for 6.4 consideration, > please let me know if you disagree. Still testing it. > > ----8<--- > > From: Joel Fernandes (Google) <joel@joelfernandes.org> > Subject: [PATCH] RDMA/rxe: Rename kfree_rcu() to kvfree_rcu_mightsleep() > > The k[v]free_rcu() macro's single-argument form is deprecated. > Therefore switch to the new k[v]free_rcu_mightsleep() variant. The goal > is to avoid accidental use of the single-argument forms, which can > introduce functionality bugs in atomic contexts and latency bugs in > non-atomic contexts. > > There is no functionality change with this patch. > > Link: https://lore.kernel.org/rcu/20230201150815.409582-1-urezki@gmail.com > Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com> > Reviewed-by: Bob Pearson <rpearsonhpe@gmail.com> > Reviewed-by: Paul E. McKenney <paulmck@kernel.org> > Fixes: 72a03627443d ("RDMA/rxe: Remove rxe_alloc()") > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > drivers/infiniband/sw/rxe/rxe_mr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c > index b10aa1580a64..ae3a100e18fb 100644 > --- a/drivers/infiniband/sw/rxe/rxe_mr.c > +++ b/drivers/infiniband/sw/rxe/rxe_mr.c > @@ -731,7 +731,7 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata) > return -EINVAL; > > rxe_cleanup(mr); > - kfree_rcu(mr); > + kfree_rcu_mightsleep(mr); > return 0; > } > I would prefer just - kfree_rcu(mr); + kfree(mr); but either one will work. Bob
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c index f50620f5a0a1..e2fa061f19b3 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.c +++ b/drivers/infiniband/sw/rxe/rxe_pool.c @@ -276,7 +276,7 @@ int __rxe_cleanup(struct rxe_pool_elem *elem, bool sleepable) pool->cleanup(elem); if (pool->type == RXE_TYPE_MR) - kfree_rcu(elem->obj); + kfree_rcu_mightsleep(elem->obj); atomic_dec(&pool->num_elem);
The kfree_rcu()'s single argument name is deprecated therefore rename it to kfree_rcu_mightsleep() variant. The goal is explicitly underline that it is for sleepable contexts. Please check the RXE driver in a way that a single argument can be used. Briefly looking at it and rcu_head should be embed to free an obj over RCU-core. The context might be atomic. Cc: Bob Pearson <rpearsonhpe@gmail.com> Cc: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> --- drivers/infiniband/sw/rxe/rxe_pool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)