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

Reinette Chatre
2024-04-20T03:59:59Z

Hi Dave,

On 4/18/2024 8:25 AM, Dave Martin wrote:
> On Wed, Apr 17, 2024 at 10:15:57PM -0700, Reinette Chatre wrote:
>> On 4/16/2024 9:19 AM, Dave Martin wrote:
>>> On Mon, Apr 15, 2024 at 11:03:05AM -0700, Reinette Chatre wrote:
>>>> 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.

ah British spelling, apologies.

>
>>> 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 [...]"
>
> ?

Sure.

>
>>
>>> 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 think you may have sent this before your response to patch #17 where you
are talking about keeping some definitions x86 specific until their usage is
clear.

I understand this is not user ABI and as I also stated previously I recognize
that these changes are easier now than later when changes need to cross two
subsystems. I do not think the goal should be to have the perfect header file
but I would like to understand why each definition in it needs to be global.

Unfortunately, based on learnings during the four year history of this work,
I do not have confidence that there will be post-merge cleanup.

Reinette