[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7e44410a-895a-4364-82b5-2fc21c88f892@intel.com>
Date: Fri, 19 Apr 2024 20:59:52 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Dave Martin <Dave.Martin@....com>
CC: James Morse <james.morse@....com>, <x86@...nel.org>,
<linux-kernel@...r.kernel.org>, Fenghua Yu <fenghua.yu@...el.com>, "Thomas
Gleixner" <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, "Borislav
Petkov" <bp@...en8.de>, H Peter Anvin <hpa@...or.com>, Babu Moger
<Babu.Moger@....com>, <shameerali.kolothum.thodi@...wei.com>, "D Scott
Phillips OS" <scott@...amperecomputing.com>, <carl@...amperecomputing.com>,
<lcherian@...vell.com>, <bobo.shaobowang@...wei.com>,
<tan.shaopeng@...itsu.com>, <baolin.wang@...ux.alibaba.com>, Jamie Iles
<quic_jiles@...cinc.com>, Xin Hao <xhao@...ux.alibaba.com>,
<peternewman@...gle.com>, <dfustini@...libre.com>, <amitsinght@...vell.com>,
David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>
Subject: Re: [PATCH v1 08/31] x86/resctrl: Move resctrl types to a separate
header
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
Powered by blists - more mailing lists