Message-ID: <aK7llvNbXZKWhtoo@e133380.arm.com>
Date: Wed, 27 Aug 2025 12:01:42 +0100
From: Dave Martin <Dave.Martin@....com>
To: James Morse <james.morse@....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,

<super-pedantic mode enabled>

(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.

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

(Or something like that.)

> 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.)


> Signed-off-by: James Morse <james.morse@....com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@...wei.com>
> ---
>  arch/arm64/Kconfig | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> 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" ?

(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.)

> +	  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--

(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"?

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--

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

> +
> +	  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.)

> +
> +	  MPAM is exposed to user-space via the resctrl pseudo filesystem.
> +
>  endmenu # "ARMv8.4 architectural features"
>  
>  menu "ARMv8.5 architectural features"

Cheers
---Dave

Powered by blists - more mailing lists