Received-SPF: pass (google.com: domain of linux-kernel+bounces-152114-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <7e44410a-895a-4364-82b5-2fc21c88f892@intel.com>
Date: Fri, 19 Apr 2024 20:59:52 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 08/31] x86/resctrl: Move resctrl types to a separate
 header
To: Dave Martin <Dave.Martin@arm.com>
CC: James Morse <james.morse@arm.com>, <x86@kernel.org>,
	<linux-kernel@vger.kernel.org>, Fenghua Yu <fenghua.yu@intel.com>, "Thomas
 Gleixner" <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, "Borislav
 Petkov" <bp@alien8.de>, H Peter Anvin <hpa@zytor.com>, Babu Moger
	<Babu.Moger@amd.com>, <shameerali.kolothum.thodi@huawei.com>, "D Scott
 Phillips OS" <scott@os.amperecomputing.com>, <carl@os.amperecomputing.com>,
	<lcherian@marvell.com>, <bobo.shaobowang@huawei.com>,
	<tan.shaopeng@fujitsu.com>, <baolin.wang@linux.alibaba.com>, Jamie Iles
	<quic_jiles@quicinc.com>, Xin Hao <xhao@linux.alibaba.com>,
	<peternewman@google.com>, <dfustini@baylibre.com>, <amitsinght@marvell.com>,
	David Hildenbrand <david@redhat.com>, Rex Nie <rex.nie@jaguarmicro.com>
References: <20240321165106.31602-1-james.morse@arm.com>
 <20240321165106.31602-9-james.morse@arm.com>
 <0ba51259-cb7a-463e-aabd-f88bd0c4007b@intel.com>
 <ZhlegNN3zZ4Q1lk6@e133380.arm.com>
 <fe8e56bb-55bf-4130-876f-3047da182da1@intel.com>
 <Zh6k9uagoaj9oTl6@e133380.arm.com>
 <a32acb5a-a6e1-4de1-841e-9a1372d624a5@intel.com>
 <ZiE7gritCvxF1XS5@e133380.arm.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZiE7gritCvxF1XS5@e133380.arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cWlZa0p5cEFRNnFmSUxJWDB6cUM3L29mbGpLVGxvaDB0Wmt1eEx2ekVMMjRT?=
 =?utf-8?B?UUtlUTR2TmlPR0x4elRaM1VxbUF0dHFMTGNMdmxmYmlqdE5LdVhQV3Y0Sjgw?=
 =?utf-8?B?Z04vQnM1MzVKYnE2Z1N3cFBidExxOTRieWwwWjBYQ0tPYW02ZW1XY0UrVW95?=
 =?utf-8?B?RTFDcmtpbGFGc1p1ajBtVndHZUh1NzB2RmY1dDgrbW5wdXBQbkg0T28xVmVa?=
 =?utf-8?B?N1FTZGVjZzJyTThjYXFmUVQwM1J1akdnN2EvS1NZcGRJSklncGZ0NG9Dc3VB?=
 =?utf-8?B?Y2dwbFo5SHd5b3hvR3JtNjcwR3hGalhvZWpSOFBxOG5kNlA0OUZnUmd3SVkz?=
 =?utf-8?B?eVhIUWtRWUxBV1lOQzJQNmlsN09iM2M2OTdOd0hOV05ZN0pJUXRnNWdpaWpw?=
 =?utf-8?B?NU5ESVY2S0U2MDRwbW11VmpUNGRMc0FObmU1ZTdZeEJoVVd6U3BLYk9DUVV3?=
 =?utf-8?B?Y0FTOU4xYmxpYXYyMkpiUWpXZk4vRTlsOUI4ZFYreEt1cE9mNFNtcUNMRDBs?=
 =?utf-8?B?V0VsVFZxNnMyd0NtUDkvQ0xOOG1Fc09SWXgwUlgrK2NmN2JXNWx2Z1hJbUFM?=
 =?utf-8?B?QStCRmUrTGxqRVRBZkZzSzNBbG9DK0VIL0gwU002M1dtUFVLTmhRS0JldkNq?=
 =?utf-8?B?QUtHcUlxeTQzbDdqaXFDejdIV0w4bnlNUm9GajRJWC8rR3JObUpyYk9VbFpF?=
 =?utf-8?B?SElaWDhvYi9FY3EwNGhSZWhuQWV5M2lCU2RmMlN6SENWYTZ4UFo1dnAzNXBx?=
 =?utf-8?B?ZzZrbVBXNWFsYmF5UzZvMEoxeVc5WHVhcSt2VTNiZjhYeHUxYkdvZkFXbVd1?=
 =?utf-8?B?a2RjMWlHQ1lJaHpYNks1eUk5VHV3OEx6S1JNVnliRVhsUW1OU2h6ZmQ0azk1?=
 =?utf-8?B?dG9ERHYvdVBnaHFsVWphWXJtOHZNeWlOV29uQy9JeG8vOWowYThoc3lGNTBC?=
 =?utf-8?B?dlRINzJ6bkJDV0hTMTRlR1kycGhhci9ndm9hZmVFSk1hY2VhL1ZYU2xZTEIr?=
 =?utf-8?B?ZjFIcTNONVU5Ky9vVG5sN2lUWGdpeXVlNDI0aWpiR3ZuSkF5WkJlQnpIaEFt?=
 =?utf-8?B?LzBBMEdiV3dhUVFmaTNXKzVkeXllLzczZTBabmNIbkRDUzQxenFveXRtdHRB?=
 =?utf-8?B?SFJVMWpXQzJNeS9LV3MyVmdXamN4ekZYSjhvN3IvVHNYd01URFY5d1Fyek4v?=
 =?utf-8?B?QVIzYVFySUdpQjk2eXptTjRiZENZekROK1lOcGdnQ3QvN0hDb0h4SnRlN2tY?=
 =?utf-8?B?OVI5dDg3VWZzRUp3dGk4OWNZakF1RDZXdWdBeFkrTjhVUFhkdHhXTEpPYVhM?=
 =?utf-8?B?akZqR1Y1OU15NC9kc3FCWWJTUkxIK3dmaGhnZHRocDYvTituQ1BiZ3IyRTRk?=
 =?utf-8?B?SXpSNkZYK1VUZlExc215SDZIVW1UL2x0OU1UZUFSYTY3eFB3d3I2bkNKNE1E?=
 =?utf-8?B?cDFYNmtMelFvU1ZWTFIrTmd2Y0VNcUZzUHpPMHlidmhDK2N6WWNTbkhQOU93?=
 =?utf-8?B?TW1KdGFWYVRhemozTHNwSTZNQlFMU2ZXbEIyWi90cE9EdGlVdkxUNW8zS296?=
 =?utf-8?B?M3BHcDdUa3Q1MytsaHNIcnYzb1JaazZxTUxFRXVJaTZ5NWVPWThEYlpxcWdZ?=
 =?utf-8?B?Y0lwNjF5OVdKRHhXNFFBaHhTdDlTVWxGY1hCemxpbFMvSW5nb0ErNHpWY1Nl?=
 =?utf-8?B?Qnh5eEZCbTBkR25RNkpoQ080ZE9ia2l5RE1tSHM3R1F5MDVMZ0J4dmF1T2NR?=
 =?utf-8?B?a05mcVA3b3k3K0phMmJLM0lDR2tsSkdwMFVmSGk2TzJlcXBwdnJJSXNPUGZW?=
 =?utf-8?B?Ny9JM0g5cUhzeEtiU1BhTmc4bXF2bzlOMDlydkdiM3d3SU9yQ043UEh3cXRB?=
 =?utf-8?B?RkF0UXo1b0JoWFZaYVRBc0dVUXRFbmlaZXc4QlJreUhnM2FmVVU1MlRyZ0ZK?=
 =?utf-8?B?R2pXNGdqNGJOelJrS3ViOTl3UnBNMENEZVVnV2hneEZyVldFNmtWaUhZVUpX?=
 =?utf-8?B?TitOb0VKVFpER0I2T3ZySmtjd3grYXBnMkVXV2d5RkRlY2Joc05OWFdHNDF5?=
 =?utf-8?B?VjRGaDJhR2pFNWFxN2VYVXQwNExWOHk2MDBXMTN6MG5FVEhuUTdIR3JvMTEr?=
 =?utf-8?B?WldKNnZ1aUhrYmNWejE0bGlidlpjVE0vWWczNER2QmpkUGRBd1J5WW1JWWI0?=
 =?utf-8?B?UVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d5e0aa3d-c597-4ee6-c83b-08dc60ee5646
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Apr 2024 03:59:54.7034
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: d0tEQToumJqvPEh0rHZ3MFdIZ9Od/91GLQQqefEjhtokdeyWu6FPfxYpsNYe8ewrRRh0mJM/rpiP4n4/retJviTCUHj2gSMIGnu3Lhhkdms=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6267
X-OriginatorOrg: intel.com

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