MIME-Version: 1.0
References: <ZAR//FKO4syzapk6@pc636> <D8B84631-860B-41CF-8311-88E220C7254F@joelfernandes.org>
 <20230305180524.GL1301832@paulmck-ThinkPad-P17-Gen-1> <20230306144948.GA3280216@google.com>
 <20230306150108.GT1301832@paulmck-ThinkPad-P17-Gen-1> <20230306151203.GC3280216@google.com>
 <ZAYYBEnIZEfUhBYW@pc636> <20230306165501.GX1301832@paulmck-ThinkPad-P17-Gen-1>
 <ZAYebhiXJAct1vUS@pc636>
In-Reply-To: <ZAYebhiXJAct1vUS@pc636>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Mon, 6 Mar 2023 14:54:28 -0500
Message-ID: <CAEXW_YSY=q2_uaE2qo4XSGjzs4+C102YMVJ7kWwuT5LGmJGGew@mail.gmail.com>
Subject: Re: [PATCH 13/13] rcu/kvfree: Eliminate k[v]free_rcu() single
 argument macro
To:     Uladzislau Rezki <urezki@gmail.com>
Cc:     "Paul E. McKenney" <paulmck@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, RCU <rcu@vger.kernel.org>,
        Oleksiy Avramchenko <oleksiy.avramchenko@sony.com>,
        Jens Axboe <axboe@kernel.dk>,
        Philipp Reisner <philipp.reisner@linbit.com>,
        Bryan Tan <bryantan@vmware.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Eric Dumazet <edumazet@google.com>,
        Bob Pearson <rpearsonhpe@gmail.com>,
        Ariel Levkovich <lariel@nvidia.com>,
        "Theodore Ts'o" <tytso@mit.edu>, Julian Anastasov <ja@ssi.bg>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Mar 6, 2023 at 12:10=E2=80=AFPM 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 tr=
ies to use old API,
> > > > > > > > and then in the meanwhile convert all users.
> > > > > > > > Though, that requires people listening to checkpatch compla=
ints.
> > > > > > >
> > > > > > > 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-ar=
g 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 =3D~ /\bk[v]?free_rcu\s*\([^(]+\)/) {
> > > > > > +                     if ($line =3D~ /\bk[v]?free_rcu\s*\([^,]+=
\)/) {
> > > > > > +                             ERROR("DEPRECATED_API",
> > > > > > +                                   "Single-argument k[v]free_r=
cu() API is deprecated, please pass an rcu_head object." . $herecurr);
> > > > >
> > > > > Nice!
> > > > >
> > > > > But could you please also tell them what to use instead?  Sure, t=
hey
> > > > > could look it up, but if it tells them directly, they are less li=
kely
> > > > > to ignore it.
> > > >
> > > > Sounds good, I will modify the warning to include the API to call a=
nd 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