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)
+ */
 struct rmid_read {
 	struct rdtgroup		*rgrp;
 	struct rdt_resource	*r;
 	struct rdt_mon_domain	*d;
 	enum resctrl_event_id	evtid;
 	bool			first;
+	struct cacheinfo	*ci;
 	int			err;
 	u64			val;
 	void			*arch_mon_ctx;
-- 
2.45.0

Hi Tony,

On 6/10/24 11:35 AM, Tony Luck 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?

>   struct rmid_read {
>   	struct rdtgroup		*rgrp;
>   	struct rdt_resource	*r;
>   	struct rdt_mon_domain	*d;
>   	enum resctrl_event_id	evtid;
>   	bool			first;
> +	struct cacheinfo	*ci;
>   	int			err;
>   	u64			val;
>   	void			*arch_mon_ctx;

Reinette

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

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?

>
> >   struct rmid_read {
> >     struct rdtgroup         *rgrp;
> >     struct rdt_resource     *r;
> >     struct rdt_mon_domain   *d;
> >     enum resctrl_event_id   evtid;
> >     bool                    first;
> > +   struct cacheinfo        *ci;
> >     int                     err;
> >     u64                     val;
> >     void                    *arch_mon_ctx;

-Tony

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

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

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

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

-Tony

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@intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette