[00/13] Rename k[v]free_rcu() single argument to k[v]free_rcu_mightsleep()

Message

Uladzislau Rezki Feb. 1, 2023, 3:08 p.m. UTC

This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.

1.
The problem is that, recently we have run into a precedent when
a user intended to give a second argument to kfree_rcu() API but
forgot to do it in a code so a call became as a single argument
of kfree_rcu() API.

2.
Such mistyping can lead to hidden bags where sleeping is forbidden.

3.
_mightsleep() prefix gives much more information for which contexts
it can be used for.

Uladzislau Rezki (Sony) (13):
  rcu/kvfree: Add kvfree_rcu_mightsleep() and kfree_rcu_mightsleep()
  drbd: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
  misc: vmw_vmci: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
  tracing: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
  lib/test_vmalloc.c: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
  net/sysctl: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
  RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()
  net/mlx5: Rename kfree_rcu() to kfree_rcu_mightsleep()
  ext4/super: Rename kfree_rcu() to kfree_rcu_mightsleep()
  ipvs: Rename kfree_rcu() to kfree_rcu_mightsleep()
  rcuscale: Rename kfree_rcu() to kfree_rcu_mightsleep()
  doc: Update whatisRCU.rst
  rcu/kvfree: Eliminate k[v]free_rcu() single argument macro

 Documentation/RCU/whatisRCU.rst               |  6 ++--
 drivers/block/drbd/drbd_nl.c                  |  6 ++--
 drivers/block/drbd/drbd_receiver.c            |  4 +--
 drivers/block/drbd/drbd_state.c               |  2 +-
 drivers/infiniband/sw/rxe/rxe_pool.c          |  2 +-
 drivers/misc/vmw_vmci/vmci_context.c          |  2 +-
 drivers/misc/vmw_vmci/vmci_event.c            |  2 +-
 .../mellanox/mlx5/core/en/tc/int_port.c       |  2 +-
 .../mellanox/mlx5/core/en_accel/macsec.c      |  4 +--
 fs/ext4/super.c                               |  2 +-
 include/linux/rcupdate.h                      | 28 ++++++-------------
 kernel/rcu/rcuscale.c                         |  2 +-
 kernel/trace/trace_osnoise.c                  |  2 +-
 kernel/trace/trace_probe.c                    |  2 +-
 lib/test_vmalloc.c                            |  2 +-
 net/core/sysctl_net_core.c                    |  4 +--
 net/netfilter/ipvs/ip_vs_est.c                |  2 +-
 17 files changed, 32 insertions(+), 42 deletions(-)

Comments

Paul E. McKenney Feb. 1, 2023, 7:12 p.m. UTC | #1

On Wed, Feb 01, 2023 at 04:08:06PM +0100, Uladzislau Rezki (Sony) wrote:
> This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
> it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
> 
> 1.
> The problem is that, recently we have run into a precedent when
> a user intended to give a second argument to kfree_rcu() API but
> forgot to do it in a code so a call became as a single argument
> of kfree_rcu() API.
> 
> 2.
> Such mistyping can lead to hidden bags where sleeping is forbidden.
> 
> 3.
> _mightsleep() prefix gives much more information for which contexts
> it can be used for.

Thank you!!!

I have queued these (aside from 10/13, which is being replaced by a
patch from Julian Anastasov) for further testing and review.  And with
the usual wordsmithing.

If testing goes well, I might try to get 1/13 into the next merge window,
which would simplify things if maintainers want to take their patches
separately.

							Thanx, Paul

Uladzislau Rezki Feb. 2, 2023, 3:54 p.m. UTC | #2

On Wed, Feb 01, 2023 at 11:12:11AM -0800, Paul E. McKenney wrote:
> On Wed, Feb 01, 2023 at 04:08:06PM +0100, Uladzislau Rezki (Sony) wrote:
> > This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
> > it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
> > 
> > 1.
> > The problem is that, recently we have run into a precedent when
> > a user intended to give a second argument to kfree_rcu() API but
> > forgot to do it in a code so a call became as a single argument
> > of kfree_rcu() API.
> > 
> > 2.
> > Such mistyping can lead to hidden bags where sleeping is forbidden.
> > 
> > 3.
> > _mightsleep() prefix gives much more information for which contexts
> > it can be used for.
> 
> Thank you!!!
> 
> I have queued these (aside from 10/13, which is being replaced by a
> patch from Julian Anastasov) for further testing and review.  And with
> the usual wordsmithing.
> 
Good. 10/13 will go with two arguments. So it is not needed. Just for
confirmation.

BTW, i see two complains from the robot due to 10/13 patch uses an old API
name.

>
> If testing goes well, I might try to get 1/13 into the next merge window,
> which would simplify things if maintainers want to take their patches
> separately.
> 
Thanks!

--
Uladzislau Rezki

Paul E. McKenney Feb. 2, 2023, 4:35 p.m. UTC | #3

On Thu, Feb 02, 2023 at 04:54:15PM +0100, Uladzislau Rezki wrote:
> On Wed, Feb 01, 2023 at 11:12:11AM -0800, Paul E. McKenney wrote:
> > On Wed, Feb 01, 2023 at 04:08:06PM +0100, Uladzislau Rezki (Sony) wrote:
> > > This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
> > > it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
> > > 
> > > 1.
> > > The problem is that, recently we have run into a precedent when
> > > a user intended to give a second argument to kfree_rcu() API but
> > > forgot to do it in a code so a call became as a single argument
> > > of kfree_rcu() API.
> > > 
> > > 2.
> > > Such mistyping can lead to hidden bags where sleeping is forbidden.
> > > 
> > > 3.
> > > _mightsleep() prefix gives much more information for which contexts
> > > it can be used for.
> > 
> > Thank you!!!
> > 
> > I have queued these (aside from 10/13, which is being replaced by a
> > patch from Julian Anastasov) for further testing and review.  And with
> > the usual wordsmithing.
> > 
> Good. 10/13 will go with two arguments. So it is not needed. Just for
> confirmation.

Got it, thank you!

> BTW, i see two complains from the robot due to 10/13 patch uses an old API
> name.

Thank you for the heads up!  I will be careful not to expose the last in
the series to -next before they get something in.  Unless they take too
long.  ;-)

							Thanx, Paul

> > If testing goes well, I might try to get 1/13 into the next merge window,
> > which would simplify things if maintainers want to take their patches
> > separately.
> > 
> Thanks!
> 
> --
> Uladzislau Rezki

Frederic Weisbecker Feb. 23, 2023, 12:45 p.m. UTC | #4

On Wed, Feb 01, 2023 at 04:08:06PM +0100, Uladzislau Rezki (Sony) wrote:
> This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
> it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
> 
> 1.
> The problem is that, recently we have run into a precedent when
> a user intended to give a second argument to kfree_rcu() API but
> forgot to do it in a code so a call became as a single argument
> of kfree_rcu() API.
> 
> 2.
> Such mistyping can lead to hidden bags where sleeping is forbidden.
> 
> 3.
> _mightsleep() prefix gives much more information for which contexts
> it can be used for.
> 
> Uladzislau Rezki (Sony) (13):
>   rcu/kvfree: Add kvfree_rcu_mightsleep() and kfree_rcu_mightsleep()
>   drbd: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
>   misc: vmw_vmci: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
>   tracing: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
>   lib/test_vmalloc.c: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
>   net/sysctl: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
>   RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()
>   net/mlx5: Rename kfree_rcu() to kfree_rcu_mightsleep()
>   ext4/super: Rename kfree_rcu() to kfree_rcu_mightsleep()
>   ipvs: Rename kfree_rcu() to kfree_rcu_mightsleep()
>   rcuscale: Rename kfree_rcu() to kfree_rcu_mightsleep()
>   doc: Update whatisRCU.rst
>   rcu/kvfree: Eliminate k[v]free_rcu() single argument macro

Hi,

Not sure if you guys noticed but on latest rcu/dev:

net/netfilter/ipvs/ip_vs_est.c: In function ‘ip_vs_stop_estimator’:
net/netfilter/ipvs/ip_vs_est.c:552:15: error: macro "kfree_rcu" requires 2 arguments, but only 1 given
   kfree_rcu(td);
               ^
net/netfilter/ipvs/ip_vs_est.c:552:3: error: ‘kfree_rcu’ undeclared (first use in this function); did you mean ‘kfree_skb’?
   kfree_rcu(td);
   ^~~~~~~~~
   kfree_skb
net/netfilter/ipvs/ip_vs_est.c:552:3: note: each undeclared identifier is reported only once for each function it appears in


Thanks.

Zhuo, Qiuxu Feb. 23, 2023, 2:29 p.m. UTC | #5

> From: Frederic Weisbecker <[e-mail]>
> Sent: Thursday, February 23, 2023 8:45 PM
> To: Uladzislau Rezki (Sony) <[e-mail]>
> Cc: LKML <[e-mail]>; RCU <[e-mail]>; Paul
> E . McKenney <[e-mail]>; Oleksiy Avramchenko
> <[e-mail]>; Jens Axboe <[e-mail]>; Philipp
> Reisner <[e-mail]>; Bryan Tan <[e-mail]>;
> Steven Rostedt <[e-mail]>; Eric Dumazet
> <[e-mail]>; Bob Pearson <[e-mail]>; Ariel
> Levkovich <[e-mail]>; Theodore Ts'o <[e-mail]>; Julian
> Anastasov <[e-mail]>
> Subject: Re: [PATCH 00/13] Rename k[v]free_rcu() single argument to
> k[v]free_rcu_mightsleep()
> 
> On Wed, Feb 01, 2023 at 04:08:06PM +0100, Uladzislau Rezki (Sony) wrote:
> > This small series is based on Paul's "dev" branch. Head is
> > 6002817348a1c610dc1b1c01ff81654cdec12be4
> > it renames a single argument of k[v]free_rcu() to its new
> k[v]free_rcu_mightsleep() name.
> >
> > 1.
> > The problem is that, recently we have run into a precedent when a user
> > intended to give a second argument to kfree_rcu() API but forgot to do
> > it in a code so a call became as a single argument of kfree_rcu() API.
> >
> > 2.
> > Such mistyping can lead to hidden bags where sleeping is forbidden.
> >
> > 3.
> > _mightsleep() prefix gives much more information for which contexts it
> > can be used for.
> >
> > Uladzislau Rezki (Sony) (13):
> >   rcu/kvfree: Add kvfree_rcu_mightsleep() and kfree_rcu_mightsleep()
> >   drbd: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> >   misc: vmw_vmci: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> >   tracing: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> >   lib/test_vmalloc.c: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> >   net/sysctl: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> >   RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()
> >   net/mlx5: Rename kfree_rcu() to kfree_rcu_mightsleep()
> >   ext4/super: Rename kfree_rcu() to kfree_rcu_mightsleep()
> >   ipvs: Rename kfree_rcu() to kfree_rcu_mightsleep()
> >   rcuscale: Rename kfree_rcu() to kfree_rcu_mightsleep()
> >   doc: Update whatisRCU.rst
> >   rcu/kvfree: Eliminate k[v]free_rcu() single argument macro
> 
> Hi,
> 
> Not sure if you guys noticed but on latest rcu/dev:
> 
> net/netfilter/ipvs/ip_vs_est.c: In function ‘ip_vs_stop_estimator’:
> net/netfilter/ipvs/ip_vs_est.c:552:15: error: macro "kfree_rcu" requires 2
> arguments, but only 1 given
>    kfree_rcu(td);
>                ^
> net/netfilter/ipvs/ip_vs_est.c:552:3: error: ‘kfree_rcu’ undeclared (first use in
> this function); did you mean ‘kfree_skb’?
>    kfree_rcu(td);
>    ^~~~~~~~~
>    kfree_skb
> net/netfilter/ipvs/ip_vs_est.c:552:3: note: each undeclared identifier is
> reported only once for each function it appears in
> 

Hi Frederic Weisbecker,

I encountered the same build error as yours. 
Per the discussion link below, the fix for this build error by Uladzislau Rezki will be picked up by some other maintainer's branch?
@Paul E . McKenney, please correct me if my understanding is wrong. 

Jens Axboe Feb. 23, 2023, 2:57 p.m. UTC | #6

On 2/1/23 8:08 AM, Uladzislau Rezki (Sony) wrote:
> This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
> it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
> 
> 1.
> The problem is that, recently we have run into a precedent when
> a user intended to give a second argument to kfree_rcu() API but
> forgot to do it in a code so a call became as a single argument
> of kfree_rcu() API.
> 
> 2.
> Such mistyping can lead to hidden bags where sleeping is forbidden.
> 
> 3.
> _mightsleep() prefix gives much more information for which contexts
> it can be used for.

This patchset seems weird to me. We have a LOT of calls that might
sleep, yet we don't suffix them all with _mightsleep(). Why is this
any different? Why isn't this just a might_sleep() call in the
actual helper, which will suffice for checkers and catch it at
runtime as well.

Paul E. McKenney Feb. 23, 2023, 3:54 p.m. UTC | #7

On Thu, Feb 23, 2023 at 02:29:42PM +0000, Zhuo, Qiuxu wrote:
> > From: Frederic Weisbecker <[e-mail]>
> > Sent: Thursday, February 23, 2023 8:45 PM
> > To: Uladzislau Rezki (Sony) <[e-mail]>
> > Cc: LKML <[e-mail]>; RCU <[e-mail]>; Paul
> > E . McKenney <[e-mail]>; Oleksiy Avramchenko
> > <[e-mail]>; Jens Axboe <[e-mail]>; Philipp
> > Reisner <[e-mail]>; Bryan Tan <[e-mail]>;
> > Steven Rostedt <[e-mail]>; Eric Dumazet
> > <[e-mail]>; Bob Pearson <[e-mail]>; Ariel
> > Levkovich <[e-mail]>; Theodore Ts'o <[e-mail]>; Julian
> > Anastasov <[e-mail]>
> > Subject: Re: [PATCH 00/13] Rename k[v]free_rcu() single argument to
> > k[v]free_rcu_mightsleep()
> > 
> > On Wed, Feb 01, 2023 at 04:08:06PM +0100, Uladzislau Rezki (Sony) wrote:
> > > This small series is based on Paul's "dev" branch. Head is
> > > 6002817348a1c610dc1b1c01ff81654cdec12be4
> > > it renames a single argument of k[v]free_rcu() to its new
> > k[v]free_rcu_mightsleep() name.
> > >
> > > 1.
> > > The problem is that, recently we have run into a precedent when a user
> > > intended to give a second argument to kfree_rcu() API but forgot to do
> > > it in a code so a call became as a single argument of kfree_rcu() API.
> > >
> > > 2.
> > > Such mistyping can lead to hidden bags where sleeping is forbidden.
> > >
> > > 3.
> > > _mightsleep() prefix gives much more information for which contexts it
> > > can be used for.
> > >
> > > Uladzislau Rezki (Sony) (13):
> > >   rcu/kvfree: Add kvfree_rcu_mightsleep() and kfree_rcu_mightsleep()
> > >   drbd: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> > >   misc: vmw_vmci: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> > >   tracing: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> > >   lib/test_vmalloc.c: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> > >   net/sysctl: Rename kvfree_rcu() to kvfree_rcu_mightsleep()
> > >   RDMA/rxe: Rename kfree_rcu() to kfree_rcu_mightsleep()
> > >   net/mlx5: Rename kfree_rcu() to kfree_rcu_mightsleep()
> > >   ext4/super: Rename kfree_rcu() to kfree_rcu_mightsleep()
> > >   ipvs: Rename kfree_rcu() to kfree_rcu_mightsleep()
> > >   rcuscale: Rename kfree_rcu() to kfree_rcu_mightsleep()
> > >   doc: Update whatisRCU.rst
> > >   rcu/kvfree: Eliminate k[v]free_rcu() single argument macro
> > 
> > Hi,
> > 
> > Not sure if you guys noticed but on latest rcu/dev:
> > 
> > net/netfilter/ipvs/ip_vs_est.c: In function ‘ip_vs_stop_estimator’:
> > net/netfilter/ipvs/ip_vs_est.c:552:15: error: macro "kfree_rcu" requires 2
> > arguments, but only 1 given
> >    kfree_rcu(td);
> >                ^
> > net/netfilter/ipvs/ip_vs_est.c:552:3: error: ‘kfree_rcu’ undeclared (first use in
> > this function); did you mean ‘kfree_skb’?
> >    kfree_rcu(td);
> >    ^~~~~~~~~
> >    kfree_skb
> > net/netfilter/ipvs/ip_vs_est.c:552:3: note: each undeclared identifier is
> > reported only once for each function it appears in
> 
> Hi Frederic Weisbecker,
> 
> I encountered the same build error as yours. 
> Per the discussion link below, the fix for this build error by Uladzislau Rezki will be picked up by some other maintainer's branch?
> @Paul E . McKenney, please correct me if my understanding is wrong. 

Julian Anastasov Feb. 23, 2023, 4:21 p.m. UTC | #8

Hello,

On Thu, 23 Feb 2023, Paul E. McKenney wrote:

> > > Not sure if you guys noticed but on latest rcu/dev:
> > > 
> > > net/netfilter/ipvs/ip_vs_est.c: In function ‘ip_vs_stop_estimator’:
> > > net/netfilter/ipvs/ip_vs_est.c:552:15: error: macro "kfree_rcu" requires 2
> > > arguments, but only 1 given
> > >    kfree_rcu(td);
> > >                ^
> > > net/netfilter/ipvs/ip_vs_est.c:552:3: error: ‘kfree_rcu’ undeclared (first use in
> > > this function); did you mean ‘kfree_skb’?
> > >    kfree_rcu(td);
> > >    ^~~~~~~~~
> > >    kfree_skb
> > > net/netfilter/ipvs/ip_vs_est.c:552:3: note: each undeclared identifier is
> > > reported only once for each function it appears in
> > 
> > Hi Frederic Weisbecker,
> > 
> > I encountered the same build error as yours. 
> > Per the discussion link below, the fix for this build error by Uladzislau Rezki will be picked up by some other maintainer's branch?
> > @Paul E . McKenney, please correct me if my understanding is wrong. 

Paul E. McKenney Feb. 23, 2023, 5:14 p.m. UTC | #9

On Thu, Feb 23, 2023 at 06:21:46PM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Thu, 23 Feb 2023, Paul E. McKenney wrote:
> 
> > > > Not sure if you guys noticed but on latest rcu/dev:
> > > > 
> > > > net/netfilter/ipvs/ip_vs_est.c: In function ‘ip_vs_stop_estimator’:
> > > > net/netfilter/ipvs/ip_vs_est.c:552:15: error: macro "kfree_rcu" requires 2
> > > > arguments, but only 1 given
> > > >    kfree_rcu(td);
> > > >                ^
> > > > net/netfilter/ipvs/ip_vs_est.c:552:3: error: ‘kfree_rcu’ undeclared (first use in
> > > > this function); did you mean ‘kfree_skb’?
> > > >    kfree_rcu(td);
> > > >    ^~~~~~~~~
> > > >    kfree_skb
> > > > net/netfilter/ipvs/ip_vs_est.c:552:3: note: each undeclared identifier is
> > > > reported only once for each function it appears in
> > > 
> > > Hi Frederic Weisbecker,
> > > 
> > > I encountered the same build error as yours. 
> > > Per the discussion link below, the fix for this build error by Uladzislau Rezki will be picked up by some other maintainer's branch?
> > > @Paul E . McKenney, please correct me if my understanding is wrong. 

Pablo Neira Ayuso Feb. 23, 2023, 5:36 p.m. UTC | #10

On Thu, Feb 23, 2023 at 09:14:32AM -0800, Paul E. McKenney wrote:
> On Thu, Feb 23, 2023 at 06:21:46PM +0200, Julian Anastasov wrote:
> > 
> > 	Hello,
> > 
> > On Thu, 23 Feb 2023, Paul E. McKenney wrote:
> > 
> > > > > Not sure if you guys noticed but on latest rcu/dev:
> > > > > 
> > > > > net/netfilter/ipvs/ip_vs_est.c: In function ‘ip_vs_stop_estimator’:
> > > > > net/netfilter/ipvs/ip_vs_est.c:552:15: error: macro "kfree_rcu" requires 2
> > > > > arguments, but only 1 given
> > > > >    kfree_rcu(td);
> > > > >                ^
> > > > > net/netfilter/ipvs/ip_vs_est.c:552:3: error: ‘kfree_rcu’ undeclared (first use in
> > > > > this function); did you mean ‘kfree_skb’?
> > > > >    kfree_rcu(td);
> > > > >    ^~~~~~~~~
> > > > >    kfree_skb
> > > > > net/netfilter/ipvs/ip_vs_est.c:552:3: note: each undeclared identifier is
> > > > > reported only once for each function it appears in
> > > > 
> > > > Hi Frederic Weisbecker,
> > > > 
> > > > I encountered the same build error as yours. 
> > > > Per the discussion link below, the fix for this build error by Uladzislau Rezki will be picked up by some other maintainer's branch?
> > > > @Paul E . McKenney, please correct me if my understanding is wrong. 

Paul E. McKenney Feb. 23, 2023, 6:21 p.m. UTC | #11

On Thu, Feb 23, 2023 at 06:36:26PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Feb 23, 2023 at 09:14:32AM -0800, Paul E. McKenney wrote:
> > On Thu, Feb 23, 2023 at 06:21:46PM +0200, Julian Anastasov wrote:
> > > 
> > > 	Hello,
> > > 
> > > On Thu, 23 Feb 2023, Paul E. McKenney wrote:
> > > 
> > > > > > Not sure if you guys noticed but on latest rcu/dev:
> > > > > > 
> > > > > > net/netfilter/ipvs/ip_vs_est.c: In function ‘ip_vs_stop_estimator’:
> > > > > > net/netfilter/ipvs/ip_vs_est.c:552:15: error: macro "kfree_rcu" requires 2
> > > > > > arguments, but only 1 given
> > > > > >    kfree_rcu(td);
> > > > > >                ^
> > > > > > net/netfilter/ipvs/ip_vs_est.c:552:3: error: ‘kfree_rcu’ undeclared (first use in
> > > > > > this function); did you mean ‘kfree_skb’?
> > > > > >    kfree_rcu(td);
> > > > > >    ^~~~~~~~~
> > > > > >    kfree_skb
> > > > > > net/netfilter/ipvs/ip_vs_est.c:552:3: note: each undeclared identifier is
> > > > > > reported only once for each function it appears in
> > > > > 
> > > > > Hi Frederic Weisbecker,
> > > > > 
> > > > > I encountered the same build error as yours. 
> > > > > Per the discussion link below, the fix for this build error by Uladzislau Rezki will be picked up by some other maintainer's branch?
> > > > > @Paul E . McKenney, please correct me if my understanding is wrong. 

Paul E. McKenney Feb. 23, 2023, 6:31 p.m. UTC | #12

On Thu, Feb 23, 2023 at 07:57:13AM -0700, Jens Axboe wrote:
> On 2/1/23 8:08 AM, Uladzislau Rezki (Sony) wrote:
> > This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
> > it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
> > 
> > 1.
> > The problem is that, recently we have run into a precedent when
> > a user intended to give a second argument to kfree_rcu() API but
> > forgot to do it in a code so a call became as a single argument
> > of kfree_rcu() API.
> > 
> > 2.
> > Such mistyping can lead to hidden bags where sleeping is forbidden.
> > 
> > 3.
> > _mightsleep() prefix gives much more information for which contexts
> > it can be used for.
> 
> This patchset seems weird to me. We have a LOT of calls that might
> sleep, yet we don't suffix them all with _mightsleep(). Why is this
> any different? Why isn't this just a might_sleep() call in the
> actual helper, which will suffice for checkers and catch it at
> runtime as well.

Fair enough, and the situation that this patchset is addressing is also a
bit unusual.  This change was requested by Eric Dumazet due to a situation
where someone forgot the optional second argument to kfree_rcu().  Now,
you are right that this would be caught if invoked from a non-sleepable
context, but there are also cases where sleeping is legal, but where the
occasional wait for an RCU grace period would be a problem.  The checkers
cannot easily catch this sort of thing, and hence the change in name.

Hey, the combined one/two-argument form seemed like a good idea at
the time!  ;-)

							Thanx, Paul

Jens Axboe Feb. 23, 2023, 7:36 p.m. UTC | #13

On 2/23/23 11:31 AM, Paul E. McKenney wrote:
> On Thu, Feb 23, 2023 at 07:57:13AM -0700, Jens Axboe wrote:
>> On 2/1/23 8:08 AM, Uladzislau Rezki (Sony) wrote:
>>> This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
>>> it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
>>>
>>> 1.
>>> The problem is that, recently we have run into a precedent when
>>> a user intended to give a second argument to kfree_rcu() API but
>>> forgot to do it in a code so a call became as a single argument
>>> of kfree_rcu() API.
>>>
>>> 2.
>>> Such mistyping can lead to hidden bags where sleeping is forbidden.
>>>
>>> 3.
>>> _mightsleep() prefix gives much more information for which contexts
>>> it can be used for.
>>
>> This patchset seems weird to me. We have a LOT of calls that might
>> sleep, yet we don't suffix them all with _mightsleep(). Why is this
>> any different? Why isn't this just a might_sleep() call in the
>> actual helper, which will suffice for checkers and catch it at
>> runtime as well.
> 
> Fair enough, and the situation that this patchset is addressing is also a
> bit unusual.  This change was requested by Eric Dumazet due to a situation
> where someone forgot the optional second argument to kfree_rcu().  Now,
> you are right that this would be caught if invoked from a non-sleepable
> context, but there are also cases where sleeping is legal, but where the
> occasional wait for an RCU grace period would be a problem.  The checkers
> cannot easily catch this sort of thing, and hence the change in name.
> 
> Hey, the combined one/two-argument form seemed like a good idea at
> the time!  ;-)

Hah, not sure what you were smoking back then!

Paul E. McKenney Feb. 23, 2023, 7:47 p.m. UTC | #14

On Thu, Feb 23, 2023 at 12:36:57PM -0700, Jens Axboe wrote:
> On 2/23/23 11:31 AM, Paul E. McKenney wrote:
> > On Thu, Feb 23, 2023 at 07:57:13AM -0700, Jens Axboe wrote:
> >> On 2/1/23 8:08 AM, Uladzislau Rezki (Sony) wrote:
> >>> This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
> >>> it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
> >>>
> >>> 1.
> >>> The problem is that, recently we have run into a precedent when
> >>> a user intended to give a second argument to kfree_rcu() API but
> >>> forgot to do it in a code so a call became as a single argument
> >>> of kfree_rcu() API.
> >>>
> >>> 2.
> >>> Such mistyping can lead to hidden bags where sleeping is forbidden.
> >>>
> >>> 3.
> >>> _mightsleep() prefix gives much more information for which contexts
> >>> it can be used for.
> >>
> >> This patchset seems weird to me. We have a LOT of calls that might
> >> sleep, yet we don't suffix them all with _mightsleep(). Why is this
> >> any different? Why isn't this just a might_sleep() call in the
> >> actual helper, which will suffice for checkers and catch it at
> >> runtime as well.
> > 
> > Fair enough, and the situation that this patchset is addressing is also a
> > bit unusual.  This change was requested by Eric Dumazet due to a situation
> > where someone forgot the optional second argument to kfree_rcu().  Now,
> > you are right that this would be caught if invoked from a non-sleepable
> > context, but there are also cases where sleeping is legal, but where the
> > occasional wait for an RCU grace period would be a problem.  The checkers
> > cannot easily catch this sort of thing, and hence the change in name.
> > 
> > Hey, the combined one/two-argument form seemed like a good idea at
> > the time!  ;-)
> 
> Hah, not sure what you were smoking back then!

If I remember, would you like me to send you some?  ;-)

							Thanx, Paul

Jens Axboe Feb. 23, 2023, 7:57 p.m. UTC | #15

On 2/23/23 12:47?PM, Paul E. McKenney wrote:
>>> Hey, the combined one/two-argument form seemed like a good idea at
>>> the time!  ;-)
>>
>> Hah, not sure what you were smoking back then!
> 
> If I remember, would you like me to send you some?  ;-)

Definitely, might improve my reviews :-)

Steven Rostedt March 15, 2023, 7:14 p.m. UTC | #16

On Wed,  1 Feb 2023 16:08:06 +0100
"Uladzislau Rezki (Sony)" <[e-mail]> wrote:

> This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
> it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
> 
> 1.
> The problem is that, recently we have run into a precedent when
> a user intended to give a second argument to kfree_rcu() API but
> forgot to do it in a code so a call became as a single argument
> of kfree_rcu() API.
> 
> 2.
> Such mistyping can lead to hidden bags where sleeping is forbidden.
> 
> 3.
> _mightsleep() prefix gives much more information for which contexts
> it can be used for.

My honest opinion is that I hate that name "kvfree_rcu_mightsleep()" ;-)

As I honestly don't know why it might sleep.

I didn't care about the name before, but now that it's touching code I
maintain I do care ;-)

Why not call it:

 kvfree_rcu_synchronize()

?

As that is much more descriptive of what it does. Especially since these
ugly names are popping up in my code because kvfree_rcu() replaced a
rcu_synchronize() in the first place.

-- Steve

Jens Axboe March 15, 2023, 7:16 p.m. UTC | #17

On 3/15/23 1:14?PM, Steven Rostedt wrote:
> On Wed,  1 Feb 2023 16:08:06 +0100
> "Uladzislau Rezki (Sony)" <[e-mail]> wrote:
> 
>> This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
>> it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
>>
>> 1.
>> The problem is that, recently we have run into a precedent when
>> a user intended to give a second argument to kfree_rcu() API but
>> forgot to do it in a code so a call became as a single argument
>> of kfree_rcu() API.
>>
>> 2.
>> Such mistyping can lead to hidden bags where sleeping is forbidden.
>>
>> 3.
>> _mightsleep() prefix gives much more information for which contexts
>> it can be used for.
> 
> My honest opinion is that I hate that name "kvfree_rcu_mightsleep()" ;-)
> 
> As I honestly don't know why it might sleep.
> 
> I didn't care about the name before, but now that it's touching code I
> maintain I do care ;-)
> 
> Why not call it:
> 
>  kvfree_rcu_synchronize()
> 
> ?
> 
> As that is much more descriptive of what it does. Especially since these
> ugly names are popping up in my code because kvfree_rcu() replaced a
> rcu_synchronize() in the first place.

This was my main complaint too, kvfree_rcu_mightsleep() is an absolutely
horrible name for an API... But nobody seemed to care about that!

I like the _synchronize() suggestion, as it matches other RCU naming.

Uladzislau Rezki March 15, 2023, 7:25 p.m. UTC | #18

On Wed, Mar 15, 2023 at 01:16:20PM -0600, Jens Axboe wrote:
> On 3/15/23 1:14?PM, Steven Rostedt wrote:
> > On Wed,  1 Feb 2023 16:08:06 +0100
> > "Uladzislau Rezki (Sony)" <[e-mail]> wrote:
> > 
> >> This small series is based on Paul's "dev" branch. Head is 6002817348a1c610dc1b1c01ff81654cdec12be4
> >> it renames a single argument of k[v]free_rcu() to its new k[v]free_rcu_mightsleep() name.
> >>
> >> 1.
> >> The problem is that, recently we have run into a precedent when
> >> a user intended to give a second argument to kfree_rcu() API but
> >> forgot to do it in a code so a call became as a single argument
> >> of kfree_rcu() API.
> >>
> >> 2.
> >> Such mistyping can lead to hidden bags where sleeping is forbidden.
> >>
> >> 3.
> >> _mightsleep() prefix gives much more information for which contexts
> >> it can be used for.
> > 
> > My honest opinion is that I hate that name "kvfree_rcu_mightsleep()" ;-)
> > 
> > As I honestly don't know why it might sleep.
> > 
> > I didn't care about the name before, but now that it's touching code I
> > maintain I do care ;-)
> > 
> > Why not call it:
> > 
> >  kvfree_rcu_synchronize()
> > 
> > ?
> > 
> > As that is much more descriptive of what it does. Especially since these
> > ugly names are popping up in my code because kvfree_rcu() replaced a
> > rcu_synchronize() in the first place.
> 
> This was my main complaint too, kvfree_rcu_mightsleep() is an absolutely
> horrible name for an API... But nobody seemed to care about that!
> 
> I like the _synchronize() suggestion, as it matches other RCU naming.
> 
This is basically about what it does. If you renamed it to "_synchronize()"
in reality it would not mean that it always a synchronous call, most of the
time it is not whereas the name would point that it is.

--
Uladzislau Rezki

Steven Rostedt March 15, 2023, 7:34 p.m. UTC | #19

On Wed, 15 Mar 2023 20:25:10 +0100
Uladzislau Rezki <[e-mail]> wrote:

> > This was my main complaint too, kvfree_rcu_mightsleep() is an absolutely
> > horrible name for an API... But nobody seemed to care about that!
> > 
> > I like the _synchronize() suggestion, as it matches other RCU naming.
> >   
> This is basically about what it does. If you renamed it to "_synchronize()"
> in reality it would not mean that it always a synchronous call, most of the
> time it is not whereas the name would point that it is.

No, just comment it.

I was going to suggest "kvfree_rcu_might_synchronize()" but that's just
getting ridiculous.

Still, I will replace that code back to a kfree() and rcu_synchonize() than
to let that other name get in.

-- Steve

Joel Fernandes March 15, 2023, 7:57 p.m. UTC | #20

Hey Steve,

On Wed, Mar 15, 2023 at 3:35 PM Steven Rostedt <[e-mail]> wrote:
>
> On Wed, 15 Mar 2023 20:25:10 +0100
> Uladzislau Rezki <[e-mail]> wrote:
>
> > > This was my main complaint too, kvfree_rcu_mightsleep() is an absolutely
> > > horrible name for an API... But nobody seemed to care about that!
> > >
> > > I like the _synchronize() suggestion, as it matches other RCU naming.
> > >
> > This is basically about what it does. If you renamed it to "_synchronize()"
> > in reality it would not mean that it always a synchronous call, most of the
> > time it is not whereas the name would point that it is.
>
> No, just comment it.
>
> I was going to suggest "kvfree_rcu_might_synchronize()" but that's just
> getting ridiculous.

No, synchronize() is incorrect. The code really can sleep for other
reasons like memory allocation. It is not that simple of an
implementation as one may imagine. mightsleep is really the correct
wording IMHO.

> Still, I will replace that code back to a kfree() and rcu_synchonize() than
> to let that other name get in.

I think it is too late for that for now, we already have conversions
going into the other subsystems, that means we'll have to redo all
that over again (even if it sounded like a good idea, which it is
not).

I would rather you just did: "#define kvfree_rcu_tracing
#kvfree_rcu_mightsleep", or something like that, if it is really a
problem. ;-)

Also you are really the first person I know of who has a problem with that name.

 - Joel

Steven Rostedt March 15, 2023, 8:28 p.m. UTC | #21

On Wed, 15 Mar 2023 15:57:02 -0400
Joel Fernandes <[e-mail]> wrote:

> > I was going to suggest "kvfree_rcu_might_synchronize()" but that's just
> > getting ridiculous.  
> 
> No, synchronize() is incorrect. The code really can sleep for other
> reasons like memory allocation. It is not that simple of an
> implementation as one may imagine. mightsleep is really the correct
> wording IMHO.
> 
> > Still, I will replace that code back to a kfree() and rcu_synchonize() than
> > to let that other name get in.  
> 
> I think it is too late for that for now, we already have conversions
> going into the other subsystems, that means we'll have to redo all
> that over again (even if it sounded like a good idea, which it is
> not).
> 
> I would rather you just did: "#define kvfree_rcu_tracing
> #kvfree_rcu_mightsleep", or something like that, if it is really a
> problem. ;-)
> 
> Also you are really the first person I know of who has a problem with that name.

I guess you didn't read Jens's reply.

The main issue I have with this, is that "might_sleep" is just an
implementation issue. It has *nothing* to do with what the call is about.
It is only about freeing something with RCU. It has nothing to do with
sleeping. I don't use it because it might sleep. I use it to free something.

If you don't like kvfree_rcu_synchronization() then call it
kvfree_rcu_headless() and note that currently it can sleep. Because in
the future, if we come up with an implementation where we it doesn't sleep,
then we don't need to go and rename all the users in the future.

See where I have the problem with the name "might_sleep"?

-- Steve

Uladzislau Rezki March 15, 2023, 9:07 p.m. UTC | #22

On Wed, Mar 15, 2023 at 04:28:40PM -0400, Steven Rostedt wrote:
> On Wed, 15 Mar 2023 15:57:02 -0400
> Joel Fernandes <[e-mail]> wrote:
> 
> > > I was going to suggest "kvfree_rcu_might_synchronize()" but that's just
> > > getting ridiculous.  
> > 
> > No, synchronize() is incorrect. The code really can sleep for other
> > reasons like memory allocation. It is not that simple of an
> > implementation as one may imagine. mightsleep is really the correct
> > wording IMHO.
> > 
> > > Still, I will replace that code back to a kfree() and rcu_synchonize() than
> > > to let that other name get in.  
> > 
> > I think it is too late for that for now, we already have conversions
> > going into the other subsystems, that means we'll have to redo all
> > that over again (even if it sounded like a good idea, which it is
> > not).
> > 
> > I would rather you just did: "#define kvfree_rcu_tracing
> > #kvfree_rcu_mightsleep", or something like that, if it is really a
> > problem. ;-)
> > 
> > Also you are really the first person I know of who has a problem with that name.
> 
> I guess you didn't read Jens's reply.
> 
> The main issue I have with this, is that "might_sleep" is just an
> implementation issue. It has *nothing* to do with what the call is about.
> It is only about freeing something with RCU. It has nothing to do with
> sleeping. I don't use it because it might sleep. I use it to free something.
> 
> If you don't like kvfree_rcu_synchronization() then call it
> kvfree_rcu_headless() and note that currently it can sleep. Because in
> the future, if we come up with an implementation where we it doesn't sleep,
> then we don't need to go and rename all the users in the future.
> 
> See where I have the problem with the name "might_sleep"?
> 
In that sense there is no need in renaming it. The current name of
single argument is kvfree_rcu(ptr). It is documented that it can sleep.

According to its name it is clear that it is headless since there
is no a second argument.

--
Uladzislau Rezki

Uladzislau Rezki March 15, 2023, 9:14 p.m. UTC | #23

On Wed, Mar 15, 2023 at 10:07:22PM +0100, Uladzislau Rezki wrote:
> On Wed, Mar 15, 2023 at 04:28:40PM -0400, Steven Rostedt wrote:
> > On Wed, 15 Mar 2023 15:57:02 -0400
> > Joel Fernandes <[e-mail]> wrote:
> > 
> > > > I was going to suggest "kvfree_rcu_might_synchronize()" but that's just
> > > > getting ridiculous.  
> > > 
> > > No, synchronize() is incorrect. The code really can sleep for other
> > > reasons like memory allocation. It is not that simple of an
> > > implementation as one may imagine. mightsleep is really the correct
> > > wording IMHO.
> > > 
> > > > Still, I will replace that code back to a kfree() and rcu_synchonize() than
> > > > to let that other name get in.  
> > > 
> > > I think it is too late for that for now, we already have conversions
> > > going into the other subsystems, that means we'll have to redo all
> > > that over again (even if it sounded like a good idea, which it is
> > > not).
> > > 
> > > I would rather you just did: "#define kvfree_rcu_tracing
> > > #kvfree_rcu_mightsleep", or something like that, if it is really a
> > > problem. ;-)
> > > 
> > > Also you are really the first person I know of who has a problem with that name.
> > 
> > I guess you didn't read Jens's reply.
> > 
> > The main issue I have with this, is that "might_sleep" is just an
> > implementation issue. It has *nothing* to do with what the call is about.
> > It is only about freeing something with RCU. It has nothing to do with
> > sleeping. I don't use it because it might sleep. I use it to free something.
> > 
> > If you don't like kvfree_rcu_synchronization() then call it
> > kvfree_rcu_headless() and note that currently it can sleep. Because in
> > the future, if we come up with an implementation where we it doesn't sleep,
> > then we don't need to go and rename all the users in the future.
> > 
> > See where I have the problem with the name "might_sleep"?
> > 
> In that sense there is no need in renaming it. The current name of
> single argument is kvfree_rcu(ptr). It is documented that it can sleep.
> 
> According to its name it is clear that it is headless since there
> is no a second argument.
> 
Forgot to add. I agree with you that currently it can sleep but it
does not mean that a future stays the same, thus there might be an
extra need in renaming again.

--
Uladzislau Rezki

Joel Fernandes March 15, 2023, 10:08 p.m. UTC | #24

Hey Steve,

On Wed, Mar 15, 2023 at 4:28 PM Steven Rostedt <[e-mail]> wrote:
>
> On Wed, 15 Mar 2023 15:57:02 -0400
> Joel Fernandes <[e-mail]> wrote:
>
> > > I was going to suggest "kvfree_rcu_might_synchronize()" but that's just
> > > getting ridiculous.
> >
> > No, synchronize() is incorrect. The code really can sleep for other
> > reasons like memory allocation. It is not that simple of an
> > implementation as one may imagine. mightsleep is really the correct
> > wording IMHO.
> >
> > > Still, I will replace that code back to a kfree() and rcu_synchonize() than
> > > to let that other name get in.
> >
> > I think it is too late for that for now, we already have conversions
> > going into the other subsystems, that means we'll have to redo all
> > that over again (even if it sounded like a good idea, which it is
> > not).
> >
> > I would rather you just did: "#define kvfree_rcu_tracing
> > #kvfree_rcu_mightsleep", or something like that, if it is really a
> > problem. ;-)
> >
> > Also you are really the first person I know of who has a problem with that name.
>
> I guess you didn't read Jens's reply.

Apologies, I am trying to keep up with email but this week is crazy.

> The main issue I have with this, is that "might_sleep" is just an
> implementation issue. It has *nothing* to do with what the call is about.
> It is only about freeing something with RCU. It has nothing to do with
> sleeping. I don't use it because it might sleep. I use it to free something.
>
> If you don't like kvfree_rcu_synchronization() then call it
> kvfree_rcu_headless() and note that currently it can sleep. Because in
> the future, if we come up with an implementation where we it doesn't sleep,
> then we don't need to go and rename all the users in the future.
>
> See where I have the problem with the name "might_sleep"?

I am doubtful there may be a future where it does not sleep. Why?
Because you need an rcu_head *somewhere*. Unlike with debubojects,
which involves a lock-free per-CPU pool and a locked global pool, and
has the liberty to shutdown if it runs out of objects -- in RCU code
it doesn't have that liberty and it has to just keep working.  The
kfree_rcu code does have pools of rcu_head as well, but that is not
thought to be enough to prevent OOM when memory needs to be given
back.  AFAIK -- the synchronize_rcu() in there is a last resort and
undesirable (supposed to happen only when running out of
objects/memory).

Also "mightsleep" means just that -- *might*.  That covers the fact
that sleeping may not happen ;-).

This is just my opinion and I will defer to Uladzislau, Paul and you
on how to proceed. Another option is "cansleep" which has the same
number of characters as headless. I don't believe expecting users to
read comments is practical, since we did already have comments and
there was a bug in the usage that triggered this whole series.

thanks,

 - Joel

Steven Rostedt March 15, 2023, 10:26 p.m. UTC | #25

On Wed, 15 Mar 2023 18:08:19 -0400
Joel Fernandes <[e-mail]> wrote:

> I am doubtful there may be a future where it does not sleep. Why?
> Because you need an rcu_head *somewhere*. Unlike with debubojects,
> which involves a lock-free per-CPU pool and a locked global pool, and
> has the liberty to shutdown if it runs out of objects -- in RCU code
> it doesn't have that liberty and it has to just keep working.  The
> kfree_rcu code does have pools of rcu_head as well, but that is not
> thought to be enough to prevent OOM when memory needs to be given
> back.  AFAIK -- the synchronize_rcu() in there is a last resort and
> undesirable (supposed to happen only when running out of
> objects/memory).

And everything you said above is still implementation, and the user of
kvfree_rcu() doesn't care.

The only thing different about the two cases is that one is headless.

> 
> Also "mightsleep" means just that -- *might*.  That covers the fact
> that sleeping may not happen ;-).

Yes, and even though you are doubtful of it not ever having a non-sleep
implementation, there is still a chance that there might be something
someday.

> 
> This is just my opinion and I will defer to Uladzislau, Paul and you
> on how to proceed. Another option is "cansleep" which has the same
> number of characters as headless. I don't believe expecting users to
> read comments is practical, since we did already have comments and
> there was a bug in the usage that triggered this whole series.

The point of "headless" is that is the rational for this version of
kvfree_rcu(). It doesn't have a head. That's an API name that users care
about.

Why not call it kvfree_rcu_alloc() ? It allocates right?

We have might_sleep() in lots of places. In fact, the default is things
might sleep. We don't need to call it out. That's what the might_sleep()
call is for. Usually it's the non sleep version that is special.

We could call the normal kvfree_rcu() "kvfree_rcu_inatomic()" ;-)

But I guess that would be a bigger change.

-- Steve

Theodore Ts'o March 16, 2023, 12:42 a.m. UTC | #26

On Wed, Mar 15, 2023 at 03:34:48PM -0400, Steven Rostedt wrote:
> 
> Still, I will replace that code back to a kfree() and rcu_synchonize() than
> to let that other name get in.

That will have a performance hit relaive to kfree_rcu_mightsleep().
If that's OK with you, sure, you can do that.

Personally, I don't have a lot of problem with that name, which is why
I ack'ed the change for ext4.

						- Ted

Theodore Ts'o March 16, 2023, 1:25 a.m. UTC | #27

On Wed, Mar 15, 2023 at 06:08:19PM -0400, Joel Fernandes wrote:
> 
> I am doubtful there may be a future where it does not sleep. Why?
> Because you need an rcu_head *somewhere*.

I think the real problem was that this won't sleep:

   kfree_rcu(ptr, rhf);

While this *could* sleep:

   kfree_rcu(ptr);

So the the original sin was to try to make the same mistake that C++
did --- which is to think that it's good to have functions that have
the same name but different function signatures, and in some cases,
different semantic meanings because they have different implementations.

Personally, this is why I refuse to use C++ for any of my personal
projects --- this kind of "magic" looks good, but it's a great way to
potentially shoot yourself (or worse, your users) in the foot.

So separating out the two-argument kfree_rcu() from the one-argument
kfree_rcu(), by renaming the latter to something else is IMHO, a
Really F***** Good Idea.  So while, sure, kfree_rcu_mightsleep() might
be a little awkward, the name documents the potential landmind
involved with using that function, that's a good thing.  Because do
you really think users will always conscientiously check the
documentation and/or the implementation before using the interface?  :-)

If you hate that name, one other possibility is to try to use the
two-argument form kfree_rcu() and arrange to *have* a rcu_head in the
structure.  That's going to be better from a performance perspective,
and thus kinder to the end user than using rcu_synchronize().

Cheers,

					- Ted

Joel Fernandes March 16, 2023, 2:13 a.m. UTC | #28

Hey Steve,

On Wed, Mar 15, 2023 at 6:26 PM Steven Rostedt <[e-mail]> wrote:
>
[...]
> > Also "mightsleep" means just that -- *might*.  That covers the fact
> > that sleeping may not happen ;-).
>
> Yes, and even though you are doubtful of it not ever having a non-sleep
> implementation, there is still a chance that there might be something
> someday.

Perhaps if it never sleeps, then we would introduce back the
single-arg kvfree_rcu() and delete the kvfree_rcu_mightsleep()` at
that point, since it would not serve any purpose.

> > This is just my opinion and I will defer to Uladzislau, Paul and you
> > on how to proceed. Another option is "cansleep" which has the same
> > number of characters as headless. I don't believe expecting users to
> > read comments is practical, since we did already have comments and
> > there was a bug in the usage that triggered this whole series.
>
> The point of "headless" is that is the rational for this version of
> kvfree_rcu(). It doesn't have a head. That's an API name that users care
> about.
>
> Why not call it kvfree_rcu_alloc() ? It allocates right?

Sure, but one can say now that allocating is an implementation detail? ;-)

Also, it may sound strange to have 'free' and 'alloc' in the same name.

> We have might_sleep() in lots of places. In fact, the default is things
> might sleep. We don't need to call it out. That's what the might_sleep()
> call is for. Usually it's the non sleep version that is special.
>
> We could call the normal kvfree_rcu() "kvfree_rcu_inatomic()" ;-)

Heh, I actually like 'inatomic' alot ;-)

> But I guess that would be a bigger change.
>

True.

thanks,

 - Joel

Steven Rostedt March 16, 2023, 2:15 a.m. UTC | #29

On Wed, 15 Mar 2023 21:25:16 -0400
"Theodore Ts'o" <[e-mail]> wrote:

> On Wed, Mar 15, 2023 at 06:08:19PM -0400, Joel Fernandes wrote:
> > 
> > I am doubtful there may be a future where it does not sleep. Why?
> > Because you need an rcu_head *somewhere*.  
> 
> I think the real problem was that this won't sleep:
> 
>    kfree_rcu(ptr, rhf);
> 
> While this *could* sleep:
> 
>    kfree_rcu(ptr);
> 
> So the the original sin was to try to make the same mistake that C++
> did --- which is to think that it's good to have functions that have
> the same name but different function signatures, and in some cases,
> different semantic meanings because they have different implementations.
> 
> Personally, this is why I refuse to use C++ for any of my personal
> projects --- this kind of "magic" looks good, but it's a great way to
> potentially shoot yourself (or worse, your users) in the foot.
> 
> So separating out the two-argument kfree_rcu() from the one-argument
> kfree_rcu(), by renaming the latter to something else is IMHO, a
> Really F***** Good Idea.  So while, sure, kfree_rcu_mightsleep() might
> be a little awkward, the name documents the potential landmind
> involved with using that function, that's a good thing.  Because do
> you really think users will always conscientiously check the
> documentation and/or the implementation before using the interface?  :-)

I agree with everything you said above, and feel that having the same name
with two different semantics was not a good way to go. Not to mention, I
avoid C++ for basically the same reasons (plus others).

> 
> If you hate that name, one other possibility is to try to use the
> two-argument form kfree_rcu() and arrange to *have* a rcu_head in the
> structure.  That's going to be better from a performance perspective,
> and thus kinder to the end user than using rcu_synchronize().

Which is the what I last suggested doing.

  https://lore.kernel.org/all/[e-mail]/

-- Steve

Steven Rostedt March 16, 2023, 2:50 a.m. UTC | #30

On Wed, 15 Mar 2023 22:13:26 -0400
Joel Fernandes <[e-mail]> wrote:

> > Why not call it kvfree_rcu_alloc() ? It allocates right?  
> 
> Sure, but one can say now that allocating is an implementation detail? ;-)

I was being sarcastic.

But as the mighty Linus once said: "In the Internet, nobody can hear your sarcasm"

-- Steve

Paul E. McKenney March 16, 2023, 2:52 a.m. UTC | #31

On Wed, Mar 15, 2023 at 09:25:16PM -0400, Theodore Ts'o wrote:
> On Wed, Mar 15, 2023 at 06:08:19PM -0400, Joel Fernandes wrote:
> > 
> > I am doubtful there may be a future where it does not sleep. Why?
> > Because you need an rcu_head *somewhere*.
> 
> I think the real problem was that this won't sleep:
> 
>    kfree_rcu(ptr, rhf);
> 
> While this *could* sleep:
> 
>    kfree_rcu(ptr);
> 
> So the the original sin was to try to make the same mistake that C++
> did --- which is to think that it's good to have functions that have
> the same name but different function signatures, and in some cases,
> different semantic meanings because they have different implementations.

Guilty to charges as read.  ;-)

> Personally, this is why I refuse to use C++ for any of my personal
> projects --- this kind of "magic" looks good, but it's a great way to
> potentially shoot yourself (or worse, your users) in the foot.
> 
> So separating out the two-argument kfree_rcu() from the one-argument
> kfree_rcu(), by renaming the latter to something else is IMHO, a
> Really F***** Good Idea.  So while, sure, kfree_rcu_mightsleep() might
> be a little awkward, the name documents the potential landmind
> involved with using that function, that's a good thing.  Because do
> you really think users will always conscientiously check the
> documentation and/or the implementation before using the interface?  :-)
> 
> If you hate that name, one other possibility is to try to use the
> two-argument form kfree_rcu() and arrange to *have* a rcu_head in the
> structure.  That's going to be better from a performance perspective,
> and thus kinder to the end user than using rcu_synchronize().

The original reason for single-argument kvfree_rcu() was to avoid
the need for that rcu_head.  The use case was a small data structure
with an extremely high population.

							Thanx, Paul

Joel Fernandes March 16, 2023, 5:01 a.m. UTC | #32

On Wed, Mar 15, 2023 at 10:50 PM Steven Rostedt <[e-mail]> wrote:
>
> On Wed, 15 Mar 2023 22:13:26 -0400
> Joel Fernandes <[e-mail]> wrote:
>
> > > Why not call it kvfree_rcu_alloc() ? It allocates right?
> >
> > Sure, but one can say now that allocating is an implementation detail? ;-)
>
> I was being sarcastic.
>
> But as the mighty Linus once said: "In the Internet, nobody can hear your sarcasm"
>

I guess it's official then - the internet has killed sarcasm. I
suppose we'll all have to resort to using more obvious forms of humor,
like dad jokes. ;)

 - Joel