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