Re: [PATCH v18 06/17] x86/resctrl: Introduce snc_nodes_per_l3_cache

[Reinette Chatre]

Hi Tony,

On 5/23/24 12:04 PM, Luck, Tony wrote:
> > >      return (is_mbm_local_enabled() &&
> > > -           r->alloc_capable && is_mba_linear());
> > > +           r->alloc_capable && is_mba_linear() &&
> > > +           snc_nodes_per_l3_cache == 1);
> > >   }
> > >
> > >   /*
> >
> > Since the software controller is a filesystem feature the above
> > now requires that snc_nodes_per_l3_cache becomes part of the resctrl
> > filesystem code and every architecture will need to set snc_nodes_per_l3_cache.
> > Every architecture will thus need to interpret what "SNC" means for them
> > using the term introduced here. That may be ok ... but the term "SNC"
> > will then surely not identify an Intel feature and Intel needs to be ok
> > that any architecture calls their "similar to SNC but not quite identical"
> > "SNC".
> >
> > I assume now that as part of the fs/arch split there needs to be
> > a new helper that allows different architectures to set this
> > filesystem variable?
>
> I can change this check to better reflect the underlying reason to
> disable the software controller. Which is that the MBM monitor scope
> does not match the MBA control scope. This seems like an architecture
> neutral expression.
>
> So code would look like this:
>
> 	struct rdt_resource *rmbm = &rdt_resources_all[RDT_RESOURCE_L3].r_rescrl;
> 	struct rdt_resource *rmba = &rdt_resources_all[RDT_RESOURCE_MBA].r_rescrl;
>
> 	...
>
> 	 return (is_mbm_local_enabled() &&
>                  r->alloc_capable && is_mba_linear() &&
>                  rmbm->mon_scope == rmba->ctrl_scope);
>
> I'm also contemplating dropping snc_nodes_per_l3_cache from being a
> global variable and making it a field in "struct rdt_resource" (only needed
> for the RDT_RESOURCE_L3 resource). N.B. Babu had suggested it
> shouldn't be global many patch versions ago.
>
> Perhaps name it .domains_per_l3_cache or .subdomains_per_l3_cache?
>
> Bad idea? Good idea (but you have a better name for the field)?

With the check in supports_mba_mbps() changed I do not see
snc_nodes_per_l3_cache needed by anything but arch specific code.
I thus do not see any reason for the resctrl filesystem (or, for
example, Arm) to know that this value even exists.

While struct rdt_hw_resource is a place to put architecture
specific information it does not seem appropriate to force every
resource to carry what is essentially a system wide (not specific to
resctrl) L3 specific property. To me this really seems like an
architecture specific global setting but I'd also like to hear the
motivations why it should not be.

Reinette