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@xxxxxxxxx>
> Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
> 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?
> +
> +/*
> + * measure_llc_resctrl - Measure resctrl llc value from resctrl
llc -> LLC
> + * @filename: Filename for writing the results
> + * @bm_pid: PID that runs the benchmark
> + *
> + * Measures llc occupancy from resctrl and writes the results into @filename.
llc -> LLC
> + * @bm_pid is written to the results file along with the measured value.
> + *
> + * Return: =0 on success. <0 on failure.
same issue ?
Reinette
