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

[Reinette Chatre]

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