Re: [PATCH v2 08/26] selftests/resctrl: Split measure_cache_vals()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4211 bytes --]

On Tue, 28 Nov 2023, Reinette Chatre wrote:

> Hi Ilpo,
> 
> On 11/20/2023 3:13 AM, Ilpo Järvinen wrote:
> > measure_cache_vals() does a different thing depending on the test case
> > that called it:
> >   - For CAT, it measures LLC misses through perf.
> >   - For CMT, it measures LLC occupancy through resctrl.
> > 
> > Split these two functionalities into own functions the CAT and CMT
> > tests can call directly. Replace passing the struct resctrl_val_param
> > parameter with the filename because it's more generic and all those
> > functions need out of resctrl_val.
> > 
> > Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  tools/testing/selftests/resctrl/cache.c       | 66 ++++++++++++-------
> >  tools/testing/selftests/resctrl/resctrl.h     |  2 +-
> >  tools/testing/selftests/resctrl/resctrl_val.c |  2 +-
> >  3 files changed, 43 insertions(+), 27 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> > index 8aa6d67db978..129d1c293518 100644
> > --- a/tools/testing/selftests/resctrl/cache.c
> > +++ b/tools/testing/selftests/resctrl/cache.c
> > @@ -147,7 +147,7 @@ static int get_llc_occu_resctrl(unsigned long *llc_occupancy)
> >   *
> >   * Return:		0 on success. non-zero on failure.
> >   */
> > -static int print_results_cache(char *filename, int bm_pid,
> > +static int print_results_cache(const char *filename, int bm_pid,
> >  			       unsigned long llc_value)
> >  {
> >  	FILE *fp;
> > @@ -169,35 +169,51 @@ static int print_results_cache(char *filename, int bm_pid,
> >  	return 0;
> >  }
> >  
> > -int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
> > +/*
> > + * perf_event_measure - Measure perf events
> > + * @filename:	Filename for writing the results
> > + * @bm_pid:	PID that runs the benchmark
> > + *
> > + * Measures perf events (e.g., cache misses) and writes the results into
> > + * @filename. @bm_pid is written to the results file along with the measured
> > + * value.
> > + *
> > + * Return: =0 on success. <0 on failure.
> 
> I do not think this is accurate. It looks like this function returns
> the return value of print_results_cache() which returns errno on failure.
> If this is the case then I think this proves that returning a
> positive integer on failure should be avoided since it just creates
> traps.
>
> > + */
> > +static int perf_event_measure(const char *filename, int bm_pid)
> >  {
> > -	unsigned long llc_perf_miss = 0, llc_occu_resc = 0, llc_value = 0;
> > +	unsigned long llc_perf_miss = 0;
> >  	int ret;
> >  
> > -	/*
> > -	 * Measure cache miss from perf.
> > -	 */
> > -	if (!strncmp(param->resctrl_val, CAT_STR, sizeof(CAT_STR))) {
> > -		ret = get_llc_perf(&llc_perf_miss);
> > -		if (ret < 0)
> > -			return ret;
> > -		llc_value = llc_perf_miss;
> > -	}
> > +	ret = get_llc_perf(&llc_perf_miss);
> > +	if (ret < 0)
> > +		return ret;
> >  
> > -	/*
> > -	 * Measure llc occupancy from resctrl.
> > -	 */
> > -	if (!strncmp(param->resctrl_val, CMT_STR, sizeof(CMT_STR))) {
> > -		ret = get_llc_occu_resctrl(&llc_occu_resc);
> > -		if (ret < 0)
> > -			return ret;
> > -		llc_value = llc_occu_resc;
> > -	}
> > -	ret = print_results_cache(param->filename, bm_pid, llc_value);
> > -	if (ret)
> > +	ret = print_results_cache(filename, bm_pid, llc_perf_miss);
> > +	return ret;
> > +}
> 
> Perhaps print_results_cache() can be made to return negative error
> and this just be "return print_results_cache(...)" and the function
> comment be accurate?

I think, I'll just change all "return errno;" to "return -1" before this,
however, one open question which impacts whether this is actually Fixes 
class issue:

It seems that perror()'s manpage doesn't answer one important question, 
whether it ifself can alter errno or not. The resctrl selftest code 
assumes it doesn't but some evidence I came across says otherwise so doing 
return errno; after calling perror() might not even be valid at all.

So I'm tempted to create an additional Fixes patch about the return change 
into the front of the series.

-- 
 i.