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

[Reinette Chatre <reinette.chatre@intel.com>]

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@intel.com>
>>> ---
>>>    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