On Wed, Aug 05, 2026 at 03:18:40PM +0800, Reinette Chatre wrote: > Hi Richard, >
Hi Reinette, Thanks for the review ! > On 6/8/26 4:06 AM, Richard Cheng wrote: > > L3_CAT needs a CPU-exclusive cache portion, so it's skipped when MPAM > > reports every CBM bit as shareable, leaving L3 allocation untested. CMT > > only checks occupancy accuracy, not that a CBM actually limits it. > > hmmm ... CMT test ensures that the LLC occupancy is within a % of the size > of the cache allocation. To me this implies that it indeed tests that > the CBM limits the allocation, no? > Yes you're correct on this. I based the patch on older version of the CMT test. After rebasing, I see the current CMT already uses a workload larger than the allocation and configures the root group with the complementary CBM. It now covers what I wanted to test. I think we're good to drop this patch in v2. > > > > L3_CAT_OCCUP gives a group a small CBM, run a workload spanning the > > whole cache, and check every occupancy sample stays within the > > allocation. An unenforced CBM would instead let occupancy grow to the > > full cache. > > > > Move CON_MON_LCC_OCCUP_PATH to resctrl.h to share it with CMT. > > > > Signed-off-by: Richard Cheng <[email protected]> > > --- > > tools/testing/selftests/resctrl/cat_test.c | 201 ++++++++++++++++++ > > tools/testing/selftests/resctrl/cmt_test.c | 3 - > > tools/testing/selftests/resctrl/resctrl.h | 4 + > > .../testing/selftests/resctrl/resctrl_tests.c | 1 + > > 4 files changed, 206 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/cat_test.c > > b/tools/testing/selftests/resctrl/cat_test.c > > index f00b622c1460..16a947f1ed16 100644 > > --- a/tools/testing/selftests/resctrl/cat_test.c > > +++ b/tools/testing/selftests/resctrl/cat_test.c > > @@ -402,3 +402,204 @@ struct resctrl_test l2_noncont_cat_test = { > > .feature_check = noncont_cat_feature_check, > > .run_test = noncont_cat_run_test, > > }; > > + > > +/* > > + * L3_CAT_OCCUP - Verify that a CAT allocation bounds cache occupancy. > > + * > > + * Unlike L3_CAT (which measures interference between groups and needs an > > + * exclusive cache portion), this test gives a control group a strict > > subset > > Please let comment just refer to what this test does. These comments are > unlikely > to be updated if L3_CAT ever changes. > > > + * of the CBM, then runs a benchmark whose buffer spans the *whole* cache - > > + * i.e. much larger than the allocation. With CAT enforced, the group can > > + * only keep its allocated portion resident, so llc_occupancy settles near > > + * the allocation size. Without enforcement occupancy would instead climb > > + * towards the full cache. This works even when all CBM bits are shareable > > + * (where L3_CAT is skipped). > > hmmm ... these statements state as fact what really depends on system load and > interference that this test make no attempt to avoid. > > > + */ > > +#define CAT_OCCUP_RESULT_FILE "result_cat_occup" > > +#define CAT_OCCUP_NUM_OF_RUNS 5 > > + > > +static int cat_occup_cpu; > > + > > +static int cat_occup_init(const struct resctrl_val_param *param, int > > domain_id) > > Please take a look at recent resctrl selftest changes that provides more > data to the init() that will avoid the cat_occup_cpu global. > Even so, why not just use cmt_init() that reduces interference from rest of > system to improve chances of workload's cache occupancy to match its > cache allocation? > > > +{ > > + char schemata[64]; > > + > > + sprintf(llc_occup_path, CON_MON_LCC_OCCUP_PATH, RESCTRL_PATH, > > + param->ctrlgrp, domain_id); > > + > > + /* > > + * Confine the benchmark to the allocated portion *before* it starts > > + * filling (resctrl_val() calls init() before forking the benchmark), > > + * so occupancy reflects the restricted CBM from the first sample. > > + */ > > + snprintf(schemata, sizeof(schemata), "%lx", param->mask); > > + > > + return write_schemata(param->ctrlgrp, schemata, cat_occup_cpu, "L3"); > > +} > > + > > +static int cat_occup_setup(const struct resctrl_test *test, > > + const struct user_params *uparams, > > + struct resctrl_val_param *p) > > +{ > > + if (p->num_of_runs >= CAT_OCCUP_NUM_OF_RUNS) > > + return END_OF_TESTS; > > + > > + p->num_of_runs++; > > + > > + return 0; > > +} > > cmt_setup()? > > > + > > +static int cat_occup_measure(const struct user_params *uparams, > > + struct resctrl_val_param *param, pid_t bm_pid) > > +{ > > + sleep(1); > > + return measure_llc_resctrl(param->filename, bm_pid); > > +} > > > cmt_measure()? > > This test has a lot in common with the existing CMT test since it > duplicates cmt_setup(), cmt_measure(), and cmt_feature_check(). From what I > can tell it can use cmt_init() also. Could the implementation be simplified > by instead considering it a CMT test, move code to cmt_test.c, and avoid > all this duplication? > > > + > > +static int cat_occup_check_results(struct resctrl_val_param *param, > > + size_t alloc_span, size_t cache_size, > > + int no_of_bits) > > +{ > > + char *token_array[8], temp[512]; > > + unsigned long occu, max_occu = 0, ceiling, floor; > > Please follow kernel coding style (throughout this series) by, in this > example, > using reverse-fir tree ordering. > > > + int runs = 0; > > + int fail = 0; > > + FILE *fp; > > + > > + /* > > + * Check every sample, not an average: CAT is a hard limit, so a single > > hmmm ... "is a hard limit" does not match what the code does > > > + * sample above the allocation is a real violation that an average > > + * could mask. > > + */ > > + ceiling = alloc_span + (cache_size - alloc_span) / 2; > > + floor = alloc_span / 2; > > + > > + ksft_print_msg("Checking for pass/fail\n"); > > + fp = fopen(param->filename, "r"); > > + if (!fp) { > > + ksft_perror("Error in opening file"); > > + > > + return -1; > > + } > > + > > + while (fgets(temp, sizeof(temp), fp)) { > > + char *token = strtok(temp, ":\t"); > > + int fields = 0; > > + > > + while (token) { > > + token_array[fields++] = token; > > + token = strtok(NULL, ":\t"); > > + } > > + > > + /* Field 3 is the resctrl-reported llc_occupancy value. */ > > + occu = strtoul(token_array[3], NULL, 0); > > + runs++; > > + > > + if (occu > max_occu) > > + max_occu = occu; > > + > > + if (occu > ceiling) { > > + ksft_print_msg("Fail: run %d occupancy %lu exceeds > > ceiling %lu\n", > > + runs, occu, ceiling); > > + fail = 1; > > KSFT_FAIL is available to avoid using magic numbers. > > > + } > > + } > > + fclose(fp); > > + > > + if (!runs) { > > + ksft_print_msg("No occupancy samples collected\n"); > > + return -1; > > + } > > + > > + if (max_occu < floor) { > > + ksft_print_msg("Fail: peak occupancy %lu never reached floor > > %lu\n", > > + max_occu, floor); > > I think this test should be dropped. We cannot control the environments in > which > the tests are run and legitimate interference may cause this test to fail > without > it meaning that there is a bug in resctrl. > > > > + fail = 1; > > + } > > + > > + ksft_print_msg("%s CAT confines occupancy to the allocated %d-bit > > portion\n", > > + fail ? "Fail:" : "Pass:", no_of_bits); > > + ksft_print_msg("occupancy=%lu alloc=%zu full=%zu ceiling=%lu > > floor=%lu\n", > > + max_occu, alloc_span, cache_size, ceiling, floor); > > + > > + return fail; > > +} > > + > > +static void cat_occup_test_cleanup(void) > > +{ > > + remove(CAT_OCCUP_RESULT_FILE); > > +} > > + > > +static int cat_occup_run_test(const struct resctrl_test *test, > > + const struct user_params *uparams) > > +{ > > + struct fill_buf_param fill_buf = {}; > > + unsigned long cache_total_size = 0; > > + unsigned long full_mask; > > + int count_of_bits; > > + size_t alloc_span; > > + int n, ret; > > + > > + ret = get_full_cbm(test->resource, &full_mask); > > + if (ret) > > + return ret; > > + > > + ret = get_cache_size(uparams->cpu, test->resource, &cache_total_size); > > + if (ret) > > + return ret; > > + ksft_print_msg("Cache size :%lu\n", cache_total_size); > > + > > + count_of_bits = count_bits(full_mask); > > + > > + /* > > + * Allocate a strict subset of the cache so the benchmark buffer > > + * is larger than the allocation and CAT has something to enforce. > > + */ > > + n = uparams->bits ? : count_of_bits / 2; > > + if (n < 1 || n >= count_of_bits) { > > + ksft_print_msg("Invalid number of CBM bits %d, expected 1 to > > %d\n", > > + n, count_of_bits - 1); > > + return -1; > > + } > > + > > + struct resctrl_val_param param = { > > + .ctrlgrp = "c1", > > + .filename = CAT_OCCUP_RESULT_FILE, > > + .mask = ~(full_mask << n) & full_mask, > > + .num_of_runs = 0, > > + .init = cat_occup_init, > > + .setup = cat_occup_setup, > > + .measure = cat_occup_measure, > > + }; > > + > > + alloc_span = cache_portion_size(cache_total_size, param.mask, > > full_mask); > > + > > + /* Benchmark buffer spans the full cache: larger than the allocation. */ > > + fill_buf.buf_size = cache_total_size; > > + fill_buf.memflush = uparams->fill_buf ? uparams->fill_buf->memflush : > > true; > > + param.fill_buf = &fill_buf; > > This prevents usage of user provided benchmark. Please compare with > cmt_run_test() > initialization. You can find more details about how the workload and > parameters are > communicated in e958c21e2ede ("selftests/resctrl: Make benchmark parameter > passing robust") > > > + cat_occup_cpu = uparams->cpu; > > + > > + remove(param.filename); > > + > > + ret = resctrl_val(test, uparams, ¶m); > > + if (ret) > > + return ret; > > + > > + return cat_occup_check_results(¶m, alloc_span, cache_total_size, n); > > +} > > Fundamentally this looks like a duplicate of cmt_run_test()? Only differences > I see > is the size of the buffer and how the test results are checked for pass/fail. > Looking > at the pass/fail I do not see a big difference with what the CMT test tests. > I do not > see what value this tests add beyond what the CMT test already provides. > > > > + > > +static bool cat_occup_feature_check(const struct resctrl_test *test) > > +{ > > + return test_resource_feature_check(test) && > > + resctrl_mon_feature_exists("L3_MON", "llc_occupancy"); > > +} > > + > > +struct resctrl_test l3_cat_occup_test = { > > + .name = "L3_CAT_OCCUP", > > + .group = "CAT", > > + .resource = "L3", > > + .feature_check = cat_occup_feature_check, > > + .run_test = cat_occup_run_test, > > + .cleanup = cat_occup_test_cleanup, > > +}; > > diff --git a/tools/testing/selftests/resctrl/cmt_test.c > > b/tools/testing/selftests/resctrl/cmt_test.c > > index d09e693dc739..ef51daa8061a 100644 > > --- a/tools/testing/selftests/resctrl/cmt_test.c > > +++ b/tools/testing/selftests/resctrl/cmt_test.c > > @@ -16,9 +16,6 @@ > > #define MAX_DIFF 2000000 > > #define MAX_DIFF_PERCENT 15 > > > > -#define CON_MON_LCC_OCCUP_PATH \ > > - "%s/%s/mon_data/mon_L3_%02d/llc_occupancy" > > - > > static int cmt_init(const struct resctrl_val_param *param, int domain_id) > > { > > sprintf(llc_occup_path, CON_MON_LCC_OCCUP_PATH, RESCTRL_PATH, > > diff --git a/tools/testing/selftests/resctrl/resctrl.h > > b/tools/testing/selftests/resctrl/resctrl.h > > index afe635b6e48d..ce3abf0bdac2 100644 > > --- a/tools/testing/selftests/resctrl/resctrl.h > > +++ b/tools/testing/selftests/resctrl/resctrl.h > > @@ -31,6 +31,9 @@ > > #define PHYS_ID_PATH "/sys/devices/system/cpu/cpu" > > #define INFO_PATH "/sys/fs/resctrl/info" > > > > +#define CON_MON_LCC_OCCUP_PATH \ > > + "%s/%s/mon_data/mon_L3_%02d/llc_occupancy" > > + > > /* > > * CPU vendor IDs > > * > > @@ -244,6 +247,7 @@ extern struct resctrl_test mbm_test; > > extern struct resctrl_test mba_test; > > extern struct resctrl_test cmt_test; > > extern struct resctrl_test l3_cat_test; > > +extern struct resctrl_test l3_cat_occup_test; > > extern struct resctrl_test l3_noncont_cat_test; > > extern struct resctrl_test l2_noncont_cat_test; > > > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c > > b/tools/testing/selftests/resctrl/resctrl_tests.c > > index dbcd5eea9fbc..324a60818aa1 100644 > > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > > @@ -19,6 +19,7 @@ static struct resctrl_test *resctrl_tests[] = { > > &mba_test, > > &cmt_test, > > &l3_cat_test, > > + &l3_cat_occup_test, > > &l3_noncont_cat_test, > > &l2_noncont_cat_test, > > }; > > Reinette Agreed for the above comments, I'll drop this change for v2. Thanks for pointing this out. Best regards, Richard Cheng.
