Re: [PATCH v1 24/31] x86/resctrl: Move get_config_index() to a header

[Dave Martin <Dave.Martin@arm.com>]

On Mon, Apr 08, 2024 at 08:25:26PM -0700, Reinette Chatre wrote:
> Hi James,
> 
> On 3/21/2024 9:50 AM, James Morse wrote:
> > get_config_index() is used by the architecture specific code to map a
> > CLOSID+type pair to an index in the configuration arrays.
> > 
> > MPAM needs to do this too to preserve the ABI to user-space, there is
> > no reason to do it differently.
> > 
> > Move the helper to a header file.
> > 
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 19 +++----------------
> >  include/linux/resctrl.h                   | 15 +++++++++++++++
> >  2 files changed, 18 insertions(+), 16 deletions(-)

[...]

> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index 3de5bc63ace0..73c111963433 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -258,6 +258,21 @@ bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt);
> >  void resctrl_arch_mon_event_config_write(void *info);
> >  void resctrl_arch_mon_event_config_read(void *info);
> >  
> > +/* For use by arch code to remap resctrl's smaller CDP CLOSID range */
> > +static inline u32 resctrl_get_config_index(u32 closid,
> > +					   enum resctrl_conf_type type)
> > +{
> > +	switch (type) {
> > +	default:
> > +	case CDP_NONE:
> > +		return closid;
> > +	case CDP_CODE:
> > +			return (closid * 2) + 1;
> > +	case CDP_DATA:
> > +			return (closid * 2);
> > +	}
> > +}
> 
> (please check the tabs)

Noted.  I also see that redundant parentheses seem spuriously added
compared with the original version of this moved code.  I can make a
note to drop them if you prefer.

> This change is unexpected to me. Could you please elaborate how
> MPAM's variant of CDP works?
> 
> Thank you very much.
> 
> Reinette

Note: I haven't discussed this specifically with James, so the following
is my best guess at the rationale...  With that in mind:

For MPAM, CDP isn't a special mode; instead, the PARTIDs for
instructions and data are always configured independently in the CPU.
If resctrl is not configured for CDP, we simply program the same PARTID
value both for instructions and data on task switch.

For a given resctrl control group we could pick two random unrelated
PARTIDs, but there seems to be no advantage in doing that since resctrl
enables cdp globally or not, and we would require more effort to
translate resctrl closids to PARTIDs if we didn't pair the IDs up
systematically.

(See [1], [2] in James' snapshot, which illustrate how he proposes
to do it.)


So, we may as well stick with the same scheme already established for
x86: nothing forces us to do that, but it looks simpler than the
alternatives.  I think that's the idea, anyway.

Then, if the same scheme is used by multiple arches (and 100% of the
arches currently known to resctrl), it probably makes sense to share the
definition of the mapping at least as a default for arches that don't
have their own different ways of doing it.

Does this make sense?

I can recommend adding some of this rationale to the commit message
if it helps (and assuming I'm right!)

Cheers
---Dave


[1] https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/mpam_resctrl.c?h=mpam/snapshot/v6.7-rc2#n206
[2] https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/arch/arm64/include/asm/mpam.h?h=mpam/snapshot/v6.7-rc2#n146