MIME-Version: 1.0
References: <20230313080403.89290-1-qiuxu.zhuo@intel.com> <d930f2a8-4b75-4109-a303-e2fc04443693@paulmck-laptop>
In-Reply-To: <d930f2a8-4b75-4109-a303-e2fc04443693@paulmck-laptop>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Tue, 14 Mar 2023 19:29:54 -0400
Message-ID: <CAEXW_YRxWGCuV_YVLqpFqOnGAMYAf2c+TAV1iAD8=2uVOqkf_Q@mail.gmail.com>
Subject: Re: [PATCH 1/1] rcu/rcuscale: Stop kfree_scale_thread thread(s) after
 unloading rcuscale
To:     paulmck@kernel.org
Cc:     Qiuxu Zhuo <qiuxu.zhuo@intel.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        Josh Triplett <josh@joshtriplett.org>,
        Neeraj Upadhyay <quic_neeraju@quicinc.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 14, 2023 at 7:02=E2=80=AFPM Paul E. McKenney <paulmck@kernel.or=
g> wrote:
>
> On Mon, Mar 13, 2023 at 04:04:03PM +0800, Qiuxu Zhuo wrote:
> > When running the 'kfree_rcu_test' test case with commands [1] the call
> > trace [2] was thrown. This was because the kfree_scale_thread thread(s)
> > still run after unloading rcuscale and torture modules. Fix the call
> > trace by invoking kfree_scale_cleanup() when removing the rcuscale modu=
le.
>
> Good catch, thank you!
>
> > [1] modprobe torture
> >     modprobe rcuscale kfree_rcu_test=3D1
>
> Given that loading the rcuscale kernel module automatically pulls in
> the rcuscale kernel module, why the "modprobe torture"?  Is doing the
> modprobes separately like this necessary to reproduce this bug?
>
> If it reproduces without manually loading and unloading the "torture"
> kernel module, could you please update the commit log to show that
> smaller reproducer?
>
> >     // After some time
> >     rmmod rcuscale
> >     rmmod torture
> >
> > [2] BUG: unable to handle page fault for address: ffffffffc0601a87
> >     #PF: supervisor instruction fetch in kernel mode
> >     #PF: error_code(0x0010) - not-present page
> >     PGD 11de4f067 P4D 11de4f067 PUD 11de51067 PMD 112f4d067 PTE 0
> >     Oops: 0010 [#1] PREEMPT SMP NOPTI
[..]
> >     Call Trace:
> >      <TASK>
> >      ? kvfree_call_rcu+0xf0/0x3a0
> >      ? kthread+0xf3/0x120
> >      ? kthread_complete_and_exit+0x20/0x20
> >      ? ret_from_fork+0x1f/0x30
> >      </TASK>
> >     Modules linked in: rfkill sunrpc ... [last unloaded: torture]
> >     CR2: ffffffffc0601a87
> >     ---[ end trace 0000000000000000 ]---
> >
> > Fixes: e6e78b004fa7 ("rcuperf: Add kfree_rcu() performance Tests")
> > Signed-off-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com>
> > ---
> >  kernel/rcu/rcuscale.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/kernel/rcu/rcuscale.c b/kernel/rcu/rcuscale.c
> > index 91fb5905a008..5e580cd08c58 100644
> > --- a/kernel/rcu/rcuscale.c
> > +++ b/kernel/rcu/rcuscale.c
> > @@ -522,6 +522,8 @@ rcu_scale_print_module_parms(struct rcu_scale_ops *=
cur_ops, const char *tag)
> >                scale_type, tag, nrealreaders, nrealwriters, verbose, sh=
utdown);
> >  }
> >
> > +static void kfree_scale_cleanup(void);
> > +
>
> I do applaud minmimizing the size of the patch, but in this case could yo=
u
> please pull the kfree_scale_cleanup() function ahead of its first use?

The only trouble with moving the function like that is, the file is
mostly split across kfree and non-kfree functions. So moving a kfree
function to be among the non-kfree ones would look a bit weird.
Perhaps a better place for the function declaration could be a new
"rcuscale.h". But I am really Ok with Paul's suggestion as well.

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel


>
>                                                         Thanx, Paul
>
> >  static void
> >  rcu_scale_cleanup(void)
> >  {
> > @@ -542,6 +544,11 @@ rcu_scale_cleanup(void)
> >       if (gp_exp && gp_async)
> >               SCALEOUT_ERRSTRING("No expedited async GPs, so went with =
async!");
> >
> > +     if (kfree_rcu_test) {
> > +             kfree_scale_cleanup();
> > +             return;
> > +     }
> > +
> >       if (torture_cleanup_begin())
> >               return;
> >       if (!cur_ops) {
> > --
> > 2.17.1
> >