Hi Tony, On 9/25/2023 5:56 PM, Tony Luck wrote: > On Mon, Sep 25, 2023 at 04:22:29PM -0700, Reinette Chatre wrote: >> On 8/29/2023 4:44 PM, Tony Luck wrote: ... >>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h >>> index 8334eeacfec5..2db1244ae642 100644 >>> --- a/include/linux/resctrl.h >>> +++ b/include/linux/resctrl.h >>> @@ -144,13 +144,18 @@ struct resctrl_membw { >>> struct rdt_parse_data; >>> struct resctrl_schema; >>> >>> +enum resctrl_scope { >>> + RESCTRL_L3_CACHE, >>> + RESCTRL_L2_CACHE, >>> +}; >> >> I'm curious why L3 appears before L2? This is not a big deal but >> I think the additional indirection that this introduces is >> not necessary. As you had in an earlier version this could be >> RESCTRL_L2_CACHE = 2 and then the value can just be used directly >> (after ensuring it is used in a valid context). > > I appear to have misundertood your earlier comments. I thought you > didn't like my use of RESCTRL_L2_CACHE = 2, and RESCTRL_L3_CACHE = 3 > so that the could be passed directly to get_cpu_cacheinfo_id(). > > But I see now the issue was "after ensuring it is used in a valid > context". Are you looking for something like this: Indeed. My concern with the earlier version was seeing a variable that may contain any scope be used in a context where it can only represent a cache level. > > enum resctrl_scope { > RESCTRL_UNINITIALIZED_SCOPE = 0, > RESCTRL_L2_CACHE = 2, > RESCTRL_L3_CACHE = 3, > RESCTRL_L3_NODE = 4, > }; I do not think RESCTRL_UNINITIALIZED_SCOPE is required (perhaps getting too defensive?) but adding it would surely not do harm and indeed make it obvious how the uninitialized case is handled. I am not familiar with customs in this regard and would be ok either way. You can decide what works best for you. About the new name RESCTRL_L3_NODE. It is not clear to me why "L3" is in the name. > > static int get_domain_id_from_scope(int cpu, enum resctrl_scope scope) > { > switch (scope) { > case RESCTRL_L2_CACHE: > case RESCTRL_L3_CACHE: > return get_cpu_cacheinfo_id(cpu, scope); > case RESCTRL_NODE: > return cpu_to_node(cpu); (oh - maybe the earlier RESCTRL_L3_NODE was a typo?) > default: > case RESCTRL_UNINITIALIZED_SCOPE: > WARN_ON_ONCE(1); > return -1; > } > > return -1; > } > I'll leave it to you to decide if you want to use RESCTRL_UNINITIALIZED_SCOPE. If you do decide to keep it could you please let the "default" case be the last one? >> >>> + >>> /** >>> * struct rdt_resource - attributes of a resctrl resource >>> * @rid: The index of the resource >>> * @alloc_capable: Is allocation available on this machine >>> * @mon_capable: Is monitor feature available on this machine >>> * @num_rmid: Number of RMIDs available >>> - * @cache_level: Which cache level defines scope of this resource >>> + * @scope: Scope of this resource >>> * @cache: Cache allocation related data >>> * @membw: If the component has bandwidth controls, their properties. >>> * @domains: All domains for this resource >>> @@ -168,7 +173,7 @@ struct rdt_resource { >>> bool alloc_capable; >>> bool mon_capable; >>> int num_rmid; >>> - int cache_level; >>> + enum resctrl_scope scope; >>> struct resctrl_cache cache; >>> struct resctrl_membw membw; >>> struct list_head domains; >>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c >>> index 030d3b409768..0d3bae523ecb 100644 >>> --- a/arch/x86/kernel/cpu/resctrl/core.c >>> +++ b/arch/x86/kernel/cpu/resctrl/core.c >>> @@ -65,7 +65,7 @@ struct rdt_hw_resource rdt_resources_all[] = { >>> .r_resctrl = { >>> .rid = RDT_RESOURCE_L3, >>> .name = "L3", >>> - .cache_level = 3, >>> + .scope = RESCTRL_L3_CACHE, >>> .domains = domain_init(RDT_RESOURCE_L3), >>> .parse_ctrlval = parse_cbm, >>> .format_str = "%d=%0*x", >>> @@ -79,7 +79,7 @@ struct rdt_hw_resource rdt_resources_all[] = { >>> .r_resctrl = { >>> .rid = RDT_RESOURCE_L2, >>> .name = "L2", >>> - .cache_level = 2, >>> + .scope = RESCTRL_L2_CACHE, >>> .domains = domain_init(RDT_RESOURCE_L2), >>> .parse_ctrlval = parse_cbm, >>> .format_str = "%d=%0*x", >>> @@ -93,7 +93,7 @@ struct rdt_hw_resource rdt_resources_all[] = { >>> .r_resctrl = { >>> .rid = RDT_RESOURCE_MBA, >>> .name = "MB", >>> - .cache_level = 3, >>> + .scope = RESCTRL_L3_CACHE, >>> .domains = domain_init(RDT_RESOURCE_MBA), >>> .parse_ctrlval = parse_bw, >>> .format_str = "%d=%*u", >>> @@ -105,7 +105,7 @@ struct rdt_hw_resource rdt_resources_all[] = { >>> .r_resctrl = { >>> .rid = RDT_RESOURCE_SMBA, >>> .name = "SMBA", >>> - .cache_level = 3, >>> + .scope = RESCTRL_L3_CACHE, >>> .domains = domain_init(RDT_RESOURCE_SMBA), >>> .parse_ctrlval = parse_bw, >>> .format_str = "%d=%*u", >>> @@ -487,6 +487,21 @@ static int arch_domain_mbm_alloc(u32 num_rmid, struct rdt_hw_domain *hw_dom) >>> return 0; >>> } >>> >>> +static int get_domain_id_from_scope(int cpu, enum resctrl_scope scope) >>> +{ >>> + switch (scope) { >>> + case RESCTRL_L3_CACHE: >>> + return get_cpu_cacheinfo_id(cpu, 3); >>> + case RESCTRL_L2_CACHE: >>> + return get_cpu_cacheinfo_id(cpu, 2); >>> + default: >>> + WARN_ON_ONCE(1); >>> + break; >>> + } >>> + >>> + return -1; >>> +} >> >> Looking ahead at the next patch some members of rdt_resources_all[] >> are left with a default "0" initialization of mon_scope that is a >> valid scope of RESCTRL_L3_CACHE in this implementation that would >> not be caught with defensive code like above. For the above to catch >> a case like this I think that there should be some default >> initialization - but if you do move to something like you >> had in v3 then this may not be necessary. If the L2 scope is 2, >> L3 scope is 3, node scope is 4, then no initialization will be zero >> and the above default can more accurately catch failure cases. > > See above (with a defensive enum constant that has the value 0). > >> >>> + >>> /* >>> * domain_add_cpu - Add a cpu to a resource's domain list. >>> * >>> @@ -502,7 +517,7 @@ static int arch_domain_mbm_alloc(u32 num_rmid, struct rdt_hw_domain *hw_dom) >>> */ >>> static void domain_add_cpu(int cpu, struct rdt_resource *r) >>> { >>> - int id = get_cpu_cacheinfo_id(cpu, r->cache_level); >>> + int id = get_domain_id_from_scope(cpu, r->scope); >>> struct list_head *add_pos = NULL; >>> struct rdt_hw_domain *hw_dom; >>> struct rdt_domain *d; >>> @@ -552,7 +567,7 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r) >>> >>> static void domain_remove_cpu(int cpu, struct rdt_resource *r) >>> { >>> - int id = get_cpu_cacheinfo_id(cpu, r->cache_level); >>> + int id = get_domain_id_from_scope(cpu, r->scope); >>> struct rdt_hw_domain *hw_dom; >>> struct rdt_domain *d; >>> >>> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c >>> index 458cb7419502..e79324676f57 100644 >>> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c >>> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c >>> @@ -279,6 +279,7 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr) >>> static int pseudo_lock_region_init(struct pseudo_lock_region *plr) >>> { >>> struct cpu_cacheinfo *ci; >>> + int cache_level; >>> int ret; >>> int i; >>> >>> @@ -296,8 +297,20 @@ static int pseudo_lock_region_init(struct pseudo_lock_region *plr) >>> >>> plr->size = rdtgroup_cbm_to_size(plr->s->res, plr->d, plr->cbm); >>> >>> + switch (plr->s->res->scope) { >>> + case RESCTRL_L3_CACHE: >>> + cache_level = 3; >>> + break; >>> + case RESCTRL_L2_CACHE: >>> + cache_level = 2; >>> + break; >>> + default: >>> + WARN_ON_ONCE(1); >>> + return -ENODEV; >>> + } >> >> I think this can be simplified without the indirection - a simplified >> WARN can just test for valid values of plr->s->res->scope directly >> (exit on failure) and then it can be used directly in the code below >> when the enum value corresponds to a cache level. > > Is this what you want here? > > if (plr->s->res->scope != RESCTRL_L2_CACHE && > plr->s->res->scope != RESCTRL_L3_CACHE) { > WARN_ON_ONCE(1); > return -ENODEV; > } Something like this, yes. It could be simplified more with a: /* Just to get line length shorter: */ enum resctrl_scope scope = plr->s->res->scope; /* or cache_level? */ if (WARN_ON_ONCE(scope != RESCTRL_L2_CACHE && scope != RESCTRL_L3_CACHE)) return -ENODEV; Reinette