This series fixes a few cleanup/error handling problems and cleans up code.
v2: - Improved changelogs - Return NULL directly from malloc_and_init_memory() - Added patch to convert memalign() to posix_memalign() - Added patch to correct function comment parameter - Dropped literal -> define patch for now (likely superceded soon)
Fenghua Yu (1): selftests/resctrl: Change name from CBM_MASK_PATH to INFO_PATH
Ilpo Järvinen (8): selftests/resctrl: Return NULL if malloc_and_init_memory() did not alloc mem selftests/resctrl: Move ->setup() call outside of test specific branches selftests/resctrl: Allow ->setup() to return errors selftests/resctrl: Check for return value after write_schemata() selftests/resctrl: Replace obsolete memalign() with posix_memalign() selftests/resctrl: Change initialize_llc_perf() return type to void selftests/resctrl: Use remount_resctrlfs() consistently with boolean selftests/resctrl: Correct get_llc_perf() param in function comment
tools/testing/selftests/resctrl/cache.c | 17 +++++++-------- tools/testing/selftests/resctrl/cat_test.c | 4 ++-- tools/testing/selftests/resctrl/cmt_test.c | 9 ++++---- tools/testing/selftests/resctrl/fill_buf.c | 7 +++++-- tools/testing/selftests/resctrl/mba_test.c | 11 +++++++--- tools/testing/selftests/resctrl/mbm_test.c | 4 ++-- tools/testing/selftests/resctrl/resctrl.h | 6 ++++-- tools/testing/selftests/resctrl/resctrl_val.c | 21 +++++++------------ tools/testing/selftests/resctrl/resctrlfs.c | 2 +- 9 files changed, 41 insertions(+), 40 deletions(-)
malloc_and_init_memory() in fill_buf isn't checking if memalign() successfully allocated memory or not before accessing the memory.
Check the return value of memalign() and return NULL if allocating aligned memory fails.
Fixes: a2561b12fe39 ("selftests/resctrl: Add built in benchmark") 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 | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index 56ccbeae0638..c20d0a7ecbe6 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -68,6 +68,8 @@ static void *malloc_and_init_memory(size_t s) size_t s64;
void *p = memalign(PAGE_SIZE, s); + if (!p) + return NULL;
p64 = (uint64_t *)p; s64 = s / sizeof(uint64_t);
Hi Ilpo,
On 2/15/2023 5:05 AM, Ilpo Järvinen wrote:
malloc_and_init_memory() in fill_buf isn't checking if memalign() successfully allocated memory or not before accessing the memory.
Check the return value of memalign() and return NULL if allocating aligned memory fails.
Fixes: a2561b12fe39 ("selftests/resctrl: Add built in benchmark") 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
Thank you.
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
resctrl_val() function is called only by MBM, MBA, and CMT tests which means the else branch is never used.
Both test branches call param->setup().
Remove the unused else branch and place the ->setup() call outside of the test specific branches reducing code duplication.
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_val.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index b32b96356ec7..787546a52849 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -734,29 +734,22 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
/* Test runs until the callback setup() tells the test to stop. */ while (1) { + ret = param->setup(1, param); + if (ret) { + ret = 0; + break; + } + if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) || !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) { - ret = param->setup(1, param); - if (ret) { - ret = 0; - break; - } - ret = measure_vals(param, &bw_resc_start); if (ret) break; } else if (!strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) { - ret = param->setup(1, param); - if (ret) { - ret = 0; - break; - } sleep(1); ret = measure_cache_vals(param, bm_pid); if (ret) break; - } else { - break; } }
Hi Ilpo,
On 2/15/2023 5:05 AM, Ilpo Järvinen wrote:
resctrl_val() function is called only by MBM, MBA, and CMT tests which
Surely not a reason for a resubmit, but just fyi ... using "()" implies that it is a function so there is no need to add the text "function".
means the else branch is never used.
Both test branches call param->setup().
Remove the unused else branch and place the ->setup() call outside of the test specific branches reducing code duplication.
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
Thank you
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
resctrl_val() assumes ->setup() always returns either 0 to continue tests or < 0 in case of the normal termination of tests after x runs. The latter overlaps with normal error returns.
Define END_OF_TESTS (=1) to differentiate the normal termination of tests and return errors as negative values. Alter callers of ->setup() to handle errors properly.
Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest") Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test") Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- tools/testing/selftests/resctrl/cache.c | 4 +++- tools/testing/selftests/resctrl/cat_test.c | 2 +- tools/testing/selftests/resctrl/cmt_test.c | 2 +- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 2 +- tools/testing/selftests/resctrl/resctrl.h | 2 ++ tools/testing/selftests/resctrl/resctrl_val.c | 4 +++- 7 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c index 68ff856d36f0..0485863a169f 100644 --- a/tools/testing/selftests/resctrl/cache.c +++ b/tools/testing/selftests/resctrl/cache.c @@ -244,10 +244,12 @@ int cat_val(struct resctrl_val_param *param) while (1) { if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) { ret = param->setup(1, param); - if (ret) { + if (ret == END_OF_TESTS) { ret = 0; break; } + if (ret < 0) + break; ret = reset_enable_llc_perf(bm_pid, param->cpu_no); if (ret) break; diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 1c5e90c63254..2d3c7c77ab6c 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -40,7 +40,7 @@ static int cat_setup(int num, ...)
/* Run NUM_OF_RUNS times */ if (p->num_of_runs >= NUM_OF_RUNS) - return -1; + return END_OF_TESTS;
if (p->num_of_runs == 0) { sprintf(schemata, "%lx", p->mask); diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 8968e36db99d..3b0454e7fc82 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -32,7 +32,7 @@ static int cmt_setup(int num, ...)
/* Run NUM_OF_RUNS times */ if (p->num_of_runs >= NUM_OF_RUNS) - return -1; + return END_OF_TESTS;
p->num_of_runs++;
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index 1a1bdb6180cf..f32289ae17ae 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -41,7 +41,7 @@ static int mba_setup(int num, ...) return 0;
if (allocation < ALLOCATION_MIN || allocation > ALLOCATION_MAX) - return -1; + return END_OF_TESTS;
sprintf(allocation_str, "%d", allocation);
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 8392e5c55ed0..280187628054 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -95,7 +95,7 @@ static int mbm_setup(int num, ...)
/* Run NUM_OF_RUNS times */ if (num_of_runs++ >= NUM_OF_RUNS) - return -1; + return END_OF_TESTS;
va_start(param, num); p = va_arg(param, struct resctrl_val_param *); diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index f0ded31fb3c7..f44fa2de4d98 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -37,6 +37,8 @@ #define ARCH_INTEL 1 #define ARCH_AMD 2
+#define END_OF_TESTS 1 + #define PARENT_EXIT(err_msg) \ do { \ perror(err_msg); \ diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 787546a52849..00864242d76c 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -735,10 +735,12 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) /* Test runs until the callback setup() tells the test to stop. */ while (1) { ret = param->setup(1, param); - if (ret) { + if (ret == END_OF_TESTS) { ret = 0; break; } + if (ret < 0) + break;
if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) || !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
Hi Ilpo,
On 2/15/2023 5:05 AM, Ilpo Järvinen wrote:
resctrl_val() assumes ->setup() always returns either 0 to continue tests or < 0 in case of the normal termination of tests after x runs. The latter overlaps with normal error returns.
Define END_OF_TESTS (=1) to differentiate the normal termination of tests and return errors as negative values. Alter callers of ->setup() to handle errors properly.
Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest") Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test") Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
Thank you
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
MBA test case writes schemata but it does not check if the write is successful or not.
Add the error check and return error properly.
Fixes: 01fee6b4d1f9 ("selftests/resctrl: Add MBA test") 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/mba_test.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index f32289ae17ae..97dc98c0c949 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -28,6 +28,7 @@ static int mba_setup(int num, ...) struct resctrl_val_param *p; char allocation_str[64]; va_list param; + int ret;
va_start(param, num); p = va_arg(param, struct resctrl_val_param *); @@ -45,7 +46,11 @@ static int mba_setup(int num, ...)
sprintf(allocation_str, "%d", allocation);
- write_schemata(p->ctrlgrp, allocation_str, p->cpu_no, p->resctrl_val); + ret = write_schemata(p->ctrlgrp, allocation_str, p->cpu_no, + p->resctrl_val); + if (ret < 0) + return ret; + allocation -= ALLOCATION_STEP;
return 0;
Hi Ilpo,
On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
MBA test case writes schemata but it does not check if the write is successful or not.
Add the error check and return error properly.
Fixes: 01fee6b4d1f9 ("selftests/resctrl: Add MBA test") 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
Thank you
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
memalign() is obsolete according to its manpage.
Replace memalign() with posix_memalign() and remove malloc.h include that was there for memalign().
As a pointer is passed into posix_memalign(), initialize *p to NULL to silence a warning about the function's return value being used as uninitialized (which is not valid anyway because the error is properly checked before p is returned).
Suggested-by: Reinette Chatre reinette.chatre@intel.com Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- tools/testing/selftests/resctrl/fill_buf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c index c20d0a7ecbe6..3cd0b337eae5 100644 --- a/tools/testing/selftests/resctrl/fill_buf.c +++ b/tools/testing/selftests/resctrl/fill_buf.c @@ -14,7 +14,6 @@ #include <sys/types.h> #include <sys/wait.h> #include <inttypes.h> -#include <malloc.h> #include <string.h>
#include "resctrl.h" @@ -64,11 +63,13 @@ static void mem_flush(void *p, size_t s)
static void *malloc_and_init_memory(size_t s) { + void *p = NULL; uint64_t *p64; size_t s64; + int ret;
- void *p = memalign(PAGE_SIZE, s); - if (!p) + ret = posix_memalign(&p, PAGE_SIZE, s); + if (ret < 0) return NULL;
p64 = (uint64_t *)p;
Hi Ilpo,
On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
memalign() is obsolete according to its manpage.
Replace memalign() with posix_memalign() and remove malloc.h include that was there for memalign().
As a pointer is passed into posix_memalign(), initialize *p to NULL to silence a warning about the function's return value being used as uninitialized (which is not valid anyway because the error is properly checked before p is returned).
Suggested-by: Reinette Chatre reinette.chatre@intel.com Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
Thank you
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
initialize_llc_perf() unconditionally returns 0.
initialize_llc_perf() performs only memory initialization, none of which can fail.
Change the return type from int to void to accurately reflect that its return value doesn't need to be checked. Remove the error checking from the only callsite.
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/cache.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c index 0485863a169f..585186c874dc 100644 --- a/tools/testing/selftests/resctrl/cache.c +++ b/tools/testing/selftests/resctrl/cache.c @@ -48,7 +48,7 @@ static int perf_event_open_llc_miss(pid_t pid, int cpu_no) return 0; }
-static int initialize_llc_perf(void) +static void initialize_llc_perf(void) { memset(&pea_llc_miss, 0, sizeof(struct perf_event_attr)); memset(&rf_cqm, 0, sizeof(struct read_format)); @@ -59,8 +59,6 @@ static int initialize_llc_perf(void) pea_llc_miss.config = PERF_COUNT_HW_CACHE_MISSES;
rf_cqm.nr = 1; - - return 0; }
static int reset_enable_llc_perf(pid_t pid, int cpu_no) @@ -234,11 +232,8 @@ int cat_val(struct resctrl_val_param *param) if (ret) return ret;
- if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) { - ret = initialize_llc_perf(); - if (ret) - return ret; - } + if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) + initialize_llc_perf();
/* Test runs until the callback setup() tells the test to stop. */ while (1) {
Hi Ilpo,
On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
initialize_llc_perf() unconditionally returns 0.
initialize_llc_perf() performs only memory initialization, none of which can fail.
Change the return type from int to void to accurately reflect that its return value doesn't need to be checked. Remove the error checking from the only callsite.
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
Thank you
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
From: Fenghua Yu fenghua.yu@intel.com
CBM_MASK_PATH is actually the path to resctrl/info.
Change the macro name to correctly indicate what it represents.
[ ij: Tweaked the changelog. ]
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.h | 2 +- tools/testing/selftests/resctrl/resctrlfs.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index f44fa2de4d98..20aaa7c0e784 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -28,7 +28,7 @@ #define MB (1024 * 1024) #define RESCTRL_PATH "/sys/fs/resctrl" #define PHYS_ID_PATH "/sys/devices/system/cpu/cpu" -#define CBM_MASK_PATH "/sys/fs/resctrl/info" +#define INFO_PATH "/sys/fs/resctrl/info" #define L3_PATH "/sys/fs/resctrl/info/L3" #define MB_PATH "/sys/fs/resctrl/info/MB" #define L3_MON_PATH "/sys/fs/resctrl/info/L3_MON" diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 6f543e470ad4..cc6cf49e3129 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -210,7 +210,7 @@ int get_cbm_mask(char *cache_type, char *cbm_mask) if (!cbm_mask) return -1;
- sprintf(cbm_mask_path, "%s/%s/cbm_mask", CBM_MASK_PATH, cache_type); + sprintf(cbm_mask_path, "%s/%s/cbm_mask", INFO_PATH, cache_type);
fp = fopen(cbm_mask_path, "r"); if (!fp) {
Hi Ilpo,
On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
From: Fenghua Yu fenghua.yu@intel.com
CBM_MASK_PATH is actually the path to resctrl/info.
Change the macro name to correctly indicate what it represents.
[ ij: Tweaked the changelog. ]
Signed-off-by: Fenghua Yu fenghua.yu@intel.com Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
Thank you
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
remount_resctrlfs() accepts a boolean value as an argument. Some tests pass 0/1 and some tests pass true/false.
Make all the callers of remount_resctrlfs() use true/false so that the parameter usage is consistent across 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/cat_test.c | 2 +- tools/testing/selftests/resctrl/cmt_test.c | 7 +++---- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 2 +- tools/testing/selftests/resctrl/resctrl.h | 2 +- 5 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 2d3c7c77ab6c..08070d4fa735 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -145,7 +145,7 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) struct resctrl_val_param param = { .resctrl_val = CAT_STR, .cpu_no = cpu_no, - .mum_resctrlfs = 0, + .mum_resctrlfs = false, .setup = cat_setup, };
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 3b0454e7fc82..47cde5c02b7f 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -82,12 +82,11 @@ void cmt_test_cleanup(void)
int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) { - int ret, mum_resctrlfs; + int ret;
cache_size = 0; - mum_resctrlfs = 1;
- ret = remount_resctrlfs(mum_resctrlfs); + ret = remount_resctrlfs(true); if (ret) return ret;
@@ -118,7 +117,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) .ctrlgrp = "c1", .mongrp = "m1", .cpu_no = cpu_no, - .mum_resctrlfs = 0, + .mum_resctrlfs = false, .filename = RESULT_FILE_NAME, .mask = ~(long_mask << n) & long_mask, .span = cache_size * n / count_of_bits, diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index 97dc98c0c949..7defb32ad0de 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -154,7 +154,7 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd) .ctrlgrp = "c1", .mongrp = "m1", .cpu_no = cpu_no, - .mum_resctrlfs = 1, + .mum_resctrlfs = true, .filename = RESULT_FILE_NAME, .bw_report = bw_report, .setup = mba_setup diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 280187628054..c9dfa54af42f 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -122,7 +122,7 @@ int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd) .mongrp = "m1", .span = span, .cpu_no = cpu_no, - .mum_resctrlfs = 1, + .mum_resctrlfs = true, .filename = RESULT_FILE_NAME, .bw_report = bw_report, .setup = mbm_setup diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 20aaa7c0e784..9555a6f683f7 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -64,7 +64,7 @@ struct resctrl_val_param { char mongrp[64]; int cpu_no; unsigned long span; - int mum_resctrlfs; + bool mum_resctrlfs; char filename[64]; char *bw_report; unsigned long mask;
Hi Ilpo,
On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
remount_resctrlfs() accepts a boolean value as an argument. Some tests pass 0/1 and some tests pass true/false.
Make all the callers of remount_resctrlfs() use true/false so that the parameter usage is consistent across 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
Thank you
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
get_llc_perf() function comment refers to cpu_no parameter that does not exist.
Correct get_llc_perf() the comment to document llc_perf_miss instead.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- tools/testing/selftests/resctrl/cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c index 585186c874dc..8a4fe8693be6 100644 --- a/tools/testing/selftests/resctrl/cache.c +++ b/tools/testing/selftests/resctrl/cache.c @@ -77,7 +77,7 @@ static int reset_enable_llc_perf(pid_t pid, int cpu_no)
/* * get_llc_perf: llc cache miss through perf events - * @cpu_no: CPU number that the benchmark PID is binded to + * @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.
Hi Ilpo,
On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
get_llc_perf() function comment refers to cpu_no parameter that does not exist.
Correct get_llc_perf() the comment to document llc_perf_miss instead.
"Correct the get_llc_perf() comment"? This is so minor and I do not think a reason to resubmit whole series.
Signed-off-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
Thank you
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
On Wed, 15 Mar 2023, Reinette Chatre wrote:
Hi Ilpo,
On 2/15/2023 5:06 AM, Ilpo Järvinen wrote:
get_llc_perf() function comment refers to cpu_no parameter that does not exist.
Correct get_llc_perf() the comment to document llc_perf_miss instead.
"Correct the get_llc_perf() comment"?
Yes. No matter how many times I read my own changelogs through, my mind goes to auto-correction mode and I often fail to spot obvious errors such as this.
Thanks for reviewing.
Hi Ilpo,
On 2/15/2023 5:05 AM, Ilpo Järvinen wrote:
This series fixes a few cleanup/error handling problems and cleans up code.
Thank you very much. These are great cleanups. Looks like I missed sending one response with the others but at this time all patches in this series should have my Reviewed-by tag.
If the Kselftest team finds them acceptable I hope that they can help to route them upstream.
Reinette
linux-kselftest-mirror@lists.linaro.org
-
Ilpo Järvinen -
Reinette Chatre