Re: [PATCH v4 00/24] x86/resctrl: Merge the CDP resources

From: Reinette Chatre <reinette.chatre@intel.com>
To: James Morse <james.morse@arm.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org
Cc: 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,
	Jamie Iles <jamie@nuviainc.com>,
	D Scott Phillips OS <scott@os.amperecomputing.com>,
	lcherian@marvell.com, "Luck, Tony" <tony.luck@intel.com>
Subject: Re: [PATCH v4 00/24] x86/resctrl: Merge the CDP resources
Date: Tue, 15 Jun 2021 11:05:40 -0700	[thread overview]
Message-ID: <f0f112c3-e17d-958f-2378-b73643c7ca58@intel.com> (raw)
In-Reply-To: <4d2ba7c1-395b-d1ec-c92b-f906e2a551a1@arm.com>

Hi James,

On 6/15/2021 9:48 AM, James Morse wrote:
> On 15/06/2021 17:16, Reinette Chatre wrote:
>> On 6/14/2021 1:09 PM, James Morse wrote:

>> For the most part I think this series looks good. The one thing I am concerned about is
>> the resctrl user interface change. On a system that supports L3 CDP there is a visible
>> change when CDP is not enabled:
>>
>> Before this series:
>> # cat schemata
>>     L3:0=fffff;1=fffff
>>
>> After this series:
>> # cat schemata
>> L3:0=fffff;1=fffff
> 
> Hmm, I thought I'd fixed this with v2, ... I see this is subtly different.
> 
> This could be tweaked by getting schemata_list_add() to include the length of the longest
> suffix if the resource supports CDP, but its not enabled. (Discovering that means
> cdp_capable moves to be something the 'fs' bits of resctrl can see.)
> 
> I'm a little nervous 'adding 4 spaces' because user-space expects them. (I agree if it
> breaks user-space it has to be done). I guess this is the problem with string parsing as
> part of the interface!

This is a tricky and interesting one. It seems that the original 
intended behavior is indeed the way you changed it to. By originally 
using for_each_enabled_rdt_resource() to determine the maximum width in 
de016df88f23 ("x86/intel_rdt: Update schemata read to show data in 
tabular format"). This was added in v4.12 and dictated the interface 
until v4.13. This was changed in 1b5c0b758317 ("x86/intel_rdt: Cleanup 
namespace to support RDT monitoring") when it used 
for_each_alloc_capable_rdt_resource(r) instead, added in v4.14 that is a 
stable kernel and the most likely interface used by users.

To me the less risky change is to maintain the existing interface, but 
perhaps there are some other guidance in this regard?

> I assume that in the (distant) future having CDP capable resources with names more than 2
> characters isn't going to be a problem. (I don't have an example)

The last statement is not clear to me. Could you please elaborate why 
two characters would be significant? From what I understand the 
expectation would be that the width is the maximum name length of all 
possible schema, whether they are enabled or not.

>> There are a few user space tools that parse the resctrl schemata file and it may be easier
>> to keep the interface consistent than to find and audit them all to ensure they will keep
>> working.

To me this continues the biggest hurdle in maintaining the behavior as 
you have it in this series.

>> A heads-up is that there are some kernel-doc fixups in the works that will conflict with
>> your series. You yourself fix at least one of these kernel-doc issues in this series - the
>> description of mbm_width in the first patch. I will ask the submitter of the kernel-doc
>> fixups to use your text to help with the merging.
> 
> Please point me at something to rebase onto!
> (as far as I can see, tip/x86/cache hasn't moved)

These patches have not yet been merged. The most recent version was sent 
yesterday. Your current base is good.

Reinette