Message ID | 20230313080403.89290-1-qiuxu.zhuo@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp1060149wrd; Mon, 13 Mar 2023 01:19:18 -0700 (PDT) X-Google-Smtp-Source: AK7set8djvxzdqDWjpuziwTJ3ExuEeh/L6TUw+kuHNuAzsBmKzMEhVnCsTAlsHOM5XTvUEEegrXH X-Received: by 2002:a05:6a20:12c3:b0:c7:40f8:73d3 with SMTP id v3-20020a056a2012c300b000c740f873d3mr43249329pzg.33.1678695558688; Mon, 13 Mar 2023 01:19:18 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1678695558; cv=none; d=google.com; s=arc-20160816; b=q4isR8389SbS4+Ui66B7PQre/TpB54XVPZA/zCPUB6DTNL7v0xTh46pRxH0u9IUdx5 aYle12xDV8dQCs6mUPSn53tlAyOxdMwTK5vbDhmQ9BMqF264aI/V+fHZTxv4LsqmWnGx 4G39PN3wWiXAxaI2bzNRpnNW/9wBZSeKYUHM4bahnbW0We1sr/vulpJHPbrmO6ODEgYQ H3qQYJ72v5r72nJT5gYyy63beqPFvvMUqTkSK7jWrIfhgw9t9jAs/PRrVaGnb+90DJ8X gg2zpqytruEen1I+yo73pnJ6opT2Zp14dgSbPU7csVwSy8pMNjJcTIpfytJdDf1dk0RU IKaQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:subject:cc:to:from :dkim-signature; bh=TDRfEiADLq4qJ/Gk7xmBChZ1FPVBW52e6SYol/Hh3uo=; b=rhQlxqpZfcriPqHuJWi66hBAPMVIJp6G1ouxCcbPJd7rIWTt4fNLhSXnjre+3e3/ru KOJw/gAAguNdxo27GyTMnv1DQAD19rno/amBuxDmHumeFovGZPwqru4MoUtHWmxm5UsM rONsnQMwDDCGTHzQKEGlXUpUMs23erCv3LhbEoMM7ackJuffknPGMVEeK1crIjy4FV6s g+AwLFU/cTB1vsxWWGxrhUS6h7Y5yrXPDZyk7sT1M1xDh2areUK3jIWKBww+1YTf/kmK jgy2kEL2QnFOSccAyXoHS6WFhE67AeV1TtGPMWymzaGGeNR/Dlo6a3T4VS+hvFSkVnqR gI4g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=Xdz1Islv; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id z12-20020a634c0c000000b004e1b5fb0683si6103921pga.486.2023.03.13.01.19.04; Mon, 13 Mar 2023 01:19:18 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=Xdz1Islv; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230400AbjCMIHc (ORCPT <rfc822;realc9580@gmail.com> + 99 others); Mon, 13 Mar 2023 04:07:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49808 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229796AbjCMIHA (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 13 Mar 2023 04:07:00 -0400 Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7D6932726; Mon, 13 Mar 2023 01:04:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1678694682; x=1710230682; h=from:to:cc:subject:date:message-id; bh=gDIv4qJwZoJtfa0NlmSgzOfssIbCR1lN3aQEVECdGbc=; b=Xdz1IslvmvmSeBFCow1Z1C8RwiNWoLArogskr9YJTzNfuqOLa7YQziEU bfMEN2+1bFe+RQeWc+++1IoHfZ0JoItyJbPfXXRTpS0qWsXUrTQXWIEos 8cYiZOmtHeOSMocjC1s1c+r8ETAPR9K01L3iXW32a9KMKJXRNhNeziEqB /Px9ZwkoS4DBGQsOPu1+I7JkiOR6lYVweVFwcp/cwr+fS8y/VlaBBTbY8 y/MICqH9qE4IL5zEuOp/mAIZijjbwovwznb6eGO3f38MGN1VS3V/Vd1UV Qv3ESVojcv7xgz+Za0bslJ6zvT7NN7M0XuOpJvffOyaXVrB/eqhZ6JLGd g==; X-IronPort-AV: E=McAfee;i="6500,9779,10647"; a="399682281" X-IronPort-AV: E=Sophos;i="5.98,256,1673942400"; d="scan'208";a="399682281" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Mar 2023 01:04:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10647"; a="708784876" X-IronPort-AV: E=Sophos;i="5.98,256,1673942400"; d="scan'208";a="708784876" Received: from qiuxu-clx.sh.intel.com ([10.239.53.105]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Mar 2023 01:04:34 -0700 From: Qiuxu Zhuo <qiuxu.zhuo@intel.com> To: "Paul E. McKenney" <paulmck@kernel.org>, Frederic Weisbecker <frederic@kernel.org>, Joel Fernandes <joel@joelfernandes.org>, Josh Triplett <josh@joshtriplett.org>, Neeraj Upadhyay <quic_neeraju@quicinc.com>, Davidlohr Bueso <dave@stgolabs.net> Cc: Qiuxu Zhuo <qiuxu.zhuo@intel.com>, 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: [PATCH 1/1] rcu/rcuscale: Stop kfree_scale_thread thread(s) after unloading rcuscale Date: Mon, 13 Mar 2023 16:04:03 +0800 Message-Id: <20230313080403.89290-1-qiuxu.zhuo@intel.com> X-Mailer: git-send-email 2.17.1 X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760239874467795146?= X-GMAIL-MSGID: =?utf-8?q?1760239874467795146?= |
Series |
[1/1] rcu/rcuscale: Stop kfree_scale_thread thread(s) after unloading rcuscale
|
|
Commit Message
Qiuxu Zhuo
March 13, 2023, 8:04 a.m. UTC
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(+)
Comments
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
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); + 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) {