Date:   Fri, 11 Jun 2021 16:17:10 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/resctrl: Fix kernel-doc in internal.h

Hi Fabio,

Please also consider my comments regarding the goal of this patch 
similar to what I mentioned in my response to your changes to the 
pseudo_lock.c file. I updated a few descriptions to improve accuracy and 
noted some formatting issues. Apart from these small issues it is 
looking good, thank you.

On 6/8/2021 5:54 PM, Fabio M. De Francesco wrote:
> Added description of undocumented parameters. Fixed some minor
> kernel-doc grammar issues. Issues largely detected by
> scripts/kernel-doc.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> ---
>   arch/x86/kernel/cpu/resctrl/internal.h | 28 ++++++++++++++++----------
>   1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index c4d320d02fd5..f360944a7ae1 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -68,8 +68,9 @@ DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
>   
>   /**
>    * struct mon_evt - Entry in the event list of a resource
> - * @evtid:		event id
> - * @name:		name of the event
> + * @evtid:		Event id
> + * @name:		Name of the event
> + * @list:		List head

The only kernel-doc issue here is the missing @list. To just fix the 
issue while remaining consistent with the existing formatting you could 
continue by describing @list with lower case even if other areas do so 
with upper case.

For that description could you please use more descriptive language - 
writing something like "List head" does not help the reader. Something like:

"@list: list entry in &rdt_resource->evt_list"

>    */
>   struct mon_evt {
>   	u32			evtid;
> @@ -78,10 +79,12 @@ struct mon_evt {
>   };
>   
>   /**
> - * struct mon_data_bits - Monitoring details for each event file
> - * @rid:               Resource id associated with the event file.
> + * union mon_data_bits - Monitoring details for each event file
> + * @priv:	       Private data for the union	
> + * @rid:               Resource id associated with the event file
>    * @evtid:             Event id associated with the event file
>    * @domid:             The domain to which the event file belongs
> + * @u:		       Name of the bit fields struct
>    */

Spacing got broken here with some unintended tabs added as well as 
trailing space.

This is a union where @priv and @u refers to the same storage. More 
detail can be added to help the reader:
"@priv: used to store monitoring event data in @u as kernfs private data"

>   union mon_data_bits {
>   	void *priv;
> @@ -119,6 +122,7 @@ enum rdt_group_type {
>    * @RDT_MODE_PSEUDO_LOCKSETUP: Resource group will be used for Pseudo-Locking
>    * @RDT_MODE_PSEUDO_LOCKED: No sharing of this resource group's allocations
>    *                          allowed AND the allocations are Cache Pseudo-Locked
> + * @RDT_NUM_MODES: Total number of modes
>    *
>    * The mode of a resource group enables control over the allowed overlap
>    * between allocations associated with different resource groups (classes
> @@ -142,7 +146,7 @@ enum rdtgrp_mode {
>   
>   /**
>    * struct mongroup - store mon group's data in resctrl fs.
> - * @mon_data_kn		kernlfs node for the mon_data directory
> + * @mon_data_kn:		kernlfs node for the mon_data directory
>    * @parent:			parent rdtgrp
>    * @crdtgrp_list:		child rdtgroup node list
>    * @rmid:			rmid for this rdtgroup
> @@ -282,11 +286,11 @@ struct rftype {
>   /**
>    * struct mbm_state - status for each MBM counter in each domain
>    * @chunks:	Total data moved (multiply by rdt_group.mon_scale to get bytes)
> - * @prev_msr	Value of IA32_QM_CTR for this RMID last time we read it
> + * @prev_msr:	Value of IA32_QM_CTR for this RMID last time we read it
>    * @prev_bw_msr:Value of previous IA32_QM_CTR for bandwidth counting
> - * @prev_bw	The most recent bandwidth in MBps
> - * @delta_bw	Difference between the current and previous bandwidth
> - * @delta_comp	Indicates whether to compute the delta_bw
> + * @prev_bw:	The most recent bandwidth in MBps
> + * @delta_bw:	Difference between the current and previous bandwidth
> + * @delta_comp:	Indicates whether to compute the delta_bw
>    */
>   struct mbm_state {
>   	u64	chunks;

Above changes look good, thanks.

> @@ -450,18 +454,20 @@ struct rdt_parse_data {
>    * @name:		Name to use in "schemata" file
>    * @num_closid:		Number of CLOSIDs available
>    * @cache_level:	Which cache level defines scope of this resource
> - * @default_ctrl:	Specifies default cache cbm or memory B/W percent.
> + * @default_ctrl:	Specifies default cache cbm or memory B/W percent
>    * @msr_base:		Base MSR address for CBMs
>    * @msr_update:		Function pointer to update QOS MSRs
>    * @data_width:		Character width of data when displaying
>    * @domains:		All domains for this resource
>    * @cache:		Cache allocation related data
> + * @membw:		Memory bandwidth allocation related data
>    * @format_str:		Per resource format string to show domain value
>    * @parse_ctrlval:	Per resource function pointer to parse control values
>    * @evt_list:		List of monitoring events
>    * @num_rmid:		Number of RMIDs available
>    * @mon_scale:		cqm counter * mon_scale = occupancy in bytes
> - * @fflags:		flags to choose base and info files
> + * @mbm_width:		Width of memory bandwidth monitoring counter
> + * @fflags:		Flags to choose base and info files
>    */
>   struct rdt_resource {
>   	int			rid;
> 

I think one small addition would be helpful to the reader:
"@mbm_width: Width of memory bandwidth monitoring hardware counter"

Thank you!

Reinette

Powered by blists - more mailing lists