Re: [PATCH v20 12/18] x86/resctrl: Create Sub-NUMA Cluster (SNC) monitor files

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

Hi Tony,

On 6/10/24 11:35 AM, Tony Luck wrote:
> When SNC mode is enabled, create subdirectories and files to monitor
> at the SNC node granularity. Legacy behavior is preserved by tagging
> the monitor files at the L3 granularity with the "sum" attribute.
> When the user reads these files the kernel will read monitor data
> from all SNC nodes that share the same L3 cache instance and return
> the aggregated value to the user.
> 
> Note that the "domid" field for files that must sum across SNC domains
> has the L3 cache instance id, while non-summing files use the domain id.
> 
> The "sum" files do not need to make a call to mon_event_read() to
> initialize the MBM counters. This will be handled by initializing the
> individual SNC nodes that share the L3.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 62 +++++++++++++++++++-------
>   1 file changed, 46 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 66acbad1c585..fc7f3f139800 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3022,7 +3022,8 @@ static void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
>   }
>   
>   static int mon_add_all_files(struct kernfs_node *kn, struct rdt_mon_domain *d,
> -			     struct rdt_resource *r, struct rdtgroup *prgrp)
> +			     struct rdt_resource *r, struct rdtgroup *prgrp,
> +			     bool do_sum)
>   {
>   	union mon_data_bits priv;
>   	struct mon_evt *mevt;
> @@ -3033,14 +3034,15 @@ static int mon_add_all_files(struct kernfs_node *kn, struct rdt_mon_domain *d,
>   		return -EPERM;
>   
>   	priv.u.rid = r->rid;
> -	priv.u.domid = d->hdr.id;
> +	priv.u.domid = do_sum ? d->ci->id : d->hdr.id;
> +	priv.u.sum = do_sum;
>   	list_for_each_entry(mevt, &r->evt_list, list) {
>   		priv.u.evtid = mevt->evtid;
>   		ret = mon_addfile(kn, mevt->name, priv.priv);
>   		if (ret)
>   			return ret;
>   
> -		if (is_mbm_event(mevt->evtid))
> +		if (!do_sum && is_mbm_event(mevt->evtid))
>   			mon_event_read(&rr, r, d, prgrp, mevt->evtid, true);
>   	}
>   
> @@ -3051,23 +3053,51 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
>   				struct rdt_mon_domain *d,
>   				struct rdt_resource *r, struct rdtgroup *prgrp)
>   {
> -	struct kernfs_node *kn;
> +	struct kernfs_node *kn, *ckn;
>   	char name[32];
> -	int ret;
> +	bool snc_mode;
> +	int ret = 0;
>   
> -	sprintf(name, "mon_%s_%02d", r->name, d->hdr.id);
> -	/* create the directory */
> -	kn = kernfs_create_dir(parent_kn, name, parent_kn->mode, prgrp);
> -	if (IS_ERR(kn))
> -		return PTR_ERR(kn);
> +	lockdep_assert_held(&rdtgroup_mutex);
>   
> -	ret = rdtgroup_kn_set_ugid(kn);
> -	if (ret)
> -		goto out_destroy;
> +	snc_mode = r->mon_scope != RESCTRL_L3_CACHE;

I think that testing that it _should_ be of particular scope will be
easier to understand than testing what it should not be. Can this instead
be a positive check of:
	snc_mode = r->mon_scope == RESCTRL_L3_NODE;

> +	sprintf(name, "mon_%s_%02d", r->name, d->ci->id);

I find this to be too subtle and potentially confusing since it uses
d->ci->id interchangeable for both SNC and non-SNC mode. I understand
that in non-SNC mode the domain id will be the same as the cache id
but I would prefer that the code use the data structures as intended
instead of relying backdoor on assumptions. Something like:

	sprintf(name, "mon_%s_%02d", r->name, snc_mode ? d->ci->id : d->hdr.id);

> +	kn = kernfs_find_and_get(parent_kn, name);
> +	if (kn) {
> +		/*
> +		 * rdtgroup_mutex will prevent this directory from being
> +		 * removed. No need to keep this hold.
> +		 */
> +		kernfs_put(kn);
> +	} else {
> +		kn = kernfs_create_dir(parent_kn, name, parent_kn->mode, prgrp);
> +		if (IS_ERR(kn))
> +			return PTR_ERR(kn);
>   
> -	ret = mon_add_all_files(kn, d, r, prgrp);
> -	if (ret)
> -		goto out_destroy;
> +		ret = rdtgroup_kn_set_ugid(kn);
> +		if (ret)
> +			goto out_destroy;
> +		ret = mon_add_all_files(kn, d, r, prgrp, snc_mode);
> +		if (ret)
> +			goto out_destroy;
> +	}
> +
> +	if (snc_mode) {
> +		sprintf(name, "mon_sub_%s_%02d", r->name, d->hdr.id);
> +		ckn = kernfs_create_dir(kn, name, parent_kn->mode, prgrp);
> +		if (IS_ERR(ckn)) {
> +			ret = -EINVAL;
> +			goto out_destroy;
> +		}
> +
> +		ret = rdtgroup_kn_set_ugid(ckn);
> +		if (ret)
> +			goto out_destroy;
> +
> +		ret = mon_add_all_files(ckn, d, r, prgrp, false);
> +		if (ret)
> +			goto out_destroy;
> +	}
>   
>   	kernfs_activate(kn);
>   	return 0;

Reinette