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

From: Reinette Chatre
Date: Fri Jun 21 2024 - 13:10:48 EST


Hi Tony,

On 6/21/24 9:07 AM, Luck, Tony wrote:
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

Should this end with a period too? In the resctrl code a few cases use ".",
most don't. So no period matches resctrl style. But the example in
Documentation/doc-guide/kernel-doc.rst does end with a period.

Having period will be ideal but since that does not match existing style it may
look out of place. I thus do not have strong opinion here.


* @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.

contains -> includes (to indicate it contains the count from parent as well as children)

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

This all looks good to me. Since you have supplied 99% of the content for this
patch in the series I should assign authorship to you (which requires your
Signed-off-by tag). Is that OK? Should I split into two parts? First to add the
kerneldoc (by you). Second to add the new field (by me).

No need to split the patch. You can keep authorship. You are welcome to add:

Co-developed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>

Reinette