Re: [PATCH v18 08/17] x86/resctrl: Add and initialize display_id field to struct rdt_mon_domain

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

Hi Tony,

On 5/15/2024 3:23 PM, Tony Luck wrote:
> When Sub-NUMA (SNC) mode is enabled monitoring domains are created at

Sub-NUMA Cluster (SNC) ?

> SNC node scope. Add a field that holds the identity of the L3 cache for

This is not necessarily the L3 cache, but instead intended to be the
monitoring display scope, no?

> each domain to make it easy to find all domains that share the same
> L3 cache instance. There are three places where this is needed. In
> all cases code is operating on a domain where "d->id" refers to the
> SNC node id.
> 
> 1) When making monitor directories.
>    Need the L3 cache instance ID to make the mon_L3_XX directory
>    that will contain the legacy monitor reporting files and the
>    mon_sub_L3_YY directory for this domain.
> 2) When removing monitor directories.
>    Similar to making directories.
> 3) When reporting data from one of the L3-scoped legacy files.
>    This requires summing data from each SNC node that shares the
>    same L3 cache instance id.

<insert motivation about why this cannot be determined dynamically
at the places identified>

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/resctrl.h            | 2 ++
>  arch/x86/kernel/cpu/resctrl/core.c | 8 ++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 98c0ff8ba005..2f8ac925bc18 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -96,6 +96,7 @@ struct rdt_ctrl_domain {
>  /**
>   * struct rdt_mon_domain - group of CPUs sharing a resctrl monitor resource
>   * @hdr:		common header for different domain types
> + * @display_id:		shared id used to identify domains to be summed for display

This description seems to indicate this is a member only used when needing to
sum domains, thus only for SNC at this time. Looking ahead the description does not
seem to capture that this value has been integrated into non-SNC support and will
always be used when creating files for all domains, whether SNC is enabled or not.
This member thus seems to be used for more than it claims to.

>   * @rmid_busy_llc:	bitmap of which limbo RMIDs are above threshold
>   * @mbm_total:		saved state for MBM total bandwidth
>   * @mbm_local:		saved state for MBM local bandwidth
> @@ -106,6 +107,7 @@ struct rdt_ctrl_domain {
>   */
>  struct rdt_mon_domain {
>  	struct rdt_domain_hdr		hdr;
> +	int				display_id;
>  	unsigned long			*rmid_busy_llc;
>  	struct mbm_state		*mbm_total;
>  	struct mbm_state		*mbm_local;
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 15856254fea7..dd40c998df72 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -614,6 +614,14 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>  
>  	d = &hw_dom->d_resctrl;
>  	d->hdr.id = id;
> +	d->display_id = get_domain_id_from_scope(cpu, r->mon_display_scope);
> +	if (d->display_id < 0) {
> +		pr_warn_once("Can't find monitor domain display id for CPU:%d scope:%d for resource %s\n",
> +			     cpu, r->mon_display_scope, r->name);
> +		mon_domain_free(hw_dom);
> +		return;
> +	}
> +
>  	d->hdr.type = RESCTRL_MON_DOMAIN;
>  	cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
>  

Reinette