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

[Dave Martin]

On Wed, Apr 17, 2024 at 10:18:48PM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/17/2024 7:41 AM, Dave Martin wrote:
> > 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.)
> 
> I am not aware of such a discussion.
> 
> Sounds like a motivation to delay this portion of the changes in patch #8.
> 
> Reinette


Ack, I'll discuss this with James.

I guess the thing to do will be to keep the affected definitions in the
x86 headers for now, and carry the exports in James' MPAM branch until
we figure out whether it really makes sense to share them.

Cheers
---Dave