Message-ID: <df9b3513-1a83-474f-8772-f10cf96c14f1@amd.com>
Date: Wed, 16 Oct 2024 10:57:47 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, corbet@....net,
 fenghua.yu@...el.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, paulmck@...nel.org, rdunlap@...radead.org,
 tj@...nel.org, peterz@...radead.org, yanjiewtw@...il.com,
 kim.phillips@....com, lukas.bulwahn@...il.com, seanjc@...gle.com,
 jmattson@...gle.com, leitao@...ian.org, jpoimboe@...nel.org,
 rick.p.edgecombe@...el.com, kirill.shutemov@...ux.intel.com,
 jithu.joseph@...el.com, kai.huang@...el.com, kan.liang@...ux.intel.com,
 daniel.sneddon@...ux.intel.com, pbonzini@...hat.com, sandipan.das@....com,
 ilpo.jarvinen@...ux.intel.com, peternewman@...gle.com,
 maciej.wieczor-retman@...el.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, eranian@...gle.com, james.morse@....com
Subject: Re: [PATCH v8 07/25] x86/resctrl: Introduce the interface to display
 monitor mode

Hi Reinette

On 10/15/24 22:12, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/9/24 10:39 AM, Babu Moger wrote:
>> Introduce the interface file "mbm_assign_mode" to list monitor modes
>> supported.
>>
>> The "mbm_cntr_assign" mode provides the option to assign a counter to
>> an RMID, event pair and monitor the bandwidth as long as it is assigned.
>>
>> On AMD systems "mbm_cntr_assign" is backed by the ABMC (Assignable
>> Bandwidth Monitoring Counters) hardware feature and is enabled by default.
>>
>> The "default" mode is the existing monitoring mode that works without the
>> explicit counter assignment, instead relying on dynamic counter assignment
>> by hardware that may result in hardware not dedicating a counter resulting
>> in monitoring data reads returning "Unavailable".
>>
>> Provide an interface to display the monitor mode on the system.
>> $cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
>> [mbm_cntr_assign]
>> default
>>
>> Switching the mbm_assign_mode will reset all the MBM counters of all
>> resctrl groups.
> 
> Please note that this now contradicts the documentation. Perhaps this sentence
> can just be dropped since there is the documentation within the patch.	

Sure. Will drop it.

> 
> 
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
> 
> ...
> 
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 30586728a4cd..e4a7d6e815f6 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -257,6 +257,40 @@ with the following files:
>>  	    # cat /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config
>>  	    0=0x30;1=0x30;3=0x15;4=0x15
>>  
>> +"mbm_assign_mode":
>> +	Reports the list of monitoring modes supported. The enclosed brackets
>> +	indicate which mode is enabled.
>> +	::
>> +
>> +	  cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
>> +	  [mbm_cntr_assign]
>> +	  default
>> +
>> +	"mbm_cntr_assign":
>> +
>> +	In mbm_cntr_assign mode user-space is able to specify which control
>> +	or monitor groups in resctrl should have a counter assigned using the
> 
> Counters cannot be assigned to control groups. How about replacing all instances
> of "control and monitor groups" with "CTRL_MON and MON groups", similarly
> "control or monitor groups" with "CTRL_MON or MON groups".

Ok.

> 
>> +	'mbm_assign_control' file. The number of counters available is described
> 
> Looking at the rest of the doc it seems that the custom is actually to place
> filenames in double quotes, like "mbm_assign_control".

Sure.

> 
>> +	in the 'num_mbm_cntrs' file. Changing the mode may cause all counters on
>> +	a resource to reset.
>> +
>> +	The mode is useful on platforms which support more control and monitor
>> +	groups than hardware counters, meaning 'unassigned' control or monitor
>> +	groups will report 'Unavailable' or count the traffic in an unpredictable
>> +	way.
> 
> Note two more instances of "control groups" above.
> 
> Please note that the above description implies that counter assignment is per-group. For
> example, "specify which control	or monitor groups in resctrl should have a counter
> assigned" and "useful on platforms which support more control and monitor groups
> than hardware counters". This needs to be reworked to reflect that counters
> are assigned to events.

How about this?

The mode is useful on platforms which support more CTRL_MON and MON groups
than the hardware counters, meaning 'unassigned' events on CTRL_MON or MON
groups will report 'Unavailable' or count the traffic in an unpredictable
way.


> 
>> +
>> +	AMD Platforms with ABMC (Assignable Bandwidth Monitoring Counters) feature
>> +	enable this mode by default so that counters remain assigned even when the
>> +	corresponding RMID is not in use by any processor.
> 
> I assume this should remain RMID since this specifically talks about an x86 system?

This was a suggestion from James. Let me know if you want me to change.

> 
>> +
>> +	"default":
>> +
>> +	By default resctrl assumes each control and monitor group has a hardware
>> +	counter. Hardware that does not support 'mbm_cntr_assign' mode will still
>> +	allow more control or monitor groups than 'num_rmids' to be created. In
>> +	that case reading the mbm_total_bytes and mbm_local_bytes may report
>> +	'Unavailable' if there is no counter associated with that group.
>> +
> 
> I reconsidered my earlier suggestion and I believe it needs a correction since
> counter assignment is not per group:
> 
> 	In default mode resctrl assumes there is a hardware counter for each
> 	event within every CTRL_MON and MON group. Reading mbm_total_bytes or
> 	mbm_local_bytes may report 'Unavailable' if there is no counter associated
> 	with that event.
> 
> Please feel free to improve.

Looks good.

> 
>>  "max_threshold_occupancy":
>>  		Read/write file provides the largest value (in
>>  		bytes) at which a previously used LLC_occupancy
> 
> The code change looks good to me.

-- 
Thanks
Babu Moger

Powered by blists - more mailing lists