[1/1] rcu/rcuscale: Stop kfree_scale_thread thread(s) after unloading rcuscale

  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 module.

[1] modprobe torture
    modprobe rcuscale kfree_rcu_test=1
    // 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
    CPU: 1 PID: 1798 Comm: kfree_scale_thr Not tainted 6.3.0-rc1-rcu+ #1
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
    RIP: 0010:0xffffffffc0601a87
    Code: Unable to access opcode bytes at 0xffffffffc0601a5d.
    RSP: 0018:ffffb25bc2e57e18 EFLAGS: 00010297
    RAX: 0000000000000000 RBX: ffffffffc061f0b6 RCX: 0000000000000000
    RDX: 0000000000000000 RSI: ffffffff962fd0de RDI: ffffffff962fd0de
    RBP: ffffb25bc2e57ea8 R08: 0000000000000000 R09: 0000000000000000
    R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000
    R13: 0000000000000000 R14: 000000000000000a R15: 00000000001c1dbe
    FS:  0000000000000000(0000) GS:ffff921fa2200000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: ffffffffc0601a5d CR3: 000000011de4c006 CR4: 0000000000370ee0
    DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
    DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
    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(+)
  

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 module.

Good catch, thank you!

> [1] modprobe torture
>     modprobe rcuscale kfree_rcu_test=1

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
>     CPU: 1 PID: 1798 Comm: kfree_scale_thr Not tainted 6.3.0-rc1-rcu+ #1
>     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015
>     RIP: 0010:0xffffffffc0601a87
>     Code: Unable to access opcode bytes at 0xffffffffc0601a5d.
>     RSP: 0018:ffffb25bc2e57e18 EFLAGS: 00010297
>     RAX: 0000000000000000 RBX: ffffffffc061f0b6 RCX: 0000000000000000
>     RDX: 0000000000000000 RSI: ffffffff962fd0de RDI: ffffffff962fd0de
>     RBP: ffffb25bc2e57ea8 R08: 0000000000000000 R09: 0000000000000000
>     R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000000
>     R13: 0000000000000000 R14: 000000000000000a R15: 00000000001c1dbe
>     FS:  0000000000000000(0000) GS:ffff921fa2200000(0000) knlGS:0000000000000000
>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     CR2: ffffffffc0601a5d CR3: 000000011de4c006 CR4: 0000000000370ee0
>     DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>     DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>     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, shutdown);
>  }
>  
> +static void kfree_scale_cleanup(void);
> +

I do applaud minmimizing the size of the patch, but in this case could you
please pull the kfree_scale_cleanup() function ahead of its first use?

							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
>
  

On Tue, Mar 14, 2023 at 7:02 PM Paul E. McKenney <paulmck@kernel.org> 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 module.
>
> Good catch, thank you!
>
> > [1] modprobe torture
> >     modprobe rcuscale kfree_rcu_test=1
>
> 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, shutdown);
> >  }
> >
> > +static void kfree_scale_cleanup(void);
> > +
>
> I do applaud minmimizing the size of the patch, but in this case could you
> 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
> >
  

> From: Paul E. McKenney <paulmck@kernel.org>
> [...]
> Subject: Re: [PATCH 1/1] rcu/rcuscale: Stop kfree_scale_thread thread(s)
> after unloading rcuscale
> 
> 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 module.
> 
> Good catch, thank you!
> 
> > [1] modprobe torture
> >     modprobe rcuscale kfree_rcu_test=1
> 
> Given that loading the rcuscale kernel module automatically pulls in the
> rcuscale kernel module, why the "modprobe torture"?  Is doing the

Oops ... 
I forgot that the system could automatically figure out and load dependent modules.
Thank you for pointing it out.

> modprobes separately like this necessary to reproduce this bug?

No. It isn't.

> If it reproduces without manually loading and unloading the "torture"
> kernel module, could you please update the commit log to show that smaller
> reproducer?

To reproduce the bug, it needs to manually unload the "torture" but doesn't need
to manually load "torture".

I'll remove the unnecessary step "modprobe torture" from the commit message in the v2 patch.

> >     // After some time
> >     rmmod rcuscale
> >     rmmod torture
> >
[...]
> > ---
> >  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,
> shutdown);
> > }
> >
> > +static void kfree_scale_cleanup(void);
> > +
> 
> I do applaud minmimizing the size of the patch, but in this case could you
> please pull the kfree_scale_cleanup() function ahead of its first use?
> 

I thought about it, but was also concerned that would make the patch bigger 
while the effective changes were just only a few lines ... 

Pulling the kfree_scale_cleanup() function ahead of rcu_scale_cleanup() also needs
to pull another two kfree_* variables ahead used by kfree_scale_cleanup(). 
This looks like will mess up kfree_* and rcu_scale_* functions/variables in the source file.

How about to pull the rcu_scale_cleanup() function after kfree_scale_cleanup().
This groups kfree_* functions and groups rcu_scale_* functions. 
Then the code would look cleaner.
So, do you think the changes below are better?

---
 kernel/rcu/rcuscale.c | 201 ++++++++++++++++++++++--------------------
 1 file changed, 103 insertions(+), 98 deletions(-)

diff --git a/kernel/rcu/rcuscale.c b/kernel/rcu/rcuscale.c
index 91fb5905a008..5a000d26f03e 100644
--- a/kernel/rcu/rcuscale.c
+++ b/kernel/rcu/rcuscale.c
@@ -522,89 +522,6 @@ rcu_scale_print_module_parms(struct rcu_scale_ops *cur_ops, const char *tag)
 		 scale_type, tag, nrealreaders, nrealwriters, verbose, shutdown);
 }
 
-static void
-rcu_scale_cleanup(void)
-{
-	int i;
-	int j;
-	int ngps = 0;
-	u64 *wdp;
-	u64 *wdpp;
-
-	/*
-	 * Would like warning at start, but everything is expedited
-	 * during the mid-boot phase, so have to wait till the end.
-	 */
-	if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
-		SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
-	if (rcu_gp_is_normal() && gp_exp)
-		SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
-	if (gp_exp && gp_async)
-		SCALEOUT_ERRSTRING("No expedited async GPs, so went with async!");
-
-	if (torture_cleanup_begin())
-		return;
-	if (!cur_ops) {
-		torture_cleanup_end();
-		return;
-	}
-
-	if (reader_tasks) {
-		for (i = 0; i < nrealreaders; i++)
-			torture_stop_kthread(rcu_scale_reader,
-					     reader_tasks[i]);
-		kfree(reader_tasks);
-	}
-
-	if (writer_tasks) {
-		for (i = 0; i < nrealwriters; i++) {
-			torture_stop_kthread(rcu_scale_writer,
-					     writer_tasks[i]);
-			if (!writer_n_durations)
-				continue;
-			j = writer_n_durations[i];
-			pr_alert("%s%s writer %d gps: %d\n",
-				 scale_type, SCALE_FLAG, i, j);
-			ngps += j;
-		}
-		pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
-			 scale_type, SCALE_FLAG,
-			 t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
-			 t_rcu_scale_writer_finished -
-			 t_rcu_scale_writer_started,
-			 ngps,
-			 rcuscale_seq_diff(b_rcu_gp_test_finished,
-					   b_rcu_gp_test_started));
-		for (i = 0; i < nrealwriters; i++) {
-			if (!writer_durations)
-				break;
-			if (!writer_n_durations)
-				continue;
-			wdpp = writer_durations[i];
-			if (!wdpp)
-				continue;
-			for (j = 0; j < writer_n_durations[i]; j++) {
-				wdp = &wdpp[j];
-				pr_alert("%s%s %4d writer-duration: %5d %llu\n",
-					scale_type, SCALE_FLAG,
-					i, j, *wdp);
-				if (j % 100 == 0)
-					schedule_timeout_uninterruptible(1);
-			}
-			kfree(writer_durations[i]);
-		}
-		kfree(writer_tasks);
-		kfree(writer_durations);
-		kfree(writer_n_durations);
-	}
-
-	/* Do torture-type-specific cleanup operations.  */
-	if (cur_ops->cleanup != NULL)
-		cur_ops->cleanup();
-
-	torture_cleanup_end();
-}
-
 /*
  * Return the number if non-negative.  If -1, the number of CPUs.
  * If less than -1, that much less than the number of CPUs, but
@@ -624,21 +541,6 @@ static int compute_real(int n)
 	return nr;
 }
 
-/*
- * RCU scalability shutdown kthread.  Just waits to be awakened, then shuts
- * down system.
- */
-static int
-rcu_scale_shutdown(void *arg)
-{
-	wait_event(shutdown_wq,
-		   atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
-	smp_mb(); /* Wake before output. */
-	rcu_scale_cleanup();
-	kernel_power_off();
-	return -EINVAL;
-}
-
 /*
  * kfree_rcu() scalability tests: Start a kfree_rcu() loop on all CPUs for number
  * of iterations and measure total time and number of GP for all iterations to complete.
@@ -875,6 +777,109 @@ kfree_scale_init(void)
 	return firsterr;
 }
 
+static void
+rcu_scale_cleanup(void)
+{
+	int i;
+	int j;
+	int ngps = 0;
+	u64 *wdp;
+	u64 *wdpp;
+
+	/*
+	 * Would like warning at start, but everything is expedited
+	 * during the mid-boot phase, so have to wait till the end.
+	 */
+	if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
+		SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
+	if (rcu_gp_is_normal() && gp_exp)
+		SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
+	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) {
+		torture_cleanup_end();
+		return;
+	}
+
+	if (reader_tasks) {
+		for (i = 0; i < nrealreaders; i++)
+			torture_stop_kthread(rcu_scale_reader,
+					     reader_tasks[i]);
+		kfree(reader_tasks);
+	}
+
+	if (writer_tasks) {
+		for (i = 0; i < nrealwriters; i++) {
+			torture_stop_kthread(rcu_scale_writer,
+					     writer_tasks[i]);
+			if (!writer_n_durations)
+				continue;
+			j = writer_n_durations[i];
+			pr_alert("%s%s writer %d gps: %d\n",
+				 scale_type, SCALE_FLAG, i, j);
+			ngps += j;
+		}
+		pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
+			 scale_type, SCALE_FLAG,
+			 t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
+			 t_rcu_scale_writer_finished -
+			 t_rcu_scale_writer_started,
+			 ngps,
+			 rcuscale_seq_diff(b_rcu_gp_test_finished,
+					   b_rcu_gp_test_started));
+		for (i = 0; i < nrealwriters; i++) {
+			if (!writer_durations)
+				break;
+			if (!writer_n_durations)
+				continue;
+			wdpp = writer_durations[i];
+			if (!wdpp)
+				continue;
+			for (j = 0; j < writer_n_durations[i]; j++) {
+				wdp = &wdpp[j];
+				pr_alert("%s%s %4d writer-duration: %5d %llu\n",
+					scale_type, SCALE_FLAG,
+					i, j, *wdp);
+				if (j % 100 == 0)
+					schedule_timeout_uninterruptible(1);
+			}
+			kfree(writer_durations[i]);
+		}
+		kfree(writer_tasks);
+		kfree(writer_durations);
+		kfree(writer_n_durations);
+	}
+
+	/* Do torture-type-specific cleanup operations.  */
+	if (cur_ops->cleanup != NULL)
+		cur_ops->cleanup();
+
+	torture_cleanup_end();
+}
+
+/*
+ * RCU scalability shutdown kthread.  Just waits to be awakened, then shuts
+ * down system.
+ */
+static int
+rcu_scale_shutdown(void *arg)
+{
+	wait_event(shutdown_wq,
+		   atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
+	smp_mb(); /* Wake before output. */
+	rcu_scale_cleanup();
+	kernel_power_off();
+	return -EINVAL;
+}
+
 static int __init
 rcu_scale_init(void)
 {
-- 
2.17.1
 
> [...]
  

On Wed, Mar 15, 2023 at 10:07 AM Zhuo, Qiuxu <qiuxu.zhuo@intel.com> wrote:
[...]
> >
> > I do applaud minmimizing the size of the patch, but in this case could you
> > please pull the kfree_scale_cleanup() function ahead of its first use?
> >
>
> I thought about it, but was also concerned that would make the patch bigger
> while the effective changes were just only a few lines ...
>
> Pulling the kfree_scale_cleanup() function ahead of rcu_scale_cleanup() also needs
> to pull another two kfree_* variables ahead used by kfree_scale_cleanup().
> This looks like will mess up kfree_* and rcu_scale_* functions/variables in the source file.
>
> How about to pull the rcu_scale_cleanup() function after kfree_scale_cleanup().
> This groups kfree_* functions and groups rcu_scale_* functions.
> Then the code would look cleaner.
> So, do you think the changes below are better?

IMHO, I don't think doing such a code move is better. Just add a new
header file and declare the function there. But see what Paul says
first.

thanks,

 - Joel



>
> ---
>  kernel/rcu/rcuscale.c | 201 ++++++++++++++++++++++--------------------
>  1 file changed, 103 insertions(+), 98 deletions(-)
>
> diff --git a/kernel/rcu/rcuscale.c b/kernel/rcu/rcuscale.c
> index 91fb5905a008..5a000d26f03e 100644
> --- a/kernel/rcu/rcuscale.c
> +++ b/kernel/rcu/rcuscale.c
> @@ -522,89 +522,6 @@ rcu_scale_print_module_parms(struct rcu_scale_ops *cur_ops, const char *tag)
>                  scale_type, tag, nrealreaders, nrealwriters, verbose, shutdown);
>  }
>
> -static void
> -rcu_scale_cleanup(void)
> -{
> -       int i;
> -       int j;
> -       int ngps = 0;
> -       u64 *wdp;
> -       u64 *wdpp;
> -
> -       /*
> -        * Would like warning at start, but everything is expedited
> -        * during the mid-boot phase, so have to wait till the end.
> -        */
> -       if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
> -               SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
> -       if (rcu_gp_is_normal() && gp_exp)
> -               SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
> -       if (gp_exp && gp_async)
> -               SCALEOUT_ERRSTRING("No expedited async GPs, so went with async!");
> -
> -       if (torture_cleanup_begin())
> -               return;
> -       if (!cur_ops) {
> -               torture_cleanup_end();
> -               return;
> -       }
> -
> -       if (reader_tasks) {
> -               for (i = 0; i < nrealreaders; i++)
> -                       torture_stop_kthread(rcu_scale_reader,
> -                                            reader_tasks[i]);
> -               kfree(reader_tasks);
> -       }
> -
> -       if (writer_tasks) {
> -               for (i = 0; i < nrealwriters; i++) {
> -                       torture_stop_kthread(rcu_scale_writer,
> -                                            writer_tasks[i]);
> -                       if (!writer_n_durations)
> -                               continue;
> -                       j = writer_n_durations[i];
> -                       pr_alert("%s%s writer %d gps: %d\n",
> -                                scale_type, SCALE_FLAG, i, j);
> -                       ngps += j;
> -               }
> -               pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
> -                        scale_type, SCALE_FLAG,
> -                        t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
> -                        t_rcu_scale_writer_finished -
> -                        t_rcu_scale_writer_started,
> -                        ngps,
> -                        rcuscale_seq_diff(b_rcu_gp_test_finished,
> -                                          b_rcu_gp_test_started));
> -               for (i = 0; i < nrealwriters; i++) {
> -                       if (!writer_durations)
> -                               break;
> -                       if (!writer_n_durations)
> -                               continue;
> -                       wdpp = writer_durations[i];
> -                       if (!wdpp)
> -                               continue;
> -                       for (j = 0; j < writer_n_durations[i]; j++) {
> -                               wdp = &wdpp[j];
> -                               pr_alert("%s%s %4d writer-duration: %5d %llu\n",
> -                                       scale_type, SCALE_FLAG,
> -                                       i, j, *wdp);
> -                               if (j % 100 == 0)
> -                                       schedule_timeout_uninterruptible(1);
> -                       }
> -                       kfree(writer_durations[i]);
> -               }
> -               kfree(writer_tasks);
> -               kfree(writer_durations);
> -               kfree(writer_n_durations);
> -       }
> -
> -       /* Do torture-type-specific cleanup operations.  */
> -       if (cur_ops->cleanup != NULL)
> -               cur_ops->cleanup();
> -
> -       torture_cleanup_end();
> -}
> -
>  /*
>   * Return the number if non-negative.  If -1, the number of CPUs.
>   * If less than -1, that much less than the number of CPUs, but
> @@ -624,21 +541,6 @@ static int compute_real(int n)
>         return nr;
>  }
>
> -/*
> - * RCU scalability shutdown kthread.  Just waits to be awakened, then shuts
> - * down system.
> - */
> -static int
> -rcu_scale_shutdown(void *arg)
> -{
> -       wait_event(shutdown_wq,
> -                  atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
> -       smp_mb(); /* Wake before output. */
> -       rcu_scale_cleanup();
> -       kernel_power_off();
> -       return -EINVAL;
> -}
> -
>  /*
>   * kfree_rcu() scalability tests: Start a kfree_rcu() loop on all CPUs for number
>   * of iterations and measure total time and number of GP for all iterations to complete.
> @@ -875,6 +777,109 @@ kfree_scale_init(void)
>         return firsterr;
>  }
>
> +static void
> +rcu_scale_cleanup(void)
> +{
> +       int i;
> +       int j;
> +       int ngps = 0;
> +       u64 *wdp;
> +       u64 *wdpp;
> +
> +       /*
> +        * Would like warning at start, but everything is expedited
> +        * during the mid-boot phase, so have to wait till the end.
> +        */
> +       if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
> +               SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
> +       if (rcu_gp_is_normal() && gp_exp)
> +               SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
> +       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) {
> +               torture_cleanup_end();
> +               return;
> +       }
> +
> +       if (reader_tasks) {
> +               for (i = 0; i < nrealreaders; i++)
> +                       torture_stop_kthread(rcu_scale_reader,
> +                                            reader_tasks[i]);
> +               kfree(reader_tasks);
> +       }
> +
> +       if (writer_tasks) {
> +               for (i = 0; i < nrealwriters; i++) {
> +                       torture_stop_kthread(rcu_scale_writer,
> +                                            writer_tasks[i]);
> +                       if (!writer_n_durations)
> +                               continue;
> +                       j = writer_n_durations[i];
> +                       pr_alert("%s%s writer %d gps: %d\n",
> +                                scale_type, SCALE_FLAG, i, j);
> +                       ngps += j;
> +               }
> +               pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
> +                        scale_type, SCALE_FLAG,
> +                        t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
> +                        t_rcu_scale_writer_finished -
> +                        t_rcu_scale_writer_started,
> +                        ngps,
> +                        rcuscale_seq_diff(b_rcu_gp_test_finished,
> +                                          b_rcu_gp_test_started));
> +               for (i = 0; i < nrealwriters; i++) {
> +                       if (!writer_durations)
> +                               break;
> +                       if (!writer_n_durations)
> +                               continue;
> +                       wdpp = writer_durations[i];
> +                       if (!wdpp)
> +                               continue;
> +                       for (j = 0; j < writer_n_durations[i]; j++) {
> +                               wdp = &wdpp[j];
> +                               pr_alert("%s%s %4d writer-duration: %5d %llu\n",
> +                                       scale_type, SCALE_FLAG,
> +                                       i, j, *wdp);
> +                               if (j % 100 == 0)
> +                                       schedule_timeout_uninterruptible(1);
> +                       }
> +                       kfree(writer_durations[i]);
> +               }
> +               kfree(writer_tasks);
> +               kfree(writer_durations);
> +               kfree(writer_n_durations);
> +       }
> +
> +       /* Do torture-type-specific cleanup operations.  */
> +       if (cur_ops->cleanup != NULL)
> +               cur_ops->cleanup();
> +
> +       torture_cleanup_end();
> +}
> +
> +/*
> + * RCU scalability shutdown kthread.  Just waits to be awakened, then shuts
> + * down system.
> + */
> +static int
> +rcu_scale_shutdown(void *arg)
> +{
> +       wait_event(shutdown_wq,
> +                  atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
> +       smp_mb(); /* Wake before output. */
> +       rcu_scale_cleanup();
> +       kernel_power_off();
> +       return -EINVAL;
> +}
> +
>  static int __init
>  rcu_scale_init(void)
>  {
> --
> 2.17.1
>
> > [...]
>
  

> From: Joel Fernandes <joel@joelfernandes.org>
> [...]
> > >  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,
> > > shutdown);  }
> > >
> > > +static void kfree_scale_cleanup(void);
> > > +
> >
> > I do applaud minmimizing the size of the patch, but in this case could
> > you 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.

Yes, this would look a bit weird ...

Please see the reply to Paul in another e-mail: 
"Pull the rcu_scale_cleanup() function after kfree_scale_cleanup().
This groups kfree_* functions and groups rcu_scale_* functions. 
Then the code would look cleaner."

> 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 for the review. :-)

> thanks,
> 
>  - Joel
  

On Wed, Mar 15, 2023 at 10:12:05AM -0400, Joel Fernandes wrote:
> On Wed, Mar 15, 2023 at 10:07 AM Zhuo, Qiuxu <qiuxu.zhuo@intel.com> wrote:
> [...]
> > >
> > > I do applaud minmimizing the size of the patch, but in this case could you
> > > please pull the kfree_scale_cleanup() function ahead of its first use?
> > >
> >
> > I thought about it, but was also concerned that would make the patch bigger
> > while the effective changes were just only a few lines ...
> >
> > Pulling the kfree_scale_cleanup() function ahead of rcu_scale_cleanup() also needs
> > to pull another two kfree_* variables ahead used by kfree_scale_cleanup().
> > This looks like will mess up kfree_* and rcu_scale_* functions/variables in the source file.
> >
> > How about to pull the rcu_scale_cleanup() function after kfree_scale_cleanup().
> > This groups kfree_* functions and groups rcu_scale_* functions.
> > Then the code would look cleaner.
> > So, do you think the changes below are better?
> 
> IMHO, I don't think doing such a code move is better. Just add a new
> header file and declare the function there. But see what Paul says
> first.

This situation is likely to be an early hint that the kvfree_rcu() testing
should be split out from kernel/rcu/rcuscale.c.

							Thanx, Paul

> thanks,
> 
>  - Joel
> 
> 
> 
> >
> > ---
> >  kernel/rcu/rcuscale.c | 201 ++++++++++++++++++++++--------------------
> >  1 file changed, 103 insertions(+), 98 deletions(-)
> >
> > diff --git a/kernel/rcu/rcuscale.c b/kernel/rcu/rcuscale.c
> > index 91fb5905a008..5a000d26f03e 100644
> > --- a/kernel/rcu/rcuscale.c
> > +++ b/kernel/rcu/rcuscale.c
> > @@ -522,89 +522,6 @@ rcu_scale_print_module_parms(struct rcu_scale_ops *cur_ops, const char *tag)
> >                  scale_type, tag, nrealreaders, nrealwriters, verbose, shutdown);
> >  }
> >
> > -static void
> > -rcu_scale_cleanup(void)
> > -{
> > -       int i;
> > -       int j;
> > -       int ngps = 0;
> > -       u64 *wdp;
> > -       u64 *wdpp;
> > -
> > -       /*
> > -        * Would like warning at start, but everything is expedited
> > -        * during the mid-boot phase, so have to wait till the end.
> > -        */
> > -       if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
> > -               SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
> > -       if (rcu_gp_is_normal() && gp_exp)
> > -               SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
> > -       if (gp_exp && gp_async)
> > -               SCALEOUT_ERRSTRING("No expedited async GPs, so went with async!");
> > -
> > -       if (torture_cleanup_begin())
> > -               return;
> > -       if (!cur_ops) {
> > -               torture_cleanup_end();
> > -               return;
> > -       }
> > -
> > -       if (reader_tasks) {
> > -               for (i = 0; i < nrealreaders; i++)
> > -                       torture_stop_kthread(rcu_scale_reader,
> > -                                            reader_tasks[i]);
> > -               kfree(reader_tasks);
> > -       }
> > -
> > -       if (writer_tasks) {
> > -               for (i = 0; i < nrealwriters; i++) {
> > -                       torture_stop_kthread(rcu_scale_writer,
> > -                                            writer_tasks[i]);
> > -                       if (!writer_n_durations)
> > -                               continue;
> > -                       j = writer_n_durations[i];
> > -                       pr_alert("%s%s writer %d gps: %d\n",
> > -                                scale_type, SCALE_FLAG, i, j);
> > -                       ngps += j;
> > -               }
> > -               pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
> > -                        scale_type, SCALE_FLAG,
> > -                        t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
> > -                        t_rcu_scale_writer_finished -
> > -                        t_rcu_scale_writer_started,
> > -                        ngps,
> > -                        rcuscale_seq_diff(b_rcu_gp_test_finished,
> > -                                          b_rcu_gp_test_started));
> > -               for (i = 0; i < nrealwriters; i++) {
> > -                       if (!writer_durations)
> > -                               break;
> > -                       if (!writer_n_durations)
> > -                               continue;
> > -                       wdpp = writer_durations[i];
> > -                       if (!wdpp)
> > -                               continue;
> > -                       for (j = 0; j < writer_n_durations[i]; j++) {
> > -                               wdp = &wdpp[j];
> > -                               pr_alert("%s%s %4d writer-duration: %5d %llu\n",
> > -                                       scale_type, SCALE_FLAG,
> > -                                       i, j, *wdp);
> > -                               if (j % 100 == 0)
> > -                                       schedule_timeout_uninterruptible(1);
> > -                       }
> > -                       kfree(writer_durations[i]);
> > -               }
> > -               kfree(writer_tasks);
> > -               kfree(writer_durations);
> > -               kfree(writer_n_durations);
> > -       }
> > -
> > -       /* Do torture-type-specific cleanup operations.  */
> > -       if (cur_ops->cleanup != NULL)
> > -               cur_ops->cleanup();
> > -
> > -       torture_cleanup_end();
> > -}
> > -
> >  /*
> >   * Return the number if non-negative.  If -1, the number of CPUs.
> >   * If less than -1, that much less than the number of CPUs, but
> > @@ -624,21 +541,6 @@ static int compute_real(int n)
> >         return nr;
> >  }
> >
> > -/*
> > - * RCU scalability shutdown kthread.  Just waits to be awakened, then shuts
> > - * down system.
> > - */
> > -static int
> > -rcu_scale_shutdown(void *arg)
> > -{
> > -       wait_event(shutdown_wq,
> > -                  atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
> > -       smp_mb(); /* Wake before output. */
> > -       rcu_scale_cleanup();
> > -       kernel_power_off();
> > -       return -EINVAL;
> > -}
> > -
> >  /*
> >   * kfree_rcu() scalability tests: Start a kfree_rcu() loop on all CPUs for number
> >   * of iterations and measure total time and number of GP for all iterations to complete.
> > @@ -875,6 +777,109 @@ kfree_scale_init(void)
> >         return firsterr;
> >  }
> >
> > +static void
> > +rcu_scale_cleanup(void)
> > +{
> > +       int i;
> > +       int j;
> > +       int ngps = 0;
> > +       u64 *wdp;
> > +       u64 *wdpp;
> > +
> > +       /*
> > +        * Would like warning at start, but everything is expedited
> > +        * during the mid-boot phase, so have to wait till the end.
> > +        */
> > +       if (rcu_gp_is_expedited() && !rcu_gp_is_normal() && !gp_exp)
> > +               SCALEOUT_ERRSTRING("All grace periods expedited, no normal ones to measure!");
> > +       if (rcu_gp_is_normal() && gp_exp)
> > +               SCALEOUT_ERRSTRING("All grace periods normal, no expedited ones to measure!");
> > +       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) {
> > +               torture_cleanup_end();
> > +               return;
> > +       }
> > +
> > +       if (reader_tasks) {
> > +               for (i = 0; i < nrealreaders; i++)
> > +                       torture_stop_kthread(rcu_scale_reader,
> > +                                            reader_tasks[i]);
> > +               kfree(reader_tasks);
> > +       }
> > +
> > +       if (writer_tasks) {
> > +               for (i = 0; i < nrealwriters; i++) {
> > +                       torture_stop_kthread(rcu_scale_writer,
> > +                                            writer_tasks[i]);
> > +                       if (!writer_n_durations)
> > +                               continue;
> > +                       j = writer_n_durations[i];
> > +                       pr_alert("%s%s writer %d gps: %d\n",
> > +                                scale_type, SCALE_FLAG, i, j);
> > +                       ngps += j;
> > +               }
> > +               pr_alert("%s%s start: %llu end: %llu duration: %llu gps: %d batches: %ld\n",
> > +                        scale_type, SCALE_FLAG,
> > +                        t_rcu_scale_writer_started, t_rcu_scale_writer_finished,
> > +                        t_rcu_scale_writer_finished -
> > +                        t_rcu_scale_writer_started,
> > +                        ngps,
> > +                        rcuscale_seq_diff(b_rcu_gp_test_finished,
> > +                                          b_rcu_gp_test_started));
> > +               for (i = 0; i < nrealwriters; i++) {
> > +                       if (!writer_durations)
> > +                               break;
> > +                       if (!writer_n_durations)
> > +                               continue;
> > +                       wdpp = writer_durations[i];
> > +                       if (!wdpp)
> > +                               continue;
> > +                       for (j = 0; j < writer_n_durations[i]; j++) {
> > +                               wdp = &wdpp[j];
> > +                               pr_alert("%s%s %4d writer-duration: %5d %llu\n",
> > +                                       scale_type, SCALE_FLAG,
> > +                                       i, j, *wdp);
> > +                               if (j % 100 == 0)
> > +                                       schedule_timeout_uninterruptible(1);
> > +                       }
> > +                       kfree(writer_durations[i]);
> > +               }
> > +               kfree(writer_tasks);
> > +               kfree(writer_durations);
> > +               kfree(writer_n_durations);
> > +       }
> > +
> > +       /* Do torture-type-specific cleanup operations.  */
> > +       if (cur_ops->cleanup != NULL)
> > +               cur_ops->cleanup();
> > +
> > +       torture_cleanup_end();
> > +}
> > +
> > +/*
> > + * RCU scalability shutdown kthread.  Just waits to be awakened, then shuts
> > + * down system.
> > + */
> > +static int
> > +rcu_scale_shutdown(void *arg)
> > +{
> > +       wait_event(shutdown_wq,
> > +                  atomic_read(&n_rcu_scale_writer_finished) >= nrealwriters);
> > +       smp_mb(); /* Wake before output. */
> > +       rcu_scale_cleanup();
> > +       kernel_power_off();
> > +       return -EINVAL;
> > +}
> > +
> >  static int __init
> >  rcu_scale_init(void)
> >  {
> > --
> > 2.17.1
> >
> > > [...]
> >
  

> From: Paul E. McKenney <paulmck@kernel.org>
> [...]
> > >
> > > How about to pull the rcu_scale_cleanup() function after
> kfree_scale_cleanup().
> > > This groups kfree_* functions and groups rcu_scale_* functions.
> > > Then the code would look cleaner.
> > > So, do you think the changes below are better?
> >
> > IMHO, I don't think doing such a code move is better. Just add a new
> > header file and declare the function there. But see what Paul says
> > first.
> 
> This situation is likely to be an early hint that the kvfree_rcu() testing should
> be split out from kernel/rcu/rcuscale.c.

Another is that it's a bit expensive to create a new header file just for 
eliminating a function declaration. ;-)

So, if no objections, I'd like to send out the v2 patch with the updates below:

   - Move rcu_scale_cleanup() after kfree_scale_cleanup() to eliminate the
     declaration of kfree_scale_cleanup(). Though this makes the patch bigger, 
     get the file rcuscale.c much cleaner.

   - Remove the unnecessary step "modprobe torture" from the commit message.

   - Add the description for why move rcu_scale_cleanup() after
     kfree_scale_cleanup() to the commit message.

Thanks!
-Qiuxu

> [...]
  

> On Mar 16, 2023, at 9:17 AM, Zhuo, Qiuxu <qiuxu.zhuo@intel.com> wrote:
> 
> 
>> 
>> From: Paul E. McKenney <paulmck@kernel.org>
>> [...]
>>>> 
>>>> How about to pull the rcu_scale_cleanup() function after
>> kfree_scale_cleanup().
>>>> This groups kfree_* functions and groups rcu_scale_* functions.
>>>> Then the code would look cleaner.
>>>> So, do you think the changes below are better?
>>> 
>>> IMHO, I don't think doing such a code move is better. Just add a new
>>> header file and declare the function there. But see what Paul says
>>> first.
>> 
>> This situation is likely to be an early hint that the kvfree_rcu() testing should
>> be split out from kernel/rcu/rcuscale.c.
> 
> Another is that it's a bit expensive to create a new header file just for 
> eliminating a function declaration. ;-)

What is so expensive about new files? It is a natural organization structure.

> So, if no objections, I'd like to send out the v2 patch with the updates below:
> 
>   - Move rcu_scale_cleanup() after kfree_scale_cleanup() to eliminate the
>     declaration of kfree_scale_cleanup(). Though this makes the patch bigger, 
>     get the file rcuscale.c much cleaner.
> 
>   - Remove the unnecessary step "modprobe torture" from the commit message.
> 
>   - Add the description for why move rcu_scale_cleanup() after
>     kfree_scale_cleanup() to the commit message.

Honestly if you are moving so many lines around, you may as well split it out into a new module.

The kfree stuff being clubbed in the same file has also been a major annoyance.

 - Joel 


> Thanks!
> -Qiuxu
> 
>> [...]
  

> From: Joel Fernandes <joel@joelfernandes.org>
> Sent: Thursday, March 16, 2023 9:29 PM
> To: Zhuo, Qiuxu <qiuxu.zhuo@intel.com>
> Cc: paulmck@kernel.org; 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
> Subject: Re: [PATCH 1/1] rcu/rcuscale: Stop kfree_scale_thread thread(s)
> after unloading rcuscale
> 
> 
> > On Mar 16, 2023, at 9:17 AM, Zhuo, Qiuxu <qiuxu.zhuo@intel.com> wrote:
> >
> > 
> >>
> >> From: Paul E. McKenney <paulmck@kernel.org> [...]
> >>>>
> >>>> How about to pull the rcu_scale_cleanup() function after
> >> kfree_scale_cleanup().
> >>>> This groups kfree_* functions and groups rcu_scale_* functions.
> >>>> Then the code would look cleaner.
> >>>> So, do you think the changes below are better?
> >>>
> >>> IMHO, I don't think doing such a code move is better. Just add a new
> >>> header file and declare the function there. But see what Paul says
> >>> first.
> >>
> >> This situation is likely to be an early hint that the kvfree_rcu()
> >> testing should be split out from kernel/rcu/rcuscale.c.
> >
> > Another is that it's a bit expensive to create a new header file just
> > for eliminating a function declaration. ;-)
> 
> What is so expensive about new files? It is a natural organization structure.
>
> > So, if no objections, I'd like to send out the v2 patch with the updates below:
> >
> >   - Move rcu_scale_cleanup() after kfree_scale_cleanup() to eliminate the
> >     declaration of kfree_scale_cleanup(). Though this makes the patch bigger,
> >     get the file rcuscale.c much cleaner.
> >
> >   - Remove the unnecessary step "modprobe torture" from the commit
> message.
> >
> >   - Add the description for why move rcu_scale_cleanup() after
> >     kfree_scale_cleanup() to the commit message.
> 
> Honestly if you are moving so many lines around, you may as well split it out
> into a new module.
> The kfree stuff being clubbed in the same file has also been a major
> annoyance.

I'm OK with creating a new kernel module for these kfree stuffs, 
but do we really need to do that?

@paulmck, what's your suggestion for the next step? 

>  - Joel
> 
> 
> > Thanks!
> > -Qiuxu
> >
> >> [...]
  

On Thu, Mar 16, 2023 at 9:53 AM Zhuo, Qiuxu <qiuxu.zhuo@intel.com> wrote:
>
[...]
> > >> From: Paul E. McKenney <paulmck@kernel.org> [...]
> > >>>>
> > >>>> How about to pull the rcu_scale_cleanup() function after
> > >> kfree_scale_cleanup().
> > >>>> This groups kfree_* functions and groups rcu_scale_* functions.
> > >>>> Then the code would look cleaner.
> > >>>> So, do you think the changes below are better?
> > >>>
> > >>> IMHO, I don't think doing such a code move is better. Just add a new
> > >>> header file and declare the function there. But see what Paul says
> > >>> first.
> > >>
> > >> This situation is likely to be an early hint that the kvfree_rcu()
> > >> testing should be split out from kernel/rcu/rcuscale.c.
> > >
> > > Another is that it's a bit expensive to create a new header file just
> > > for eliminating a function declaration. ;-)
> >
> > What is so expensive about new files? It is a natural organization structure.
> >
> > > So, if no objections, I'd like to send out the v2 patch with the updates below:
> > >
> > >   - Move rcu_scale_cleanup() after kfree_scale_cleanup() to eliminate the
> > >     declaration of kfree_scale_cleanup(). Though this makes the patch bigger,
> > >     get the file rcuscale.c much cleaner.
> > >
> > >   - Remove the unnecessary step "modprobe torture" from the commit
> > message.
> > >
> > >   - Add the description for why move rcu_scale_cleanup() after
> > >     kfree_scale_cleanup() to the commit message.
> >
> > Honestly if you are moving so many lines around, you may as well split it out
> > into a new module.
> > The kfree stuff being clubbed in the same file has also been a major
> > annoyance.
>
> I'm OK with creating a new kernel module for these kfree stuffs,
> but do we really need to do that?

If it were me doing this, I would try to split it just because in the
long term I may have to maintain or deal with it.

I was also thinking a new scale directory _may_ make sense for
performance tests.

kernel/rcu/scaletests/kfree.c
kernel/rcu/scaletests/core.c
kernel/rcu/scaletests/ref.c

Or something like that.

and then maybe putt common code into: kernel/rcu/scaletests/common.c

 - Joel

>
> @paulmck, what's your suggestion for the next step?
>
> >  - Joel
> >
> >
> > > Thanks!
> > > -Qiuxu
> > >
> > >> [...]
  

On Thu, Mar 16, 2023 at 10:57:06AM -0400, Joel Fernandes wrote:
> On Thu, Mar 16, 2023 at 9:53 AM Zhuo, Qiuxu <qiuxu.zhuo@intel.com> wrote:
> >
> [...]
> > > >> From: Paul E. McKenney <paulmck@kernel.org> [...]
> > > >>>>
> > > >>>> How about to pull the rcu_scale_cleanup() function after
> > > >> kfree_scale_cleanup().
> > > >>>> This groups kfree_* functions and groups rcu_scale_* functions.
> > > >>>> Then the code would look cleaner.
> > > >>>> So, do you think the changes below are better?
> > > >>>
> > > >>> IMHO, I don't think doing such a code move is better. Just add a new
> > > >>> header file and declare the function there. But see what Paul says
> > > >>> first.
> > > >>
> > > >> This situation is likely to be an early hint that the kvfree_rcu()
> > > >> testing should be split out from kernel/rcu/rcuscale.c.
> > > >
> > > > Another is that it's a bit expensive to create a new header file just
> > > > for eliminating a function declaration. ;-)
> > >
> > > What is so expensive about new files? It is a natural organization structure.
> > >
> > > > So, if no objections, I'd like to send out the v2 patch with the updates below:
> > > >
> > > >   - Move rcu_scale_cleanup() after kfree_scale_cleanup() to eliminate the
> > > >     declaration of kfree_scale_cleanup(). Though this makes the patch bigger,
> > > >     get the file rcuscale.c much cleaner.
> > > >
> > > >   - Remove the unnecessary step "modprobe torture" from the commit
> > > message.
> > > >
> > > >   - Add the description for why move rcu_scale_cleanup() after
> > > >     kfree_scale_cleanup() to the commit message.
> > >
> > > Honestly if you are moving so many lines around, you may as well split it out
> > > into a new module.
> > > The kfree stuff being clubbed in the same file has also been a major
> > > annoyance.
> >
> > I'm OK with creating a new kernel module for these kfree stuffs,
> > but do we really need to do that?

It is not a particularly high priority.

> If it were me doing this, I would try to split it just because in the
> long term I may have to maintain or deal with it.
> 
> I was also thinking a new scale directory _may_ make sense for
> performance tests.
> 
> kernel/rcu/scaletests/kfree.c
> kernel/rcu/scaletests/core.c
> kernel/rcu/scaletests/ref.c
> 
> Or something like that.

I don't believe we are there yet, but...

> and then maybe putt common code into: kernel/rcu/scaletests/common.c

...splitting out the common code within the current directory/file
structure makes a lot of sense to me.  Not that I have checked up on
exactly how much common code there really is.  ;-)

							Thanx, Paul

>  - Joel
> 
> >
> > @paulmck, what's your suggestion for the next step?
> >
> > >  - Joel
> > >
> > >
> > > > Thanks!
> > > > -Qiuxu
> > > >
> > > >> [...]
  

> From: Joel Fernandes <joel@joelfernandes.org>
> [...]
> > I'm OK with creating a new kernel module for these kfree stuffs, but
> > do we really need to do that?
> 
> If it were me doing this, I would try to split it just because in the long term I
> may have to maintain or deal with it.
> 
> I was also thinking a new scale directory _may_ make sense for performance
> tests.
> 
> kernel/rcu/scaletests/kfree.c
> kernel/rcu/scaletests/core.c
> kernel/rcu/scaletests/ref.c

This looks like really a good constructive suggestion.
But today, please give me a break. ;-).

Thanks!
-Qiuxu

> Or something like that.
> 
> and then maybe putt common code into: kernel/rcu/scaletests/common.c