Hi Ilpo,

On 10/24/2023 2:26 AM, Ilpo Järvinen wrote:
> The measure_cache_vals() function does a different thing depending on

No need to say "function" when using "()". The above can just be:
"measure_cache_vals() does a different ..."

> the test case that called it:
>   - For CAT, it measures LLC perf misses.
>   - For CMT, it measures LLC occupancy through resctrl.
> 
> Split these two functionalities into own functions the CAT and CMT
> tests can call directly.
> 
> Co-developed-by: Fenghua Yu <fenghua...@intel.com>
> Signed-off-by: Fenghua Yu <fenghua...@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvi...@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cache.c       | 37 ++++++++++---------
>  tools/testing/selftests/resctrl/resctrl.h     |  2 +-
>  tools/testing/selftests/resctrl/resctrl_val.c |  2 +-
>  3 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c 
> b/tools/testing/selftests/resctrl/cache.c
> index bcbca356d56a..299d9508221f 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -170,35 +170,36 @@ static int print_results_cache(char *filename, int 
> bm_pid,
>       return 0;
>  }
>  
> -int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
> +static int measure_llc_perf(struct resctrl_val_param *param, 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.
>        */

I'd expect these comments to move as part of a change like this,
but seems like this is merged with other changes in patch 10. Does not
seem like an issue done in this way.

> -     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;
> +
> +     ret = print_results_cache(param->filename, bm_pid, llc_perf_miss);
> +     return ret;
> +}
> +
> +int measure_llc_resctrl(struct resctrl_val_param *param, int bm_pid)
> +{
> +     unsigned long llc_occu_resc = 0;
> +     int 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 = get_llc_occu_resctrl(&llc_occu_resc);
> +     if (ret < 0)
>               return ret;
>  
> -     return 0;
> +     ret = print_results_cache(param->filename, bm_pid, llc_occu_resc);
> +     return ret;
>  }
>  
>  /*
> @@ -253,7 +254,7 @@ int cat_val(struct resctrl_val_param *param, size_t span)
>               }
>  
>               sleep(1);
> -             ret = measure_cache_vals(param, bm_pid);
> +             ret = measure_llc_perf(param, bm_pid);
>               if (ret)
>                       goto pe_close;
>       }


Reinette

Reply via email to