diff mbox series

[v2,13/14] RDMA/rxe: Rename kfree_rcu() to kvfree_rcu_mightsleep()

Message ID 20230315181902.4177819-13-joel@joelfernandes.org (mailing list archive)
State Not Applicable
Headers show
Series None | expand

Commit Message

Joel Fernandes March 15, 2023, 6:19 p.m. UTC 
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(-)

Comments

Bob Pearson March 15, 2023, 6:38 p.m. UTC | #1 
On 3/15/23 13:19, Joel Fernandes (Google) wrote:
> 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;
>  }
>  
Please replace kfree_rcu_mightsleep() by kfree(). There is no need to delay the kfree(mr).

Bob
Joel Fernandes March 15, 2023, 6:41 p.m. UTC | #2 
On Wed, Mar 15, 2023 at 2:38 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 3/15/23 13:19, Joel Fernandes (Google) wrote:
> > 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;
> >  }
> >
> Please replace kfree_rcu_mightsleep() by kfree(). There is no need to delay the kfree(mr).

I thought you said either was Ok, but yeah that's fine with me. I was
trying to play it safe ;-). An extra GP may not hurt, but not having
one when it is needed might. I will update it to just be kfree.

<quote>
>   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
</quote>

 - Joel
Bob Pearson March 15, 2023, 7:05 p.m. UTC | #3 
On 3/15/23 13:41, Joel Fernandes wrote:
> On Wed, Mar 15, 2023 at 2:38 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>>
>> On 3/15/23 13:19, Joel Fernandes (Google) wrote:
>>> 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;
>>>  }
>>>
>> Please replace kfree_rcu_mightsleep() by kfree(). There is no need to delay the kfree(mr).
> 
> I thought you said either was Ok, but yeah that's fine with me. I was
> trying to play it safe ;-). An extra GP may not hurt, but not having
> one when it is needed might. I will update it to just be kfree.

Thanks. The reason to not add the pause is that it really isn't required and will just make
people think that it is. With the current state of the driver the mr cleanup code will disable
looking up the mr from its rkey or lkey and then wait until all the references to the mr are dropped
before calling kfree. This will always work (unless a new bug is introduced) so no reason to
add the rcu delay.

Bob
> 
> <quote>
>>   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
> </quote>
> 
>  - Joel
Joel Fernandes March 15, 2023, 7:59 p.m. UTC | #4 
On Wed, Mar 15, 2023 at 3:05 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 3/15/23 13:41, Joel Fernandes wrote:
> > On Wed, Mar 15, 2023 at 2:38 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >>
> >> On 3/15/23 13:19, Joel Fernandes (Google) wrote:
> >>> 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;
> >>>  }
> >>>
> >> Please replace kfree_rcu_mightsleep() by kfree(). There is no need to delay the kfree(mr).
> >
> > I thought you said either was Ok, but yeah that's fine with me. I was
> > trying to play it safe ;-). An extra GP may not hurt, but not having
> > one when it is needed might. I will update it to just be kfree.
>
> Thanks. The reason to not add the pause is that it really isn't required and will just make
> people think that it is. With the current state of the driver the mr cleanup code will disable
> looking up the mr from its rkey or lkey and then wait until all the references to the mr are dropped
> before calling kfree. This will always work (unless a new bug is introduced) so no reason to
> add the rcu delay.
>

Sounds good! I updated it in my queue, and I will repost it in the
future pending test results and other comments.

thanks,

 - Joel
diff mbox series

Patch

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