Re: [PATCH v10 12/24] x86/resctrl: Introduce cntr_cfg to track assignable counters at domain

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

Hi Babu,

Did subject intend to use name of new struct?

On 12/12/24 12:15 PM, Babu Moger wrote:
> In mbm_assign_mode, the MBM counters are assigned/unassigned to an RMID,
> event pair in a resctrl group and monitor the bandwidth as long as it is
> assigned. Counters are assigned/unassigned at domain level and needs to
> be tracked at domain level.
> 
> Add the mbm_assign_cntr_cfg data structure to struct rdt_ctrl_domain to

"mbm_assign_cntr_cfg" -> "mbm_cntr_cfg"

> manage and track MBM counter assignments at the domain level.

This can really use some more information about this data structure. I think
it will be helpful to provide more information about how the data structure
looks ... for example, that it is an array indexed by counter ID where the
assignment details of each counter is stored. I also think it will be helpful
to describe how interactions with  this data structure works, that a NULL
rdtgrp means that the counter is free and that it is not possible to find
a counter from a resource group and arrays need to be searched instead and doing
so is ok for $REASON (when considering the number of RMID and domain combinations
possible on AMD). A lot is left for the reader to figure out.
> 
> Suggested-by: Peter Newman <peternewman@google.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v10: Patch changed completely to handle the counters at domain level.
>      https://lore.kernel.org/lkml/CALPaoCj+zWq1vkHVbXYP0znJbe6Ke3PXPWjtri5AFgD9cQDCUg@mail.gmail.com/
>      Removed Reviewed-by tag.
>      Did not see the need to add cntr_id in mbm_state structure. Not used in the code.
> 
> v9: Added Reviewed-by tag. No other changes.
> 
> v8: Minor commit message changes.
> 
> v7: Added check mbm_cntr_assignable for allocating bitmap mbm_cntr_map
> 
> v6: New patch to add domain level assignment.
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 11 +++++++++++
>  include/linux/resctrl.h                | 12 ++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 682f47e0beb1..1ee008a63d8b 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -4068,6 +4068,7 @@ static void __init rdtgroup_setup_default(void)
>  
>  static void domain_destroy_mon_state(struct rdt_mon_domain *d)
>  {
> +	kfree(d->cntr_cfg);
>  	bitmap_free(d->rmid_busy_llc);
>  	kfree(d->mbm_total);
>  	kfree(d->mbm_local);
> @@ -4141,6 +4142,16 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_mon_domain
>  			return -ENOMEM;
>  		}
>  	}
> +	if (is_mbm_enabled() && r->mon.mbm_cntr_assignable) {
> +		tsize = sizeof(*d->cntr_cfg);
> +		d->cntr_cfg = kcalloc(r->mon.num_mbm_cntrs, tsize, GFP_KERNEL);
> +		if (!d->cntr_cfg) {
> +			bitmap_free(d->rmid_busy_llc);
> +			kfree(d->mbm_total);
> +			kfree(d->mbm_local);
> +			return -ENOMEM;
> +		}
> +	}
>  
>  	return 0;
>  }
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index c8ab3d7a0dab..03c67d9156f3 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -94,6 +94,16 @@ struct rdt_ctrl_domain {
>  	u32				*mbps_val;
>  };
>  
> +/**
> + * struct mbm_cntr_cfg -Assignable counter configuration

Please compare with style use in rest of the file. For example,
"-Assignable" -> "- assignable"

> + * @evtid:		Event type

This description is not useful. Consider: "MBM event to which
the counter is assigned. Only valid if @rdtgroup is not NULL."
(This was the first thing that came to my mind, please improve)

> + * @rdtgroup:		Resctrl group assigned to the counter

Can add "NULL if counter is free"

> + */
> +struct mbm_cntr_cfg {
> +	enum resctrl_event_id	evtid;
> +	struct rdtgroup		*rdtgrp;
> +};
> +
>  /**
>   * struct rdt_mon_domain - group of CPUs sharing a resctrl monitor resource
>   * @hdr:		common header for different domain types
> @@ -105,6 +115,7 @@ struct rdt_ctrl_domain {
>   * @cqm_limbo:		worker to periodically read CQM h/w counters
>   * @mbm_work_cpu:	worker CPU for MBM h/w counters
>   * @cqm_work_cpu:	worker CPU for CQM h/w counters
> + * @cntr_cfg:		Assignable counters configuration

Match capitalization of surrounding text. 
Will be helpful to add that this is an array indexed by counter ID.

>   */
>  struct rdt_mon_domain {
>  	struct rdt_domain_hdr		hdr;
> @@ -116,6 +127,7 @@ struct rdt_mon_domain {
>  	struct delayed_work		cqm_limbo;
>  	int				mbm_work_cpu;
>  	int				cqm_work_cpu;
> +	struct mbm_cntr_cfg		*cntr_cfg;
>  };
>  
>  /**

Reinette