Re: [PATCH 10/24] selftests/resctrl: Remove nested calls in perf event handling

From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
Cc: linux-kselftest@vger.kernel.org, "Shuah Khan" <shuah@kernel.org>,
	"Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>,
	"Maciej Wieczór-Retman" <maciej.wieczor-retman@intel.com>,
	"Fenghua Yu" <fenghua.yu@intel.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 10/24] selftests/resctrl: Remove nested calls in perf event handling
Date: Fri, 3 Nov 2023 11:45:31 +0200 (EET)	[thread overview]
Message-ID: <aae1ad6f-4c31-7fb6-255d-7011fb67a784@linux.intel.com> (raw)
In-Reply-To: <93113f94-9663-4cbb-962f-c415bc975f12@intel.com>

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

On Thu, 2 Nov 2023, Reinette Chatre wrote:

> Hi Ilpo,
> 
> On 10/24/2023 2:26 AM, Ilpo Järvinen wrote:
> > Perf event handling has functions that are the sole caller of another
> > perf event handling related function:
> >   - reset_enable_llc_perf() calls perf_event_open_llc_miss()
> >   - reset_enable_llc_perf() calls ioctl_perf_event_ioc_reset_enable()
> >   - measure_llc_perf() calls get_llc_perf()
> > 
> > Remove the extra layer of calls to make the code easier to follow by
> > moving the code into the calling function.
> > 
> > In addition, converts print_results_cache() unsigned long parameter to
> > __u64 that matches the type coming from perf.
> 
> Is this referring to work from previous patch?

Yes, this got split into own patch and it unfortunately lingered on in the 
changelog. I'll remove it.

> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> ---
> >
> >  1 file changed, 25 insertions(+), 61 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> > index d39ef4eebc37..208af1ecae28 100644
> > --- a/tools/testing/selftests/resctrl/cache.c
> > +++ b/tools/testing/selftests/resctrl/cache.c

> > -/*
> > - * get_llc_perf:	llc cache miss through perf events
> > - * @llc_perf_miss:	LLC miss counter that is filled on success
> > - *
> > - * Perf events like HW_CACHE_MISSES could be used to validate number of
> > - * cache lines allocated.
> > - *
> > - * Return: =0 on success.  <0 on failure.
> > - */
> > -static int get_llc_perf(__u64 *llc_perf_miss)
> > -{
> > -	int ret;
> > -
> > -	/* Stop counters after one span to get miss rate */
> > -
> > -	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
> > -
> > -	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
> > -	if (ret == -1) {
> > -		perror("Could not get llc misses through perf");
> > +	fd_lm = perf_event_open(&pea_llc_miss, pid, cpu_no, -1, PERF_FLAG_FD_CLOEXEC);
> > +	if (fd_lm == -1) {
> > +		perror("Error opening leader");
> > +		ctrlc_handler(0, NULL, NULL);
> 
> I understand you just copied the code here ... but it is not clear to me
> why this particular error handling deserves a ctrlc_handler().

Good catch! It's first time I even notice this line.

It certainly looks a bit too big hammer for error handling. I'll try to 
create a separate patch which properly removes it (I suspect all error 
handling rollbacks are there already so it can just be removed but I'll 
have to confirm that).

> >  		return -1;
> >  	}
> >  
> > -	*llc_perf_miss = rf_cqm.values[0].value;
> > +	/* Start counters to log values */
> > +	ioctl(fd_lm, PERF_EVENT_IOC_RESET, 0);
> > +	ioctl(fd_lm, PERF_EVENT_IOC_ENABLE, 0);
> >  
> >  	return 0;
> >  }
> > @@ -166,20 +121,29 @@ static int print_results_cache(char *filename, int bm_pid, __u64 llc_value)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * measure_llc_perf:	measure perf events
> > + * @bm_pid:	child pid that runs benchmark
> 
> I expected "bm_pid" to reflect a "benchmark pid" that
> is not unique to the child. Are both parent and child
> not running the benchmark?
> 
> Missing doc of a parameter here.
> 
> > + *
> > + * Measure things like cache misses from perf events.
> 
> "things like cache misses" is vague. The function's name 
> still contains "llc" which makes me think it is not quite
> generic yet.

I suppose I tried to be intentionally vague because I knew I was going to 
use it measure also LLC accesses in the case of L2 CAT test. I'll see if I 
can improve this wording somehow.

> > + *
> > + * Return: =0 on success.  <0 on failure.
> > + */
> >  static int measure_llc_perf(struct resctrl_val_param *param, int bm_pid)
> >  {
> > -	__u64 llc_perf_miss = 0;
> >  	int ret;
> >  
> > -	/*
> > -	 * Measure cache miss from perf.
> > -	 */
> > -	ret = get_llc_perf(&llc_perf_miss);
> > -	if (ret < 0)
> > -		return ret;
> > +	/* Stop counters after one span to get miss rate */
> > +	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
> >  
> > -	ret = print_results_cache(param->filename, bm_pid, llc_perf_miss);
> > -	return ret;
> > +	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
> > +	close(fd_lm);
> 
> I am not able to tell where this close() moved from.

Another good catch.

It seems to be a conflict resolution fallout from the time when I changed 
to close() logic in that fix patch related to this fd.

-- 
 i.