Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Thu, 14 Sep 2023 13:16:57 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Reinette Chatre <reinette.chatre@intel.com>
cc:     Shuah Khan <shuah@kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>,
        linux-kselftest@vger.kernel.org,
        =?ISO-8859-15?Q?Maciej_Wiecz=F3r-Retman?= 
        <maciej.wieczor-retman@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>,
        stable@vger.kernel.org
Subject: Re: [PATCH 1/5] selftests/resctrl: Extend signal handler coverage
 to unmount on receiving signal
In-Reply-To: <d6cc3829-9b7d-6a0b-c734-c33f6a66959c@intel.com>
Message-ID: <9dfe761f-6779-44c6-c227-5175d53ac25@linux.intel.com>
References: <20230911111930.16088-1-ilpo.jarvinen@linux.intel.com> <20230911111930.16088-2-ilpo.jarvinen@linux.intel.com> <4176a620-4cec-5d57-42a3-a15c0fe3eb73@intel.com> <aab4b1cc-6eb5-c324-e97e-c6699e2d165@linux.intel.com>
 <d6cc3829-9b7d-6a0b-c734-c33f6a66959c@intel.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; BOUNDARY="8323329-1304429825-1694686039=:1814"
Content-ID: <763f45ca-bbf4-67b9-6a86-20e4cae7a6f1@linux.intel.com>
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-1304429825-1694686039=:1814
Content-Type: text/plain; CHARSET=ISO-8859-15
Content-Transfer-Encoding: 8BIT
Content-ID: <ecbe4e3f-6a73-628f-ca63-fef6d244f48@linux.intel.com>

On Wed, 13 Sep 2023, Reinette Chatre wrote:
> On 9/13/2023 3:01 AM, Ilpo J?rvinen wrote:
> > On Tue, 12 Sep 2023, Reinette Chatre wrote:
> >> On 9/11/2023 4:19 AM, Ilpo J?rvinen wrote:
> >>> Unmounting resctrl FS has been moved into the per test functions in
> >>> resctrl_tests.c by commit caddc0fbe495 ("selftests/resctrl: Move
> >>> resctrl FS mount/umount to higher level"). In case a signal (SIGINT,
> >>> SIGTERM, or SIGHUP) is received, the running selftest is aborted by
> >>> ctrlc_handler() which then unmounts resctrl fs before exiting. The
> >>> current section between signal_handler_register() and
> >>> signal_handler_unregister(), however, does not cover the entire
> >>> duration when resctrl FS is mounted.
> >>>
> >>> Move signal_handler_register() and signal_handler_unregister() call
> >>> into the test functions in resctrl_tests.c to properly unmount resctrl
> >>> fs. Adjust child process kill() call in ctrlc_handler() to only be
> >>> invoked if the child was already forked.
> >>
> >> Thank you for catching this.
> >>
> >>>
> >>> Fixes: caddc0fbe495 ("selftests/resctrl: Move resctrl FS mount/umount to higher level")
> >>> Signed-off-by: Ilpo J?rvinen <ilpo.jarvinen@linux.intel.com>
> >>> Cc: <stable@vger.kernel.org>
> >>> ---
> >>>  tools/testing/selftests/resctrl/cat_test.c    |  8 -------
> >>>  .../testing/selftests/resctrl/resctrl_tests.c | 24 +++++++++++++++++++
> >>>  tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++---------
> >>>  3 files changed, 34 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> >>> index 97b87285ab2a..224ba8544d8a 100644
> >>> --- a/tools/testing/selftests/resctrl/cat_test.c
> >>> +++ b/tools/testing/selftests/resctrl/cat_test.c
> >>> @@ -167,12 +167,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> >>>  		strcpy(param.filename, RESULT_FILE_NAME1);
> >>>  		param.num_of_runs = 0;
> >>>  		param.cpu_no = sibling_cpu_no;
> >>> -	} else {
> >>> -		ret = signal_handler_register();
> >>> -		if (ret) {
> >>> -			kill(bm_pid, SIGKILL);
> >>> -			goto out;
> >>> -		}
> >>>  	}
> >>>  
> >>>  	remove(param.filename);
> >>> @@ -209,10 +203,8 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
> >>>  		}
> >>>  		close(pipefd[0]);
> >>>  		kill(bm_pid, SIGKILL);
> >>> -		signal_handler_unregister();
> >>>  	}
> >>>  
> >>> -out:
> >>>  	cat_test_cleanup();
> >>>  
> >>>  	return ret;
> >>> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> >>> index 823672a20a43..3d66fbdc2df3 100644
> >>> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> >>> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> >>> @@ -73,8 +73,13 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
> >>>  
> >>>  	ksft_print_msg("Starting MBM BW change ...\n");
> >>>  
> >>> +	res = signal_handler_register();
> >>> +	if (res)
> >>> +		return;
> >>> +
> >>>  	res = mount_resctrlfs();
> >>>  	if (res) {
> >>> +		signal_handler_unregister();
> >>>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> >>>  		return;
> >>>  	}
> >>> @@ -91,6 +96,7 @@ static void run_mbm_test(const char * const *benchmark_cmd, int cpu_no)
> >>>  
> >>>  umount:
> >>>  	umount_resctrlfs();
> >>> +	signal_handler_unregister();
> >>>  }
> >>>  
> >>>  static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
> >>> @@ -99,8 +105,13 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
> >>>  
> >>>  	ksft_print_msg("Starting MBA Schemata change ...\n");
> >>>  
> >>> +	res = signal_handler_register();
> >>> +	if (res)
> >>> +		return;
> >>> +
> >>>  	res = mount_resctrlfs();
> >>>  	if (res) {
> >>> +		signal_handler_unregister();
> >>>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> >>>  		return;
> >>>  	}
> >>> @@ -115,6 +126,7 @@ static void run_mba_test(const char * const *benchmark_cmd, int cpu_no)
> >>>  
> >>>  umount:
> >>>  	umount_resctrlfs();
> >>> +	signal_handler_unregister();
> >>>  }
> >>>  
> >>
> >> This adds more duplicated code for every test. Have you considered a
> >> single test setup function that can be used to mount resctrl FS and setup
> >> the signal handler paired with a single test teardown function?
> > 
> > Yes. Consolidating all these is among my not-yet submitted patches.
> > I just had to do a backport-friendly Fixes patch first for this.
> > 
> 
> Could you please help me understand how the duplicate calls are more
> backport friendly?

Hi,

It's simply because the refactoring that has to be done to be able to 
introduce the generalized test framework is much more invasive and far 
reaching than this patch. Essentially, all the call signatures of the test 
functions need to match and the feature checks need to be done in new per 
test functions too. This is the diffstat of those changes alone:

 tools/testing/selftests/resctrl/cat_test.c      |  21 +++--
 tools/testing/selftests/resctrl/cmt_test.c      |  26 +++--
 tools/testing/selftests/resctrl/mba_test.c      |  20 +++-
 tools/testing/selftests/resctrl/mbm_test.c      |  20 +++-
 tools/testing/selftests/resctrl/resctrl.h       |  43 ++++++++-
 tools/testing/selftests/resctrl/resctrl_tests.c | 220 +++++++++++++++----------------------------
 tools/testing/selftests/resctrl/resctrlfs.c     |   5 +

(tools/testing/selftests/resctrl/resctrl_tests.c --- part would 
be slightly less if I'd reorder this patch but that only 24 lines off as 
per diffstat of this patch).

But that's not all.... To be able to push the generalized test framework 
to stable, you need to also count in the benchmark cmd changes which 
worked towards making the call signatures identical. So here's the 
diffstat for that series for quick reference:

 tools/testing/selftests/resctrl/cache.c       |   5 +-
 tools/testing/selftests/resctrl/cat_test.c    |  13 +--
 tools/testing/selftests/resctrl/cmt_test.c    |  34 ++++--
 tools/testing/selftests/resctrl/mba_test.c    |   4 +-
 tools/testing/selftests/resctrl/mbm_test.c    |   7 +-
 tools/testing/selftests/resctrl/resctrl.h     |  16 +--
 .../testing/selftests/resctrl/resctrl_tests.c | 100 ++++++++----------
 tools/testing/selftests/resctrl/resctrl_val.c |  10 +-

That's ~500 lines changed vs ~50 so it's a magnitude worse and much less 
localized.

And rest assured, I did not like introducing the duplicated calls any more 
than you do (I did not write the generalized test framework for nothing, 
after all) but the way taken in this patch seemed the most reasonable 
option under these circumstances.

-- 
 i.
--8323329-1304429825-1694686039=:1814--