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

[Dave Martin]

On Wed, Apr 17, 2024 at 10:15:57PM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/16/2024 9:19 AM, Dave Martin wrote:
> > 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,
> 
> specializations

Debatable, but OK, fine.

> > 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
> 
> To help with motivation please be specific (somebody may interpret above
> that it may not be tricky to maintain). So just ... "that are difficult
> to maintain ...".

Rather than the text encouraging questions about whether there are
reasonable alternative approaches, perhaps this can just be, simply:

"To avoid such dependencies, move the affected types into a new
header [...]"

?

> 
> > 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--
> > 
> 
> This explains the motivation for this file well, but its contents
> is not obvious to me and after reading [1] I am more weary of including
> code before use. Not all of these definitions are needed
> by the end of this series so there needs to be a good motivation for
> making things global without any visible user.

That's fair.  I guess we need to review the contents of this header and
make sure that everything that's here really should be here.

However, this is not user ABI and there are only 1.5 users of this
interface (given that MPAM is not yet merged).  So, the penalty for
not getting this quite right and fixing it later seems low.

If you agree that adding this header is appropriate, are you OK with
some post-merge cleanup, or do you think it's essential to sanitise this
fully up-front?

> 
> I do understand that in the next stage of this work we may need to deal
> with potentially three subsystems when making changes and there is thus 
> a strong motivation for laying a good foundation now. I do not think this
> should be an excuse to not be diligent in ensuring the changes are
> required.
> 
> > 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?
> 
> Yes.
> 
> Reinette
> 
> [1] https://lore.kernel.org/lkml/ZhecyLQsGZ9Iv8wU@xxxxxxxxx/
>  

Cheers
---Dave