Received-SPF: pass (google.com: domain of linux-kernel+bounces-188075-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <f326387d-9636-4377-b82f-10e8d335c6ff@intel.com>
Date: Thu, 23 May 2024 15:31:45 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v18 06/17] x86/resctrl: Introduce snc_nodes_per_l3_cache
To: "Luck, Tony" <tony.luck@intel.com>, "Yu, Fenghua" <fenghua.yu@intel.com>,
	"Wieczor-Retman, Maciej" <maciej.wieczor-retman@intel.com>, Peter Newman
	<peternewman@google.com>, James Morse <james.morse@arm.com>, Babu Moger
	<babu.moger@amd.com>, Drew Fustini <dfustini@baylibre.com>, Dave Martin
	<Dave.Martin@arm.com>
CC: "x86@kernel.org" <x86@kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "patches@lists.linux.dev"
	<patches@lists.linux.dev>
References: <20240515222326.74166-1-tony.luck@intel.com>
 <20240515222326.74166-7-tony.luck@intel.com>
 <beee3369-0075-462e-8449-88fee807463e@intel.com>
 <SJ1PR11MB6083117715CD53FA4360C56CFCF42@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <013c9532-cacf-4604-b1d9-e90fdc80e7dd@intel.com>
 <SJ1PR11MB6083C6A9DFFF58FE305A33F3FCF42@SJ1PR11MB6083.namprd11.prod.outlook.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <SJ1PR11MB6083C6A9DFFF58FE305A33F3FCF42@SJ1PR11MB6083.namprd11.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
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?NHJENmVKdjY1U3Mzb3JRZ1Fmb0FUdGphWm1RMzVZUmIrTDl2WlhUSmtpMG1Y?=
 =?utf-8?B?VU51eXpXaG80Q29yMVd0UUVOekdsMXR4RTJ5aXJDRGJxb0JlMmhLMHFnYlY0?=
 =?utf-8?B?MlhkR3hNNjIyYm4yRU5OUmFYb3VQTUREeWEwTUtyWVBwM0o5TGlzMU1sVWFE?=
 =?utf-8?B?cVpNaGgrVWREeHRUcjJrTHNIK1R2OTUxSmlON1pZS0RCNFpNNTkxMWxldm9V?=
 =?utf-8?B?eEJReHFGK1hNSFlRMXRmT2IvQWNwODRtektjUmtlMlRRUUQ0OFcvMGZ3SUxR?=
 =?utf-8?B?d1dhazdyVENZUDdUZFYwdUNTL3BzRUEvdk45Qjd5Uzk3bXhoOFplRGI5YUpJ?=
 =?utf-8?B?TklOeHdXK1BEWHlzSHoraUJGRVFQZDZ6TXBtd1RQTmlRdG91OFpyR1JMSWJy?=
 =?utf-8?B?d1JLK0wwTGNhSDhQTm5uOVkvaDV1RVl1Mmk3OUdPZm9JTDNQcElyTS9ZS012?=
 =?utf-8?B?OVpJdEI3T3hIUjBTNmhFcjY2NDlaTmhSbGJJdW1aWG5MMHlLMDFVS1VVZXVz?=
 =?utf-8?B?Z2NXTnUxM0JTbnJXVnhiQmVpcHBBWWtJa2NoWGJPKzRBaTg2NktmWWxCN3Jo?=
 =?utf-8?B?eWRVTldmdzJDZ0pIYnVBOEdRZ3g1ak5iR0t0dG44T2xCYTFpbm5jY21uOHNs?=
 =?utf-8?B?Umx3UWY0MzZ1RnY4aGx4SkYrVE5aL3RJOG1OUzZ0eFk3VEErYnMwK3QxK0tP?=
 =?utf-8?B?WWZTcS8rMXp6UlN3MnRVeXBaMVQvZ2RrK2kxSnhYK1NObHRpVnpUT1lDWU40?=
 =?utf-8?B?ck82R3RHcndoNG9hN2V6QlFmc3FNcG1URDhYZ0xLelBDQ3JIa0M0UjBObkJM?=
 =?utf-8?B?KzNSTUptUmc5bXZnWnJiUHNGMU9EWUNYbldPUWZ0eDVicWNmOFhZdEFHS2JI?=
 =?utf-8?B?bDNQL2ZTR25La1l6c21xbTRwMWk5clVvazI1LzNyODZ4UzdTWWpSUWJSUldO?=
 =?utf-8?B?WGt5TWN6akNRZkdYcHR2bDNNTFJqVVFoUE5NckV4YSswM1dqbHI0ei9WcVdZ?=
 =?utf-8?B?aFlBeUROMHQ1OFVPUk5qc0dMYkVMM3k4c2lWWFQxc2NzQUpzQjg0Uk9VOU90?=
 =?utf-8?B?ZmZyY1diNmE4aWx0ZTFGQ0VZTU9FazhhRGhrM0xmUnFzeXRsdnczNi9QdEJ6?=
 =?utf-8?B?TmxQU0RxTEJXRURBOEFiTjZQYXdQSnZuVVZ1UUJKTXl0eVZWS3Y0K2c4LzdN?=
 =?utf-8?B?QUVPZDZkdmZ0OUlvWTZJb1oxS3dpeG1BdlJYZlU5UFRVc1VLajh1MUI1RU9w?=
 =?utf-8?B?ZTdvWEo5WlFnRTlWWDN1bTNTVllPZFNqT2ozM0xSTHcrVjZvTmZwS3M2K2dT?=
 =?utf-8?B?dk5RTUtRN1hhTXdUNHBOQmJ0TjVyVkxrVUtia2h6eUxUNDgyZHNpREVKZkFn?=
 =?utf-8?B?d1c0VjVtWllWTEtaekxQYXAyc21zZE5JTUtQNURPZWNxMUw5dC9LSldieGdm?=
 =?utf-8?B?Ly9BakMzOWhvcEN6Zk85a1FZOXhmQlVXbExLdVdVdllFdlFheVU2emRvN2dz?=
 =?utf-8?B?MGEvTlI1cXlybCtxWmpicG95Szlsdll5cEZhbVNkTG5uTHNNaStyZ3YrdklN?=
 =?utf-8?B?emRmUUFxMEl1a1MvcStzMVIwY0t5U3IyZFR3Z0JpNW1NazBPNjZQZUNPWEFv?=
 =?utf-8?B?cGh5cXFkbWErelYvUENVRFJ4ZkQ0QThnRFhGOXBlb0pXYjhkTG54eU5NejAy?=
 =?utf-8?B?ZkNxQzFLWnpzT1diYjhUVml4VzhRU2IwbkIvR1dBbTI5bGRadWRiSVQwZDlS?=
 =?utf-8?B?ZnB1Tk40UGdmbElWODlHZ3NCOThBdXo3dmh3cXJsVlJaS1ltS0NLdk9wa3hy?=
 =?utf-8?B?ZGVUZVdobEk0RkVyWG4yaG9PcXEzcU1Ra2tqdVFXY25XRFhtU0I0ZFlkbE5y?=
 =?utf-8?B?d21EQVN5U3dQdTBHL3ZuMmQ1dnpvTmxReGQ0MzI1YXFDeDU2endidVUrTkFL?=
 =?utf-8?B?MnI3b0NORDF6aXpPaUI3ZWt2MHBUbEpHZGRDb2h6TUNZTmhQemZEZHdEako4?=
 =?utf-8?B?N2RFVHpDYU52bXFteWUzeEpLcC9TN0cwY1RCTHQwaWdyN21NTkpBWkZ4Qnpu?=
 =?utf-8?B?QkdUdU9RMVV2WXMwa0pJekdoclVoTGdCZHFJRnk3UXBua014cWcvZHhjQVhF?=
 =?utf-8?B?RDlOT2R4K0JTSWpTQ3llY2IxSWRlTE1ibDQxV1lLV3YzdGRvNHlVeVBjU2VC?=
 =?utf-8?B?L1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2eb9e202-3c66-425f-52a5-08dc7b78223e
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 May 2024 22:31:48.1647
 (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: IB7VDu7lz9ToNDUbAByLPsQ+Btir9ag4KmwEaIwt2nv1EuCnZ3oou0l/zzWMluvq7eFbg0Q5thtg0ZxR/uMb71yaRjGBbWtzlJ29CaQbG1Q=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7811
X-OriginatorOrg: intel.com

Hi Tony,

On 5/23/24 2:25 PM, Luck, Tony wrote:
>>> I'm also contemplating dropping snc_nodes_per_l3_cache from being a
>>> global variable and making it a field in "struct rdt_resource" (only needed
>>> for the RDT_RESOURCE_L3 resource). N.B. Babu had suggested it
>>> shouldn't be global many patch versions ago.
>>>
>>> Perhaps name it .domains_per_l3_cache or .subdomains_per_l3_cache?
>>>
>>> Bad idea? Good idea (but you have a better name for the field)?
>>
>> With the check in supports_mba_mbps() changed I do not see
>> snc_nodes_per_l3_cache needed by anything but arch specific code.
>> I thus do not see any reason for the resctrl filesystem (or, for
>> example, Arm) to know that this value even exists.
>>
>> While struct rdt_hw_resource is a place to put architecture
>> specific information it does not seem appropriate to force every
>> resource to carry what is essentially a system wide (not specific to
>> resctrl) L3 specific property. To me this really seems like an
>> architecture specific global setting but I'd also like to hear the
>> motivations why it should not be.
> 
> So (in arch/x86/kernel/cpu/resctrl/monitor.c)
> 
> static int snc_nodes_per_l3_cache = 1;
> 
> Set and use only in this (arch specific) file.

Since this series initializes this value in
arch/x86/kernel/cpu/resctrl/core.c it is not clear to
me from just this one line how you envision the changes.

Just to be clear ... when I refer to arch specific and
filesystem code I am considering how this series integrates with [1]
since that is the direction resctrl is headed in.
Being "arch specific" thus does not require that it be moved into
monitor.c - it could be added to arch/x86/kernel/cpu/resctrl/internal.h
where it can remain after the fs/arch split.

It will be very helpful if you view your series while taking
[1] into account. For example, when looking at [1] you will see that
mon_event_count() and __mon_event_count() are resctrl filesystem
functions. When you consider that it should be clear that adding
an arch specific get_node_rmid() between these functions will make
the arch/fs split more difficult.

Reinette

[1] https://lore.kernel.org/lkml/20240321165106.31602-1-james.morse@arm.com/