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

[Reinette Chatre]

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

> 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 ...".

> 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.

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/