Received-SPF: pass (google.com: domain of linux-kernel+bounces-49174-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: <7c26af23-fa1e-4e01-8088-8fbd9be3d6f3@intel.com>
Date: Thu, 1 Feb 2024 20:09:15 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 00/17] x86/resctrl : Support AMD Assignable Bandwidth
 Monitoring Counters (ABMC)
Content-Language: en-US
To: Babu Moger <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>
References: <20231201005720.235639-1-babu.moger@amd.com>
 <cover.1705688538.git.babu.moger@amd.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <cover.1705688538.git.babu.moger@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?c2JORnpFSTBDaENjcXhaMmxXdzlxbzVBTzNVVHJkczdaTzdIMlcyZ2w5SXhP?=
 =?utf-8?B?eFMzR1liSVFMaGpjQTBiaTdnM0ZZQU9IM0RGamdKL29CanJHS0tyNzNMK3R6?=
 =?utf-8?B?bFE4TDBUQlVSb1VyNFB4RjFGMVF0cDhMbHZUMjJIWFRpSURObjJDRXgvejlV?=
 =?utf-8?B?VFBnMmZ0S0phWE5tNXZDUEhKSGc2MklFQ3VzcU4ybFlKS29MdlVRYnRBNXFs?=
 =?utf-8?B?RVUwZnRSdlEwZXlZT09SRVNqcEFNRU1Nc25TRWhxbHN5OG4ydEprNm9zbjly?=
 =?utf-8?B?MW52S1E1Wlozb2luYzRGOWhMcDg0dC9LZzFBdHJrMW8zWXdEcWNiVi9JM0V3?=
 =?utf-8?B?UFNmcmxQSXlkUWFJRDQ0VmNkNnR2N2FiVTlyRVVvOS9iYUpRd25hbGU5MldD?=
 =?utf-8?B?V3dDck12bWJnQXpZblJaeGUraDB1ZWhGbmVlKy90OEFoU2RLQTFYaEI5b0JF?=
 =?utf-8?B?Nkg0WHpnUFVXeVVQbzZ5bVJaRDNKTVZVanNNZDZjTkpoWmJDSTliQ3d2aTFN?=
 =?utf-8?B?TUZ2Mlowc1V3VnNyYWRiZURxQmNNTUdFQVNhbXFVYTlDSDZzM3gxZHBZTGIw?=
 =?utf-8?B?UXRmRGJZQmZnTUFuSkVyMGl3bDluNmo5Uy8xRzV3RmVsYmg2dVhjU0xqekRm?=
 =?utf-8?B?TE9Md0FZd2tQbStlTTZJemc2WFpiWUcwWjBDUFZad09xTm1pZHZhQUJ5T2Qr?=
 =?utf-8?B?V0dzL1BZb0RpWXBkVnpPeVpxRk11Wi8xV29kdUE3U2pHV0I2MjlmelJWUTlL?=
 =?utf-8?B?WEZPdzJkNTdjWlE3MERwejNkd0U5OWRiN2xhSGR0bytIdVFES3dqZ0pFR2hz?=
 =?utf-8?B?dWk0QjQ4L0UrWU5IaThpaUtMMW1TUDQ4L2JUTjBidUxWMzF0a2ovWW16eEh6?=
 =?utf-8?B?OW5uYkZtcFpqRXhvWjBOVk5na0w5SE4vdm42bEN0MWFKb0hZcE1wRVI1NzFx?=
 =?utf-8?B?MUJLa0l3M0ErU2JuT2RVaGlXbUlUOS9rSTZZUHJkOFowWWNkT3Z4UUtnMFpt?=
 =?utf-8?B?SjZvbXJUczFuWEFDTUZrTGdEYkVJSzlmb1Y2WEl2S1JyeTFEVWZxMVJ2UDN4?=
 =?utf-8?B?Um9KWE5zZHNkVWVpRDJKZTQxRmlzc29HWTdaQU5VVlpHSmRKMkRiZk4yWnBQ?=
 =?utf-8?B?U2V5N2kvZHRNdjF0cCtRVERZVExEUVZPaGZORjJsTjdXR216QWhTUUpsUjlt?=
 =?utf-8?B?SzQ0dUF4SEVocjUzQnBJSkhSR3pTcHZrZWZ3RWxjUTJSbzJtRmY5UWhCWmd4?=
 =?utf-8?B?c0hZalZ3cVppdm52NnN1NXJacTlMcXFIQU4rKzdqZytvRkFMVGFiTGRwR0hC?=
 =?utf-8?B?MkV5dDR1WDlPK0lydmw1dzBXdU1KajhQdmJiU21RNkFyekROc3E3NUx3Ukxi?=
 =?utf-8?B?UGpNaU5uZGRFbkxsL3pBM2grdnRJSWt2UXJ2eDdLWlI4NitwVHU5d21rRUhJ?=
 =?utf-8?B?K0tGeWdJUHlpSXJHbzZ3UG11Zlh4b3JFbEdGWmJuckNaNHhOMlUzVDVxaVdW?=
 =?utf-8?B?SWFySzhpME5HWVozMUhpM2dYS1BJbTRyM3ZKTGZ5U094dHozTHFNelZtWVRu?=
 =?utf-8?B?akp2dVluM3VwVXBNUVFPSWRGS3JIbUNrNkpTb2pJQVJ5SWMyOGx5cjJGaHBB?=
 =?utf-8?B?ZmVRSGRMRkwxbGNsc3ROT0pTZDVJcTVtSzJJM1dmMTExR2RXcFl1VkYwMDZD?=
 =?utf-8?B?a2RFMXR1SnhYYVA2Q1hHbG9ka2drblpDa0NBREwxMURXYlBrZVIwQTZFb2Nu?=
 =?utf-8?B?NjQ5NkVhSzFldVBnbE5nOU5tSE5LYVJuYjNpZHBWdm1pbVZza1I2dzJLMDVy?=
 =?utf-8?B?bnlaSHd3T1d1dmhZT2w4aThBbGNpcDIyOGh6L1p0aERjSjA2U29CNmJOZWdU?=
 =?utf-8?B?czJmNGFCOWNiNkZ2NjFFczlEcGhHRXpQODJlTzY1OUdWMER3UE1PN1cxNEpo?=
 =?utf-8?B?U0lwOTlLSGxmekE5YXlnN0hyb2VRaFlqc1RwZmhobld5Q3R3Szlha1Y2YUZE?=
 =?utf-8?B?c0gwTnlqTitpdHF2Qm9uUjRXTnhUdEZXS2U3ZUZzcmlLaldjQnB1OEtRTUQw?=
 =?utf-8?B?bmhQT1B5dXhMU3V4VWFwYW1pU1VTR0RmcnlQZDY3SFF2TjZ5VGsvWGpqSWg2?=
 =?utf-8?B?bW9SdGl4ZDhyK2l5blkzdDlyU1NJYU5Bb21WS1ZKUFB2bTVweDE2bTZxVkhn?=
 =?utf-8?B?M2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 461ecda7-03bb-42f7-2a09-08dc23a4ba79
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2024 04:09:19.1149
 (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: nLkzPJKryzcLpwBui9LSrJJndCERpRQZ43hFIOcfyeSD2cLd+McMu+qNGddA0d/z7WGOusmoJBPVkbam1axd8wUFhc/AzGcXSz9f3r5W/oo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6389
X-OriginatorOrg: intel.com

Hi Babu,

On 1/19/2024 10:22 AM, Babu Moger wrote:
> 
> These series adds the support for Assignable Bandwidth Monitoring Counters

Not a good start ([1]).

> (ABMC). It is also called QoS RMID Pinning feature
> 
> The feature details are documented in the  APM listed below [1].
> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
> Monitoring (ABMC). The documentation is available at
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> 
> The patches are based on top of commit
> 1ac6b49423e83af2abed9be7fbdf2e491686c66b (tip/master)
> 
> # Introduction
> 
> AMD hardware can support 256 or more RMIDs. However, bandwidth monitoring
> feature only guarantees that RMIDs currently assigned to a processor will
> be tracked by hardware. The counters of any other RMIDs which are no longer
> being tracked will be reset to zero. The MBM event counters return
> "Unavailable" for the RMIDs that are not active.
>     
> Users can create 256 or more monitor groups. But there can be only limited
> number of groups that can be give guaranteed monitoring numbers.  With ever

"can be given"?

> changing configurations there is no way to definitely know which of these
> groups will be active for certain point of time. Users do not have the
> option to monitor a group or set of groups for certain period of time
> without worrying about RMID being reset in between.
>     
> The ABMC feature provides an option to the user to assign an RMID to the
> hardware counter and monitor the bandwidth for a longer duration.
> The assigned RMID will be active until the user unassigns it manually.
> There is no need to worry about counters being reset during this period.
> Additionally, the user can specify a bitmask identifying the specific
> bandwidth types from the given source to track with the counter.
> 
> Without ABMC enabled, monitoring will work in current mode without
> assignment option.
> 
> # Linux Implementation
> 
> Linux resctrl subsystem provides the interface to count maximum of two
> memory bandwidth events per group, from a combination of available total
> and local events. Keeping the current interface, users can assign a maximum
> of 2 ABMC counters per group. User will also have the option to assign only
> one counter to the group. If the system runs out of assignable ABMC
> counters, kernel will display an error. Users need to unassign an already
> assigned counter to make space for new assignments.
> 
> 
> # Examples
> 
> a. Check if ABMC support is available
> 	#mount -t resctrl resctrl /sys/fs/resctrl/
> 
> 	#cat /sys/fs/resctrl/info/L3_MON/mon_features 
> 	llc_occupancy
> 	mbm_total_bytes
> 	mbm_total_bytes_config
> 	mbm_local_bytes
> 	mbm_local_bytes_config
> 	mbm_assign_capable ←  Linux kernel detected ABMC feature
> 
> b. Check if ABMC is enabled. By default, ABMC feature is disabled.
>    Monitoring works in legacy monitor mode when ABMC is not enabled.
> 
> 	#cat /sys/fs/resctrl/info/L3_MON/mbm_assign_enable
> 	0
> 

With the introduction of "mbm_assign_enable" the entry in mon_features seems
to provide duplicate information.

> c. There will be new file "monitor_state" for each monitor group when ABMC
>    feature is supported. However, monitor_state is not available if ABMC is
>    disabled.
> 	
> 	#cat /sys/fs/resctrl/monitor_state 
> 	Unsupported

This sounds potentially confusing since users will still be able to monitor
the groups ...

> 	
> d. Read the event mbm_total_bytes and mbm_local_bytes. Without ABMC
>    enabled, monitoring will work in current mode without assignment option.
> 	
> 	# cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes
> 	779247936
> 	# cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_bytes 
> 	765207488
> 	
> e. Enable ABMC mode.
> 
> 	#echo 1 > /sys/fs/resctrl/info/L3_MON/mbm_assign_enable
>         #cat /sys/fs/resctrl/info/L3_MON/mbm_assign_enable
>         1
> 
> f. Read the monitor states. By default, both total and local MBM
> 	events are in "unassign" state.
> 	
> 	#cat /sys/fs/resctrl/monitor_state
> 	total=unassign;local=unassign

This interface does not seem to take into account that hardware
can support assignment per domain. I understand that this is
not something you want to implement at this time but the user interface
has to accommodate such an enhancement. This was already mentioned, and
you did acknowledge the point [3] to this new version that does not
reflect this is unexpected.

My previous suggestions do seem to still stand and and I also am not able to
see how Peter's requests [2] were considered. This same interface needs to
accommodate usages apart from ABMC. For example, how to use this interface
to address the same counter issue on AMD hardware without ABMC, and MPAM
(pending James's feedback). 

I understand that until we hear from Arm we do not know all the requirements
that this interface needs to support, but I do expect this interface to
at least consider requirements and usage scenarios that are already known.
 
> g. Read the event mbm_total_bytes and mbm_local_bytes. In ABMC mode,
>    the MBA events are not available until the user assigns the events
>    explicitly.
> 	
> 	#cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes
> 	Unsupported
> 	
> 	#cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_bytes 
> 	Unsupported
> 

This needs some more thought to accommodate Peter's scenario where the counter
can be expected to return the final count after the counter is disabled.

> h. The event llc_occupancy is not affected by ABMC mode. Users can still
>    read the llc_occupancy.
> 
> 	#cat /sys/fs/resctrl/mon_data/mon_L3_00/llc_occupancy 
> 	557056
> 
> i. Now assign the total event and read the monitor_state.
> 	
> 	#echo total=assign > /sys/fs/resctrl/monitor_state
> 	#cat /sys/fs/resctrl/monitor_state 
> 	total=assign;local=unassign
> 	

I do not see the "global assign/unassign" scenario addressed.

This version seems to ignore (without discussion) a lot of earlier
feedback.

Reinette

[1] https://lore.kernel.org/lkml/5ce67d8f-e207-4029-8fb3-0bc7deab1e9f@amd.com/
[2] https://lore.kernel.org/lkml/CALPaoCiRD6j_Rp7ffew+PtGTF4rWDORwbuRQqH2i-cY5SvWQBg@mail.gmail.com/
[3] https://lore.kernel.org/lkml/38421428-84cb-b67e-f3ce-b7a0233e016b@amd.com/