Re: [PATCH v12 08/31] x86,fs/resctrl: Rename some L3 specific functions

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

Hi Tony,

On 10/13/25 3:33 PM, Tony Luck wrote:
> With the arrival of monitor events tied to new domains associated with
> a different resource it would be clearer if the L3 resource specific
> functions are more accurately named.
> 
> Rename three groups of functions:
> 
> Functions that allocate/free architecture per-RMID MBM state information:
> arch_domain_mbm_alloc()		-> l3_mon_domain_mbm_alloc()
> mon_domain_free()		-> l3_mon_domain_free()
> 
> Functions that allocate/free filesystem per-RMID MBM state information:
> domain_setup_mon_state()	-> domain_setup_l3_mon_state()
> domain_destroy_mon_state()	-> domain_destroy_l3_mon_state()
> 
> Initialization/exit:
> rdt_get_mon_l3_config()		-> rdt_get_l3_mon_config()
> resctrl_mon_resource_init()	-> resctrl_l3_mon_resource_init()
> resctrl_mon_resource_exit()	-> resctrl_l3_mon_resource_exit()
> 
> Fixed kerneldoc "Return:" for l3_mon_domain_mbm_alloc(),

Please always use imperative tone, be clear what problem is being fixed,
and avoid descriptions that can be seen from the patch self.

Consider, for example:
"Ensure kernel-doc descriptions of these functions' return values are
 present and correctly formatted."

> @@ -396,11 +396,13 @@ static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_ctrl_domain *
>  }
>  
>  /**
> - * arch_domain_mbm_alloc() - Allocate arch private storage for the MBM counters
> + * l3_mon_domain_mbm_alloc() - Allocate arch private storage for the MBM counters
>   * @num_rmid:	The size of the MBM counter array
>   * @hw_dom:	The domain that owns the allocated arrays
> + *
> + * Return:	%0 for success; Error code otherwise.

While using % to format a constant is correct kernel-doc syntax it is not consistent
with how resctrl formats these statements. Please rather keep the addition consistent
with rest of resctrl. Since only -ENOMEM is possible this can also be made consistent
with other changes in this regard:
	Return: 0 for success, or -ENOMEM.

>   */
> -static int arch_domain_mbm_alloc(u32 num_rmid, struct rdt_hw_l3_mon_domain *hw_dom)
> +static int l3_mon_domain_mbm_alloc(u32 num_rmid, struct rdt_hw_l3_mon_domain *hw_dom)
>  {
>  	size_t tsize = sizeof(*hw_dom->arch_mbm_states[0]);
>  	enum resctrl_event_id eventid;

...

> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 69b6aa31b986..26e2ecbddb5c 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -1754,7 +1754,7 @@ ssize_t mbm_L3_assignments_write(struct kernfs_open_file *of, char *buf,
>  }
>  
>  /**
> - * resctrl_mon_resource_init() - Initialise global monitoring structures.
> + * resctrl_l3_mon_resource_init() - Initialise global monitoring structures.
>   *
>   * Allocate and initialise global monitor resources that do not belong to a
>   * specific domain. i.e. the rmid_ptrs[] used for the limbo and free lists.
> @@ -1763,9 +1763,9 @@ ssize_t mbm_L3_assignments_write(struct kernfs_open_file *of, char *buf,
>   * Resctrl's cpuhp callbacks may be called before this point to bring a domain
>   * online.
>   *
> - * Returns 0 for success, or -ENOMEM.
> + * Return: %0 for success; -ENOMEM otherwise.
>   */

The only actual fix is to add a ":", no? The rest are style changes that (a) diverge from the rest of
resctrl and (b) create inconsistency with itself since % is used for prefix of the 0 constant but not
as prefix for -ENOMEM. What is motivation for the style changes?

> -int resctrl_mon_resource_init(void)
> +int resctrl_l3_mon_resource_init(void)
>  {
>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>  	int ret;

...

> @@ -4249,11 +4249,11 @@ void resctrl_offline_mon_domain(struct rdt_resource *r, struct rdt_domain_hdr *h
>   * Called when the first CPU of a domain comes online, regardless of whether
>   * the filesystem is mounted.
>   * During boot this may be called before global allocations have been made by
> - * resctrl_mon_resource_init().
> + * resctrl_l3_mon_resource_init().
>   *
> - * Returns 0 for success, or -ENOMEM.
> + * Return: %0 for success; -ENOMEM otherwise.

same here.

>   */
> -static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_l3_mon_domain *d)
> +static int domain_setup_l3_mon_state(struct rdt_resource *r, struct rdt_l3_mon_domain *d)
>  {
>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>  	size_t tsize = sizeof(*d->mbm_states[0]);

Reinette