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