linux-kernel - Re: [PATCH v2 15/24] selftests/resctrl: Refactor get_cbm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c54c301e-ff18-90a6-3e68-d77d0df23b77@intel.com>
Date:   Fri, 21 Apr 2023 17:17:01 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        <linux-kselftest@...r.kernel.org>,
        Fenghua Yu <fenghua.yu@...el.com>,
        "Shuah Khan" <shuah@...nel.org>, <linux-kernel@...r.kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH v2 15/24] selftests/resctrl: Refactor get_cbm_mask()

Hi Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
> Callers of get_cbm_mask() are required to pass a string into which
> the CBM bit mask is read into. Neither CAT nor CMT tests need the

There is a double "into" above. Perhaps the second can be dropped?

> mask as string but just convert it into an unsigned long value.
> 
> The bit mask reader can only read .../cbm_mask files.
> 
> Generalize the bit mask reading function into get_bit_mask() such that
> it can be used to handle other files besides the .../cbm_mask and
> handle the unsigned long conversion within within get_bit_mask() using
> fscanf(). Alter get_cbm_mask() to construct the filename for
> get_bit_mask().
> 
> Co-developed-by: Fenghua Yu <fenghua.yu@...el.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@...el.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cat_test.c  |  5 +--
>  tools/testing/selftests/resctrl/cmt_test.c  |  5 +--
>  tools/testing/selftests/resctrl/resctrl.h   |  2 +-
>  tools/testing/selftests/resctrl/resctrlfs.c | 50 +++++++++++++++------
>  4 files changed, 40 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index a998e6397518..9bf5d05d9e74 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -18,7 +18,6 @@
>  #define MAX_DIFF		1000000
>  
>  static int count_of_bits;
> -static char cbm_mask[256];
>  static unsigned long long_mask;
>  static unsigned long cache_size;
>  
> @@ -101,12 +100,10 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  	cache_size = 0;
>  
>  	/* Get default cbm mask for L3/L2 cache */
> -	ret = get_cbm_mask(cache_type, cbm_mask);
> +	ret = get_cbm_mask(cache_type, &long_mask);
>  	if (ret)
>  		return ret;
>  
> -	long_mask = strtoul(cbm_mask, NULL, 16);
> -
>  	/* Get L3/L2 cache size */
>  	ret = get_cache_size(cpu_no, cache_type, &cache_size);
>  	if (ret)
> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index 2d434c03cbba..ae54bbabbd91 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -17,7 +17,6 @@
>  #define MAX_DIFF_PERCENT	15
>  
>  static int count_of_bits;
> -static char cbm_mask[256];
>  static unsigned long long_mask;
>  static unsigned long cache_size;
>  
> @@ -82,12 +81,10 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>  	if (!validate_resctrl_feature_request(CMT_STR))
>  		return -1;
>  
> -	ret = get_cbm_mask("L3", cbm_mask);
> +	ret = get_cbm_mask("L3", &long_mask);
>  	if (ret)
>  		return ret;

I think this is a good change. It does raise the question why long_mask
is a global variable so I think it may make things go smoother if
the patch making long_mask local is moved to be before this patch.

Reinette