Re: [PATCH v1 08/31] x86/resctrl: Move resctrl types to a separate header

[Dave Martin]

On Mon, Apr 15, 2024 at 11:03:05AM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/12/2024 9:17 AM, Dave Martin wrote:
> > On Mon, Apr 08, 2024 at 08:18:00PM -0700, Reinette Chatre wrote:
> >> On 3/21/2024 9:50 AM, James Morse wrote:
> >>> To avoid sticky problems in the mpam glue code, move the resctrl
> >>> enums into a separate header.
> >>
> >> Could you please elaborate so that "sticky problems in the mpam glue code" is
> >> specific about what problems are avoided?
> > 
> > Maybe just delete the the sticky clause, and leave:
> > 
> > 	Move the resctrl enums into a separate header.
> > 
> > ...since the next paragraph explains the rationale?
> 
> In the x86 area changelogs start with a context paragraph to
> provide reader with foundation to parse the subsequent problem and
> solution statements (that are also expected to be in separate
> paragraphs in that order). 

Acknowledged; I was hoping to avoid a rewrite, but ...
> 
> >>> This lets the arch code declare prototypes that use these enums without
> >>> creating a loop via asm<->linux resctrl.h The same logic applies to the
> >>> monitor-configuration defines, move these too.

[...]

OK, how about the following:

--8<--

When resctrl is fully factored into core and per-arch code, each arch
will need to use some resctrl common definitions in order to define its
own specialisations and helpers.  Following conventional practice,
it would be desirable to put the dependent arch definitions in an
<asm/resctrl.h> header that is included by the common <linux/resctrl.h>
header.  However, this can make it awkward to avoid a circular
dependency between <linux/resctrl.h> and the arch header.

To avoid solving this issue via conditional inclusion tricks that are
likely to be tricky to maintain, move the affected common types and
constants into a new header that does not need to depend on
<linux/resctrl.h> or on the arch headers.

The same logic applies to the monitor-configuration defines, move
these too.

-->8--

Then:

> >>>
> >>> The maintainers entry for these headers was missed when resctrl.h
> >>> was created. Add a wildcard entry to match both resctrl.h and
> >>> resctrl_types.h.
> >>>
> >>> Signed-off-by: James Morse <james.morse@xxxxxxx>

The last paragraph looks like it can stand as-is.

Does that look OK?


[...]

> >>> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
> >>> new file mode 100644
> >>> index 000000000000..4788bd95dac6
> >>> --- /dev/null
> >>> +++ b/include/linux/resctrl_types.h
> >>> @@ -0,0 +1,68 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>> +/*
> >>> + * Copyright (C) 2024 Arm Ltd.
> >>> + * Based on arch/x86/kernel/cpu/resctrl/internal.h
> >>> + */
> >>
> >> Could this header please explain how this file is intended to be used?
> >> What is it intended to contain?
> >>
> >> Reinette
> > 
> > Maybe something like the following?
> > 
> >  * Resctrl types and constants needed for inline function definitions in
> >  * the arch-specific <asm/resctrl.h> headers.
> > 
> 
> ok.
> 
> Reinette
> 

OK, I'll propose to add that.

Cheers
---Dave