Received-SPF: pass (google.com: domain of linux-kernel+bounces-147533-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <f4265c87-4f57-46fe-bb1d-b4d1ce3df06f@amd.com>
Date: Tue, 16 Apr 2024 15:44:29 -0500
User-Agent: Mozilla Thunderbird
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v1 02/31] x86/resctrl: Add a helper to avoid reaching into
 the arch code resource list
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>,
 Reinette Chatre <reinette.chatre@intel.com>,
 Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
 Borislav Petkov <bp@alien8.de>, H Peter Anvin <hpa@zytor.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-3-james.morse@arm.com>
 <0790c576-944a-45f8-bc43-01eb0a8f67a9@amd.com>
 <Zh6kDStUw8AThV4s@e133380.arm.com>
Content-Language: en-US
From: "Moger, Babu" <babu.moger@amd.com>
In-Reply-To: <Zh6kDStUw8AThV4s@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?OFErTDF4Q29VdUIxcGM2Q0RhU0p6QmNubEJEdklsUzhEWE9WVVJ6MTNrTTVp?=
 =?utf-8?B?NEw2TE9NRjJJQU9KWE9Ga2l1d0c5MHRFOTROYmZDMUVTblZOSlAyL3ZkR0E5?=
 =?utf-8?B?MUJRNUphSkRIaFBRalJSZUhWTDREbXczUGdlRW91bFRSUGE3NUg0ZVZNMGpJ?=
 =?utf-8?B?T29ud1cxRTRsS0tENFY4UnpELzdrZTFaQ3c3by9pekNJQXpNa3oyK1lRNnI2?=
 =?utf-8?B?dlZDbUQvOTRaZTdXQlZHUXZXcmdzbzAwbkt3WXR1SVBSd0V2MWVNSFZxcHlT?=
 =?utf-8?B?VUE4QVR4c1FpWTRydUt1cTdPOGl5aytQWTVYWnl6N2czaStYRE5vSytucjhP?=
 =?utf-8?B?cHlmMTk2NU1MQnI5S3Y1eEV2QlcyYy9YZHd6QTlPQjQvVUcyMzlITUg5aVJS?=
 =?utf-8?B?OTBvQVBrV0ZOT1ZWYjllUXFsWE4ydzR0YWR3eHl0M013Z2Z1OFBSK2l6NytY?=
 =?utf-8?B?U2ExdUVOTGFWR2REc1daNWFzVVJ2dzNYaUQ3OHFWNFlPN1FrYXphUkF0cFp3?=
 =?utf-8?B?VFJobldHYkZnUDFFQjhpSXBQajZtM2E1djQ2NU50ZXd3a09BWi9JaFVSY1Ix?=
 =?utf-8?B?SlJpRDF3aHJjZm9HYTRMZDNKenpqdU1xY3AveXkvYzE4TnhHeDJpQURMaDZZ?=
 =?utf-8?B?MlI1ZDcxeHBYNVdNaFFhRTIvSk1ReEtPYlVaTkx1MkgyaWxzU1lKWnZGN0R5?=
 =?utf-8?B?RW9MdWNOUTJldXhuaWlHV0Q1M1FuYVp0TGZyMUpmQjN5cHBUQ3htQk1NMity?=
 =?utf-8?B?S05UcldETjNMUDhKVi9zK2JaKzFWOUtmZHQ0eU9wZjJDYWI5Y1EySTdCZFVm?=
 =?utf-8?B?UENQNko3UlJ0RVkyWlUyWnUrUnVvbStxRFB4aFV2R3dtS1BUQ0NzL3ZFZ1FE?=
 =?utf-8?B?Q0NFYmhYZmNBNWtOMmlnUVVyb1BHaDA5V3lsNzRhdGxjZTFXU3dYM2F4U1Bx?=
 =?utf-8?B?aEtSdGxCSXpTUHRzYnUvK29Oby8xQUxoRzJ1RXkvMWozZGhkb0ZBVEM4KzB2?=
 =?utf-8?B?Qis0dzQ1ZjlFT1ZoRUsyNXVGbDk4cnlLWmFJcjBIaUNNMHFmMzF3czBkV0pC?=
 =?utf-8?B?RXJ0WUtWRXRCM3BXd2dLS2Q5SkRLQjBhcjNhYU9vRWNOeWp1T1VrUHRwQVhL?=
 =?utf-8?B?dzVIYkpLSm5GOVdGNEQ0c0xhL0Y4eFoyMzdyemEzTzhkYk1ZZVhjcXUwQjU4?=
 =?utf-8?B?bjE5Q0QxcGl4RXM4bWxVeURQU3NRQUY3Mm5NY1I3cTRKeStPbW5jbzlvR1o1?=
 =?utf-8?B?bDlFWkZQakxmdkd3ejFyVE1QK25MUnI0LzlYM24yWUo2TWt6eXZRNWdDbE42?=
 =?utf-8?B?NzIrT2NFODFLT0x5V0o2cktuNFRCMHBhSWtGOGNmblpiNk1zbEFUY01VaXdP?=
 =?utf-8?B?dTdTbmQ4aVlQOGJCT2E0WFBQZnZPNXR3RVBWMWRLVlhuRmhxTlF6RGhEUjlw?=
 =?utf-8?B?eDNIMXhVZ3RheTRRUlVoQS8xRmFzSUt5YTMwY1RqWUdMaGNTMk9SelM5ZEp4?=
 =?utf-8?B?aFcxRlhrbUtZdENZNE1sTUFZczlkUkxWVXV5ODFtMjAwbnpPTis4OHNzamtv?=
 =?utf-8?B?T1htVkIrQUM3Skg1ci8xZ1JhK0lCdHJkWEdSNTVEdlNTdDhpZ2dDOU5OV1pK?=
 =?utf-8?B?aU1WaHpIWDMxczBuT2FzdmVnNGZKL1ZwN080dEFUMjlIS3RncGpFQVdlMGlB?=
 =?utf-8?B?dThPbjNHS3NCOFplVjNrZHk0dXJnRDFDcnEwdFgweEEvUWl6THJndDUyR3lp?=
 =?utf-8?B?dXBrbVBDMmZxckZNSmc3YS9vUEJqUjIvNi9zUSszclB2NitsOElGQ0hHbUhT?=
 =?utf-8?B?VEUyNDZidTBpS0R0ajBJVHhqbUxZcDY0QVc3VWd5UFovdldoWDlwRUlTcjRp?=
 =?utf-8?B?RmhoYnpxUnVrakJLUlVjVFZLR2c2a2dIWDRlT05ucXJUUCtlOVYram5reGg5?=
 =?utf-8?B?ZlR5a2ZQRHBXVkJPd0FvMCt1TnNBSlhYaFRXamNRQ2k4YThWZHFaWTFPa1ZN?=
 =?utf-8?B?T0dhZEhRdEJZT2ppR1V2NlBzK2R5K3ZCcDczeDJMbE1uRkRhRlR4THEvS3R1?=
 =?utf-8?B?TmJZMDVSSWZBdHlTelN6dlVSd1d2enI5NlkrSHBGTytPVkxreWFEd0d5MElX?=
 =?utf-8?Q?hSVA=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d9e4215e-a99c-409c-1746-08dc5e560556
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Apr 2024 20:44:33.1016
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: oFuKL067VbrqU7vWJrHi2X45qP0QSoqgF37Uaw2c4sYv3G9u8psGEt6b8mSg4tYD
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB6205

Hi Dave,

On 4/16/24 11:15, Dave Martin wrote:
> On Mon, Apr 15, 2024 at 03:28:18PM -0500, Moger, Babu wrote:
>> Hi James/Dave,
>>
>> On 3/21/24 11:50, James Morse wrote:
>>> Resctrl occasionally wants to know something about a specific resource,
>>> in these cases it reaches into the arch code's rdt_resources_all[]
>>> array.
>>>
>>> Once the filesystem parts of resctrl are moved to /fs/, this means it
>>> will need visibility of the architecture specific struct
>>> resctrl_hw_resource definition, and the array of all resources.
>>> All architectures would also need a r_resctrl member in this struct.
>>>
>>> Instead, abstract this via a helper to allow architectures to do
>>> different things here. Move the level enum to the resctrl header and
>>> add a helper to retrieve the struct rdt_resource by 'rid'.
>>>
>>> resctrl_arch_get_resource() should not return NULL for any value in
>>> the enum, it may instead return a dummy resource that is
>>> !alloc_enabled && !mon_enabled.
>>
>> Nit.
>> You may want to drop the second half of the statement. We don't have a
>> dummy resource.
> 
> I guess not, but MPAM will, although I haven't fully understood the
> logic.  See:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/mpam_resctrl.c?h=mpam/snapshot/v6.7-rc2
> 
> (Search for "dummy".)
> 
> 
> In any case, the statement above is part of the definition of the new
> interface: the resctrl core code is going to explicitly need to cope
> with a dummy resource being returned, and the arch code is required
> to return a pointer to something and not NULL.
> 
> So I would say that it is appropriate (or, at the very least, harmless)
> to keep that statement here?

Ok. fine.
> 
>>
>>>
>>> Signed-off-by: James Morse <james.morse@arm.com>
>>> ---
>>>  arch/x86/kernel/cpu/resctrl/core.c        | 10 +++++++++-
>>>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  2 +-
>>>  arch/x86/kernel/cpu/resctrl/internal.h    | 10 ----------
>>>  arch/x86/kernel/cpu/resctrl/monitor.c     |  8 ++++----
>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 15 +++++++--------
>>>  include/linux/resctrl.h                   | 17 +++++++++++++++++
>>>  6 files changed, 38 insertions(+), 24 deletions(-)
>>>
> 
> [...]
> 
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 1767c1affa60..45372b6a6215 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> 
> [...]
> 
>>> @@ -2625,10 +2625,10 @@ static void schemata_list_destroy(void)
>>>  
>>>  static int rdt_get_tree(struct fs_context *fc)
>>>  {
>>> +	struct rdt_resource *l3 = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>>
>> Its is probably best to keep the resource name as r here to be consistent
>> with other changes.
>>
>>>  	struct rdt_fs_context *ctx = rdt_fc2context(fc);
>>>  	unsigned long flags = RFTYPE_CTRL_BASE;
>>>  	struct rdt_domain *dom;
>>> -	struct rdt_resource *r;
>>>  	int ret;
>>>  
>>>  	cpus_read_lock();
>>> @@ -2701,8 +2701,7 @@ static int rdt_get_tree(struct fs_context *fc)
>>>  		resctrl_mounted = true;
>>>  
>>>  	if (is_mbm_enabled()) {
>>> -		r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>> -		list_for_each_entry(dom, &r->domains, list)
>>> +		list_for_each_entry(dom, &l3->domains, list)
>>>  			mbm_setup_overflow_handler(dom, MBM_OVERFLOW_INTERVAL,
>>>  						   RESCTRL_PICK_ANY_CPU);
>>>  	}
>>> @@ -3878,7 +3877,7 @@ static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
> 
> [...]
> 
>> Thanks
>> Babu Moger
> 
> [...]
> 
> Yes, this does look a bit odd.
> 
> This looks like a no-op change to me -- I think that
> resctrl_arch_get_resource() is supposed to be without side-effects,
> so I would have expected this to be a one-line change at the assignment
> to r, with no particular need for renaming r.
> 
> Does that make sense to you, or is there some complexity I'm not
> noticing here?

No other complexity..  Just keep the variable name as r.

struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);

-- 
Thanks
Babu Moger