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

[Reinette Chatre]

Hi Tony,

On 5/23/24 2:25 PM, Luck, Tony wrote:
> > > 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.
>
> So (in arch/x86/kernel/cpu/resctrl/monitor.c)
>
> static int snc_nodes_per_l3_cache = 1;
>
> Set and use only in this (arch specific) file.

Since this series initializes this value in
arch/x86/kernel/cpu/resctrl/core.c it is not clear to
me from just this one line how you envision the changes.

Just to be clear ... when I refer to arch specific and
filesystem code I am considering how this series integrates with [1]
since that is the direction resctrl is headed in.
Being "arch specific" thus does not require that it be moved into
monitor.c - it could be added to arch/x86/kernel/cpu/resctrl/internal.h
where it can remain after the fs/arch split.

It will be very helpful if you view your series while taking
[1] into account. For example, when looking at [1] you will see that
mon_event_count() and __mon_event_count() are resctrl filesystem
functions. When you consider that it should be clear that adding
an arch specific get_node_rmid() between these functions will make
the arch/fs split more difficult.

Reinette

[1] https://lore.kernel.org/lkml/20240321165106.31602-1-james.morse@xxxxxxx/