| Message ID | 20230201150954.409693-4-urezki@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1c6d306af363befca676f61d10bb8e11701e9176 |
| Headers | show |
| Series | Rename k[v]free_rcu() single argument to k[v]free_rcu_mightsleep() | expand |
Hi, All, On Wed, Feb 1, 2023 at 10:11 AM Uladzislau Rezki (Sony) <urezki@gmail.com> wrote: > > For a single argument invocations a new kfree_rcu_mightsleep() > and kvfree_rcu_mightsleep() macroses are used. This is done in > order to prevent users from calling a single argument from > atomic contexts as "_mightsleep" prefix signals that it can > schedule(). > Since this commit in -dev branch [1] suggests more users still need conversion, let us drop this single patch for 6.4 and move the rest of the series forward? Let me know if you disagree. https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=9bf5e3a2626ed474d080f695007541b6ecd6e60b All -- please supply Ack/Review tags for patches 1-12. thanks, - Joel > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> > --- > include/linux/rcupdate.h | 29 ++++++++--------------------- > 1 file changed, 8 insertions(+), 21 deletions(-) > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index 094321c17e48..7571dbfecb18 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -957,9 +957,8 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) > > /** > * kfree_rcu() - kfree an object after a grace period. > - * @ptr: pointer to kfree for both single- and double-argument invocations. > - * @rhf: the name of the struct rcu_head within the type of @ptr, > - * but only for double-argument invocations. > + * @ptr: pointer to kfree for double-argument invocations. > + * @rhf: the name of the struct rcu_head within the type of @ptr. > * > * Many rcu callbacks functions just call kfree() on the base structure. > * These functions are trivial, but their size adds up, and furthermore > @@ -982,26 +981,18 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) > * The BUILD_BUG_ON check must not involve any function calls, hence the > * checks are done in macros here. > */ > -#define kfree_rcu(ptr, rhf...) kvfree_rcu(ptr, ## rhf) > +#define kfree_rcu(ptr, rhf) kvfree_rcu_arg_2(ptr, rhf) > +#define kvfree_rcu(ptr, rhf) kvfree_rcu_arg_2(ptr, rhf) > > /** > - * kvfree_rcu() - kvfree an object after a grace period. > - * > - * This macro consists of one or two arguments and it is > - * based on whether an object is head-less or not. If it > - * has a head then a semantic stays the same as it used > - * to be before: > - * > - * kvfree_rcu(ptr, rhf); > - * > - * where @ptr is a pointer to kvfree(), @rhf is the name > - * of the rcu_head structure within the type of @ptr. > + * kfree_rcu_mightsleep() - kfree an object after a grace period. > + * @ptr: pointer to kfree for single-argument invocations. > * > * When it comes to head-less variant, only one argument > * is passed and that is just a pointer which has to be > * freed after a grace period. Therefore the semantic is > * > - * kvfree_rcu(ptr); > + * kfree_rcu_mightsleep(ptr); > * > * where @ptr is the pointer to be freed by kvfree(). > * > @@ -1010,13 +1001,9 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) > * annotation. Otherwise, please switch and embed the > * rcu_head structure within the type of @ptr. > */ > -#define kvfree_rcu(...) KVFREE_GET_MACRO(__VA_ARGS__, \ > - kvfree_rcu_arg_2, kvfree_rcu_arg_1)(__VA_ARGS__) > - > +#define kfree_rcu_mightsleep(ptr) kvfree_rcu_arg_1(ptr) > #define kvfree_rcu_mightsleep(ptr) kvfree_rcu_arg_1(ptr) > -#define kfree_rcu_mightsleep(ptr) kvfree_rcu_mightsleep(ptr) > > -#define KVFREE_GET_MACRO(_1, _2, NAME, ...) NAME > #define kvfree_rcu_arg_2(ptr, rhf) \ > do { \ > typeof (ptr) ___p = (ptr); \ > -- > 2.30.2 >
> On Mar 5, 2023, at 5:29 AM, Joel Fernandes <joel@joelfernandes.org> wrote: > > Hi, All, > >> On Wed, Feb 1, 2023 at 10:11 AM Uladzislau Rezki (Sony) >> <urezki@gmail.com> wrote: >> >> For a single argument invocations a new kfree_rcu_mightsleep() >> and kvfree_rcu_mightsleep() macroses are used. This is done in >> order to prevent users from calling a single argument from >> atomic contexts as "_mightsleep" prefix signals that it can >> schedule(). >> > > Since this commit in -dev branch [1] suggests more users still need > conversion, let us drop this single patch for 6.4 and move the rest of > the series forward? Let me know if you disagree. > https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=9bf5e3a2626ed474d080f695007541b6ecd6e60b > > All -- please supply Ack/Review tags for patches 1-12. Or put another way, what is the transition plan for these remaining users? I am getting on a plane right now but I can research which users are remaining later. - Joel > > thanks, > > - Joel > > >> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> >> --- >> include/linux/rcupdate.h | 29 ++++++++--------------------- >> 1 file changed, 8 insertions(+), 21 deletions(-) >> >> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h >> index 094321c17e48..7571dbfecb18 100644 >> --- a/include/linux/rcupdate.h >> +++ b/include/linux/rcupdate.h >> @@ -957,9 +957,8 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) >> >> /** >> * kfree_rcu() - kfree an object after a grace period. >> - * @ptr: pointer to kfree for both single- and double-argument invocations. >> - * @rhf: the name of the struct rcu_head within the type of @ptr, >> - * but only for double-argument invocations. >> + * @ptr: pointer to kfree for double-argument invocations. >> + * @rhf: the name of the struct rcu_head within the type of @ptr. >> * >> * Many rcu callbacks functions just call kfree() on the base structure. >> * These functions are trivial, but their size adds up, and furthermore >> @@ -982,26 +981,18 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) >> * The BUILD_BUG_ON check must not involve any function calls, hence the >> * checks are done in macros here. >> */ >> -#define kfree_rcu(ptr, rhf...) kvfree_rcu(ptr, ## rhf) >> +#define kfree_rcu(ptr, rhf) kvfree_rcu_arg_2(ptr, rhf) >> +#define kvfree_rcu(ptr, rhf) kvfree_rcu_arg_2(ptr, rhf) >> >> /** >> - * kvfree_rcu() - kvfree an object after a grace period. >> - * >> - * This macro consists of one or two arguments and it is >> - * based on whether an object is head-less or not. If it >> - * has a head then a semantic stays the same as it used >> - * to be before: >> - * >> - * kvfree_rcu(ptr, rhf); >> - * >> - * where @ptr is a pointer to kvfree(), @rhf is the name >> - * of the rcu_head structure within the type of @ptr. >> + * kfree_rcu_mightsleep() - kfree an object after a grace period. >> + * @ptr: pointer to kfree for single-argument invocations. >> * >> * When it comes to head-less variant, only one argument >> * is passed and that is just a pointer which has to be >> * freed after a grace period. Therefore the semantic is >> * >> - * kvfree_rcu(ptr); >> + * kfree_rcu_mightsleep(ptr); >> * >> * where @ptr is the pointer to be freed by kvfree(). >> * >> @@ -1010,13 +1001,9 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) >> * annotation. Otherwise, please switch and embed the >> * rcu_head structure within the type of @ptr. >> */ >> -#define kvfree_rcu(...) KVFREE_GET_MACRO(__VA_ARGS__, \ >> - kvfree_rcu_arg_2, kvfree_rcu_arg_1)(__VA_ARGS__) >> - >> +#define kfree_rcu_mightsleep(ptr) kvfree_rcu_arg_1(ptr) >> #define kvfree_rcu_mightsleep(ptr) kvfree_rcu_arg_1(ptr) >> -#define kfree_rcu_mightsleep(ptr) kvfree_rcu_mightsleep(ptr) >> >> -#define KVFREE_GET_MACRO(_1, _2, NAME, ...) NAME >> #define kvfree_rcu_arg_2(ptr, rhf) \ >> do { \ >> typeof (ptr) ___p = (ptr); \ >> -- >> 2.30.2 >>
> > On Mar 5, 2023, at 5:29 AM, Joel Fernandes <joel@joelfernandes.org> wrote: > > > > Hi, All, > > > >> On Wed, Feb 1, 2023 at 10:11 AM Uladzislau Rezki (Sony) > >> <urezki@gmail.com> wrote: > >> > >> For a single argument invocations a new kfree_rcu_mightsleep() > >> and kvfree_rcu_mightsleep() macroses are used. This is done in > >> order to prevent users from calling a single argument from > >> atomic contexts as "_mightsleep" prefix signals that it can > >> schedule(). > >> > > > > Since this commit in -dev branch [1] suggests more users still need > > conversion, let us drop this single patch for 6.4 and move the rest of > > the series forward? Let me know if you disagree. > > https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=9bf5e3a2626ed474d080f695007541b6ecd6e60b > > > > All -- please supply Ack/Review tags for patches 1-12. > > Or put another way, what is the transition plan for these remaining users? > > I am getting on a plane right now but I can research which users are remaining later. > I am not sure. I think we can cover it on the meeting. My feeling is that, we introduced "_mightsleep" macros first and after that try to convert users. @Paul what is your view? Thanks! -- Uladzislau Rezki
> On Mar 5, 2023, at 6:41 AM, Uladzislau Rezki <urezki@gmail.com> wrote: > > >> >>>> On Mar 5, 2023, at 5:29 AM, Joel Fernandes <joel@joelfernandes.org> wrote: >>> >>> Hi, All, >>> >>>> On Wed, Feb 1, 2023 at 10:11 AM Uladzislau Rezki (Sony) >>>> <urezki@gmail.com> wrote: >>>> >>>> For a single argument invocations a new kfree_rcu_mightsleep() >>>> and kvfree_rcu_mightsleep() macroses are used. This is done in >>>> order to prevent users from calling a single argument from >>>> atomic contexts as "_mightsleep" prefix signals that it can >>>> schedule(). >>>> >>> >>> Since this commit in -dev branch [1] suggests more users still need >>> conversion, let us drop this single patch for 6.4 and move the rest of >>> the series forward? Let me know if you disagree. >>> https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=9bf5e3a2626ed474d080f695007541b6ecd6e60b >>> >>> All -- please supply Ack/Review tags for patches 1-12. >> >> Or put another way, what is the transition plan for these remaining users? >> >> I am getting on a plane right now but I can research which users are remaining later. >> > I am not sure. I think we can cover it on the meeting. Cool, thanks. > My feeling is > that, we introduced "_mightsleep" macros first and after that try to > convert users. One stopgap could be to add a checkpatch error if anyone tries to use old API, and then in the meanwhile convert all users. Though, that requires people listening to checkpatch complaints. Thanks, - Joel > > @Paul what is your view? > > Thanks! > > -- > Uladzislau Rezki
On Sun, Mar 05, 2023 at 07:56:33AM -0500, Joel Fernandes wrote: > > > > On Mar 5, 2023, at 6:41 AM, Uladzislau Rezki <urezki@gmail.com> wrote: > > > > > >> > >>>> On Mar 5, 2023, at 5:29 AM, Joel Fernandes <joel@joelfernandes.org> wrote: > >>> > >>> Hi, All, > >>> > >>>> On Wed, Feb 1, 2023 at 10:11 AM Uladzislau Rezki (Sony) > >>>> <urezki@gmail.com> wrote: > >>>> > >>>> For a single argument invocations a new kfree_rcu_mightsleep() > >>>> and kvfree_rcu_mightsleep() macroses are used. This is done in > >>>> order to prevent users from calling a single argument from > >>>> atomic contexts as "_mightsleep" prefix signals that it can > >>>> schedule(). > >>>> > >>> > >>> Since this commit in -dev branch [1] suggests more users still need > >>> conversion, let us drop this single patch for 6.4 and move the rest of > >>> the series forward? Let me know if you disagree. > >>> https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=9bf5e3a2626ed474d080f695007541b6ecd6e60b > >>> > >>> All -- please supply Ack/Review tags for patches 1-12. > >> > >> Or put another way, what is the transition plan for these remaining users? > >> > >> I am getting on a plane right now but I can research which users are remaining later. > >> > > I am not sure. I think we can cover it on the meeting. > > Cool, thanks. My current plan is as follows: 1. Addition of kvfree_rcu_mightsleep() went into v6.3. 2. After creating branches, I send out the series, including 12/12. The -rcu tree's "dev" branch continues to have a revert to avoid breaking -next until we achieve clean -next and clean "dev" branch. 3. Any conversion patches that get maintainer acks go into v6.4. Along with a checkpatch error, as Joel notes below. 4. There are periodic checks for new code using the single-argument forms of kfree_rcu() and kvfree_rcu(). Patches are produced for them, or responses to the patches introducing them, as appropriate. A coccinelle script might be helpful, perhaps even as part of kernel test robot or similar. 5. The -rcu tree's "dev" branch will revert to unclean from time to time as maintainers choose to take conversion patches into their own trees. 6. Once mainline is clean, we push 12/12 into the next merge window. 7. We then evaluate whether further cleanups are needed. > > My feeling is > > that, we introduced "_mightsleep" macros first and after that try to > > convert users. > One stopgap could be to add a checkpatch error if anyone tries to use old API, > and then in the meanwhile convert all users. > Though, that requires people listening to checkpatch complaints. Every person who listens is that much less hassle. It doesn't have to be perfect. ;-) Thanx, Paul > Thanks, > > - Joel > > > > > > @Paul what is your view? > > > > Thanks! > > > > -- > > Uladzislau Rezki
On Sun, Mar 05, 2023 at 10:05:24AM -0800, Paul E. McKenney wrote: > On Sun, Mar 05, 2023 at 07:56:33AM -0500, Joel Fernandes wrote: > > > > > > > On Mar 5, 2023, at 6:41 AM, Uladzislau Rezki <urezki@gmail.com> wrote: > > > > > > > > >> > > >>>> On Mar 5, 2023, at 5:29 AM, Joel Fernandes <joel@joelfernandes.org> wrote: > > >>> > > >>> Hi, All, > > >>> > > >>>> On Wed, Feb 1, 2023 at 10:11 AM Uladzislau Rezki (Sony) > > >>>> <urezki@gmail.com> wrote: > > >>>> > > >>>> For a single argument invocations a new kfree_rcu_mightsleep() > > >>>> and kvfree_rcu_mightsleep() macroses are used. This is done in > > >>>> order to prevent users from calling a single argument from > > >>>> atomic contexts as "_mightsleep" prefix signals that it can > > >>>> schedule(). > > >>>> > > >>> > > >>> Since this commit in -dev branch [1] suggests more users still need > > >>> conversion, let us drop this single patch for 6.4 and move the rest of > > >>> the series forward? Let me know if you disagree. > > >>> https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=9bf5e3a2626ed474d080f695007541b6ecd6e60b > > >>> > > >>> All -- please supply Ack/Review tags for patches 1-12. > > >> > > >> Or put another way, what is the transition plan for these remaining users? > > >> > > >> I am getting on a plane right now but I can research which users are remaining later. > > >> > > > I am not sure. I think we can cover it on the meeting. > > > > Cool, thanks. > > My current plan is as follows: > > 1. Addition of kvfree_rcu_mightsleep() went into v6.3. > > 2. After creating branches, I send out the series, including 12/12. > The -rcu tree's "dev" branch continues to have a revert to avoid > breaking -next until we achieve clean -next and clean "dev" > branch. > > 3. Any conversion patches that get maintainer acks go into v6.4. > Along with a checkpatch error, as Joel notes below. > > 4. There are periodic checks for new code using the single-argument > forms of kfree_rcu() and kvfree_rcu(). Patches are produced > for them, or responses to the patches introducing them, as > appropriate. A coccinelle script might be helpful, perhaps > even as part of kernel test robot or similar. > > 5. The -rcu tree's "dev" branch will revert to unclean from time > to time as maintainers choose to take conversion patches into > their own trees. > > 6. Once mainline is clean, we push 12/12 into the next merge > window. Since in theory, mainline could also be after 6.4-rc1, I am assuming next merge window could also mean 6.5 right? But yes, agreed. > 7. We then evaluate whether further cleanups are needed. > > > > My feeling is > > > that, we introduced "_mightsleep" macros first and after that try to > > > convert users. > > > One stopgap could be to add a checkpatch error if anyone tries to use old API, > > and then in the meanwhile convert all users. > > Though, that requires people listening to checkpatch complaints. > > Every person who listens is that much less hassle. It doesn't have to > be perfect. ;-) The below checkpatch change can catch at least simple single-arg uses (i.e. not having compound expressions inside of k[v]free_rcu() args). I will submit a proper patch to it which we can include in this set. Thoughts? --- scripts/checkpatch.pl | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 78cc595b98ce..fc73786064b3 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -6362,6 +6362,15 @@ sub process { } } +# check for soon-to-be-deprecated single-argument k[v]free_rcu() API + if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) { + if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) { + ERROR("DEPRECATED_API", + "Single-argument k[v]free_rcu() API is deprecated, please pass an rcu_head object." . $herecurr); + } + } + + # check for unnecessary "Out of Memory" messages if ($line =~ /^\+.*\b$logFunctions\s*\(/ && $prevline =~ /^[ \+]\s*if\s*\(\s*(\!\s*|NULL\s*==\s*)?($Lval)(\s*==\s*NULL\s*)?\s*\)/ &&
On Mon, Mar 06, 2023 at 02:49:48PM +0000, Joel Fernandes wrote: > On Sun, Mar 05, 2023 at 10:05:24AM -0800, Paul E. McKenney wrote: > > On Sun, Mar 05, 2023 at 07:56:33AM -0500, Joel Fernandes wrote: > > > > > > > > > > On Mar 5, 2023, at 6:41 AM, Uladzislau Rezki <urezki@gmail.com> wrote: > > > > > > > > > > > >> > > > >>>> On Mar 5, 2023, at 5:29 AM, Joel Fernandes <joel@joelfernandes.org> wrote: > > > >>> > > > >>> Hi, All, > > > >>> > > > >>>> On Wed, Feb 1, 2023 at 10:11 AM Uladzislau Rezki (Sony) > > > >>>> <urezki@gmail.com> wrote: > > > >>>> > > > >>>> For a single argument invocations a new kfree_rcu_mightsleep() > > > >>>> and kvfree_rcu_mightsleep() macroses are used. This is done in > > > >>>> order to prevent users from calling a single argument from > > > >>>> atomic contexts as "_mightsleep" prefix signals that it can > > > >>>> schedule(). > > > >>>> > > > >>> > > > >>> Since this commit in -dev branch [1] suggests more users still need > > > >>> conversion, let us drop this single patch for 6.4 and move the rest of > > > >>> the series forward? Let me know if you disagree. > > > >>> https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/commit/?h=dev&id=9bf5e3a2626ed474d080f695007541b6ecd6e60b > > > >>> > > > >>> All -- please supply Ack/Review tags for patches 1-12. > > > >> > > > >> Or put another way, what is the transition plan for these remaining users? > > > >> > > > >> I am getting on a plane right now but I can research which users are remaining later. > > > >> > > > > I am not sure. I think we can cover it on the meeting. > > > > > > Cool, thanks. > > > > My current plan is as follows: > > > > 1. Addition of kvfree_rcu_mightsleep() went into v6.3. > > > > 2. After creating branches, I send out the series, including 12/12. > > The -rcu tree's "dev" branch continues to have a revert to avoid > > breaking -next until we achieve clean -next and clean "dev" > > branch. > > > > 3. Any conversion patches that get maintainer acks go into v6.4. > > Along with a checkpatch error, as Joel notes below. > > > > 4. There are periodic checks for new code using the single-argument > > forms of kfree_rcu() and kvfree_rcu(). Patches are produced > > for them, or responses to the patches introducing them, as > > appropriate. A coccinelle script might be helpful, perhaps > > even as part of kernel test robot or similar. > > > > 5. The -rcu tree's "dev" branch will revert to unclean from time > > to time as maintainers choose to take conversion patches into > > their own trees. > > > > 6. Once mainline is clean, we push 12/12 into the next merge > > window. > > Since in theory, mainline could also be after 6.4-rc1, I am assuming next merge > window could also mean 6.5 right? But yes, agreed. I would rather not waste Linus's time with a separate pull request for this. It is after all not a regression. ;-) > > 7. We then evaluate whether further cleanups are needed. > > > > > > My feeling is > > > > that, we introduced "_mightsleep" macros first and after that try to > > > > convert users. > > > > > One stopgap could be to add a checkpatch error if anyone tries to use old API, > > > and then in the meanwhile convert all users. > > > Though, that requires people listening to checkpatch complaints. > > > > Every person who listens is that much less hassle. It doesn't have to > > be perfect. ;-) > > The below checkpatch change can catch at least simple single-arg uses (i.e. > not having compound expressions inside of k[v]free_rcu() args). I will submit > a proper patch to it which we can include in this set. > > Thoughts? > --- > scripts/checkpatch.pl | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 78cc595b98ce..fc73786064b3 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -6362,6 +6362,15 @@ sub process { > } > } > > +# check for soon-to-be-deprecated single-argument k[v]free_rcu() API > + if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) { > + if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) { > + ERROR("DEPRECATED_API", > + "Single-argument k[v]free_rcu() API is deprecated, please pass an rcu_head object." . $herecurr); Nice! But could you please also tell them what to use instead? Sure, they could look it up, but if it tells them directly, they are less likely to ignore it. Thanx, Paul > + } > + } > + > + > # check for unnecessary "Out of Memory" messages > if ($line =~ /^\+.*\b$logFunctions\s*\(/ && > $prevline =~ /^[ \+]\s*if\s*\(\s*(\!\s*|NULL\s*==\s*)?($Lval)(\s*==\s*NULL\s*)?\s*\)/ && > -- > 2.40.0.rc0.216.gc4246ad0f0-goog >
On Mon, Mar 06, 2023 at 07:01:08AM -0800, Paul E. McKenney wrote: [..] > > > 7. We then evaluate whether further cleanups are needed. > > > > > > > > My feeling is > > > > > that, we introduced "_mightsleep" macros first and after that try to > > > > > convert users. > > > > > > > One stopgap could be to add a checkpatch error if anyone tries to use old API, > > > > and then in the meanwhile convert all users. > > > > Though, that requires people listening to checkpatch complaints. > > > > > > Every person who listens is that much less hassle. It doesn't have to > > > be perfect. ;-) > > > > The below checkpatch change can catch at least simple single-arg uses (i.e. > > not having compound expressions inside of k[v]free_rcu() args). I will submit > > a proper patch to it which we can include in this set. > > > > Thoughts? > > --- > > scripts/checkpatch.pl | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 78cc595b98ce..fc73786064b3 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -6362,6 +6362,15 @@ sub process { > > } > > } > > > > +# check for soon-to-be-deprecated single-argument k[v]free_rcu() API > > + if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) { > > + if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) { > > + ERROR("DEPRECATED_API", > > + "Single-argument k[v]free_rcu() API is deprecated, please pass an rcu_head object." . $herecurr); > > Nice! > > But could you please also tell them what to use instead? Sure, they > could look it up, but if it tells them directly, they are less likely > to ignore it. Sounds good, I will modify the warning to include the API to call and send out a patch soon. thanks, - Joel
On Mon, Mar 06, 2023 at 03:12:03PM +0000, Joel Fernandes wrote: > On Mon, Mar 06, 2023 at 07:01:08AM -0800, Paul E. McKenney wrote: > [..] > > > > 7. We then evaluate whether further cleanups are needed. > > > > > > > > > > My feeling is > > > > > > that, we introduced "_mightsleep" macros first and after that try to > > > > > > convert users. > > > > > > > > > One stopgap could be to add a checkpatch error if anyone tries to use old API, > > > > > and then in the meanwhile convert all users. > > > > > Though, that requires people listening to checkpatch complaints. > > > > > > > > Every person who listens is that much less hassle. It doesn't have to > > > > be perfect. ;-) > > > > > > The below checkpatch change can catch at least simple single-arg uses (i.e. > > > not having compound expressions inside of k[v]free_rcu() args). I will submit > > > a proper patch to it which we can include in this set. > > > > > > Thoughts? > > > --- > > > scripts/checkpatch.pl | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > index 78cc595b98ce..fc73786064b3 100755 > > > --- a/scripts/checkpatch.pl > > > +++ b/scripts/checkpatch.pl > > > @@ -6362,6 +6362,15 @@ sub process { > > > } > > > } > > > > > > +# check for soon-to-be-deprecated single-argument k[v]free_rcu() API > > > + if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) { > > > + if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) { > > > + ERROR("DEPRECATED_API", > > > + "Single-argument k[v]free_rcu() API is deprecated, please pass an rcu_head object." . $herecurr); > > > > Nice! > > > > But could you please also tell them what to use instead? Sure, they > > could look it up, but if it tells them directly, they are less likely > > to ignore it. > > Sounds good, I will modify the warning to include the API to call and send > out a patch soon. > Maybe compile warnings? Or is it too aggressive? Thanks! -- Uladzislau Rezki
On Mon, Mar 06, 2023 at 05:42:44PM +0100, Uladzislau Rezki wrote: > On Mon, Mar 06, 2023 at 03:12:03PM +0000, Joel Fernandes wrote: > > On Mon, Mar 06, 2023 at 07:01:08AM -0800, Paul E. McKenney wrote: > > [..] > > > > > 7. We then evaluate whether further cleanups are needed. > > > > > > > > > > > > My feeling is > > > > > > > that, we introduced "_mightsleep" macros first and after that try to > > > > > > > convert users. > > > > > > > > > > > One stopgap could be to add a checkpatch error if anyone tries to use old API, > > > > > > and then in the meanwhile convert all users. > > > > > > Though, that requires people listening to checkpatch complaints. > > > > > > > > > > Every person who listens is that much less hassle. It doesn't have to > > > > > be perfect. ;-) > > > > > > > > The below checkpatch change can catch at least simple single-arg uses (i.e. > > > > not having compound expressions inside of k[v]free_rcu() args). I will submit > > > > a proper patch to it which we can include in this set. > > > > > > > > Thoughts? > > > > --- > > > > scripts/checkpatch.pl | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > > index 78cc595b98ce..fc73786064b3 100755 > > > > --- a/scripts/checkpatch.pl > > > > +++ b/scripts/checkpatch.pl > > > > @@ -6362,6 +6362,15 @@ sub process { > > > > } > > > > } > > > > > > > > +# check for soon-to-be-deprecated single-argument k[v]free_rcu() API > > > > + if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) { > > > > + if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) { > > > > + ERROR("DEPRECATED_API", > > > > + "Single-argument k[v]free_rcu() API is deprecated, please pass an rcu_head object." . $herecurr); > > > > > > Nice! > > > > > > But could you please also tell them what to use instead? Sure, they > > > could look it up, but if it tells them directly, they are less likely > > > to ignore it. > > > > Sounds good, I will modify the warning to include the API to call and send > > out a patch soon. > > > Maybe compile warnings? Or is it too aggressive? That is an excellent option if people ignore the checkpatch.pl warnings, thus forcing us to delay past v6.5. So Murphy would argue that we will in fact take your good advice at some point. ;-) Thanx, Paul
On Mon, Mar 06, 2023 at 08:55:01AM -0800, Paul E. McKenney wrote: > On Mon, Mar 06, 2023 at 05:42:44PM +0100, Uladzislau Rezki wrote: > > On Mon, Mar 06, 2023 at 03:12:03PM +0000, Joel Fernandes wrote: > > > On Mon, Mar 06, 2023 at 07:01:08AM -0800, Paul E. McKenney wrote: > > > [..] > > > > > > 7. We then evaluate whether further cleanups are needed. > > > > > > > > > > > > > > My feeling is > > > > > > > > that, we introduced "_mightsleep" macros first and after that try to > > > > > > > > convert users. > > > > > > > > > > > > > One stopgap could be to add a checkpatch error if anyone tries to use old API, > > > > > > > and then in the meanwhile convert all users. > > > > > > > Though, that requires people listening to checkpatch complaints. > > > > > > > > > > > > Every person who listens is that much less hassle. It doesn't have to > > > > > > be perfect. ;-) > > > > > > > > > > The below checkpatch change can catch at least simple single-arg uses (i.e. > > > > > not having compound expressions inside of k[v]free_rcu() args). I will submit > > > > > a proper patch to it which we can include in this set. > > > > > > > > > > Thoughts? > > > > > --- > > > > > scripts/checkpatch.pl | 9 +++++++++ > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > > > index 78cc595b98ce..fc73786064b3 100755 > > > > > --- a/scripts/checkpatch.pl > > > > > +++ b/scripts/checkpatch.pl > > > > > @@ -6362,6 +6362,15 @@ sub process { > > > > > } > > > > > } > > > > > > > > > > +# check for soon-to-be-deprecated single-argument k[v]free_rcu() API > > > > > + if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) { > > > > > + if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) { > > > > > + ERROR("DEPRECATED_API", > > > > > + "Single-argument k[v]free_rcu() API is deprecated, please pass an rcu_head object." . $herecurr); > > > > > > > > Nice! > > > > > > > > But could you please also tell them what to use instead? Sure, they > > > > could look it up, but if it tells them directly, they are less likely > > > > to ignore it. > > > > > > Sounds good, I will modify the warning to include the API to call and send > > > out a patch soon. > > > > > Maybe compile warnings? Or is it too aggressive? > > That is an excellent option if people ignore the checkpatch.pl warnings, > thus forcing us to delay past v6.5. So Murphy would argue that we will > in fact take your good advice at some point. ;-) > OK. On this step it sounds like a bit aggressive. checkpatch.pl should be fine as a light reminder :) -- Uladzislau Rezki
On Mon, Mar 6, 2023 at 12:10 PM Uladzislau Rezki <urezki@gmail.com> wrote: > > On Mon, Mar 06, 2023 at 08:55:01AM -0800, Paul E. McKenney wrote: > > On Mon, Mar 06, 2023 at 05:42:44PM +0100, Uladzislau Rezki wrote: > > > On Mon, Mar 06, 2023 at 03:12:03PM +0000, Joel Fernandes wrote: > > > > On Mon, Mar 06, 2023 at 07:01:08AM -0800, Paul E. McKenney wrote: > > > > [..] > > > > > > > 7. We then evaluate whether further cleanups are needed. > > > > > > > > > > > > > > > > My feeling is > > > > > > > > > that, we introduced "_mightsleep" macros first and after that try to > > > > > > > > > convert users. > > > > > > > > > > > > > > > One stopgap could be to add a checkpatch error if anyone tries to use old API, > > > > > > > > and then in the meanwhile convert all users. > > > > > > > > Though, that requires people listening to checkpatch complaints. > > > > > > > > > > > > > > Every person who listens is that much less hassle. It doesn't have to > > > > > > > be perfect. ;-) > > > > > > > > > > > > The below checkpatch change can catch at least simple single-arg uses (i.e. > > > > > > not having compound expressions inside of k[v]free_rcu() args). I will submit > > > > > > a proper patch to it which we can include in this set. > > > > > > > > > > > > Thoughts? > > > > > > --- > > > > > > scripts/checkpatch.pl | 9 +++++++++ > > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > > > > index 78cc595b98ce..fc73786064b3 100755 > > > > > > --- a/scripts/checkpatch.pl > > > > > > +++ b/scripts/checkpatch.pl > > > > > > @@ -6362,6 +6362,15 @@ sub process { > > > > > > } > > > > > > } > > > > > > > > > > > > +# check for soon-to-be-deprecated single-argument k[v]free_rcu() API > > > > > > + if ($line =~ /\bk[v]?free_rcu\s*\([^(]+\)/) { > > > > > > + if ($line =~ /\bk[v]?free_rcu\s*\([^,]+\)/) { > > > > > > + ERROR("DEPRECATED_API", > > > > > > + "Single-argument k[v]free_rcu() API is deprecated, please pass an rcu_head object." . $herecurr); > > > > > > > > > > Nice! > > > > > > > > > > But could you please also tell them what to use instead? Sure, they > > > > > could look it up, but if it tells them directly, they are less likely > > > > > to ignore it. > > > > > > > > Sounds good, I will modify the warning to include the API to call and send > > > > out a patch soon. > > > > > > > Maybe compile warnings? Or is it too aggressive? > > > > That is an excellent option if people ignore the checkpatch.pl warnings, > > thus forcing us to delay past v6.5. So Murphy would argue that we will > > in fact take your good advice at some point. ;-) > > > OK. On this step it sounds like a bit aggressive. checkpatch.pl should > be fine as a light reminder :) Agreed :). Also on some configs AFAIK, I believe the build will break with _any_ build warning so checkpatch is a good first step instead. I will post the checkpatch change today (also to get any early feedback). thanks, - Joel
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index 094321c17e48..7571dbfecb18 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -957,9 +957,8 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) /** * kfree_rcu() - kfree an object after a grace period. - * @ptr: pointer to kfree for both single- and double-argument invocations. - * @rhf: the name of the struct rcu_head within the type of @ptr, - * but only for double-argument invocations. + * @ptr: pointer to kfree for double-argument invocations. + * @rhf: the name of the struct rcu_head within the type of @ptr. * * Many rcu callbacks functions just call kfree() on the base structure. * These functions are trivial, but their size adds up, and furthermore @@ -982,26 +981,18 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) * The BUILD_BUG_ON check must not involve any function calls, hence the * checks are done in macros here. */ -#define kfree_rcu(ptr, rhf...) kvfree_rcu(ptr, ## rhf) +#define kfree_rcu(ptr, rhf) kvfree_rcu_arg_2(ptr, rhf) +#define kvfree_rcu(ptr, rhf) kvfree_rcu_arg_2(ptr, rhf) /** - * kvfree_rcu() - kvfree an object after a grace period. - * - * This macro consists of one or two arguments and it is - * based on whether an object is head-less or not. If it - * has a head then a semantic stays the same as it used - * to be before: - * - * kvfree_rcu(ptr, rhf); - * - * where @ptr is a pointer to kvfree(), @rhf is the name - * of the rcu_head structure within the type of @ptr. + * kfree_rcu_mightsleep() - kfree an object after a grace period. + * @ptr: pointer to kfree for single-argument invocations. * * When it comes to head-less variant, only one argument * is passed and that is just a pointer which has to be * freed after a grace period. Therefore the semantic is * - * kvfree_rcu(ptr); + * kfree_rcu_mightsleep(ptr); * * where @ptr is the pointer to be freed by kvfree(). * @@ -1010,13 +1001,9 @@ static inline notrace void rcu_read_unlock_sched_notrace(void) * annotation. Otherwise, please switch and embed the * rcu_head structure within the type of @ptr. */ -#define kvfree_rcu(...) KVFREE_GET_MACRO(__VA_ARGS__, \ - kvfree_rcu_arg_2, kvfree_rcu_arg_1)(__VA_ARGS__) - +#define kfree_rcu_mightsleep(ptr) kvfree_rcu_arg_1(ptr) #define kvfree_rcu_mightsleep(ptr) kvfree_rcu_arg_1(ptr) -#define kfree_rcu_mightsleep(ptr) kvfree_rcu_mightsleep(ptr) -#define KVFREE_GET_MACRO(_1, _2, NAME, ...) NAME #define kvfree_rcu_arg_2(ptr, rhf) \ do { \ typeof (ptr) ___p = (ptr); \
For a single argument invocations a new kfree_rcu_mightsleep() and kvfree_rcu_mightsleep() macroses are used. This is done in order to prevent users from calling a single argument from atomic contexts as "_mightsleep" prefix signals that it can schedule(). Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com> --- include/linux/rcupdate.h | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-)