| Message ID | 26086dda86f062bba4116878a012a553503924b2.1583657204.git.sai.praneeth.prakhya@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Miscellaneous fixes for resctrl selftests | expand |
Hi Sai, On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > The present CQM test runs fill_buff continuously with some user specified Should this not be referred to as Cache Monitoring Technology (CMT) instead? There is a significant usage of "cqm" throughout this series of tests and creates confusion with it not being the accurate acronym for it as is currently done with cat, mbm, and mba. From what I can tell AMD does not refer to it as CQM either. I understand the resctrl code uses cqm internally, that may be for historical reasons, but that usage seems to be limited to the code itself and not leaking to the user as done here. > buffer size and reads cqm_llc_occupancy every 1 second and tests if resctrl > reported value is in 15% range of buffer that fill_buff is working on. If > the difference is greater than 15% the test fails. This test assumes that > the buffer fill_buff is working on will be identity mapped into cache from > memory i.e. there won't be any overlap. But that might not always be true > because of the way cache indexing works (two physical addresses could get > indexed into the same cache line). If this happens, cqm_llc_occupancy will > be less than buffer size and we cannot guarantee the percentage by which > this might be less. Another issue with the test case is that, although it > has 15% of guard band, the cache occupied by code (or other parts) of the > process may not be within this range. While we are actively looking into > approximating llc_occupancy through perf, fix this test case with the help > of CAT. > > The new CQM test runs fill_buff continuously with a buffer size that is > much greater than cache size and uses CAT to change schemata (from 1 bit to > max_bits available without shareable bits). For every change in schemata, > it then averages cqm_llc_occupancy and checks if it is less than allocated > cache size (with 5% guard band). If the average cqm_llc_occupancy is less > than allocated cache size, the test passes. Please note that there is no > lower bound on the expected cqm_llc_occupancy because presently that cannot > be determined. > > Note: > The new test case assumes that > 1. The system supports CAT > 2. CAT is working as expected on the system > > Reported-by: Reinette Chatre <reinette.chatre@intel.com> > Suggested-by: Tony Luck <tony.luck@intel.com> > Co-developed-by: Fenghua Yu <fenghua.yu@intel.com> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> > --- > tools/testing/selftests/resctrl/cache.c | 4 + > tools/testing/selftests/resctrl/cqm_test.c | 203 +++++++++++++++--------- > tools/testing/selftests/resctrl/resctrl.h | 3 +- > tools/testing/selftests/resctrl/resctrl_tests.c | 6 +- > tools/testing/selftests/resctrl/resctrl_val.c | 22 +-- > tools/testing/selftests/resctrl/resctrlfs.c | 6 +- > 6 files changed, 143 insertions(+), 101 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c > index e30cdd7b851c..ca794ad6fcfc 100644 > --- a/tools/testing/selftests/resctrl/cache.c > +++ b/tools/testing/selftests/resctrl/cache.c > @@ -224,11 +224,15 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid) > > /* Measure llc occupancy from resctrl */ > if (!strcmp(param->resctrl_val, "cqm")) { > + /* Sleep for a second so that benchmark gets to run */ > + sleep(1); > + > ret = get_llc_occu_resctrl(&llc_occu_resc); > if (ret < 0) > return ret; > llc_value = llc_occu_resc; > } > + Extra newline here > ret = print_results_cache(param->filename, bm_pid, llc_value); > if (ret) > return ret; > diff --git a/tools/testing/selftests/resctrl/cqm_test.c b/tools/testing/selftests/resctrl/cqm_test.c > index f27b0363e518..3406c04ff110 100644 > --- a/tools/testing/selftests/resctrl/cqm_test.c > +++ b/tools/testing/selftests/resctrl/cqm_test.c > @@ -13,73 +13,74 @@ > > #define RESULT_FILE_NAME "result_cqm" > #define NUM_OF_RUNS 5 > -#define MAX_DIFF 2000000 > -#define MAX_DIFF_PERCENT 15 > +#define MAX_DIFF_PERCENT 5 > > -int count_of_bits; > char cbm_mask[256]; > -unsigned long long_mask; > -unsigned long cache_size; > > -static int cqm_setup(struct resctrl_val_param *p) > -{ > - /* Run NUM_OF_RUNS times */ > - if (p->num_of_runs >= NUM_OF_RUNS) > - return -1; > - > - p->num_of_runs++; > - > - return 0; > -} > +static int count_of_bits, count_of_shareable_bits; > +static unsigned long cache_size, shareable_mask; > > -static void show_cache_info(unsigned long sum_llc_occu_resc, int no_of_bits, > - unsigned long span) > +static void show_cache_info(unsigned long *llc_occu_resc) > { > - unsigned long avg_llc_occu_resc = 0; > - float diff_percent; > - long avg_diff = 0; > - bool res; > + int bits, runs; > + bool failed = false; > > - avg_llc_occu_resc = sum_llc_occu_resc / (NUM_OF_RUNS - 1); > - avg_diff = (long)abs(span - avg_llc_occu_resc); > - > - diff_percent = (((float)span - avg_llc_occu_resc) / span) * 100; > + printf("# Results are displayed in (Bytes)\n"); Why are the parentheses used in "(Bytes)"? Printing the numbers in parentheses does not add value ... but it does not seem to be done either. Perhaps just "Results are displayed in bytes"? > > - if ((abs((int)diff_percent) <= MAX_DIFF_PERCENT) || > - (abs(avg_diff) <= MAX_DIFF)) > - res = true; > - else > - res = false; > + for (bits = 0; bits < count_of_bits - count_of_shareable_bits; bits++) { > + unsigned long avg_llc_occu_resc, sum_llc_occu_resc = 0; > + unsigned long alloc_llc, mask = 0; > + int llc_occu_diff, i; > + > + /* > + * The first run is discarded due to inaccurate value from > + * phase transition. > + */ > + for (runs = NUM_OF_RUNS * bits + 1; > + runs < NUM_OF_RUNS * bits + NUM_OF_RUNS; runs++) > + sum_llc_occu_resc += llc_occu_resc[runs]; > + > + avg_llc_occu_resc = sum_llc_occu_resc / (NUM_OF_RUNS - 1); > + alloc_llc = cache_size * ((float)(bits + 1) / count_of_bits); > + llc_occu_diff = avg_llc_occu_resc - alloc_llc; > + > + for (i = 0; i < bits + 1; i++) { > + mask <<= 1; > + mask |= 1; > + } > > - printf("%sok CQM: diff within %d, %d\%%\n", res ? "" : "not ", > - MAX_DIFF, (int)MAX_DIFF_PERCENT); > + if (llc_occu_diff > 0 && > + llc_occu_diff > alloc_llc * ((float)MAX_DIFF_PERCENT / 100)) > + failed = true; > > - printf("# diff: %ld\n", avg_diff); > - printf("# percent diff=%d\n", abs((int)diff_percent)); > - printf("# Results are displayed in (Bytes)\n"); > - printf("# Number of bits: %d\n", no_of_bits); > - printf("# Avg_llc_occu_resc: %lu\n", avg_llc_occu_resc); > - printf("# llc_occu_exp (span): %lu\n", span); > + printf("%sok CQM: diff within %d%% for mask %lx\n", > + failed ? "not " : "", MAX_DIFF_PERCENT, mask); > + printf("# alloc_llc_cache_size: %lu\n", alloc_llc); > + printf("# avg_llc_occu_resc: %lu\n", avg_llc_occu_resc); > + tests_run++; > + } > > + printf("%sok schemata change for CQM%s\n", failed ? "not " : "", > + failed ? " # at least one test failed" : ""); > tests_run++; > } > > -static int check_results(struct resctrl_val_param *param, int no_of_bits) > +static int check_results(void) > { > - char *token_array[8], temp[512]; > - unsigned long sum_llc_occu_resc = 0; > - int runs = 0; > + char *token_array[8], output[] = RESULT_FILE_NAME, temp[512]; > + unsigned long llc_occu_resc[count_of_bits * NUM_OF_RUNS]; > + int runs; > FILE *fp; > > - printf("# checking for pass/fail\n"); > - fp = fopen(param->filename, "r"); > + fp = fopen(output, "r"); > if (!fp) { > - perror("# Error in opening file\n"); > + perror(output); > > return errno; > } > > - while (fgets(temp, 1024, fp)) { > + runs = 0; > + while (fgets(temp, sizeof(temp), fp)) { > char *token = strtok(temp, ":\t"); > int fields = 0; > > @@ -88,13 +89,14 @@ static int check_results(struct resctrl_val_param *param, int no_of_bits) > token = strtok(NULL, ":\t"); > } > > - /* Field 3 is llc occ resc value */ > - if (runs > 0) > - sum_llc_occu_resc += strtoul(token_array[3], NULL, 0); > + /* Field 3 is resctrl LLC occupancy value */ > + llc_occu_resc[runs] = strtoul(token_array[3], NULL, 0); > runs++; > } > + > fclose(fp); > - show_cache_info(sum_llc_occu_resc, no_of_bits, param->span); > + > + show_cache_info(llc_occu_resc); > > return 0; > } > @@ -104,62 +106,107 @@ void cqm_test_cleanup(void) > remove(RESULT_FILE_NAME); > } > > -int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > +/* > + * Change schemata from 1 to count_of_bits - 1. Write schemata to specified > + * con_mon grp, mon_grp in resctrl FS. For each allocation, run "NUM_OF_RUNS" > + * times to get average values. > + */ > +static int cqm_setup(struct resctrl_val_param *p) > { > - int ret, mum_resctrlfs; > + static int runs_per_allocation = 0, num_of_bits = 1; > + unsigned long mask = 0; > + char schemata[64]; > + int i, ret; > > - cache_size = 0; > - mum_resctrlfs = 1; > + if (runs_per_allocation >= NUM_OF_RUNS) > + runs_per_allocation = 0; > > - ret = remount_resctrlfs(mum_resctrlfs); > - if (ret) > - return ret; > + /* Only set up schemata once every NUM_OF_RUNS of allocations */ > + if (runs_per_allocation++ != 0) > + return 0; > > - if (!validate_resctrl_feature_request("cqm")) > + if (num_of_bits > count_of_bits - count_of_shareable_bits) > return -1; > > - ret = get_cbm_mask("L3"); > - if (ret) > - return ret; > - > - long_mask = strtoul(cbm_mask, NULL, 16); > + /* Prepare cbm mask without any shareable bits */ > + for (i = 0; i < num_of_bits; i++) { > + mask <<= 1; > + mask |= 1; > + } > + mask = ~shareable_mask & mask; If I understand correctly this function assumes that the shareable bits will also be the high order bits of the schemata. I do not believe that this is part of a spec. It also does not seem as though the code follows what the comment at the top of the function states. The comment states "Change schemata from 1 to count_of_bits - 1" while the code seems to change schemata from 1 to count_of_bits - count_of_shareable_bits ... > > - ret = get_cache_size(cpu_no, "L3", &cache_size); > + sprintf(schemata, "%lx", mask); > + ret = write_schemata(p->ctrlgrp, schemata, p->cpu_no, "cat"); > if (ret) > return ret; > - printf("cache size :%lu\n", cache_size); > > - count_of_bits = count_bits(long_mask); > + p->mask = mask; > + num_of_bits++; > > - if (n < 1 || n > count_of_bits) { > - printf("Invalid input value for numbr_of_bits n!\n"); > - printf("Please Enter value in range 1 to %d\n", count_of_bits); > - return -1; > - } > + return 0; > +} > > +int cqm_schemata_change(int cpu_no, int span, char *cache_type, > + char **benchmark_cmd) > +{ > struct resctrl_val_param param = { > .resctrl_val = "cqm", > .ctrlgrp = "c1", > .mongrp = "m1", > .cpu_no = cpu_no, > + .span = span, This function received the new function parameter "span" to be used here ... I am having trouble finding where this member is used within this test. Could you please help me navigate to this? > .mum_resctrlfs = 0, > .filename = RESULT_FILE_NAME, > - .mask = ~(long_mask << n) & long_mask, > - .span = cache_size * n / count_of_bits, > .num_of_runs = 0, > - .setup = cqm_setup, > + .setup = cqm_setup > }; > + int ret; > + char schemata[64]; > + unsigned long long_mask; > > - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) > - sprintf(benchmark_cmd[1], "%lu", param.span); > + ret = remount_resctrlfs(1); > + if (ret) > + return ret; Here resctrl is remounted and followed by some changes to the root group's schemata. That is followed by a call to resctrl_val that attempts to remount resctrl again that will undo all the configurations inbetween. > > - remove(RESULT_FILE_NAME); > + /* Check for both 'cat' and 'cqm' because CQM is validated using CAT */ > + if (!validate_resctrl_feature_request("cqm")) > + return -1; > + > + if (!validate_resctrl_feature_request("cat")) > + return -1; > + > + ret = get_cache_size(cpu_no, cache_type, &cache_size); > + if (ret) > + return ret; > + printf("# cache size: %lu\n", cache_size); > + > + ret = get_cbm_mask(cache_type); > + if (ret) > + return ret; > + > + long_mask = strtoul(cbm_mask, NULL, 16); > + count_of_bits = count_bits(long_mask); > + > + /* > + * Change root con_mon grp schemata to minimum (i.e. '1' bit) so that > + * test could use all other bits > + */ > + sprintf(schemata, "%x", 1); > + ret = write_schemata("", schemata, cpu_no, "cqm"); > + if (ret) > + return ret; ... here the schemata is written to resctrl > + > + ret = get_shareable_mask(cache_type, &shareable_mask); > + if (ret) > + return ret; > + > + count_of_shareable_bits = count_bits(shareable_mask); > > ret = resctrl_val(benchmark_cmd, ¶m); > if (ret) > return ret; here is the call to resctrl_val() that attempts to remount resctrl. > > - ret = check_results(¶m, n); > + ret = check_results(); > if (ret) > return ret; > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index 79148cbbd7a4..cb67ad689475 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -103,7 +103,8 @@ int setup_critical_process(pid_t pid, struct resctrl_val_param *param); > int run_critical_process(pid_t pid, struct resctrl_val_param *param); > void cat_test_cleanup(void); > int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type); > -int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd); > +int cqm_schemata_change(int cpu_no, int span, char *cache_type, > + char **benchmark_cmd); > unsigned int count_bits(unsigned long n); > void cqm_test_cleanup(void); > int get_core_sibling(int cpu_no); > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index 60db128312a6..3c408c636b6d 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -180,9 +180,11 @@ int main(int argc, char **argv) > > if (cqm_test) { > printf("# Starting CQM test ...\n"); > - if (!has_ben) > + if (!has_ben) { > + sprintf(benchmark_cmd[1], "%d", span); > sprintf(benchmark_cmd[5], "%s", "cqm"); > - res = cqm_resctrl_val(cpu_no, no_of_bits, benchmark_cmd); > + } > + res = cqm_schemata_change(cpu_no, span, "L3", benchmark_cmd); > printf("%sok CQM: test\n", res ? "not " : ""); > cqm_test_cleanup(); > tests_run++; > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 271cb5c976f5..c59fad6cb9b0 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -705,29 +705,21 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) > goto out; > } > > - /* Give benchmark enough time to fully run */ > - sleep(1); > - > /* Test runs until the callback setup() tells the test to stop. */ > while (1) { > + ret = param->setup(param); > + if (ret) { > + ret = 0; > + break; > + } > + > + /* Measure vals sleeps for a second */ > if ((strcmp(resctrl_val, "mbm") == 0) || > (strcmp(resctrl_val, "mba") == 0)) { > - ret = param->setup(param); > - if (ret) { > - ret = 0; > - break; > - } > - > ret = measure_vals(param, &bw_resc_start); > if (ret) > break; > } else if (strcmp(resctrl_val, "cqm") == 0) { > - ret = param->setup(param); > - if (ret) { > - ret = 0; > - break; > - } > - sleep(1); > ret = measure_cache_vals(param, bm_pid); > if (ret) > break; This change affects not just the cache monitoring test. Could this change be extracted into its own patch to be clear what is done here and how it impacts the other tests? > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > index 52452bb0178a..bd81a13ff9df 100644 > --- a/tools/testing/selftests/resctrl/resctrlfs.c > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > @@ -365,11 +365,7 @@ void run_benchmark(int signum, siginfo_t *info, void *ucontext) > memflush = atoi(benchmark_cmd[3]); > operation = atoi(benchmark_cmd[4]); > sprintf(resctrl_val, "%s", benchmark_cmd[5]); > - > - if (strcmp(resctrl_val, "cqm") != 0) > - buffer_span = span * MB; > - else > - buffer_span = span; > + buffer_span = span * MB; This change seems to change the buffer_span used by the other tests. It is not obvious why this change is made to other tests while this commit intends to focus on the cache monitoring test. Perhaps this can be split into a separate patch to make this clear? > > if (run_fill_buf(buffer_span, malloc_and_init_memory, memflush, > operation, resctrl_val)) > Reinette
Hi Reinette, On Tue, 2020-03-10 at 15:18 -0700, Reinette Chatre wrote: > Hi Sai, > > On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > > The present CQM test runs fill_buff continuously with some user specified > > Should this not be referred to as Cache Monitoring Technology (CMT) > instead? There is a significant usage of "cqm" throughout this series of > tests and creates confusion with it not being the accurate acronym for > it as is currently done with cat, mbm, and mba. From what I can tell AMD > does not refer to it as CQM either. > > I understand the resctrl code uses cqm internally, that may be for > historical reasons, but that usage seems to be limited to the code > itself and not leaking to the user as done here. Ok.. makes sense. CQM is just how I was introduced to the feature and it sticked along with me. I will change it to CMT. > > buffer size and reads cqm_llc_occupancy every 1 second and tests if > > resctrl > > reported value is in 15% range of buffer that fill_buff is working on. If > > the difference is greater than 15% the test fails. This test assumes that > > the buffer fill_buff is working on will be identity mapped into cache from > > memory i.e. there won't be any overlap. But that might not always be true > > because of the way cache indexing works (two physical addresses could get > > indexed into the same cache line). If this happens, cqm_llc_occupancy will > > be less than buffer size and we cannot guarantee the percentage by which > > this might be less. Another issue with the test case is that, although it > > has 15% of guard band, the cache occupied by code (or other parts) of the > > process may not be within this range. While we are actively looking into > > approximating llc_occupancy through perf, fix this test case with the help > > of CAT. > > > > The new CQM test runs fill_buff continuously with a buffer size that is > > much greater than cache size and uses CAT to change schemata (from 1 bit > > to > > max_bits available without shareable bits). For every change in schemata, > > it then averages cqm_llc_occupancy and checks if it is less than allocated > > cache size (with 5% guard band). If the average cqm_llc_occupancy is less > > than allocated cache size, the test passes. Please note that there is no > > lower bound on the expected cqm_llc_occupancy because presently that > > cannot > > be determined. > > > > Note: > > The new test case assumes that > > 1. The system supports CAT > > 2. CAT is working as expected on the system > > > > Reported-by: Reinette Chatre <reinette.chatre@intel.com> > > Suggested-by: Tony Luck <tony.luck@intel.com> > > Co-developed-by: Fenghua Yu <fenghua.yu@intel.com> > > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com> > > Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com> > > --- > > tools/testing/selftests/resctrl/cache.c | 4 + > > tools/testing/selftests/resctrl/cqm_test.c | 203 +++++++++++++++-- > > ------- > > tools/testing/selftests/resctrl/resctrl.h | 3 +- > > tools/testing/selftests/resctrl/resctrl_tests.c | 6 +- > > tools/testing/selftests/resctrl/resctrl_val.c | 22 +-- > > tools/testing/selftests/resctrl/resctrlfs.c | 6 +- > > 6 files changed, 143 insertions(+), 101 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/cache.c > > b/tools/testing/selftests/resctrl/cache.c > > index e30cdd7b851c..ca794ad6fcfc 100644 > > --- a/tools/testing/selftests/resctrl/cache.c > > +++ b/tools/testing/selftests/resctrl/cache.c > > @@ -224,11 +224,15 @@ int measure_cache_vals(struct resctrl_val_param > > *param, int bm_pid) > > > > /* Measure llc occupancy from resctrl */ > > if (!strcmp(param->resctrl_val, "cqm")) { > > + /* Sleep for a second so that benchmark gets to run */ > > + sleep(1); > > + > > ret = get_llc_occu_resctrl(&llc_occu_resc); > > if (ret < 0) > > return ret; > > llc_value = llc_occu_resc; > > } > > + > > Extra newline here Ok. Will remove it. > > ret = print_results_cache(param->filename, bm_pid, llc_value); > > if (ret) > > return ret; > > diff --git a/tools/testing/selftests/resctrl/cqm_test.c > > b/tools/testing/selftests/resctrl/cqm_test.c > > index f27b0363e518..3406c04ff110 100644 > > --- a/tools/testing/selftests/resctrl/cqm_test.c > > +++ b/tools/testing/selftests/resctrl/cqm_test.c > > @@ -13,73 +13,74 @@ > > > > #define RESULT_FILE_NAME "result_cqm" > > #define NUM_OF_RUNS 5 > > -#define MAX_DIFF 2000000 > > -#define MAX_DIFF_PERCENT 15 > > +#define MAX_DIFF_PERCENT 5 > > > > -int count_of_bits; > > char cbm_mask[256]; > > -unsigned long long_mask; > > -unsigned long cache_size; > > > > -static int cqm_setup(struct resctrl_val_param *p) > > -{ > > - /* Run NUM_OF_RUNS times */ > > - if (p->num_of_runs >= NUM_OF_RUNS) > > - return -1; > > - > > - p->num_of_runs++; > > - > > - return 0; > > -} > > +static int count_of_bits, count_of_shareable_bits; > > +static unsigned long cache_size, shareable_mask; > > > > -static void show_cache_info(unsigned long sum_llc_occu_resc, int > > no_of_bits, > > - unsigned long span) > > +static void show_cache_info(unsigned long *llc_occu_resc) > > { > > - unsigned long avg_llc_occu_resc = 0; > > - float diff_percent; > > - long avg_diff = 0; > > - bool res; > > + int bits, runs; > > + bool failed = false; > > > > - avg_llc_occu_resc = sum_llc_occu_resc / (NUM_OF_RUNS - 1); > > - avg_diff = (long)abs(span - avg_llc_occu_resc); > > - > > - diff_percent = (((float)span - avg_llc_occu_resc) / span) * 100; > > + printf("# Results are displayed in (Bytes)\n"); > > Why are the parentheses used in "(Bytes)"? Printing the numbers in > parentheses does not add value ... but it does not seem to be done > either. Perhaps just "Results are displayed in bytes"? Good catch. Will fix it. > > > > - if ((abs((int)diff_percent) <= MAX_DIFF_PERCENT) || > > - (abs(avg_diff) <= MAX_DIFF)) > > - res = true; > > - else > > - res = false; > > + for (bits = 0; bits < count_of_bits - count_of_shareable_bits; bits++) > > { > > + unsigned long avg_llc_occu_resc, sum_llc_occu_resc = 0; > > + unsigned long alloc_llc, mask = 0; > > + int llc_occu_diff, i; > > + > > + /* > > + * The first run is discarded due to inaccurate value from > > + * phase transition. > > + */ > > + for (runs = NUM_OF_RUNS * bits + 1; > > + runs < NUM_OF_RUNS * bits + NUM_OF_RUNS; runs++) > > + sum_llc_occu_resc += llc_occu_resc[runs]; > > + > > + avg_llc_occu_resc = sum_llc_occu_resc / (NUM_OF_RUNS - 1); > > + alloc_llc = cache_size * ((float)(bits + 1) / count_of_bits); > > + llc_occu_diff = avg_llc_occu_resc - alloc_llc; > > + > > + for (i = 0; i < bits + 1; i++) { > > + mask <<= 1; > > + mask |= 1; > > + } > > > > - printf("%sok CQM: diff within %d, %d\%%\n", res ? "" : "not ", > > - MAX_DIFF, (int)MAX_DIFF_PERCENT); > > + if (llc_occu_diff > 0 && > > + llc_occu_diff > alloc_llc * ((float)MAX_DIFF_PERCENT / > > 100)) > > + failed = true; > > > > - printf("# diff: %ld\n", avg_diff); > > - printf("# percent diff=%d\n", abs((int)diff_percent)); > > - printf("# Results are displayed in (Bytes)\n"); > > - printf("# Number of bits: %d\n", no_of_bits); > > - printf("# Avg_llc_occu_resc: %lu\n", avg_llc_occu_resc); > > - printf("# llc_occu_exp (span): %lu\n", span); > > + printf("%sok CQM: diff within %d%% for mask %lx\n", > > + failed ? "not " : "", MAX_DIFF_PERCENT, mask); > > + printf("# alloc_llc_cache_size: %lu\n", alloc_llc); > > + printf("# avg_llc_occu_resc: %lu\n", avg_llc_occu_resc); > > + tests_run++; > > + } > > > > + printf("%sok schemata change for CQM%s\n", failed ? "not " : "", > > + failed ? " # at least one test failed" : ""); > > tests_run++; > > } > > > > -static int check_results(struct resctrl_val_param *param, int no_of_bits) > > +static int check_results(void) > > { > > - char *token_array[8], temp[512]; > > - unsigned long sum_llc_occu_resc = 0; > > - int runs = 0; > > + char *token_array[8], output[] = RESULT_FILE_NAME, temp[512]; > > + unsigned long llc_occu_resc[count_of_bits * NUM_OF_RUNS]; > > + int runs; > > FILE *fp; > > > > - printf("# checking for pass/fail\n"); > > - fp = fopen(param->filename, "r"); > > + fp = fopen(output, "r"); > > if (!fp) { > > - perror("# Error in opening file\n"); > > + perror(output); > > > > return errno; > > } > > > > - while (fgets(temp, 1024, fp)) { > > + runs = 0; > > + while (fgets(temp, sizeof(temp), fp)) { > > char *token = strtok(temp, ":\t"); > > int fields = 0; > > > > @@ -88,13 +89,14 @@ static int check_results(struct resctrl_val_param > > *param, int no_of_bits) > > token = strtok(NULL, ":\t"); > > } > > > > - /* Field 3 is llc occ resc value */ > > - if (runs > 0) > > - sum_llc_occu_resc += strtoul(token_array[3], NULL, 0); > > + /* Field 3 is resctrl LLC occupancy value */ > > + llc_occu_resc[runs] = strtoul(token_array[3], NULL, 0); > > runs++; > > } > > + > > fclose(fp); > > - show_cache_info(sum_llc_occu_resc, no_of_bits, param->span); > > + > > + show_cache_info(llc_occu_resc); > > > > return 0; > > } > > @@ -104,62 +106,107 @@ void cqm_test_cleanup(void) > > remove(RESULT_FILE_NAME); > > } > > > > -int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > > +/* > > + * Change schemata from 1 to count_of_bits - 1. Write schemata to > > specified > > + * con_mon grp, mon_grp in resctrl FS. For each allocation, run > > "NUM_OF_RUNS" > > + * times to get average values. > > + */ > > +static int cqm_setup(struct resctrl_val_param *p) > > { > > - int ret, mum_resctrlfs; > > + static int runs_per_allocation = 0, num_of_bits = 1; > > + unsigned long mask = 0; > > + char schemata[64]; > > + int i, ret; > > > > - cache_size = 0; > > - mum_resctrlfs = 1; > > + if (runs_per_allocation >= NUM_OF_RUNS) > > + runs_per_allocation = 0; > > > > - ret = remount_resctrlfs(mum_resctrlfs); > > - if (ret) > > - return ret; > > + /* Only set up schemata once every NUM_OF_RUNS of allocations */ > > + if (runs_per_allocation++ != 0) > > + return 0; > > > > - if (!validate_resctrl_feature_request("cqm")) > > + if (num_of_bits > count_of_bits - count_of_shareable_bits) > > return -1; > > > > - ret = get_cbm_mask("L3"); > > - if (ret) > > - return ret; > > - > > - long_mask = strtoul(cbm_mask, NULL, 16); > > + /* Prepare cbm mask without any shareable bits */ > > + for (i = 0; i < num_of_bits; i++) { > > + mask <<= 1; > > + mask |= 1; > > + } > > + mask = ~shareable_mask & mask; > > If I understand correctly this function assumes that the shareable bits > will also be the high order bits of the schemata. I do not believe that > this is part of a spec. Yes, that's right. Will fix it. > It also does not seem as though the code follows > what the comment at the top of the function states. The comment states > "Change schemata from 1 to count_of_bits - 1" while the code seems to > change schemata from 1 to count_of_bits - count_of_shareable_bits ... My bad! Will fix the comment. > > > > - ret = get_cache_size(cpu_no, "L3", &cache_size); > > + sprintf(schemata, "%lx", mask); > > + ret = write_schemata(p->ctrlgrp, schemata, p->cpu_no, "cat"); > > if (ret) > > return ret; > > - printf("cache size :%lu\n", cache_size); > > > > - count_of_bits = count_bits(long_mask); > > + p->mask = mask; > > + num_of_bits++; > > > > - if (n < 1 || n > count_of_bits) { > > - printf("Invalid input value for numbr_of_bits n!\n"); > > - printf("Please Enter value in range 1 to %d\n", > > count_of_bits); > > - return -1; > > - } > > + return 0; > > +} > > > > +int cqm_schemata_change(int cpu_no, int span, char *cache_type, > > + char **benchmark_cmd) > > +{ > > struct resctrl_val_param param = { > > .resctrl_val = "cqm", > > .ctrlgrp = "c1", > > .mongrp = "m1", > > .cpu_no = cpu_no, > > + .span = span, > > This function received the new function parameter "span" to be used here > ... I am having trouble finding where this member is used within this > test. Could you please help me navigate to this? My bad. Will fix it. > > .mum_resctrlfs = 0, > > .filename = RESULT_FILE_NAME, > > - .mask = ~(long_mask << n) & long_mask, > > - .span = cache_size * n / count_of_bits, > > .num_of_runs = 0, > > - .setup = cqm_setup, > > + .setup = cqm_setup > > }; > > + int ret; > > + char schemata[64]; > > + unsigned long long_mask; > > > > - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) > > - sprintf(benchmark_cmd[1], "%lu", param.span); > > + ret = remount_resctrlfs(1); > > + if (ret) > > + return ret; > > Here resctrl is remounted and followed by some changes to the root > group's schemata. That is followed by a call to resctrl_val that > attempts to remount resctrl again that will undo all the configurations > inbetween. No, it wouldn't because mum_resctrlfs is 0. When resctrl FS is already mounted and mum_resctrlfs is 0, then remount_resctrlfs() is a noop. > > > > - remove(RESULT_FILE_NAME); > > + /* Check for both 'cat' and 'cqm' because CQM is validated using CAT > > */ > > + if (!validate_resctrl_feature_request("cqm")) > > + return -1; > > + > > + if (!validate_resctrl_feature_request("cat")) > > + return -1; > > + > > + ret = get_cache_size(cpu_no, cache_type, &cache_size); > > + if (ret) > > + return ret; > > + printf("# cache size: %lu\n", cache_size); > > + > > + ret = get_cbm_mask(cache_type); > > + if (ret) > > + return ret; > > + > > + long_mask = strtoul(cbm_mask, NULL, 16); > > + count_of_bits = count_bits(long_mask); > > + > > + /* > > + * Change root con_mon grp schemata to minimum (i.e. '1' bit) so that > > + * test could use all other bits > > + */ > > + sprintf(schemata, "%x", 1); > > + ret = write_schemata("", schemata, cpu_no, "cqm"); > > + if (ret) > > + return ret; > > ... here the schemata is written to resctrl > > > + > > + ret = get_shareable_mask(cache_type, &shareable_mask); > > + if (ret) > > + return ret; > > + > > + count_of_shareable_bits = count_bits(shareable_mask); > > > > ret = resctrl_val(benchmark_cmd, ¶m); > > if (ret) > > return ret; > > here is the call to resctrl_val() that attempts to remount resctrl. > > > > > - ret = check_results(¶m, n); > > + ret = check_results(); > > if (ret) > > return ret; > > > > diff --git a/tools/testing/selftests/resctrl/resctrl.h > > b/tools/testing/selftests/resctrl/resctrl.h > > index 79148cbbd7a4..cb67ad689475 100644 > > --- a/tools/testing/selftests/resctrl/resctrl.h > > +++ b/tools/testing/selftests/resctrl/resctrl.h > > @@ -103,7 +103,8 @@ int setup_critical_process(pid_t pid, struct > > resctrl_val_param *param); > > int run_critical_process(pid_t pid, struct resctrl_val_param *param); > > void cat_test_cleanup(void); > > int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type); > > -int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd); > > +int cqm_schemata_change(int cpu_no, int span, char *cache_type, > > + char **benchmark_cmd); > > unsigned int count_bits(unsigned long n); > > void cqm_test_cleanup(void); > > int get_core_sibling(int cpu_no); > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c > > b/tools/testing/selftests/resctrl/resctrl_tests.c > > index 60db128312a6..3c408c636b6d 100644 > > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > > @@ -180,9 +180,11 @@ int main(int argc, char **argv) > > > > if (cqm_test) { > > printf("# Starting CQM test ...\n"); > > - if (!has_ben) > > + if (!has_ben) { > > + sprintf(benchmark_cmd[1], "%d", span); > > sprintf(benchmark_cmd[5], "%s", "cqm"); > > - res = cqm_resctrl_val(cpu_no, no_of_bits, benchmark_cmd); > > + } > > + res = cqm_schemata_change(cpu_no, span, "L3", benchmark_cmd); > > printf("%sok CQM: test\n", res ? "not " : ""); > > cqm_test_cleanup(); > > tests_run++; > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c > > b/tools/testing/selftests/resctrl/resctrl_val.c > > index 271cb5c976f5..c59fad6cb9b0 100644 > > --- a/tools/testing/selftests/resctrl/resctrl_val.c > > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > > @@ -705,29 +705,21 @@ int resctrl_val(char **benchmark_cmd, struct > > resctrl_val_param *param) > > goto out; > > } > > > > - /* Give benchmark enough time to fully run */ > > - sleep(1); > > - > > /* Test runs until the callback setup() tells the test to stop. */ > > while (1) { > > + ret = param->setup(param); > > + if (ret) { > > + ret = 0; > > + break; > > + } > > + > > + /* Measure vals sleeps for a second */ > > if ((strcmp(resctrl_val, "mbm") == 0) || > > (strcmp(resctrl_val, "mba") == 0)) { > > - ret = param->setup(param); > > - if (ret) { > > - ret = 0; > > - break; > > - } > > - > > ret = measure_vals(param, &bw_resc_start); > > if (ret) > > break; > > } else if (strcmp(resctrl_val, "cqm") == 0) { > > - ret = param->setup(param); > > - if (ret) { > > - ret = 0; > > - break; > > - } > > - sleep(1); > > ret = measure_cache_vals(param, bm_pid); > > if (ret) > > break; > > This change affects not just the cache monitoring test. Could this > change be extracted into its own patch to be clear what is done here and > how it impacts the other tests? This change shouldn't impact other tests (i.e. CAT) because CAT will not call resctrl_val(). > > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c > > b/tools/testing/selftests/resctrl/resctrlfs.c > > index 52452bb0178a..bd81a13ff9df 100644 > > --- a/tools/testing/selftests/resctrl/resctrlfs.c > > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > > @@ -365,11 +365,7 @@ void run_benchmark(int signum, siginfo_t *info, void > > *ucontext) > > memflush = atoi(benchmark_cmd[3]); > > operation = atoi(benchmark_cmd[4]); > > sprintf(resctrl_val, "%s", benchmark_cmd[5]); > > - > > - if (strcmp(resctrl_val, "cqm") != 0) > > - buffer_span = span * MB; > > - else > > - buffer_span = span; > > + buffer_span = span * MB; > > This change seems to change the buffer_span used by the other tests. It > is not obvious why this change is made to other tests while this commit > intends to focus on the cache monitoring test. Perhaps this can be split > into a separate patch to make this clear? The change here is that we don't need to check for test case. I think for previous CQM test, it was directly passing "bytes" and hence span was not being multiplied by MB. But for the new CQM test case, span has to be multiplied and hence is same as MBM/MBA and hence the change, don't check for test case (and please note that CAT test doesn't get here, so it's only three tests). Regards, Sai
Hi Sai, On 3/10/2020 7:46 PM, Sai Praneeth Prakhya wrote: > On Tue, 2020-03-10 at 15:18 -0700, Reinette Chatre wrote: >> On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: >>> .mum_resctrlfs = 0, >>> .filename = RESULT_FILE_NAME, >>> - .mask = ~(long_mask << n) & long_mask, >>> - .span = cache_size * n / count_of_bits, >>> .num_of_runs = 0, >>> - .setup = cqm_setup, >>> + .setup = cqm_setup >>> }; >>> + int ret; >>> + char schemata[64]; >>> + unsigned long long_mask; >>> >>> - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) >>> - sprintf(benchmark_cmd[1], "%lu", param.span); >>> + ret = remount_resctrlfs(1); >>> + if (ret) >>> + return ret; >> >> Here resctrl is remounted and followed by some changes to the root >> group's schemata. That is followed by a call to resctrl_val that >> attempts to remount resctrl again that will undo all the configurations >> inbetween. > > No, it wouldn't because mum_resctrlfs is 0. When resctrl FS is already mounted > and mum_resctrlfs is 0, then remount_resctrlfs() is a noop. > I missed that. Thank you. fyi ... when I tried these tests I encountered the following error related to unmounting: [SNIP] ok Write schema "L3:1=7fff" to resctrl FS ok Write schema "L3:1=ffff" to resctrl FS ok Write schema "L3:1=1ffff" to resctrl FS ok Write schema "L3:1=3ffff" to resctrl FS # Unable to umount resctrl: Device or resource busy # Results are displayed in (Bytes) ok CQM: diff within 5% for mask 1 # alloc_llc_cache_size: 2883584 # avg_llc_occu_resc: 2973696 ok CQM: diff within 5% for mask 3 [SNIP] This seems to originate from resctrl_val() that forces an unmount but if that fails the error is not propagated. >>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c >>> b/tools/testing/selftests/resctrl/resctrl_val.c >>> index 271cb5c976f5..c59fad6cb9b0 100644 >>> --- a/tools/testing/selftests/resctrl/resctrl_val.c >>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c >>> @@ -705,29 +705,21 @@ int resctrl_val(char **benchmark_cmd, struct >>> resctrl_val_param *param) >>> goto out; >>> } >>> >>> - /* Give benchmark enough time to fully run */ >>> - sleep(1); >>> - >>> /* Test runs until the callback setup() tells the test to stop. */ >>> while (1) { >>> + ret = param->setup(param); >>> + if (ret) { >>> + ret = 0; >>> + break; >>> + } >>> + >>> + /* Measure vals sleeps for a second */ >>> if ((strcmp(resctrl_val, "mbm") == 0) || >>> (strcmp(resctrl_val, "mba") == 0)) { >>> - ret = param->setup(param); >>> - if (ret) { >>> - ret = 0; >>> - break; >>> - } >>> - (I refer to the above snippet in my comment below) >>> ret = measure_vals(param, &bw_resc_start); >>> if (ret) >>> break; >>> } else if (strcmp(resctrl_val, "cqm") == 0) { >>> - ret = param->setup(param); >>> - if (ret) { >>> - ret = 0; >>> - break; >>> - } >>> - sleep(1); >>> ret = measure_cache_vals(param, bm_pid); >>> if (ret) >>> break; >> >> This change affects not just the cache monitoring test. Could this >> change be extracted into its own patch to be clear what is done here and >> how it impacts the other tests? > > This change shouldn't impact other tests (i.e. CAT) because CAT will not call > resctrl_val(). I was referring to the snippet above that seems to impact the "mbm" and "mba" tests by moving the call to "param->setup" for the them. > >>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c >>> b/tools/testing/selftests/resctrl/resctrlfs.c >>> index 52452bb0178a..bd81a13ff9df 100644 >>> --- a/tools/testing/selftests/resctrl/resctrlfs.c >>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c >>> @@ -365,11 +365,7 @@ void run_benchmark(int signum, siginfo_t *info, void >>> *ucontext) >>> memflush = atoi(benchmark_cmd[3]); >>> operation = atoi(benchmark_cmd[4]); >>> sprintf(resctrl_val, "%s", benchmark_cmd[5]); >>> - >>> - if (strcmp(resctrl_val, "cqm") != 0) >>> - buffer_span = span * MB; >>> - else >>> - buffer_span = span; >>> + buffer_span = span * MB; >> >> This change seems to change the buffer_span used by the other tests. It >> is not obvious why this change is made to other tests while this commit >> intends to focus on the cache monitoring test. Perhaps this can be split >> into a separate patch to make this clear? > Got it. Thank you. Reinette
Hi Reinette, On Wed, 2020-03-11 at 10:19 -0700, Reinette Chatre wrote: > Hi Sai, > > On 3/10/2020 7:46 PM, Sai Praneeth Prakhya wrote: > > On Tue, 2020-03-10 at 15:18 -0700, Reinette Chatre wrote: > > > On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > > > > .mum_resctrlfs = 0, > > > > .filename = RESULT_FILE_NAME, > > > > - .mask = ~(long_mask << n) & long_mask, > > > > - .span = cache_size * n / count_of_bits, > > > > .num_of_runs = 0, > > > > - .setup = cqm_setup, > > > > + .setup = cqm_setup > > > > }; > > > > + int ret; > > > > + char schemata[64]; > > > > + unsigned long long_mask; > > > > > > > > - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) > > > > - sprintf(benchmark_cmd[1], "%lu", param.span); > > > > + ret = remount_resctrlfs(1); > > > > + if (ret) > > > > + return ret; > > > > > > Here resctrl is remounted and followed by some changes to the root > > > group's schemata. That is followed by a call to resctrl_val that > > > attempts to remount resctrl again that will undo all the configurations > > > inbetween. > > > > No, it wouldn't because mum_resctrlfs is 0. When resctrl FS is already > > mounted > > and mum_resctrlfs is 0, then remount_resctrlfs() is a noop. > > > > I missed that. Thank you. > > fyi ... when I tried these tests I encountered the following error > related to unmounting: > > [SNIP] > ok Write schema "L3:1=7fff" to resctrl FS > ok Write schema "L3:1=ffff" to resctrl FS > ok Write schema "L3:1=1ffff" to resctrl FS > ok Write schema "L3:1=3ffff" to resctrl FS > # Unable to umount resctrl: Device or resource busy > # Results are displayed in (Bytes) > ok CQM: diff within 5% for mask 1 > # alloc_llc_cache_size: 2883584 > # avg_llc_occu_resc: 2973696 > ok CQM: diff within 5% for mask 3 > [SNIP] > > This seems to originate from resctrl_val() that forces an unmount but if > that fails the error is not propagated. Yes, that's right and it's a good test. I didn't encounter this issue during my testing because I wasn't using resctrl FS from other terminals (I think you were using resctrl FS from other terminal and hence resctrl_test was unable to unmount it). I think the error should not be propagated because unmounting resctrl FS shouldn't stop us from checking the results. If measuring values reports an error then we shouldn't check for results. > > > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c > > > > b/tools/testing/selftests/resctrl/resctrl_val.c > > > > index 271cb5c976f5..c59fad6cb9b0 100644 > > > > --- a/tools/testing/selftests/resctrl/resctrl_val.c > > > > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > > > > @@ -705,29 +705,21 @@ int resctrl_val(char **benchmark_cmd, struct > > > > resctrl_val_param *param) > > > > goto out; > > > > } > > > > > > > > - /* Give benchmark enough time to fully run */ > > > > - sleep(1); > > > > - > > > > /* Test runs until the callback setup() tells the test to > > > > stop. */ > > > > while (1) { > > > > + ret = param->setup(param); > > > > + if (ret) { > > > > + ret = 0; > > > > + break; > > > > + } > > > > + > > > > + /* Measure vals sleeps for a second */ > > > > if ((strcmp(resctrl_val, "mbm") == 0) || > > > > (strcmp(resctrl_val, "mba") == 0)) { > > > > - ret = param->setup(param); > > > > - if (ret) { > > > > - ret = 0; > > > > - break; > > > > - } > > > > - > > (I refer to the above snippet in my comment below) > > > > > ret = measure_vals(param, &bw_resc_start); > > > > if (ret) > > > > break; > > > > } else if (strcmp(resctrl_val, "cqm") == 0) { > > > > - ret = param->setup(param); > > > > - if (ret) { > > > > - ret = 0; > > > > - break; > > > > - } > > > > - sleep(1); > > > > ret = measure_cache_vals(param, bm_pid); > > > > if (ret) > > > > break; > > > > > > This change affects not just the cache monitoring test. Could this > > > change be extracted into its own patch to be clear what is done here and > > > how it impacts the other tests? > > > > This change shouldn't impact other tests (i.e. CAT) because CAT will not > > call > > resctrl_val(). > > I was referring to the snippet above that seems to impact the "mbm" and > "mba" tests by moving the call to "param->setup" for the them. Ok.. makes sense. Sure! I will make it into separate patch. Regards, Sai
Hi Sai, On 3/11/2020 10:33 AM, Sai Praneeth Prakhya wrote: > On Wed, 2020-03-11 at 10:19 -0700, Reinette Chatre wrote: >> On 3/10/2020 7:46 PM, Sai Praneeth Prakhya wrote: >>> On Tue, 2020-03-10 at 15:18 -0700, Reinette Chatre wrote: >>>> On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: >>>>> .mum_resctrlfs = 0, >>>>> .filename = RESULT_FILE_NAME, >>>>> - .mask = ~(long_mask << n) & long_mask, >>>>> - .span = cache_size * n / count_of_bits, >>>>> .num_of_runs = 0, >>>>> - .setup = cqm_setup, >>>>> + .setup = cqm_setup >>>>> }; >>>>> + int ret; >>>>> + char schemata[64]; >>>>> + unsigned long long_mask; >>>>> >>>>> - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) >>>>> - sprintf(benchmark_cmd[1], "%lu", param.span); >>>>> + ret = remount_resctrlfs(1); >>>>> + if (ret) >>>>> + return ret; >>>> >>>> Here resctrl is remounted and followed by some changes to the root >>>> group's schemata. That is followed by a call to resctrl_val that >>>> attempts to remount resctrl again that will undo all the configurations >>>> inbetween. >>> >>> No, it wouldn't because mum_resctrlfs is 0. When resctrl FS is already >>> mounted >>> and mum_resctrlfs is 0, then remount_resctrlfs() is a noop. >>> >> >> I missed that. Thank you. >> >> fyi ... when I tried these tests I encountered the following error >> related to unmounting: >> >> [SNIP] >> ok Write schema "L3:1=7fff" to resctrl FS >> ok Write schema "L3:1=ffff" to resctrl FS >> ok Write schema "L3:1=1ffff" to resctrl FS >> ok Write schema "L3:1=3ffff" to resctrl FS >> # Unable to umount resctrl: Device or resource busy >> # Results are displayed in (Bytes) >> ok CQM: diff within 5% for mask 1 >> # alloc_llc_cache_size: 2883584 >> # avg_llc_occu_resc: 2973696 >> ok CQM: diff within 5% for mask 3 >> [SNIP] >> >> This seems to originate from resctrl_val() that forces an unmount but if >> that fails the error is not propagated. > > Yes, that's right and it's a good test. I didn't encounter this issue during > my testing because I wasn't using resctrl FS from other terminals (I think you > were using resctrl FS from other terminal and hence resctrl_test was unable to > unmount it). I was not explicitly testing for this but this may have been the case. As a sidenote ... could remount_resctrlfs() be called consistently? It seems to switch between being called with true/false and 1/0. Since its parameter type is boolean using true/false seems most appropriate. > I think the error should not be propagated because unmounting resctrl FS > shouldn't stop us from checking the results. If measuring values reports an > error then we shouldn't check for results. This sounds right. It is inconsistent though ... the CQM test unmounts resctrl after it is run but the CAT test does not. Looking closer the CAT test seems to leave its artifacts around in resctrl and this should be cleaned up. I am not sure about the expectations here. Unmounting resctrl after a test is run is indeed the easiest to clean up and may be ok. It may be a surprise to the user though. Perhaps there can be a snippet in the README that warns people about this? Thank you very much Reinette
Hi Reinette, On Wed, 2020-03-11 at 11:03 -0700, Reinette Chatre wrote: > > > > > > [SNIP] > Hi Sai, > > On 3/11/2020 10:33 AM, Sai Praneeth Prakhya wrote: > > On Wed, 2020-03-11 at 10:19 -0700, Reinette Chatre wrote: > > > On 3/10/2020 7:46 PM, Sai Praneeth Prakhya wrote: > > > > On Tue, 2020-03-10 at 15:18 -0700, Reinette Chatre wrote: > > > > > On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote: > > > I missed that. Thank you. > > > > > > fyi ... when I tried these tests I encountered the following error > > > related to unmounting: > > > > > > [SNIP] > > > ok Write schema "L3:1=7fff" to resctrl FS > > > ok Write schema "L3:1=ffff" to resctrl FS > > > ok Write schema "L3:1=1ffff" to resctrl FS > > > ok Write schema "L3:1=3ffff" to resctrl FS > > > # Unable to umount resctrl: Device or resource busy > > > # Results are displayed in (Bytes) > > > ok CQM: diff within 5% for mask 1 > > > # alloc_llc_cache_size: 2883584 > > > # avg_llc_occu_resc: 2973696 > > > ok CQM: diff within 5% for mask 3 > > > [SNIP] > > > > > > This seems to originate from resctrl_val() that forces an unmount but if > > > that fails the error is not propagated. > > > > Yes, that's right and it's a good test. I didn't encounter this issue > > during > > my testing because I wasn't using resctrl FS from other terminals (I think > > you > > were using resctrl FS from other terminal and hence resctrl_test was > > unable to > > unmount it). > > I was not explicitly testing for this but this may have been the case. > > As a sidenote ... could remount_resctrlfs() be called consistently? It > seems to switch between being called with true/false and 1/0. Since its > parameter type is boolean using true/false seems most appropriate. Agreed and make sense. I will fix this in a separate patch. > > I think the error should not be propagated because unmounting resctrl FS > > shouldn't stop us from checking the results. If measuring values reports > > an > > error then we shouldn't check for results. > > This sounds right. It is inconsistent though ... the CQM test unmounts > resctrl after it is run but the CAT test does not. Looking closer the > CAT test seems to leave its artifacts around in resctrl and this should > be cleaned up. Yes makes sense. I will fix CAT test to cleanup things. > I am not sure about the expectations here. Unmounting resctrl after a > test is run is indeed the easiest to clean up and may be ok. The main reason for unmounting is that assume user hasn't mounted resctrl FS before running the test then we want to make sure we get back to the same state as before running test and also to clean up any changes made to resctrl FS during test. > It may be a > surprise to the user though. Perhaps there can be a snippet in the > README that warns people about this? Sure! makes sense. I will add it. Regards, Sai
diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c index e30cdd7b851c..ca794ad6fcfc 100644 --- a/tools/testing/selftests/resctrl/cache.c +++ b/tools/testing/selftests/resctrl/cache.c @@ -224,11 +224,15 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid) /* Measure llc occupancy from resctrl */ if (!strcmp(param->resctrl_val, "cqm")) { + /* Sleep for a second so that benchmark gets to run */ + sleep(1); + ret = get_llc_occu_resctrl(&llc_occu_resc); if (ret < 0) return ret; llc_value = llc_occu_resc; } + ret = print_results_cache(param->filename, bm_pid, llc_value); if (ret) return ret; diff --git a/tools/testing/selftests/resctrl/cqm_test.c b/tools/testing/selftests/resctrl/cqm_test.c index f27b0363e518..3406c04ff110 100644 --- a/tools/testing/selftests/resctrl/cqm_test.c +++ b/tools/testing/selftests/resctrl/cqm_test.c @@ -13,73 +13,74 @@ #define RESULT_FILE_NAME "result_cqm" #define NUM_OF_RUNS 5 -#define MAX_DIFF 2000000 -#define MAX_DIFF_PERCENT 15 +#define MAX_DIFF_PERCENT 5 -int count_of_bits; char cbm_mask[256]; -unsigned long long_mask; -unsigned long cache_size; -static int cqm_setup(struct resctrl_val_param *p) -{ - /* Run NUM_OF_RUNS times */ - if (p->num_of_runs >= NUM_OF_RUNS) - return -1; - - p->num_of_runs++; - - return 0; -} +static int count_of_bits, count_of_shareable_bits; +static unsigned long cache_size, shareable_mask; -static void show_cache_info(unsigned long sum_llc_occu_resc, int no_of_bits, - unsigned long span) +static void show_cache_info(unsigned long *llc_occu_resc) { - unsigned long avg_llc_occu_resc = 0; - float diff_percent; - long avg_diff = 0; - bool res; + int bits, runs; + bool failed = false; - avg_llc_occu_resc = sum_llc_occu_resc / (NUM_OF_RUNS - 1); - avg_diff = (long)abs(span - avg_llc_occu_resc); - - diff_percent = (((float)span - avg_llc_occu_resc) / span) * 100; + printf("# Results are displayed in (Bytes)\n"); - if ((abs((int)diff_percent) <= MAX_DIFF_PERCENT) || - (abs(avg_diff) <= MAX_DIFF)) - res = true; - else - res = false; + for (bits = 0; bits < count_of_bits - count_of_shareable_bits; bits++) { + unsigned long avg_llc_occu_resc, sum_llc_occu_resc = 0; + unsigned long alloc_llc, mask = 0; + int llc_occu_diff, i; + + /* + * The first run is discarded due to inaccurate value from + * phase transition. + */ + for (runs = NUM_OF_RUNS * bits + 1; + runs < NUM_OF_RUNS * bits + NUM_OF_RUNS; runs++) + sum_llc_occu_resc += llc_occu_resc[runs]; + + avg_llc_occu_resc = sum_llc_occu_resc / (NUM_OF_RUNS - 1); + alloc_llc = cache_size * ((float)(bits + 1) / count_of_bits); + llc_occu_diff = avg_llc_occu_resc - alloc_llc; + + for (i = 0; i < bits + 1; i++) { + mask <<= 1; + mask |= 1; + } - printf("%sok CQM: diff within %d, %d\%%\n", res ? "" : "not ", - MAX_DIFF, (int)MAX_DIFF_PERCENT); + if (llc_occu_diff > 0 && + llc_occu_diff > alloc_llc * ((float)MAX_DIFF_PERCENT / 100)) + failed = true; - printf("# diff: %ld\n", avg_diff); - printf("# percent diff=%d\n", abs((int)diff_percent)); - printf("# Results are displayed in (Bytes)\n"); - printf("# Number of bits: %d\n", no_of_bits); - printf("# Avg_llc_occu_resc: %lu\n", avg_llc_occu_resc); - printf("# llc_occu_exp (span): %lu\n", span); + printf("%sok CQM: diff within %d%% for mask %lx\n", + failed ? "not " : "", MAX_DIFF_PERCENT, mask); + printf("# alloc_llc_cache_size: %lu\n", alloc_llc); + printf("# avg_llc_occu_resc: %lu\n", avg_llc_occu_resc); + tests_run++; + } + printf("%sok schemata change for CQM%s\n", failed ? "not " : "", + failed ? " # at least one test failed" : ""); tests_run++; } -static int check_results(struct resctrl_val_param *param, int no_of_bits) +static int check_results(void) { - char *token_array[8], temp[512]; - unsigned long sum_llc_occu_resc = 0; - int runs = 0; + char *token_array[8], output[] = RESULT_FILE_NAME, temp[512]; + unsigned long llc_occu_resc[count_of_bits * NUM_OF_RUNS]; + int runs; FILE *fp; - printf("# checking for pass/fail\n"); - fp = fopen(param->filename, "r"); + fp = fopen(output, "r"); if (!fp) { - perror("# Error in opening file\n"); + perror(output); return errno; } - while (fgets(temp, 1024, fp)) { + runs = 0; + while (fgets(temp, sizeof(temp), fp)) { char *token = strtok(temp, ":\t"); int fields = 0; @@ -88,13 +89,14 @@ static int check_results(struct resctrl_val_param *param, int no_of_bits) token = strtok(NULL, ":\t"); } - /* Field 3 is llc occ resc value */ - if (runs > 0) - sum_llc_occu_resc += strtoul(token_array[3], NULL, 0); + /* Field 3 is resctrl LLC occupancy value */ + llc_occu_resc[runs] = strtoul(token_array[3], NULL, 0); runs++; } + fclose(fp); - show_cache_info(sum_llc_occu_resc, no_of_bits, param->span); + + show_cache_info(llc_occu_resc); return 0; } @@ -104,62 +106,107 @@ void cqm_test_cleanup(void) remove(RESULT_FILE_NAME); } -int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd) +/* + * Change schemata from 1 to count_of_bits - 1. Write schemata to specified + * con_mon grp, mon_grp in resctrl FS. For each allocation, run "NUM_OF_RUNS" + * times to get average values. + */ +static int cqm_setup(struct resctrl_val_param *p) { - int ret, mum_resctrlfs; + static int runs_per_allocation = 0, num_of_bits = 1; + unsigned long mask = 0; + char schemata[64]; + int i, ret; - cache_size = 0; - mum_resctrlfs = 1; + if (runs_per_allocation >= NUM_OF_RUNS) + runs_per_allocation = 0; - ret = remount_resctrlfs(mum_resctrlfs); - if (ret) - return ret; + /* Only set up schemata once every NUM_OF_RUNS of allocations */ + if (runs_per_allocation++ != 0) + return 0; - if (!validate_resctrl_feature_request("cqm")) + if (num_of_bits > count_of_bits - count_of_shareable_bits) return -1; - ret = get_cbm_mask("L3"); - if (ret) - return ret; - - long_mask = strtoul(cbm_mask, NULL, 16); + /* Prepare cbm mask without any shareable bits */ + for (i = 0; i < num_of_bits; i++) { + mask <<= 1; + mask |= 1; + } + mask = ~shareable_mask & mask; - ret = get_cache_size(cpu_no, "L3", &cache_size); + sprintf(schemata, "%lx", mask); + ret = write_schemata(p->ctrlgrp, schemata, p->cpu_no, "cat"); if (ret) return ret; - printf("cache size :%lu\n", cache_size); - count_of_bits = count_bits(long_mask); + p->mask = mask; + num_of_bits++; - if (n < 1 || n > count_of_bits) { - printf("Invalid input value for numbr_of_bits n!\n"); - printf("Please Enter value in range 1 to %d\n", count_of_bits); - return -1; - } + return 0; +} +int cqm_schemata_change(int cpu_no, int span, char *cache_type, + char **benchmark_cmd) +{ struct resctrl_val_param param = { .resctrl_val = "cqm", .ctrlgrp = "c1", .mongrp = "m1", .cpu_no = cpu_no, + .span = span, .mum_resctrlfs = 0, .filename = RESULT_FILE_NAME, - .mask = ~(long_mask << n) & long_mask, - .span = cache_size * n / count_of_bits, .num_of_runs = 0, - .setup = cqm_setup, + .setup = cqm_setup }; + int ret; + char schemata[64]; + unsigned long long_mask; - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) - sprintf(benchmark_cmd[1], "%lu", param.span); + ret = remount_resctrlfs(1); + if (ret) + return ret; - remove(RESULT_FILE_NAME); + /* Check for both 'cat' and 'cqm' because CQM is validated using CAT */ + if (!validate_resctrl_feature_request("cqm")) + return -1; + + if (!validate_resctrl_feature_request("cat")) + return -1; + + ret = get_cache_size(cpu_no, cache_type, &cache_size); + if (ret) + return ret; + printf("# cache size: %lu\n", cache_size); + + ret = get_cbm_mask(cache_type); + if (ret) + return ret; + + long_mask = strtoul(cbm_mask, NULL, 16); + count_of_bits = count_bits(long_mask); + + /* + * Change root con_mon grp schemata to minimum (i.e. '1' bit) so that + * test could use all other bits + */ + sprintf(schemata, "%x", 1); + ret = write_schemata("", schemata, cpu_no, "cqm"); + if (ret) + return ret; + + ret = get_shareable_mask(cache_type, &shareable_mask); + if (ret) + return ret; + + count_of_shareable_bits = count_bits(shareable_mask); ret = resctrl_val(benchmark_cmd, ¶m); if (ret) return ret; - ret = check_results(¶m, n); + ret = check_results(); if (ret) return ret; diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 79148cbbd7a4..cb67ad689475 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -103,7 +103,8 @@ int setup_critical_process(pid_t pid, struct resctrl_val_param *param); int run_critical_process(pid_t pid, struct resctrl_val_param *param); void cat_test_cleanup(void); int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type); -int cqm_resctrl_val(int cpu_no, int n, char **benchmark_cmd); +int cqm_schemata_change(int cpu_no, int span, char *cache_type, + char **benchmark_cmd); unsigned int count_bits(unsigned long n); void cqm_test_cleanup(void); int get_core_sibling(int cpu_no); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 60db128312a6..3c408c636b6d 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -180,9 +180,11 @@ int main(int argc, char **argv) if (cqm_test) { printf("# Starting CQM test ...\n"); - if (!has_ben) + if (!has_ben) { + sprintf(benchmark_cmd[1], "%d", span); sprintf(benchmark_cmd[5], "%s", "cqm"); - res = cqm_resctrl_val(cpu_no, no_of_bits, benchmark_cmd); + } + res = cqm_schemata_change(cpu_no, span, "L3", benchmark_cmd); printf("%sok CQM: test\n", res ? "not " : ""); cqm_test_cleanup(); tests_run++; diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 271cb5c976f5..c59fad6cb9b0 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -705,29 +705,21 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) goto out; } - /* Give benchmark enough time to fully run */ - sleep(1); - /* Test runs until the callback setup() tells the test to stop. */ while (1) { + ret = param->setup(param); + if (ret) { + ret = 0; + break; + } + + /* Measure vals sleeps for a second */ if ((strcmp(resctrl_val, "mbm") == 0) || (strcmp(resctrl_val, "mba") == 0)) { - ret = param->setup(param); - if (ret) { - ret = 0; - break; - } - ret = measure_vals(param, &bw_resc_start); if (ret) break; } else if (strcmp(resctrl_val, "cqm") == 0) { - ret = param->setup(param); - if (ret) { - ret = 0; - break; - } - sleep(1); ret = measure_cache_vals(param, bm_pid); if (ret) break; diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 52452bb0178a..bd81a13ff9df 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -365,11 +365,7 @@ void run_benchmark(int signum, siginfo_t *info, void *ucontext) memflush = atoi(benchmark_cmd[3]); operation = atoi(benchmark_cmd[4]); sprintf(resctrl_val, "%s", benchmark_cmd[5]); - - if (strcmp(resctrl_val, "cqm") != 0) - buffer_span = span * MB; - else - buffer_span = span; + buffer_span = span * MB; if (run_fill_buf(buffer_span, malloc_and_init_memory, memflush, operation, resctrl_val))