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? > > Signed-off-by: Ilpo Järvinen <[email protected]> > --- > tools/testing/selftests/resctrl/cache.c | 86 +++++++------------------ > 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 > @@ -29,25 +29,6 @@ static void initialize_perf_event_attr(void) > pea_llc_miss.disabled = 1; > } > > -static void ioctl_perf_event_ioc_reset_enable(void) > -{ > - ioctl(fd_lm, PERF_EVENT_IOC_RESET, 0); > - ioctl(fd_lm, PERF_EVENT_IOC_ENABLE, 0); > -} > - > -static int perf_event_open_llc_miss(pid_t pid, int cpu_no) > -{ > - 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); > - return -1; > - } > - > - return 0; > -} > - > static void initialize_llc_perf(void) > { > memset(&pea_llc_miss, 0, sizeof(struct perf_event_attr)); > @@ -63,42 +44,16 @@ static void initialize_llc_perf(void) > > static int reset_enable_llc_perf(pid_t pid, int cpu_no) > { > - int ret = 0; > - > - ret = perf_event_open_llc_miss(pid, cpu_no); > - if (ret < 0) > - return ret; > - > - /* Start counters to log values */ > - ioctl_perf_event_ioc_reset_enable(); > - > - return 0; > -} > - > -/* > - * 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(). > 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. > + * > + * 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. > + if (ret == -1) { > + perror("Could not get perf value"); > + return -1; > + } > + > + return print_results_cache(param->filename, bm_pid, > rf_cqm.values[0].value); > } > > int measure_llc_resctrl(struct resctrl_val_param *param, int bm_pid) Reinette
