Re: [PATCH v20 09/18] x86/resctrl: Add a new field to struct rmid_read for summation of domains

From: Reinette Chatre
Date: Thu Jun 20 2024 - 21:59:38 EST


Hi Tony,

On 6/20/24 3:42 PM, Luck, Tony wrote:
When a user reads a monitor file rdtgroup_mondata_show() calls
mon_event_read() to package up all the required details into an rmid_read
structure which is passed across the smp_call*() infrastructure to code
that will read data from hardware and return the value (or error status)
in the rmid_read structure.

Sub-NUMA Cluster (SNC) mode adds files with new semantics. These require
the smp_call-ed code to sum event data from all domains that share an
L3 cache.

Add a pointer to the L3 "cacheinfo" structure to struct rmid_read
for the data collection routines to use to pick the domains to be
summed.

Reinette suggested that the rmid_read structure has become complex
enough to warrant documentation of each of its fields. Add the kerneldoc
documentation for struct rmid_read.

Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
---
arch/x86/kernel/cpu/resctrl/internal.h | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 99f601d05f3b..d29c7b58c151 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -145,12 +145,25 @@ union mon_data_bits {
} u;
};

+/**
+ * struct rmid_read - Data passed across smp_call*() to read event count
+ * @rgrp: Resctrl group
+ * @r: Resource
+ * @d: Domain. If NULL then sum all domains in @r sharing L3 @ci.id
+ * @evtid: Which monitor event to read
+ * @first: Initializes MBM counter when true
+ * @ci: Cacheinfo for L3. Used when summing domains
+ * @err: Return error indication
+ * @val: Return value of event counter
+ * @arch_mon_ctx: Hardware monitor allocated for this read request (MPAM only)
+ */

Thank you for adding the kerneldoc. I understand that this file is not
consistent on how these kerneldoc are formatted, but could you please
pick whether you think sentences need to end with a period and then stick
to it in this portion?

This is about the @d and @ci entries that have a "sentence" ending with period,
and then more text that doesn't (matching other lines in this block).

Correct.


Maybe some other punctuation to split the parts? Do you like "colon"

* @d: Domain: If NULL then sum all domains in @r sharing L3 @ci.id
* @ci: Cacheinfo for L3: Used when summing domains

of maybe "dash"

* @d: Domain - If NULL then sum all domains in @r sharing L3 @ci.id
* @ci: Cacheinfo for L3 - Used when summing domains

Or something else?

I do not think there is a need to introduce new syntax. It will be easiest
to just have all sentences end with a period. The benefit of this is that it
encourages useful full sentence descriptions. For example, below is a _draft_ of
such a description. Please note that I wrote it quickly and hope it will be improved
(and corrected!). The goal of it being here is to give ideas on how this kerneldoc
can be written to be useful and consistent.

/**
* struct rmid_read - Data passed across smp_call*() to read event count
* @rgrp: Resource group for which the counter is being read. If it is a parent
* resource group then its event count is summed with the count from all
* its child resource groups.
* @r: Resource describing the properties of the event being read.
* @d: Domain that the counter should be read from. If NULL then sum all
* domains in @r sharing L3 @ci.id
* @evtid: Which monitor event to read.
* @first: Initialize MBM counter when true.
* @ci: Cacheinfo for L3. Only set when @d is NULL. Used when summing domains.
* @err: Error encountered when reading counter.
* @val: Returned value of event counter. If @rgrp is a parent resource group,
* @val contains the sum of event counts from its child resource groups.
* If @d is NULL, @val contains the sum of all domains in @r sharing @ci.id,
* (summed across child resource groups if @rgrp is a parent resource group).
* @arch_mon_ctx: Hardware monitor allocated for this read request (MPAM only).
*/

Reinette