Messages in this thread | | | Date | Thu, 11 Apr 2024 10:41:03 -0700 | Subject | Re: [PATCH v1 24/31] x86/resctrl: Move get_config_index() to a header | From | Reinette Chatre <> |
| |
Hi Dave,
On 4/11/2024 7:25 AM, Dave Martin wrote: > 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?
It does, thank you very much.
> > I can recommend adding some of this rationale to the commit message > if it helps (and assuming I'm right!)
Sounds good, thank you.
Reinette
| |