Re: [PATCH 13/13] rcu/kvfree: Eliminate k[v]free_rcu() single argument macro

[Paul E. McKenney]

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@xxxxxxxxx> wrote:
> > > > 
> > > > 
> > > >> 
> > > >>>> On Mar 5, 2023, at 5:29 AM, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
> > > >>> 
> > > >>> Hi, All,
> > > >>> 
> > > >>>> On Wed, Feb 1, 2023 at 10:11 AM Uladzislau Rezki (Sony)
> > > >>>> <urezki@xxxxxxxxx> 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
>