Hi Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:

Makefile only lists *.c as build dependecies for the restctrl_tests

dependecies -> dependencies

restctrl_tests -> resctrl_tests

executable which excludes resctrl.h.

Add *.h to wildcard() cover also resctrl.h.

I find this a bit hard to parse. How about "Add *.h to wildcard() to include resctrl.h."

(considering the problem statement indicates that resctrl.h was "excluded", having it now "included" seems to match)

Fixes: 591a6e8588fc ("selftests/resctrl: Add basic resctrl file system operations and data") Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

---

tools/testing/selftests/resctrl/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile index 73d53257df42..2dc7da221795 100644 --- a/tools/testing/selftests/resctrl/Makefile +++ b/tools/testing/selftests/resctrl/Makefile @@ -7,4 +7,4 @@ TEST_GEN_PROGS := resctrl_tests include ../lib.mk -$(OUTPUT)/resctrl_tests: $(wildcard *.c) +$(OUTPUT)/resctrl_tests: $(wildcard *.c *.h)

How about a simpler *.[ch]? Seems like this pattern is popular in selftest code.

Reinette

Hi Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:

CAT test only validates that the number of CBM bits is not too large but it could be too small too.

Could you please elaborate how this scenario could occur?

Check and return error before starting the CAT test if the number of CBM bits is too small.

Fixes: 09a67934625a ("selftests/resctrl: Don't hard code value of "no_of_bits" variable")

This fix is not clear to me. This commit being fixed already contains an explicit test that will bail out of no_of_bits <= 0. It is not clear to me why it is necessary to adding a test for < 1 as a fix for this commit.

Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

tools/testing/selftests/resctrl/cat_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index fb1443f888c4..722c9cd4120d 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -129,7 +129,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) if (!n) n = count_of_bits / 2;

• if (n > count_of_bits - 1) {

• if (n < 1 || n > count_of_bits - 1) { ksft_print_msg("Invalid input value for no_of_bits n!\n"); ksft_print_msg("Please enter value in range 1 to %d\n", count_of_bits - 1);

Reinette

On Fri, 21 Apr 2023, Reinette Chatre wrote:

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:

...

A few places currently lack umounting resctrl FS on error paths.

You mention "A few places" (plural). In the patch I do see that cmt_resctrl_val() is missing an unmount. Where are the other places?

- cmt_resctrl_val() has multiple error paths with direct return. - cat_perf_miss_val() has multiple error paths with direct return.

In addition, validate_resctrl_feature_request() is called by run_mbm_test() and run_mba_test(). Neither MBA nor MBM test tries to umount resctrl FS.

I improved the changelog accordingly.

While doing this, I took a more careful look into how it can result in problems and I think the only way is through PARENT_EXIT() because main has the umount in the end (and the remounting trickery kinda seems to work even if it was hard to track).

Fixing the PARENT_EXIT() problem required yet another change which I add in v3.

As the only failure I could think of is because of PARENT_EXIT(), I removed Fixes tags from this change and put one into the PARENT_EXIT() umount fix. So this change will just be part of the move towards more tractable resctrl FS handling, not a fix anymore.

In the end, after some reshuffling, I ended up having 5 changes related to this:

selftests/resctrl: Remove mum_resctrlfs from struct resctrl_val_param selftests/resctrl: Refactor remount_resctrl(bool mum_resctrlfs) to mount_resctrl() selftests/resctrl: Move resctrl FS mount/umount to higher level selftests/resctrl: Unmount resctrl FS before starting the first test selftests/resctrl: Unmount resctrl FS if child fails to run benchmark

...

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index af71b2141271..426d11189a99 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -86,10 +86,6 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) cache_size = 0;

• ret = remount_resctrlfs(true);
• if (ret)

• return ret;
  

• if (!validate_resctrl_feature_request(CMT_STR)) return -1;

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 9b9751206e1c..5c9ed52b69f2 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -77,9 +77,15 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span, ksft_print_msg("Starting MBM BW change ...\n");

• res = remount_resctrlfs(false);

I think that should be remount_resctrlfs(true).

Please note that any of the tests could be run separately from the command line and thus each test need to ensure a clean environment, it cannot assume that (a) user space provided it with a clean and/or unmounted resctrl or (b) that any test was run before it.

I think I got tripped by the level of complexity here and trying to split patch to minimal parts. I was somehow thinking that remount_resctrlfs(false) would return error if resctrl fs is already mounted.

I've now changed this to pass true instead even if the argument is removed by the other change.

Hi Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:

Resctrl FS mount/umount are now cleanly paired.

Removed mum_resctrlfs that is what is left of the earlier remount

Removed -> Remove? I am not sure what is meant with "that is what is left" ...

trickery to make the code cleaner. Rename remount_resctrlfs() to mount_resctrlfs() to match the reduced functionality.

These two logical changes would be easier to review if done in two patches.

...

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 5c9ed52b69f2..f3303459136d 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -77,7 +77,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span, ksft_print_msg("Starting MBM BW change ...\n");

• res = remount_resctrlfs(false);

• res = mount_resctrlfs();

As mentioned in previous patch the way I see it remount is still needed.

if (res) { ksft_exit_fail_msg("Failed to mount resctrl FS\n"); return; @@ -106,7 +106,7 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span, ksft_print_msg("Starting MBA Schemata change ...\n");

• res = remount_resctrlfs(false);

• res = mount_resctrlfs(); if (res) { ksft_exit_fail_msg("Failed to mount resctrl FS\n"); return;

@@ -132,7 +132,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no) ksft_print_msg("Starting CMT test ...\n");

• res = remount_resctrlfs(false);

• res = mount_resctrlfs(); if (res) { ksft_exit_fail_msg("Failed to mount resctrl FS\n"); return;

@@ -160,7 +160,7 @@ static void run_cat_test(int cpu_no, int no_of_bits) ksft_print_msg("Starting CAT test ...\n");

• res = remount_resctrlfs(false);

• res = mount_resctrlfs(); if (res) { ksft_exit_fail_msg("Failed to mount resctrl FS\n"); return;

diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 42f547a81e25..45f785213143 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -48,29 +48,19 @@ static int find_resctrl_mount(char *buffer) } /*

• • remount_resctrlfs - Remount resctrl FS at /sys/fs/resctrl
• • @mum_resctrlfs: Should the resctrl FS be remounted?

• • mount_resctrlfs - Mount resctrl FS at /sys/fs/resctrl
  • If not mounted, mount it.

• • If mounted and mum_resctrlfs then remount resctrl FS.
• • If mounted and !mum_resctrlfs then noop
  • Return: 0 on success, non-zero on failure
  */

-int remount_resctrlfs(bool mum_resctrlfs) +int mount_resctrlfs(void) {

• char mountpoint[256]; int ret;

• ret = find_resctrl_mount(mountpoint);
• if (ret)

• strcpy(mountpoint, RESCTRL_PATH);
  

• if (!ret && mum_resctrlfs && umount(mountpoint))

• ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint);
  

• if (!ret && !mum_resctrlfs)

• return 0;
  

• ret = find_resctrl_mount(NULL);
• if (!ret)

• return -1;
  

This seems to assume that resctrl is always unmounted. Should the main program thus start by unmounting resctrl before it runs any test in case it is mounted when user space starts the tests?

Reinette

On Fri, 21 Apr 2023, Reinette Chatre wrote:

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:

...

When no benchmark_cmd is given, benchmark_cmd[1] is set to span in main(). There's no need to do it again in run_mba_test().

Remove the duplicated preparation for span argument into benchmark_cmd[1] from run_mba_test(). It enables also removing has_ben argument from run_mba_test() as unnecessary.

I find the last sentence a bit hard to read. How about "After this, the has_ben argument to run_mba_test() can be removed.".

...

Co-developed-by: Fenghua Yu fenghua.yu@intel.com Signed-off-by: Fenghua Yu fenghua.yu@intel.com Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

---

tools/testing/selftests/resctrl/resctrl_tests.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index f1ed2c89f228..3c8ec68eb507 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -99,8 +99,8 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, unsigned long span, umount_resctrlfs(); } -static void run_mba_test(bool has_ben, char **benchmark_cmd, unsigned long span,

• 	 int cpu_no, char *bw_report)
  

+static void run_mba_test(char **benchmark_cmd, unsigned long span, int cpu_no,

• 	 char *bw_report)
  

{ int res; @@ -117,8 +117,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, unsigned long span, goto umount; }

• if (!has_ben)

• sprintf(benchmark_cmd[1], "%lu", span);
  

Can "span" also be removed?

Yes.

Hi Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:

MBM, MBA and CMT test cases use run_fill_buf() to loop indefinitely loop around the buffer. CAT test case is different and doesn't want to

"loop indefinitely loop" -> "loop indefinitely" ?

loop around the buffer continuously.

Split run_fill_buf() into helpers so that both the use cases are easier to control by creating separate functions for buffer allocation, looping around the buffer and the deallocation. Make those functions available for tests.

Co-developed-by: Fenghua Yu fenghua.yu@intel.com Signed-off-by: Fenghua Yu fenghua.yu@intel.com Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

---

tools/testing/selftests/resctrl/fill_buf.c | 46 ++++++++++++++++------ tools/testing/selftests/resctrl/resctrl.h | 3 ++ 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 5cdb421a2f6c..6f0438aa71a6 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -24,6 +24,11 @@ static unsigned char *startptr; +void free_buffer(void) +{

• free(startptr);

+}

From what I understand startptr is a global variable because there used to be a signal handler that attempted to free the buffer as part of its cleanup. This was not necessary and this behavior no longer exists, yet the global buffer pointer remains. See commit 73c55fa5ab55 ("selftests/resctrl: Commonize the signal handler register/unregister for all tests")

I do not see why a global buffer pointer with all these indirections are needed. Why not just use a local pointer and pass it to functions as needed? In the above case, just call free(pointer) directly from the test.

static void sb(void) { #if defined(__i386) || defined(__x86_64) @@ -138,36 +143,53 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr, return 0; } -static int -fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val) +int alloc_buffer(unsigned long long buf_size, int memflush) {

This can be an allocation function that returns a pointer to allocated buffer, NULL if error.

• unsigned char *start_ptr, *end_ptr;
• int ret;

• unsigned char *start_ptr;

start_ptr = malloc_and_init_memory(buf_size); if (!start_ptr) return -1; startptr = start_ptr;

• end_ptr = start_ptr + buf_size;

/* Flush the memory before using to avoid "cache hot pages" effect */ if (memflush) mem_flush(start_ptr, buf_size);

• return 0;

+}

+int use_buffer(unsigned long long buf_size, int op, char *resctrl_val) +{

• unsigned char *end_ptr;
• int ret;
• end_ptr = startptr + buf_size; if (op == 0)

• ret = fill_cache_read(start_ptr, end_ptr, resctrl_val);
  

• ret = fill_cache_read(startptr, end_ptr, resctrl_val);
  

  else

• ret = fill_cache_write(start_ptr, end_ptr, resctrl_val);
  

• ret = fill_cache_write(startptr, end_ptr, resctrl_val);
  

• if (ret) {

• if (ret) printf("\n Error in fill cache read/write...\n");

• return -1;
  

• }

• free(startptr);

• return ret;

+}

This seems like an unnecessary level of abstraction to me. Could callers not just call fill_cache_read()/fill_cache_write() directly? I think doing so will make tests easier to understand. Looking ahead at how cat_val() turns out in the final patch I do think a call to fill_cache_read() is easier to follow than this abstraction.

• return 0;

+static int +fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val) +{

• int ret;
• ret = alloc_buffer(buf_size, memflush);
• if (ret)

• return ret;
  

• ret = use_buffer(buf_size, op, resctrl_val);
• free_buffer();
• return ret;

} int run_fill_buf(unsigned long span, int memflush, int op, char *resctrl_val) diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 574adac8932d..75bfa2b2746d 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -95,6 +95,9 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, char *resctrl_val); int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu, int group_fd, unsigned long flags); +void free_buffer(void); +int alloc_buffer(unsigned long long buf_size, int memflush); +int use_buffer(unsigned long long buf_size, int op, char *resctrl_val); int run_fill_buf(unsigned long span, int memflush, int op, char *resctrl_val); int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param); int mbm_bw_change(unsigned long span, int cpu_no, char *bw_report, char **benchmark_cmd);

Reinette

Hi Ilpo,

On 4/24/2023 9:01 AM, Ilpo Järvinen wrote:

On Fri, 21 Apr 2023, Reinette Chatre wrote:

...

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:

...

...

...

...

static void sb(void) { #if defined(__i386) || defined(__x86_64) @@ -138,36 +143,53 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr, return 0; } -static int -fill_cache(unsigned long long buf_size, int memflush, int op, char *resctrl_val) +int alloc_buffer(unsigned long long buf_size, int memflush) {

This can be an allocation function that returns a pointer to allocated buffer, NULL if error.

...

• unsigned char *start_ptr, *end_ptr;
• int ret;

• unsigned char *start_ptr;

start_ptr = malloc_and_init_memory(buf_size); if (!start_ptr) return -1; startptr = start_ptr;

• end_ptr = start_ptr + buf_size;

/* Flush the memory before using to avoid "cache hot pages" effect */ if (memflush) mem_flush(start_ptr, buf_size);

• return 0;

+}

+int use_buffer(unsigned long long buf_size, int op, char *resctrl_val) +{

• unsigned char *end_ptr;
• int ret;
• end_ptr = startptr + buf_size; if (op == 0)

• ret = fill_cache_read(start_ptr, end_ptr, resctrl_val);
  

• ret = fill_cache_read(startptr, end_ptr, resctrl_val);
  

  else

• ret = fill_cache_write(start_ptr, end_ptr, resctrl_val);
  

• ret = fill_cache_write(startptr, end_ptr, resctrl_val);
  

• if (ret) {

• if (ret) printf("\n Error in fill cache read/write...\n");

• return -1;
  

• }

• free(startptr);

• return ret;

+}

This seems like an unnecessary level of abstraction to me. Could callers not just call fill_cache_read()/fill_cache_write() directly? I think doing so will make tests easier to understand. Looking ahead at how cat_val() turns out in the final patch I do think a call to fill_cache_read() is easier to follow than this abstraction.

Passing a custom benchmark command with -b would lose some functionality if this abstraction is removed. CAT test could make a direct call though as it doesn't care about the benchmark command.

How useful that -b functionality is for selftesting is somewhat questionable though.

I do not think we are speaking about the same thing here. I think that use_buffer() is unnecessary. fill_cache() can just call fill_cache_read() or fill_cache_write() directly, depending on the op value. Could you please elaborate how that impacts the custom benchmark?

Looking ahead at patch 24/24: "selftests/resctrl: Rewrite Cache Allocation Technology (CAT) test" I feel more strongly that use_buffer() is unnecessary since it adds an unnecessary layer and makes it harder to see what the test does.

Reinette

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@intel.com Signed-off-by: Fenghua Yu fenghua.yu@intel.com Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.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

On Wed, 31 May 2023, Shaopeng Tan (Fujitsu) wrote:

Hi Ilpo,

...

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 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@intel.com Signed-off-by: Fenghua Yu fenghua.yu@intel.com Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.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;

• long_mask = strtoul(cbm_mask, NULL, 16);
• ret = get_cache_size(cpu_no, "L3", &cache_size); if (ret) return ret;

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index ba36eb5fdf0d..bcc95faa5b4e 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -106,7 +106,7 @@ void tests_cleanup(void); void mbm_test_cleanup(void); int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd); void mba_test_cleanup(void); -int get_cbm_mask(char *cache_type, char *cbm_mask); +int get_cbm_mask(char *cache_type, unsigned long *mask); int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size); void ctrlc_handler(int signum, siginfo_t *info, void *ptr); int signal_handler_register(void); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 7fef9068d7fd..f01ecfa64063 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -186,30 +186,29 @@ int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size) #define CORE_SIBLINGS_PATH "/sys/bus/cpu/devices/cpu"

/*

• • get_cbm_mask - Get cbm mask for given cache
• • @cache_type: Cache level L2/L3
• • @cbm_mask: cbm_mask returned as a string

• • get_bit_mask - Get bit mask from given file
• • @filename: File containing the mask
• • @mask: The bit mask returned as unsigned long
  • Return: = 0 on success, < 0 on failure.
  */

-int get_cbm_mask(char *cache_type, char *cbm_mask) +static int get_bit_mask(char *filename, unsigned long *mask) {

• char cbm_mask_path[1024]; FILE *fp;

• if (!cbm_mask)

• if (!filename || !mask) return -1;

• sprintf(cbm_mask_path, "%s/%s/cbm_mask", INFO_PATH,

cache_type);

• fp = fopen(cbm_mask_path, "r");

• fp = fopen(filename, "r"); if (!fp) {

• perror("Failed to open cache level");
  

• fprintf(stderr, "Failed to open bit mask file '%s': %s\n",
  

• 	filename, strerror(errno));
  

  return -1; }

• if (fscanf(fp, "%s", cbm_mask) <= 0) {

• perror("Could not get max cbm_mask");
  

• if (fscanf(fp, "%lx", mask) <= 0) {

• fprintf(stderr, "Could not read bit mask file '%s': %s\n",
  

• 	filename, strerror(errno));
  

  fclose(fp);

  return -1;

@@ -219,6 +218,31 @@ int get_cbm_mask(char *cache_type, char *cbm_mask) return 0; }

+/*

• • get_cbm_bits - Get number of bits in cbm mask

Is it get_cbm_mask?

Sure, thanks for noticing this. While correcting it, I also improved the description to match what the function now does.

Hi Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:

CAT and CMT tests calculate the span size from the n-bits cache allocation on their own.

Add cache_alloc_size() helper which calculates size of the cache allocation for the given number of bits to avoid duplicating code.

This patch is very heavy on the usage of allocation when I think it only refers to the cache size ... how that size is used by the caller is independent from this.

Compare to how it sounds with some small changes to changelog:

CAT and CMT tests calculate the span size from the capacity bitmask independently. Add cache_size() helper which calculates the size of the cache for the given number of bits to avoid duplicating code.

I think removing "alloc" helps to convey what this code actually does.

Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

tools/testing/selftests/resctrl/cache.c | 27 ++++++++++++++++++++++ tools/testing/selftests/resctrl/cat_test.c | 8 +++++-- tools/testing/selftests/resctrl/cmt_test.c | 4 +++- tools/testing/selftests/resctrl/resctrl.h | 2 ++ 4 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c index 6bc912de38be..b983af394e33 100644 --- a/tools/testing/selftests/resctrl/cache.c +++ b/tools/testing/selftests/resctrl/cache.c @@ -15,6 +15,33 @@ static struct read_format rf_cqm; static int fd_lm; char llc_occup_path[1024]; +/*

• • cache_alloc_size - Calculate alloc size for given cache alloc mask

"cache_size - Calculate number of bytes represented by bitmask" ? Please feel free to improve.

• • @cpu_no: CPU number
• • @cache_type: Cache level L2/L3
• • @alloc_mask: Cache alloc mask

The description is mostly a rewrite of the variable name. Can it be more descriptive?

• • @alloc_size: Alloc size returned on success

I do not think the utility should assume anything about how the value it provides should be used. Instead it should just reflect what the value is.

• • Returns: 0 on success with @alloc_size filled, non-zero on error.
• */

+int cache_alloc_size(int cpu_no, char *cache_type, unsigned long alloc_mask,

•      unsigned long *alloc_size)
  

+{

• unsigned long cache_size, full_mask;
• int ret;
• ret = get_cbm_mask(cache_type, &full_mask);
• if (ret)

• return ret;
  

• ret = get_cache_size(cpu_no, cache_type, &cache_size);
• if (ret)

• return ret;
  

• *alloc_size = cache_size * count_bits(alloc_mask) / count_bits(full_mask);
• return 0;

+}

static void initialize_perf_event_attr(void) { pea_llc_miss.type = PERF_TYPE_HARDWARE; diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 9bf5d05d9e74..d3fbd4de9f8a 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -140,7 +140,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) /* Set param values for parent thread which will be allocated bitmask * with (max_bits - n) bits */

• param.span = cache_size * (count_of_bits - n) / count_of_bits;

• ret = cache_alloc_size(cpu_no, cache_type, l_mask, &param.span);
• if (ret)

• return ret;
  

  strcpy(param.ctrlgrp, "c2"); strcpy(param.mongrp, "m2"); strcpy(param.filename, RESULT_FILE_NAME2);

@@ -162,7 +164,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) param.mask = l_mask_1; strcpy(param.ctrlgrp, "c1"); strcpy(param.mongrp, "m1");

• param.span = cache_size * n / count_of_bits;
  

• ret = cache_alloc_size(cpu_no, cache_type, l_mask_1, &param.span);
  

• if (ret)
  

• 	exit(-1);
  

  strcpy(param.filename, RESULT_FILE_NAME1); param.num_of_runs = 0; param.cpu_no = sibling_cpu_no;

Did this change intend to remove the duplicate code mentioned in the changelog? I was expecting the calls to get_cbm_mask() and get_cache_size() within cat_perf_miss_val() to be removed.

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index ae54bbabbd91..efe77e0f1d4c 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -105,10 +105,12 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) .cpu_no = cpu_no, .filename = RESULT_FILE_NAME, .mask = ~(long_mask << n) & long_mask,

• .span		= cache_size * n / count_of_bits,
  

  .num_of_runs = 0, .setup = cmt_setup, };

• ret = cache_alloc_size(cpu_no, "L3", param.mask, &param.span);
• if (ret)

• return ret;
  

if (strcmp(benchmark_cmd[0], "fill_buf") == 0) sprintf(benchmark_cmd[1], "%lu", param.span);

Same here regarding removal of code.

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcc95faa5b4e..65425d92684e 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -108,6 +108,8 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd); void mba_test_cleanup(void); int get_cbm_mask(char *cache_type, unsigned long *mask); int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size); +int cache_alloc_size(int cpu_no, char *cache_type, unsigned long alloc_mask,

•      unsigned long *alloc_size);
  

void ctrlc_handler(int signum, siginfo_t *info, void *ptr); int signal_handler_register(void); void signal_handler_unregister(void);

Reinette

Hi Ilpo,

On 4/24/2023 9:28 AM, Ilpo Järvinen wrote:

On Fri, 21 Apr 2023, Reinette Chatre wrote:

...

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:

...

CAT and CMT tests calculate the span size from the n-bits cache allocation on their own.

Add cache_alloc_size() helper which calculates size of the cache allocation for the given number of bits to avoid duplicating code.

This patch is very heavy on the usage of allocation when I think it only refers to the cache size ... how that size is used by the caller is independent from this.

Compare to how it sounds with some small changes to changelog:

CAT and CMT tests calculate the span size from the capacity bitmask independently. Add cache_size() helper which calculates the size of the cache for the given number of bits to avoid duplicating code.

I think removing "alloc" helps to convey what this code actually does.

Does it? Without something to indicate its not the full cache size, there's possiblity for confusion. While the tests are mostly interested in the allocated size, the full cache size is also collected (solely for printing it out, IIRC). Maybe I should rename those variable to total_cache_size or something like that to mitigate the confusion?

This patch adds and use a utility that converts a bitmask into bytes. I do not think it should dictate what the meaning or usage of the bitmask is.

...

...

Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

tools/testing/selftests/resctrl/cache.c | 27 ++++++++++++++++++++++ tools/testing/selftests/resctrl/cat_test.c | 8 +++++-- tools/testing/selftests/resctrl/cmt_test.c | 4 +++- tools/testing/selftests/resctrl/resctrl.h | 2 ++ 4 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c index 6bc912de38be..b983af394e33 100644 --- a/tools/testing/selftests/resctrl/cache.c +++ b/tools/testing/selftests/resctrl/cache.c @@ -15,6 +15,33 @@ static struct read_format rf_cqm; static int fd_lm; char llc_occup_path[1024]; +/*

• • cache_alloc_size - Calculate alloc size for given cache alloc mask

"cache_size - Calculate number of bytes represented by bitmask" ? Please feel free to improve.

...

• • @cpu_no: CPU number
• • @cache_type: Cache level L2/L3
• • @alloc_mask: Cache alloc mask

The description is mostly a rewrite of the variable name. Can it be more descriptive?

...

• • @alloc_size: Alloc size returned on success

I do not think the utility should assume anything about how the value it provides should be used. Instead it should just reflect what the value is.

I was just referring to that the value is filled only on success.

I understand. My comment was about the naming of the parameter, which can, for example, just be "size".

...

...

• • Returns: 0 on success with @alloc_size filled, non-zero on error.
• */

+int cache_alloc_size(int cpu_no, char *cache_type, unsigned long alloc_mask,

•      unsigned long *alloc_size)
  

+{

• unsigned long cache_size, full_mask;
• int ret;
• ret = get_cbm_mask(cache_type, &full_mask);
• if (ret)

• return ret;
  

• ret = get_cache_size(cpu_no, cache_type, &cache_size);
• if (ret)

• return ret;
  

• *alloc_size = cache_size * count_bits(alloc_mask) / count_bits(full_mask);
• return 0;

+}

static void initialize_perf_event_attr(void) { pea_llc_miss.type = PERF_TYPE_HARDWARE; diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 9bf5d05d9e74..d3fbd4de9f8a 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -140,7 +140,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) /* Set param values for parent thread which will be allocated bitmask * with (max_bits - n) bits */

• param.span = cache_size * (count_of_bits - n) / count_of_bits;

• ret = cache_alloc_size(cpu_no, cache_type, l_mask, &param.span);
• if (ret)

• return ret;
  

  strcpy(param.ctrlgrp, "c2"); strcpy(param.mongrp, "m2"); strcpy(param.filename, RESULT_FILE_NAME2);

@@ -162,7 +164,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) param.mask = l_mask_1; strcpy(param.ctrlgrp, "c1"); strcpy(param.mongrp, "m1");

• param.span = cache_size * n / count_of_bits;
  

• ret = cache_alloc_size(cpu_no, cache_type, l_mask_1, &param.span);
  

• if (ret)
  

• 	exit(-1);
  

  strcpy(param.filename, RESULT_FILE_NAME1); param.num_of_runs = 0; param.cpu_no = sibling_cpu_no;

Did this change intend to remove the duplicate code mentioned in the changelog?

It removes n CBM bits -> cache size calculations by collecting the calculation into one place.

cache_alloc_size() takes mask instead of n (CBM bits) as input which makes things easier down the line when the new CAT test starts to tweak the alloc size. The new CAT test would otherwise need to track both the mask and n.

cache_alloc_size() is independent of what caller requires so the full mask is not passed from the caller.

...

I was expecting the calls to get_cbm_mask() and get_cache_size() within cat_perf_miss_val() to be removed.

I would have wanted to remove get_cache_size() but it would mean removing cache size print or moving it to elsewhere.

get_cbm_mask() cannot be removed as it's used by the test to calculate the mask the test wants (but it no longer has to determine the size itself but uses this new helper instead).

I can try to amend the changelog to explain things better.

The current motivation for this patch is to avoid duplicating code but as I see it it introduces more duplicated code.

Reinette

On Wed, 31 May 2023, Shaopeng Tan (Fujitsu) wrote:

Hi Ilpo,

...

CAT and CMT tests depends on masks being continuous.

Replace count_bits with more appropriate variant that counts consecutive bits.

Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

...

@@ -218,6 +220,34 @@ static int get_bit_mask(char *filename, unsigned long *mask) return 0; }

+/*

• • count_consecutive_bits - Returns the longest train of bits in a bit mask
• • @val A bit mask
• • @start The location of the least-significant bit of the longest train
• • Return: The length of the consecutive bits in the longest train of bits
• */

+unsigned int count_consecutive_bits(unsigned long val, unsigned int +*start) {

• unsigned long last_val;
• int count = 0;
• while (val) {

• last_val = val;
  

• val &= (val >> 1);
  

• count++;
  

• }

There may not be a case that the most-significant bit is 1 at present, but if this case exists, it cannot count correctly.

Can you please rephrase what is your concern here please?

I test 0U, 1U, ~0U, and a few more complex combinations of bits, and all returned correct count so I might not have understood what case you meant with your description.

This function does not count nor calculate the most-significant bit in any phase but the longest train of bits using the count variable (and the least-significant bit of the longest train in the code that is not included into this partial snippet).

Hi Ilpo,

On 4/18/2023 4:45 AM, Ilpo Järvinen wrote:

CAT and CMT tests have count_of_bits, long_mask, and cache_size global variables that can be moved into the sole using function.

Make the global variables local variables of the relevant function to scope them better.

Could you please move this patch earlier, before usage of long_mask in earlier patch.

Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

tools/testing/selftests/resctrl/cat_test.c | 7 +++---- tools/testing/selftests/resctrl/cmt_test.c | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index ae21e656cf6e..ef3ba22bdde5 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -17,10 +17,6 @@ #define MAX_DIFF_PERCENT 4 #define MAX_DIFF 1000000 -static int count_of_bits; -static unsigned long long_mask; -static unsigned long cache_size;

/*

• Change schemata. Write schemata to specified
• con_mon grp, mon_grp in resctrl FS.

@@ -95,6 +91,9 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) { unsigned long l_mask, l_mask_1; int ret, pipefd[2], sibling_cpu_no;

• unsigned long cache_size;
• unsigned long long_mask;
• int count_of_bits; char pipe_message;

cache_size = 0;

Seems like this initialization can be moved to the definition?

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 087378a775ee..6adee08661e7 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -16,10 +16,6 @@ #define MAX_DIFF 2000000 #define MAX_DIFF_PERCENT 15 -static int count_of_bits; -static unsigned long long_mask; -static unsigned long cache_size;

static int cmt_setup(struct resctrl_val_param *p) { /* Run NUM_OF_RUNS times */ @@ -74,6 +70,9 @@ void cmt_test_cleanup(void) int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) {

• unsigned long cache_size;
• unsigned long long_mask;
• int count_of_bits; int ret;

cache_size = 0;

Same here.

Reinette

Hi Ilpo,

When reading memory in order, HW prefetching optimizations will interfere with measuring how caches and memory are being accessed. This adds noise into the results.

Change the fill_buf reading loop to not use an obvious in-order access using multiply by a prime and modulo.

Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

tools/testing/selftests/resctrl/fill_buf.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 7e0d3a1ea555..049a520498a9 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -88,14 +88,17 @@ static void *malloc_and_init_memory(size_t s)

static int fill_one_span_read(unsigned char *start_ptr, unsigned char *end_ptr) {

• unsigned char sum, *p;

• unsigned int size = (end_ptr - start_ptr) / (CL_SIZE / 2);
• unsigned int count = size;
• unsigned char sum;
• /*

• * Read the buffer in an order that is unexpected by HW prefetching
  

• * optimizations to prevent them interfering with the caching pattern.
  

• */
  

  sum = 0;

• p = start_ptr;
• while (p < end_ptr) {

• sum += *p;
  

• p += (CL_SIZE / 2);
  

• }

• while (count--)

• sum += start_ptr[((count * 59) % size) * CL_SIZE / 2];
  

Could you please elaborate why 59 is used?

Best regards, Shaopeng TAN

Hi Ilpo,

...

...

When reading memory in order, HW prefetching optimizations will interfere with measuring how caches and memory are being accessed. This adds noise into the results.

Change the fill_buf reading loop to not use an obvious in-order access using multiply by a prime and modulo.

Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

tools/testing/selftests/resctrl/fill_buf.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 7e0d3a1ea555..049a520498a9 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -88,14 +88,17 @@ static void *malloc_and_init_memory(size_t s)

static int fill_one_span_read(unsigned char *start_ptr, unsigned char *end_ptr) {

• unsigned char sum, *p;

• unsigned int size = (end_ptr - start_ptr) / (CL_SIZE / 2);
• unsigned int count = size;
• unsigned char sum;
• /*

• * Read the buffer in an order that is unexpected by HW prefetching
  

• * optimizations to prevent them interfering with the caching pattern.
  

• */
  

  sum = 0;

• p = start_ptr;
• while (p < end_ptr) {

• sum += *p;
  

• p += (CL_SIZE / 2);
  

• }

• while (count--)

• sum += start_ptr[((count * 59) % size) * CL_SIZE / 2];
  

Could you please elaborate why 59 is used?

The main reason is that it's a prime number ensuring the whole buffer gets read. I picked something that doesn't make it to wrap on almost every iteration.

Thanks for your explanation. It seems there is no problem.

Perhaps you have already tested this patch in your environment and got a test result of "ok". Because HW prefetching does not work well, the IMC counter fluctuates a lot in my environment, and the test result is "not ok".

In order to ensure this test set runs in any environments and gets "ok", would you consider changing the value of MAX_DIFF_PERCENT of each test? or changing something else?

``` Environment: Kernel: 6.4.0-rc2 CPU: Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz

Test result(MBM as an example): # # Starting MBM BW change ... # # Mounting resctrl to "/sys/fs/resctrl" # # Benchmark PID: 8671 # # Writing benchmark parameters to resctrl FS # # Write schema "MB:0=100" to resctrl FS # # Checking for pass/fail # # Fail: Check MBM diff within 5% # # avg_diff_per: 9% # # Span in bytes: 262144000 # # avg_bw_imc: 6202 # # avg_bw_resc: 5585 # not ok 1 MBM: bw change ```

Best regards, Shaopeng TAN

Hi Ilpo,

On Thu, 1 Jun 2023, Shaopeng Tan (Fujitsu) wrote:

...

...

...

...

When reading memory in order, HW prefetching optimizations will interfere with measuring how caches and memory are being accessed. This adds noise into the results.

Change the fill_buf reading loop to not use an obvious in-order access using multiply by a prime and modulo.

Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

tools/testing/selftests/resctrl/fill_buf.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 7e0d3a1ea555..049a520498a9 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -88,14 +88,17 @@ static void *malloc_and_init_memory(size_t s)

static int fill_one_span_read(unsigned char *start_ptr, unsigned char *end_ptr) {

• unsigned char sum, *p;

• unsigned int size = (end_ptr - start_ptr) / (CL_SIZE / 2);
• unsigned int count = size;
• unsigned char sum;
• /*

• * Read the buffer in an order that is unexpected by HW
  

prefetching

...

...

...

...

• * optimizations to prevent them interfering with the caching
  

pattern.

...

...

...

...

• */
  

  sum = 0;

• p = start_ptr;
• while (p < end_ptr) {

• sum += *p;
  

• p += (CL_SIZE / 2);
  

• }

• while (count--)

• sum += start_ptr[((count * 59) % size) * CL_SIZE / 2];
  

Could you please elaborate why 59 is used?

The main reason is that it's a prime number ensuring the whole buffer gets

read.

...

...

I picked something that doesn't make it to wrap on almost every iteration.

Thanks for your explanation. It seems there is no problem.

Perhaps you have already tested this patch in your environment and got a test

result of "ok".

...

Because HW prefetching does not work well, the IMC counter fluctuates a lot in my environment, and the test result is "not ok".

In order to ensure this test set runs in any environments and gets "ok", would you consider changing the value of MAX_DIFF_PERCENT of

each test?

...

or changing something else?

Environment:
 Kernel: 6.4.0-rc2
 CPU: Intel(R) Xeon(R) Gold 6254 CPU @ 3.10GHz

Test result(MBM as an example):
# # Starting MBM BW change ...
# # Mounting resctrl to "/sys/fs/resctrl"
# # Benchmark PID: 8671
# # Writing benchmark parameters to resctrl FS # # Write schema
"MB:0=100" to resctrl FS # # Checking for pass/fail # # Fail: Check
MBM diff within 5% # # avg_diff_per: 9% # # Span in bytes: 262144000 #
# avg_bw_imc: 6202 # # avg_bw_resc: 5585 # not ok 1 MBM: bw change ```

Could you try if the approach below works better (I think it should apply cleanly on top of the fixes+cleanups v3 series which you recently tested, no need to have the other CAT test changes).

I ran the test set several times. MBA and MBM seem fine, but CAT is always "not ok". The result is below.

--- $ sudo make -C tools/testing/selftests/resctrl run_tests make: Entering directory '/**/tools/testing/selftests/resctrl' TAP version 13 1..1 # selftests: resctrl: resctrl_tests # TAP version 13 # # Pass: Check kernel supports resctrl filesystem # # Pass: Check resctrl mountpoint "/sys/fs/resctrl" exists # # resctrl filesystem not mounted # # dmesg: [ 3.658029] resctrl: L3 allocation detected # # dmesg: [ 3.658420] resctrl: MB allocation detected # # dmesg: [ 3.658604] resctrl: L3 monitoring detected # 1..4 # # Starting MBM BW change ... # # Mounting resctrl to "/sys/fs/resctrl" # # Benchmark PID: 9555 # # Writing benchmark parameters to resctrl FS # # Write schema "MB:0=100" to resctrl FS # # Checking for pass/fail # # Pass: Check MBM diff within 5% # # avg_diff_per: 0% # # Span (MB): 250 # # avg_bw_imc: 6880 # # avg_bw_resc: 6895 # ok 1 MBM: bw change # # Starting MBA Schemata change ... # # Mounting resctrl to "/sys/fs/resctrl" # # Benchmark PID: 9561 # # Writing benchmark parameters to resctrl FS # # Write schema "MB:0=100" to resctrl FS # # Write schema "MB:0=90" to resctrl FS # # Write schema "MB:0=80" to resctrl FS # # Write schema "MB:0=70" to resctrl FS # # Write schema "MB:0=60" to resctrl FS # # Write schema "MB:0=50" to resctrl FS # # Write schema "MB:0=40" to resctrl FS # # Write schema "MB:0=30" to resctrl FS # # Write schema "MB:0=20" to resctrl FS # # Write schema "MB:0=10" to resctrl FS # # Results are displayed in (MB) # # Pass: Check MBA diff within 5% for schemata 100 # # avg_diff_per: 0% # # avg_bw_imc: 6874 # # avg_bw_resc: 6904 # # Pass: Check MBA diff within 5% for schemata 90 # # avg_diff_per: 1% # # avg_bw_imc: 6738 # # avg_bw_resc: 6807 # # Pass: Check MBA diff within 5% for schemata 80 # # avg_diff_per: 1% # # avg_bw_imc: 6735 # # avg_bw_resc: 6803 # # Pass: Check MBA diff within 5% for schemata 70 # # avg_diff_per: 1% # # avg_bw_imc: 6702 # # avg_bw_resc: 6770 # # Pass: Check MBA diff within 5% for schemata 60 # # avg_diff_per: 1% # # avg_bw_imc: 6632 # # avg_bw_resc: 6725 # # Pass: Check MBA diff within 5% for schemata 50 # # avg_diff_per: 1% # # avg_bw_imc: 6510 # # avg_bw_resc: 6635 # # Pass: Check MBA diff within 5% for schemata 40 # # avg_diff_per: 2% # # avg_bw_imc: 6206 # # avg_bw_resc: 6342 # # Pass: Check MBA diff within 5% for schemata 30 # # avg_diff_per: 1% # # avg_bw_imc: 3826 # # avg_bw_resc: 3896 # # Pass: Check MBA diff within 5% for schemata 20 # # avg_diff_per: 1% # # avg_bw_imc: 2820 # # avg_bw_resc: 2862 # # Pass: Check MBA diff within 5% for schemata 10 # # avg_diff_per: 1% # # avg_bw_imc: 1876 # # avg_bw_resc: 1898 # # Pass: Check schemata change using MBA # ok 2 MBA: schemata change # # Starting CMT test ... # # Mounting resctrl to "/sys/fs/resctrl" # # Cache size :25952256 # # Benchmark PID: 9573 # # Writing benchmark parameters to resctrl FS # # Checking for pass/fail # # Pass: Check cache miss rate within 15% # # Percent diff=10 # # Number of bits: 5 # # Average LLC val: 12994560 # # Cache span (bytes): 11796480 # ok 3 CMT: test # # Starting CAT test ... # # Mounting resctrl to "/sys/fs/resctrl" # # Cache size :25952256 # # Writing benchmark parameters to resctrl FS # # Write schema "L3:0=3f" to resctrl FS # # Checking for pass/fail # # Fail: Check cache miss rate within 4% # # Percent diff=24 # # Number of bits: 6 # # Average LLC val: 275418 # # Cache span (lines): 221184 # not ok 4 CAT: test # # Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0 not ok 1 selftests: resctrl: resctrl_tests # exit=1 make: Leaving directory '/**/tools/testing/selftests/resctrl' ---

Best regards, Shaopeng TAN

On Fri, 21 Apr 2023, Reinette Chatre wrote:

Hi Ilpo,

On 4/18/2023 4:45 AM, Ilpo Järvinen wrote:

...

CAT test spawns two processes into two different control groups with exclusive schemata. Both the processes alloc a buffer from memory matching their allocated LLC block size and flush the entire buffer out of caches. Since the processes are reading through the buffer only once during the measurement and initially all the buffer was flushed, the test isn't testing CAT.

Rewrite the CAT test to allocated a buffer sized to half of LLC. Then

"allocated a buffer" -> "allocate a buffer" ?

...

perform a sequence of tests with different LLC alloc sizes starting from half of the CBM bits down to 1-bit CBM. Flush the buffer before each test and read the buffer twice. Observe the LLC misses on the second read through the buffer. As the allocated LLC block gets smaller and smaller, the LLC misses will become larger and larger giving a strong signal on CAT working properly.

Since the changelog starts by describing the CAT test needing two processes I think it would help to highlight that this test uses a single process. I think it would also help to describing how the cache is used by the rest while this test is running.

Sure, good points, I'll add the info.

...

Suggested-by: Reinette Chatre reinette.chatre@intel.com Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com

---

tools/testing/selftests/resctrl/cache.c | 20 +- tools/testing/selftests/resctrl/cat_test.c | 204 +++++++++------------ 2 files changed, 97 insertions(+), 127 deletions(-)

diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c index 7970239413da..64f08ba5edc2 100644 --- a/tools/testing/selftests/resctrl/cache.c +++ b/tools/testing/selftests/resctrl/cache.c @@ -224,10 +224,10 @@ int measure_llc_resctrl(struct resctrl_val_param *param, int bm_pid) */ int cat_val(struct resctrl_val_param *param) {

• int memflush = 1, operation = 0, ret = 0; char *resctrl_val = param->resctrl_val; unsigned long llc_perf_miss = 0; pid_t bm_pid;

• int ret;

if (strcmp(param->filename, "") == 0) sprintf(param->filename, "stdio"); @@ -245,6 +245,10 @@ int cat_val(struct resctrl_val_param *param) if (ret) return ret;

• ret = alloc_buffer(param->span, 1);
• if (ret)

• return ret;
  

• initialize_llc_perf();

/* Test runs until the callback setup() tells the test to stop. */ @@ -256,17 +260,15 @@ int cat_val(struct resctrl_val_param *param) } if (ret < 0) break;

• flush_buffer(param->span);
  

• use_buffer(param->span, 0, true);
  

• ret = reset_enable_llc_perf(bm_pid, param->cpu_no); if (ret) break;

• if (run_fill_buf(param->span, memflush, operation, true)) {
  

• 	fprintf(stderr, "Error-running fill buffer\n");
  

• 	ret = -1;
  

• 	break;
  

• }
  

• sleep(1);
  

• use_buffer(param->span, 0, true);
  

/* Measure cache miss from perf */ ret = get_llc_perf(&llc_perf_miss); @@ -279,6 +281,8 @@ int cat_val(struct resctrl_val_param *param) break; }

• free_buffer();
• return ret;

} diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 4b505fdb35d7..85053829b9c5 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -11,11 +11,12 @@ #include "resctrl.h" #include <unistd.h> -#define RESULT_FILE_NAME1 "result_cat1" -#define RESULT_FILE_NAME2 "result_cat2" -#define NUM_OF_RUNS 5 -#define MAX_DIFF_PERCENT 4 -#define MAX_DIFF 1000000 +#define RESULT_FILE_NAME "result_cat" +#define NUM_OF_RUNS 5 +#define MIN_DIFF_PERCENT_PER_BIT 2

Could you please start a new trend that adds documentation that explains what this constant means and how it was chosen?

I can try although that particular 2 was a bit handwavy that just seems to work with the tests I performed.

...

+static unsigned long current_mask; +static long prev_avg_llc_val; /*

• Change schemata. Write schemata to specified

@@ -28,13 +29,24 @@ static int cat_setup(struct resctrl_val_param *p) int ret = 0; /* Run NUM_OF_RUNS times */

• if (p->num_of_runs >= NUM_OF_RUNS)

• return END_OF_TESTS;
  

• if (p->num_of_runs >= NUM_OF_RUNS) {

• /* Remove one bit from the consecutive block */
  

• current_mask &= current_mask >> 1;
  

• if (!current_mask)
  

• 	return END_OF_TESTS;
  

• p->num_of_runs = 0;
  

This seems like a workaround to get the schemata to be written. It is problematic since now p->num_of_runs no longer accurately reflects the number of test runs.

This is already the case. MBA test works around this very same problem by using a custom static variable (runs_per_allocation) which is reset to 0 every NUM_OF_RUNS tests and not keeping ->num_of_runs at all. If MBA test would replace runs_per_allocation with use of ->num_of_runs, it would match what the new CAT test does.

Nothing currently relies on ->num_of_runs counting across the different "tests" that are run inside CAT and MBA tests. And I don't have anything immediately around the corner that would require ->num_of_runs to count total number of repetitions that were ran.

I guess it would be possible to attempt to consolidate that second layer MBA and the rewritten CAT tests need somehow into resctrl_val_param. But IMHO that too is low-prio refactor as nothing is broken as is.

I was expecting this mask manipulation to be in cat_val() so that it is clear how test works instead of part of the logic handled here.

That seems to be moving into opposite direction from how things are currently handled. Doing it in cat_val() would be relying less on ->setup(). If that's the preferred direction, then the question becomes, should CAT test do anything in ->setup() because also the schemata writing could be done in directly cat_val().

What I would prefer not to do is to have a rule which says: if there's a test-specific function, don't use ->setup() but do any setup directly in the test-specific function but, otherwise use ->setup(). Such an inconsistency would make things hard to track.

...

• }

if (p->num_of_runs == 0) {

• sprintf(schemata, "%lx", p->mask);
  

• ret = write_schemata(p->ctrlgrp, schemata, p->cpu_no,
  

• 		     p->resctrl_val);
  

• snprintf(schemata, sizeof(schemata), "%lx", p->mask & ~current_mask);
  

• ret = write_schemata("", schemata, p->cpu_no, p->resctrl_val);
  

• if (ret)
  

• 	return ret;
  

• snprintf(schemata, sizeof(schemata), "%lx", current_mask);
  

• ret = write_schemata(p->ctrlgrp, schemata, p->cpu_no, p->resctrl_val);
  

• if (ret)
  

• 	return ret;
  

  } p->num_of_runs++;

...

...

@@ -126,7 +162,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) ret = get_mask_no_shareable(cache_type, &long_mask); if (ret) return ret;

• count_of_bits = count_consecutive_bits(long_mask, NULL);

• count_of_bits = count_consecutive_bits(long_mask, &start);

/* Get L3/L2 cache size */ ret = get_cache_size(cpu_no, cache_type, &cache_size); @@ -143,99 +179,29 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) count_of_bits - 1); return -1; }

• /* Get core id from same socket for running another thread */
• sibling_cpu_no = get_core_sibling(cpu_no);

Do any users of get_core_sibling() remain after this?

Correct observation, there seems to be no other users after this is removed.

On Wed, 26 Apr 2023, Reinette Chatre wrote:

On 4/26/2023 6:58 AM, Ilpo Järvinen wrote:

...

On Fri, 21 Apr 2023, Reinette Chatre wrote:

...

On 4/18/2023 4:45 AM, Ilpo Järvinen wrote:

...

...

...

...

diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 4b505fdb35d7..85053829b9c5 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -11,11 +11,12 @@ #include "resctrl.h" #include <unistd.h> -#define RESULT_FILE_NAME1 "result_cat1" -#define RESULT_FILE_NAME2 "result_cat2" -#define NUM_OF_RUNS 5 -#define MAX_DIFF_PERCENT 4 -#define MAX_DIFF 1000000 +#define RESULT_FILE_NAME "result_cat" +#define NUM_OF_RUNS 5 +#define MIN_DIFF_PERCENT_PER_BIT 2

Could you please start a new trend that adds documentation that explains what this constant means and how it was chosen?

I can try although that particular 2 was a bit handwavy that just seems to work with the tests I performed.

The changelog claims that the existing CAT test does not work with this new test offered as replacement. Considering that I do think it is important to have confidence that this test is able to test CAT. The words "handwave" and "seems to work" are red flags to me. When merged, these tests will be run on a variety of platforms with various configurations. Using test criteria based on measurements from one particular system may work but there needs to be confidence that the criteria maps to all systems these tests will be run on.

My "tests" (in plural) were not limited to one particular system but included systems from different generations.

...

...

...

+static unsigned long current_mask; +static long prev_avg_llc_val; /*

• Change schemata. Write schemata to specified

@@ -28,13 +29,24 @@ static int cat_setup(struct resctrl_val_param *p) int ret = 0; /* Run NUM_OF_RUNS times */

• if (p->num_of_runs >= NUM_OF_RUNS)

• return END_OF_TESTS;
  

• if (p->num_of_runs >= NUM_OF_RUNS) {

• /* Remove one bit from the consecutive block */
  

• current_mask &= current_mask >> 1;
  

• if (!current_mask)
  

• 	return END_OF_TESTS;
  

• p->num_of_runs = 0;
  

This seems like a workaround to get the schemata to be written. It is problematic since now p->num_of_runs no longer accurately reflects the number of test runs.

This is already the case. MBA test works around this very same problem by using a custom static variable (runs_per_allocation) which is reset to 0 every NUM_OF_RUNS tests and not keeping ->num_of_runs at all. If MBA test would replace runs_per_allocation with use of ->num_of_runs, it would match what the new CAT test does.

Nothing currently relies on ->num_of_runs counting across the different "tests" that are run inside CAT and MBA tests. And I don't have anything immediately around the corner that would require ->num_of_runs to count total number of repetitions that were ran.

I guess it would be possible to attempt to consolidate that second layer MBA and the rewritten CAT tests need somehow into resctrl_val_param. But IMHO that too is low-prio refactor as nothing is broken as is.

I do not think that I would use any of the other tests as reference since all the other tests rely on the same wrapper (resctrl_val()) by providing it their own customization (via aptly named ... struct resctrl_val_param).

Oh, I see. I never made the connection to the function name before this. (To be honest, it's pretty stupid name for that particular function, given what the function does, but that's an entirely separate issue.)