Received-SPF: pass (google.com: domain of linux-kernel+bounces-175240-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <50d006e0-8488-4579-ab4e-701e1a8f410a@intel.com>
Date: Thu, 9 May 2024 20:18:00 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v3 03/17] x86/resctrl: Detect Assignable Bandwidth
 Monitoring feature details
To: <babu.moger@amd.com>, <corbet@lwn.net>, <fenghua.yu@intel.com>,
	<tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
	<dave.hansen@linux.intel.com>
CC: <x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>,
	<rdunlap@infradead.org>, <tj@kernel.org>, <peterz@infradead.org>,
	<yanjiewtw@gmail.com>, <kim.phillips@amd.com>, <lukas.bulwahn@gmail.com>,
	<seanjc@google.com>, <jmattson@google.com>, <leitao@debian.org>,
	<jpoimboe@kernel.org>, <rick.p.edgecombe@intel.com>,
	<kirill.shutemov@linux.intel.com>, <jithu.joseph@intel.com>,
	<kai.huang@intel.com>, <kan.liang@linux.intel.com>,
	<daniel.sneddon@linux.intel.com>, <pbonzini@redhat.com>,
	<sandipan.das@amd.com>, <ilpo.jarvinen@linux.intel.com>,
	<peternewman@google.com>, <maciej.wieczor-retman@intel.com>,
	<linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<eranian@google.com>, <james.morse@arm.com>
References: <cover.1711674410.git.babu.moger@amd.com>
 <63134ddf18208bbd11bcec0ec75c33ef6e5635b2.1711674410.git.babu.moger@amd.com>
 <ac22b96d-c4bb-42d9-bac8-22b9e032b59e@intel.com>
 <80b33bc6-7b70-4853-92a0-718c2d8728d3@amd.com>
 <753844b6-b68d-4b05-89b0-3ac36e1c8564@intel.com>
 <be20ffe1-ef8c-41fa-b359-9ebfaa326ebc@amd.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <be20ffe1-ef8c-41fa-b359-9ebfaa326ebc@amd.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?VUZyamRpZ29seU10UnkzUG1oMGRucnRNNFE3cmJqU1cybm1KYmFyTDZCeFpx?=
 =?utf-8?B?OExMSGlBVU51S0crVFBEVVVuaGpZSnEyWUhWbXlMMmxuaWZDRVQ5MnVLVVZN?=
 =?utf-8?B?S3d0VFRFdjZSSWYxeEFSaFpzTE42UzgwdDBtUWJrWmVXekcxMHVGL21jV0dW?=
 =?utf-8?B?ZVpCYytTaFJtNGN1YUhZNGlKUkswV01KRCtWVW9RUGpzUDY3OFNJMWhaV3I1?=
 =?utf-8?B?aGhlcFRacUt5ckdFTGFHazFqclJEcjd6d01TbHR3d3dWNnE5K0Y1QUFDbFZm?=
 =?utf-8?B?NWQ1SGdkZ3JpR28vMWdmS2o5K3l0QUFGOWlZSEJ2YllCSkIrcHY0R2Z0TE1k?=
 =?utf-8?B?SU5KVmlIVjlEakt0REdnYXBzK3BHVGtkRUlDSXNrVG94czZUSmkySHdWUGNx?=
 =?utf-8?B?dzhYSWg1emIwNFRCTHVpREdVQnNnL0svZGt0QndJRTFad2l4Ri84UkcyMHh3?=
 =?utf-8?B?bldpY0g1TzNjeEtXSEhLMGpHN01sVzFXejRBZmNBWGw5QVYwWXhieVoraXdo?=
 =?utf-8?B?OUhBSXNZS1Y0RzJJSzhEbGwyNVEzbHhHdUNoY2dvV3BJZXZvZWRMWVgySHha?=
 =?utf-8?B?NFBKcmN3bUZORGhtMGxCZnYxa29XMTlxRUlXZmRVNE0vRFZRZVBtNGNvcTdp?=
 =?utf-8?B?cnlTdG9MSW5QTkE2VExqbnBTenN3NStwRXc1Y2VJczlaVFNBVjBJSGFYRXJq?=
 =?utf-8?B?V0YrNC9IWDR2dGhEblRNZm1RMUMxZlJyVm1WNHI5R0RHZ24vL3ZtRE45a2NE?=
 =?utf-8?B?MU5oVXpCNUpmdmVHcVBTY1RJbEtLa29EUGpueFY1TEg2amFYTitXQ3dydENL?=
 =?utf-8?B?T0hiTndMZC9mYndUYUNQR3BhOFRVaEdYd1RIeHkzMFBnN1VQQzNuMVJPRUFj?=
 =?utf-8?B?ejUzR3hLTmFaRGpCb0NFZ2h0ZWpwSXBqTENVWjJrY2hhSnZydFdUdWZZdjVV?=
 =?utf-8?B?cXZiUXpuZUpSYzhtODRYV1pIbDNVYkE0L3hiUllQT2VRa0Vwb0NCSlUxMHpv?=
 =?utf-8?B?U0hmWlE2eStNRkFiMEM4bysraUxHVDFaZjZ6T3l5MHJRSlZQNmtMWEZGVVM5?=
 =?utf-8?B?SFVySlJFc0dsaXgwWW02KzFnT3ZWL081Wmtxa2pveHphcTY2NDFUMDJIL3RV?=
 =?utf-8?B?U05TUUNDWGIxUkhYZWtUYit1b2FjSjBycFNTamR2YU9rTU8yZk5mRmFQelJS?=
 =?utf-8?B?VnJFSFpST2NpdFFEU1JFRXJMQ09iQU1GcjBvS3B0RFBpUFlJN1FGT0p6WHhm?=
 =?utf-8?B?ZXJ5cHZwbTlCSDRMMncvYyt4OGx5UVd5blY2RzVTam8yeG9sQW5UeG5lVDNT?=
 =?utf-8?B?OXRxQXluN2VnNnp5ZEZlNzlpdThqc3VCbExydHZ4RUliSGhXM2c3Z1lSL2dH?=
 =?utf-8?B?L1ZpZGVIR2ZQSzRPMkZKajQ4c21palZjdnVzaXZlSHVwdEFFVmVFVTZvT3Ix?=
 =?utf-8?B?bzZsMDVxWlRzZVlDRU5jNHdYNkJvQzZvS0VZenEvR044TUljQjJFUld0K3E0?=
 =?utf-8?B?VjdnNGJvVWFJbUd5c0tQekwyV0grR3NGZmVJcFEraERROEV2MUdNaTBTYVdH?=
 =?utf-8?B?WHREOWZHQUg5RDFYZW1DRFVYczg4aXRtTWo4b1lIQ3BIVzZJU2lxc0NRNitL?=
 =?utf-8?B?dDJzUXdKK2hOK3dpZWtreWphalZ0N3V5ZXM5LzhGL0NvTldTMW9yR1JTZHdB?=
 =?utf-8?B?aXFabnl5NFJSK20waUFFUFYxV1NnaWQya0szalNTZzI5a2owbUFvVy9NUnFT?=
 =?utf-8?B?UlAyT21ERFRRbm5Dcjk3NVhTZE90NmFyaFY2aXY0cXM0UWNDaStJblBHdU1i?=
 =?utf-8?B?ZDluMEZSQUYzVU0xbkFyVGRDelN3aVdDTkhJcTYyYXZNYk1mbTgzUjFwVkxN?=
 =?utf-8?B?MUNFbDRPbEpYRTVlZGQ0YXdCRFFPOWdZcHo5SG1aVDNxcjdXMk9EMVdwRjlq?=
 =?utf-8?B?YmJoN3ptT011OU9Gb2U1UFZnZlR1T2g1dVJqK2FNTXMvNTJDdEFSTVZNQUd4?=
 =?utf-8?B?aU1XemltL3dLYzhtVmg5ZVFocjNuSE53M1UxY1VGUk5xS1lSRE1YREZUUzRw?=
 =?utf-8?B?TnlHdEhJcnNBNU1RdllTbDNKMkxzODhYSWlMTExkWnQ0Y1BrdDZTUHVKclVL?=
 =?utf-8?B?bFl1UFB4NmdaRGtCOC9nVm9lZk1sWU9uRmtrVXRhRWd4cTNqWFFCS0hqcnFM?=
 =?utf-8?B?SHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9a06c4dd-48a8-4918-4b97-08dc709fcd77
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2024 03:18:03.0387
 (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: x31T/elYjMgcGe4Y6D1fRRktfCQpaPMiSiyBSc4PEWrJh0bHjEmVHny4wGoOh60Ca8GzDkxyfPzTEfmAGRqVKjNZNJm/Oj8EJF/l8TcTZmk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4663
X-OriginatorOrg: intel.com

Hi Babu,

On 5/9/2024 3:34 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 5/7/24 15:27, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 5/6/2024 12:09 PM, Moger, Babu wrote:
>>> On 5/3/24 18:26, Reinette Chatre wrote:
>>>> On 3/28/2024 6:06 PM, Babu Moger wrote:
>>
>> ...
>>
>>>>> + * @mbm_assign_cntrs:	Maximum number of assignable counters
>>>>>   */
>>>>>  struct rdt_resource {
>>>>>  	int			rid;
>>>>> @@ -188,6 +198,8 @@ struct rdt_resource {
>>>>>  	struct list_head	evt_list;
>>>>>  	unsigned long		fflags;
>>>>>  	bool			cdp_capable;
>>>>> +	bool			mbm_assign_capable;
>>>>> +	u32                     mbm_assign_cntrs;
>>>>>  };
>>>>
>>>> Please check tabs vs spaces (in this whole series please).
>>>
>>> Sure. Will do.
>>>
>>>>
>>>> I'm thinking that a new "MBM specific" struct within
>>>> struct rdt_resource will be helpful to clearly separate the MBM related
>>>> data. This will be similar to struct resctrl_cache for 
>>>> cache allocation and struct resctrl_membw for memory bandwidth
>>>> allocation.
>>>
>>> Did you mean to move all the fields for monitoring to move to new struct?
>>>
>>> Struct resctrl_mbm {
>>>           int                     num_rmid;
>>>           bool                    mbm_assign_capable;
>>>           u32                     mbm_assign_cntrs;
>>> }:
>>>
>>
>> Indeed, so not actually MBM specific but monitoring specific as you state (with
>> appropriate naming?). This is purely to help organize data within struct rdt_resource
>> and (similar to struct resctrl_cache and struct resctrl_membw) not a new
>> structure expected to be passed around. I think evt_list can also be a member.
> 
> How about this?
> 
> Lets keep assign_capable in main structure(like we have mon_capable) and
> move rest of them into new structure.
> 
> Struct resctrl_mon {
>            int                     num_rmid;
> 	   struct list_head        evt_list;
>            u32                     num_assign_cntrs;
> }:

This looks like a good start. It certainly supports ABMC. I do not yet
have a clear understanding about how this will support MPAM and soft-RMID/ABMC
since the current implementation has an implicit scope of one counter per event per
monitor group. It thus seem reasonable to have a new generic name of "cntrs".

Reinette