Re: [PATCH v1 30/31] x86/resctrl: Move the filesystem bits to headers visible to fs/resctrl

Dave Martin
2024-04-12T16:20:44Z

On Thu, Apr 11, 2024 at 10:43:55AM -0700, Reinette Chatre wrote:
> Hi Dave,
>
> On 4/11/2024 7:28 AM, Dave Martin wrote:
> > On Mon, Apr 08, 2024 at 08:42:00PM -0700, Reinette Chatre wrote:
> >> Hi James,
> >>
> >> On 3/21/2024 9:51 AM, James Morse wrote:
> >> ..
> >>> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> >>> index 4788bd95dac6..fe0b10b589c0 100644
> >>> --- a/include/linux/resctrl_types.h
> >>> +++ b/include/linux/resctrl_types.h
> >>> @@ -7,6 +7,36 @@
> >>> #ifndef __LINUX_RESCTRL_TYPES_H
> >>> #define __LINUX_RESCTRL_TYPES_H
> >>>
> >>> +#define CQM_LIMBOCHECK_INTERVAL 1000
> >>> +
> >>> +#define MBM_CNTR_WIDTH_BASE 24
> >>> +#define MBM_OVERFLOW_INTERVAL 1000
> >>> +#define MAX_MBA_BW 100u
> >>> +#define MBA_IS_LINEAR 0x4
> >>> +
> >>> +/* rdtgroup.flags */
> >>> +#define RDT_DELETED 1
> >>> +
> >>> +/* rftype.flags */
> >>> +#define RFTYPE_FLAGS_CPUS_LIST 1
> >>> +
> >>> +/*
> >>> + * Define the file type flags for base and info directories.
> >>> + */
> >>> +#define RFTYPE_INFO BIT(0)
> >>> +#define RFTYPE_BASE BIT(1)
> >>> +#define RFTYPE_CTRL BIT(4)
> >>> +#define RFTYPE_MON BIT(5)
> >>> +#define RFTYPE_TOP BIT(6)
> >>> +#define RFTYPE_RES_CACHE BIT(8)
> >>> +#define RFTYPE_RES_MB BIT(9)
> >>> +#define RFTYPE_DEBUG BIT(10)
> >>> +#define RFTYPE_CTRL_INFO (RFTYPE_INFO | RFTYPE_CTRL)
> >>> +#define RFTYPE_MON_INFO (RFTYPE_INFO | RFTYPE_MON)
> >>> +#define RFTYPE_TOP_INFO (RFTYPE_INFO | RFTYPE_TOP)
> >>> +#define RFTYPE_CTRL_BASE (RFTYPE_BASE | RFTYPE_CTRL)
> >>> +#define RFTYPE_MON_BASE (RFTYPE_BASE | RFTYPE_MON)
> >>> +
> >>> /* Reads to Local DRAM Memory */
> >>> #define READS_TO_LOCAL_MEM BIT(0)
> >>>
> >>
> >> Not all these new seem to belong in this file. Could you please confirm?
> >>
> >> For example:
> >> Earlier in series it was mentioned that struct rdtgroup is private to the
> >> fs so having RDT_DELETED is unexpected as it implies access to struct rdtgroup.
> >>
> >> CQM_LIMBOCHECK_INTERVAL seems private to the fs code, so too
> >> RFTYPE_FLAGS_CPUS_LIST.
> >>
> >> Reinette
> >>
> >
> > I'll flag this for James to review.
> >
> > These have to be moved out of the x86 private headers, but you're right
> > that some of them seem logically private to the resctrl core.
> >
> > I guess some of these could move to fs/resctrl/internal.h?
>
> It looks to me that way.
>
> >
> > OTOH, might it be preferable to keep all the flag definitions for a
> > given member together for ease of maintenance, even if some are for
> > resctrl internal use only?
>
> Indeed, those RFTYPE flags really seem to be fs code but I agree that
> architectures' use of RFTYPE_RES_CACHE and RFTYPE_RES_MB does make this
> complicated and having these in a central place is reasonable to me.
>
> Reinette

Maybe we could split these into two groups, and clearly comment the ones
that have no user outside resctrl as internal use only?

That's not as clean as removing those definitions from a shared header,
but at least would help document the issue until/unless a better
solution is found...

Cheers
---Dave