| Message ID | 20231024092634.7122-11-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: CAT test improvements & generalized test framework | expand |
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 <ilpo.jarvinen@linux.intel.com> > --- > 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
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.
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); 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 + * + * Measure things like cache misses from perf events. + * + * 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); + 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)
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. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- tools/testing/selftests/resctrl/cache.c | 86 +++++++------------------ 1 file changed, 25 insertions(+), 61 deletions(-)