Re: [PATCH 07/13] RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()

Re: [PATCH 07/13] RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()

[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

Re: [PATCH 07/13] RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()

[Joel Fernandes]

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;
 }
 

Re: [PATCH 07/13] RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()

[Bob Pearson]

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>

Re: [PATCH 07/13] RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()

[Zhu Yanjun]

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

Re: [PATCH 07/13] RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()

[Joel Fernandes]

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;
 }
 
-- 
2.40.0.rc1.284.g88254d51c5-goog

Re: [PATCH 07/13] RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()

[Bob Pearson]

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