Message-ID: <978cf822-4d7c-4301-bbc4-752f184c93d6@arm.com>
Date: Thu, 4 Sep 2025 18:28:14 +0100
From: James Morse <james.morse@....com>
To: Dave Martin <Dave.Martin@....com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-acpi@...r.kernel.org, devicetree@...r.kernel.org,
 shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
 Koba Ko <kobak@...dia.com>, Shanker Donthineni <sdonthineni@...dia.com>,
 fenghuay@...dia.com, baisheng.gao@...soc.com,
 Jonathan Cameron <jonathan.cameron@...wei.com>, Rob Herring
 <robh@...nel.org>, Rohit Mathew <rohit.mathew@....com>,
 Rafael Wysocki <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>, Hanjun Guo
 <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Catalin Marinas <catalin.marinas@....com>,
 Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH 07/33] arm64: kconfig: Add Kconfig entry for MPAM

Hi Dave,

On 27/08/2025 12:01, Dave Martin wrote:
> <super-pedantic mode enabled>

Uh oh!

> (Since this likely be people's go-to patch for understanding what MPAM
> is, it is probably worth going the extra mile.)
> 
> On Fri, Aug 22, 2025 at 03:29:48PM +0000, James Morse wrote:
>> The bulk of the MPAM driver lives outside the arch code because it
>> largely manages MMIO devices that generate interrupts. The driver
>> needs a Kconfig symbol to enable it, as MPAM is only found on arm64
> 
> Prefer -> "[...] to enable it. As MPAM is only [...]"
> 
>> platforms, that is where the Kconfig option makes the most sense.
> 
> It could be clearer what "where" refers to, here.

Sure,


> Maybe reword from ", that is [...]" -> ", the arm64 tree is the most
> natural home for the Kconfig option."
> 
> (Or something like that.)

Sure,


>> This Kconfig option will later be used by the arch code to enable
>> or disable the MPAM context-switch code, and registering the CPUs
> 
> Nit: "registering" -> "to register"
> 
>> properties with the MPAM driver.
> 
> Nit: "CPUs properties" -> "properties of CPUs" ?
> 
> (Maybe there was just a missed apostrophe, but it may be more readable
> here if written out longhand.)

Done, it just takes one person to think its clearer!


>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index e9bbfacc35a6..658e47fc0c5a 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -2060,6 +2060,23 @@ config ARM64_TLB_RANGE
>>  	  ARMv8.4-TLBI provides TLBI invalidation instruction that apply to a
>>  	  range of input addresses.
>>  
>> +config ARM64_MPAM
>> +	bool "Enable support for MPAM"
>> +	help
> 
> <pedantic mode on>
> 
>> +	  Memory Partitioning and Monitoring is an optional extension
>> +	  that allows the CPUs to mark load and store transactions with
> 
> Nit: "memory transactions" ?

Sure,

> (I'm wondering whether there are some transactions such as atomic
> exchanges that are not neatly characterised as "load" or "store".
> Possibly MPAM labels some transactions that really are neither.)

Equally instruction fetch and possibly even CMOs get these labels.
I wanted something other than 'transactions' so it wasn't confused with
transactional memory - and traffic seemed to vauge.

I don't think anyone expects a formal definition in the Kconfig text...


>> +	  labels for partition-id and performance-monitoring-group.
> 
> Nit: the hyphenation suggests that these are known terms (in this
> specific, hyphenated, form) with specific definitions somewhere.
> I don't think that this is the case?  At least, I have not seen the
> terms presented in this way anywhere else.
> 
> Also, the partition ID is itself a label, so "label for partition-id"
> is a tautology.
> 
> How about:
> 
> --8<--
> 
> 	  Memory System Resource Partitioning and Monitoring (MPAM) is an
> 	  optional extension to the Arm architecture that allows each
> 	  transaction issued to the memory system to be labelled with a
> 	  Partition identifier (PARTID) and Performance Monitoring Group
> 	  identifier (PMG).
> 
> -->8--

Done,


> (Yes, that really seems to be what MPAM stands for in the published
> specs.  That's quite a mounthful, and news to me...  I can't say I paid
> much attention to the document titles beyond "MPAM"!)
> 
>> +	  System components, such as the caches, can use the partition-id
>> +	  to apply a performance policy. MPAM monitors can use the
> 
> What is a "performance policy"?

A bunch of controls, the value of which reflect some kind of policy.


> The MPAM specs talk about resource controls; it's probably best to
> stick to the same terminology.
> 
>> +	  partition-id and performance-monitoring-group to measure the
>> +	  cache occupancy or data throughput.
> 
> So, how about something like:
> 
> --8<--
> 
> 	  Memory system components, such as the caches, can be configured with
> 	  policies to control how much of various physical resources (such as
> 	  memory bandwidth or cache memory) the transactions labelled with each
> 	  PARTID can consume.  Depending on the capabilities of the hardware,
> 	  the PARTID and PMG can also be used as filtering criteria to measure
> 	  the memory system resource consumption of different parts of a
> 	  workload.
> 
> -->8--

Done,


> (Where "Memory system components" is used in a generic sense and so not
> capitalised.)

(I can't wait for the Memory System Component on the Memory Side Cache!)


>> +
>> +	  Use of this extension requires CPU support, support in the
>> +	  memory system components (MSC), and a description from firmware
> 
> But here, we are explicitly using an architectural term now, so
> 
> 	"Memory System Components" (MSC)
> 
> makes sense.
> 
>> +	  of where the MSC are in the address space.
> 
> Prefer "MSCs" ?  (Not everyone agrees about whether TLAs are
> pluralisable but it is easier on the reader if "are" has an obviously
> plural noun to bind to.)

Sure,


Thanks,

James

Powered by blists - more mailing lists