Fenghua Yu (1): selftests/resctrl: Don't hard code divisor in MBM results
Ilpo Järvinen (2): selftests/resctrl: Change initialize_llc_perf() return type to void selftests/resctrl: Use remount_resctrlfs() consistently with boolean
Signed-off-by: Fenghua Yu (1): selftests/resctrl: Change name from CBM_MASK_PATH to INFO_PATH
tools/testing/selftests/resctrl/cache.c | 11 +++-------- 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 | 6 +++--- tools/testing/selftests/resctrl/resctrl.h | 4 ++-- tools/testing/selftests/resctrl/resctrlfs.c | 2 +- 7 files changed, 14 insertions(+), 20 deletions(-)
initialize_llc_perf() unconditionally does return 0 so no point in having it's return type as int. Hence, change it's return type from int to void.
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 68ff856d36f0..4b8ee81aedae 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/8/2023 1:40 AM, Ilpo Järvinen wrote:
initialize_llc_perf() unconditionally does return 0 so no point in having it's return type as int. Hence, change it's return type from int to void.
Thank you very much for contributing to resctrl. As a new resctrl contributor I would like to share that resctrl follows the x86 style guidance and to be consistent this is for the most part true for the resctrl selftest area.
To that point, changelogs are easier to read if the context, problem, and solution are clearly separated by placing them in separate paragraphs. See "Changelog" in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docu...
Please compare to a changelog as follows:
" initialize_llc_perf() unconditionally returns 0.
initialize_llc_perf() performs memory initialization, none of which can fail.
Change the return type from int to void to accurately reflect that there is no checking of return value needed. "
For such a small change as this, the changelog could possibly be simplified but the context, problem, and solution should always be clear to the reader. This may be significant changelog feedback for such a small change. This is because it is your first patch to this area and my goal is to point out the style that will help your future resctrl contributions to have the pattern that x86 maintainers expect.
Reinette
From: Fenghua Yu fenghua.yu@intel.com
Presently, while calculating MBM results, the divisor is hard coded as 4. It's hard coded to 4 because "NUM_OF_RUNS" is defined as 5 and the test does not count first result and hence 4. So, instead of hard coding the value to 4, change it to NUM_OF_RUNS - 1.
Signed-off-by: Fenghua Yu fenghua.yu@intel.com --- tools/testing/selftests/resctrl/mbm_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 8392e5c55ed0..4f85cbbfd037 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -31,8 +31,8 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, int span) sum_bw_resc += bw_resc[runs]; }
- avg_bw_imc = sum_bw_imc / 4; - avg_bw_resc = sum_bw_resc / 4; + avg_bw_imc = sum_bw_imc / (NUM_OF_RUNS - 1); + avg_bw_resc = sum_bw_resc / (NUM_OF_RUNS - 1); avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc; avg_diff_per = (int)(avg_diff * 100);
Hi Ilpo,
The subject is "Don't hard code divisor ..." yet it seems to me that the hard coding persists. It is just changed from a magic constant to a macro.
On 2/8/2023 1:40 AM, Ilpo Järvinen wrote:
From: Fenghua Yu fenghua.yu@intel.com
Presently, while calculating MBM results, the divisor is hard coded as 4.
"Presently" can be removed. Here and in the rest of the series the usage of "presently" and "currently" can usually be removed to improve clarity.
It's hard coded to 4 because "NUM_OF_RUNS" is defined as 5 and the test does not count first result and hence 4. So, instead of hard coding the value to 4, change it to NUM_OF_RUNS - 1.
Are there any plans surrounding using struct resctrl_val_param::num_of_runs instead?
Signed-off-by: Fenghua Yu fenghua.yu@intel.com
Missing your Signed-off-by?
tools/testing/selftests/resctrl/mbm_test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index 8392e5c55ed0..4f85cbbfd037 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -31,8 +31,8 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, int span) sum_bw_resc += bw_resc[runs]; }
- avg_bw_imc = sum_bw_imc / 4;
- avg_bw_resc = sum_bw_resc / 4;
- avg_bw_imc = sum_bw_imc / (NUM_OF_RUNS - 1);
- avg_bw_resc = sum_bw_resc / (NUM_OF_RUNS - 1); avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc; avg_diff_per = (int)(avg_diff * 100);
Reinette
On Mon, 13 Feb 2023, Reinette Chatre wrote:
Hi Ilpo,
The subject is "Don't hard code divisor ..." yet it seems to me that the hard coding persists. It is just changed from a magic constant to a macro.
Yeah, it was a bad wording.
On 2/8/2023 1:40 AM, Ilpo Järvinen wrote:
From: Fenghua Yu fenghua.yu@intel.com
Presently, while calculating MBM results, the divisor is hard coded as 4.
"Presently" can be removed. Here and in the rest of the series the usage of "presently" and "currently" can usually be removed to improve clarity.
It's hard coded to 4 because "NUM_OF_RUNS" is defined as 5 and the test does not count first result and hence 4. So, instead of hard coding the value to 4, change it to NUM_OF_RUNS - 1.
Are there any plans surrounding using struct resctrl_val_param::num_of_runs instead?
Actually no.
What I'd want to do is that the functions which call these result calculator functions would specify the number of tests they passed into the result calculator. It seems safer because the results are read back from a file which could have changed (or got deleted due to an ipc bug prematurely cleaning up the file) and would better take account those cases where the first value is skipped when reading the results.
I think I'll drop this patch for now.
Hi Ilpo,
On 2/14/2023 2:00 AM, Ilpo Järvinen wrote:
On Mon, 13 Feb 2023, Reinette Chatre wrote:
On 2/8/2023 1:40 AM, Ilpo Järvinen wrote:
From: Fenghua Yu fenghua.yu@intel.com
Presently, while calculating MBM results, the divisor is hard coded as 4.
"Presently" can be removed. Here and in the rest of the series the usage of "presently" and "currently" can usually be removed to improve clarity.
It's hard coded to 4 because "NUM_OF_RUNS" is defined as 5 and the test does not count first result and hence 4. So, instead of hard coding the value to 4, change it to NUM_OF_RUNS - 1.
Are there any plans surrounding using struct resctrl_val_param::num_of_runs instead?
Actually no.
What I'd want to do is that the functions which call these result calculator functions would specify the number of tests they passed into the result calculator. It seems safer because the results are read
Would it not simplify things to pass the test parameters (struct resctrl_val_param) to the result calculator? That contains the number of tests run and would reign in the hard coding.
back from a file which could have changed (or got deleted due to an ipc bug prematurely cleaning up the file) and would better take account those cases where the first value is skipped when reading the results.
This sounds good. Thank you.
Reinette
From: "Signed-off-by: Fenghua Yu" fenghua.yu@intel.com
CBM_MASK_PATH is actually the path to resctrl/info, so change the macro name to correctly indicate what it represents.
Signed-off-by: Fenghua Yu fenghua.yu@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 f0ded31fb3c7..4f0976f12634 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) {
On Wed, 8 Feb 2023, Ilpo Järvinen wrote:
From: "Signed-off-by: Fenghua Yu" fenghua.yu@intel.com
I seem to have managed to do a little copy paste error there. I can resubmit the series if needed.
Hi Ilpo,
On 2/8/2023 6:04 AM, Ilpo Järvinen wrote:
On Wed, 8 Feb 2023, Ilpo Järvinen wrote:
From: "Signed-off-by: Fenghua Yu" fenghua.yu@intel.com
I seem to have managed to do a little copy paste error there. I can resubmit the series if needed.
Yes please.
-- i.
CBM_MASK_PATH is actually the path to resctrl/info, so change the macro name to correctly indicate what it represents.
Signed-off-by: Fenghua Yu fenghua.yu@intel.com
Missing your Signed-off-by?
Reinette
remount_resctrlfs() accepts a boolean value as an argument and presently, some tests pass 0/1 and some tests pass true/false. Make all the callers of remount_resctrlfs() use true/false so that it's 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 1c5e90c63254..265302094095 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 8968e36db99d..4903e5eb04a1 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 1a1bdb6180cf..7c2a5668e215 100644 --- a/tools/testing/selftests/resctrl/mba_test.c +++ b/tools/testing/selftests/resctrl/mba_test.c @@ -149,7 +149,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 4f85cbbfd037..b7f53c000252 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 4f0976f12634..2bd593d7661f 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -62,7 +62,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;
linux-kselftest-mirror@lists.linaro.org
-
Ilpo Järvinen -
Reinette Chatre