Re: [PATCH v1 17/31] x86/resctrl: Move mbm_cfg_mask to struct rdt_resource

[Dave Martin]

On Thu, Apr 11, 2024 at 10:39:06AM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/11/2024 7:16 AM, Dave Martin wrote:
> > On Mon, Apr 08, 2024 at 08:21:24PM -0700, Reinette Chatre wrote:
> >> Hi James,
> >>
> >> On 3/21/2024 9:50 AM, James Morse wrote:
> >>> The mbm_cfg_mask field lists the bits that user-space can set when
> >>> configuring an event. This value is output via the last_cmd_status
> >>> file.
> >>>
> >>> Once the filesystem parts of resctrl are moved to live in /fs/, the
> >>> struct rdt_hw_resource is inaccessible to the filesystem code. Because
> >>> this value is output to user-space, it has to be accessible to the
> >>> filesystem code.
> >>>
> >>> Move it to struct rdt_resource.
> >>>
> >>> Signed-off-by: James Morse <james.morse@xxxxxxx>
> >>
> >> ..
> >>
> >>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> >>> index 975b80102fbe..8a7367d1ce45 100644
> >>> --- a/include/linux/resctrl.h
> >>> +++ b/include/linux/resctrl.h
> >>> @@ -140,6 +140,8 @@ struct resctrl_membw {
> >>>   * @format_str:		Per resource format string to show domain value
> >>>   * @evt_list:		List of monitoring events
> >>>   * @fflags:		flags to choose base and info files
> >>> + * @mbm_cfg_mask:	Bandwidth sources that can be tracked when Bandwidth
> >>> + *			Monitoring Event Configuration (BMEC) is supported.
> > 
> > [...]
> > 
> >> Reinette
> >>
> >> BMEC is an AMD term. If MPAM is planning to use this member then this comment
> >> should be made generic.
> > 
> > MPAM can (at least) filter reads and/or writes, and it looks like James
> > is expecting to support this.
> > 
> > However, it probably does make sense to keep the comments neutral in a
> > common header.
> > 
> > Would the following work?
> > 
> > 	* @ mbm_cfg_mask:	Arch-specific event configuration flags
> > 
> > 
> > 
> > I think that's broad enough to cover all bases, but I'll wait for your
> > response.
> 
> Since this is exposed to user space, ideally all architectures would use
> the same bits to mean the same thing. I assumed that is what James intended
> by placing the existing (AMD BMEC) bits in the global resctrl_types.h.
> 
> Reinette

Maybe, but the bits as defined by AMD BMEC look rather architecture and
bus specific, and I am suspicious that there is no guaranteed clean
mapping between MPAM's config and BMEC's config.

MPAM currently just has "reads" and "writes" (or both), though as for
BMEC, the meanings of these are not fully defined.  I suppose finer
filtering granularity might be supported in future (at least, it is not
explicitly ruled out).

James' current approach seems to be to pick a single BMEC flag that's
in the right sort of area for each MPAM bit (though not equivalent) and
translate that bit across to drive a corresponding the MPAM bit.  But
I'd say that this is arch-specific configuration masquerading as
generic configuration IMHO and not really generic at all.

See "untested: arm_mpam: resctrl: Allow monitors to be configured"
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/commit/?h=mpam/snapshot/v6.7-rc2&id=db0ac51f60675b6c4a54ccd24fa7198ec321c56d

I guess this needs discussion with James, since there will have been
additional thought process behind all this that is not captured; either
way, I guess it could be resolved after this series, but it will need a
decision before the MPAM support is merged (or at least, before MPAM
exposes userspace support for event configuration upstream).

(If this has already been discussed and James' current approach has
already been agreed as the least worst option, then I guess I can live
with it; I just find it icky, and it looks odd to have AMD specifics in
a common header.)

Cheers
---Dave