Re: [PATCH v3 21/26] fs-x86/resctrl: Handle RDT_RESOURCE_PERF_PKG in domain create/delete

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

Hi Tony,

On 4/7/25 4:40 PM, Tony Luck wrote:
> Add a new rdt_perf_pkg_mon_domain structure. This only consists of
> the common rdt_domain_hdr as there is no need for any per-domain
> data structures.
> 
> Use as much as possible of the existing domain setup and tear down
> infrastructure. In many cases the RDT_RESOURCE_PERF_PKG uses the
> same functions but just skips over the pieces it does not need.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/resctrl.h            |  8 ++++++++
>  arch/x86/kernel/cpu/resctrl/core.c | 32 ++++++++++++++++++++++++++++++
>  fs/resctrl/rdtgroup.c              | 11 ++++++++--
>  3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index c03e7dc1f009..6f598a64b192 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -169,6 +169,14 @@ struct rdt_mon_domain {
>  	int				cqm_work_cpu;
>  };
>  
> +/**
> + * struct rdt_perf_pkg_mon_domain - CPUs sharing an Intel-PMT-scoped resctrl monitor resource

It is a red flag when architecture specific things ("rdt" and "Intel-PMT-scoped") land
in include/linux/resctrl.h

> + * @hdr:               common header for different domain types
> + */
> +struct rdt_perf_pkg_mon_domain {
> +	struct rdt_domain_hdr   hdr;
> +};
> +
>  /**
>   * struct resctrl_cache - Cache allocation related data
>   * @cbm_len:		Length of the cache bit mask
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 9578d9c7260c..6f5d52a8219b 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -542,6 +542,29 @@ static void setup_l3_mon_domain(int cpu, int id, struct rdt_resource *r, struct
>  	}
>  }
>  
> +static void setup_intel_aet_mon_domain(int cpu, int id, struct rdt_resource *r,
> +				       struct list_head *add_pos)
> +{
> +	struct rdt_perf_pkg_mon_domain *d;
> +	int err;
> +
> +	d = kzalloc_node(sizeof(*d), GFP_KERNEL, cpu_to_node(cpu));
> +	if (!d)
> +		return;
> +
> +	d->hdr.id = id;
> +	d->hdr.type = DOMTYPE(r->rid, DOMTYPE_MON);
> +	cpumask_set_cpu(cpu, &d->hdr.cpu_mask);
> +	list_add_tail_rcu(&d->hdr.list, add_pos);
> +
> +	err = resctrl_online_mon_domain(r, &d->hdr);
> +	if (err) {
> +		list_del_rcu(&d->hdr.list);
> +		synchronize_rcu();
> +		kfree(d);
> +	}
> +}
> +
>  static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>  {
>  	int id = get_domain_id_from_scope(cpu, r->mon_scope);
> @@ -571,6 +594,9 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
>  	case RDT_RESOURCE_L3:
>  		setup_l3_mon_domain(cpu, id, r, add_pos);
>  		break;
> +	case RDT_RESOURCE_PERF_PKG:
> +		setup_intel_aet_mon_domain(cpu, id, r, add_pos);
> +		break;
>  	default:
>  		WARN_ON_ONCE(1);
>  	}
> @@ -668,6 +694,12 @@ static void domain_remove_cpu_mon(int cpu, struct rdt_resource *r)
>  		synchronize_rcu();
>  		mon_domain_free(hw_dom);
>  		break;
> +	case RDT_RESOURCE_PERF_PKG:
> +		resctrl_offline_mon_domain(r, d);

Something should have complained about using an uninitialized variable here (d).

> +		list_del_rcu(&hdr->list);
> +		synchronize_rcu();
> +		kfree(container_of(hdr, struct rdt_perf_pkg_mon_domain, hdr));
> +		break;
>  	}
>  }
>  
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 5ca6de6a6e5c..34fcd20f8dd7 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -4020,6 +4020,9 @@ void resctrl_offline_mon_domain(struct rdt_resource *r, struct rdt_mon_domain *d
>  	if (resctrl_mounted && resctrl_arch_mon_capable())
>  		rmdir_mondata_subdir_allrdtgrp(r, &d->hdr);
>  
> +	if (r->rid == RDT_RESOURCE_PERF_PKG)
> +		goto done;

Could you please change this test to 
	if (r->rid != RDT_RESOURCE_L3)

this makes it clear about what the code that follows supports as opposed to one resource that it does
not support (and possibly cause issue in future when adding another monitoring resource).

> +
>  	if (resctrl_is_mbm_enabled())
>  		cancel_delayed_work(&d->mbm_over);
>  	if (resctrl_arch_is_llc_occupancy_enabled() && has_busy_rmid(d)) {
> @@ -4036,7 +4039,7 @@ void resctrl_offline_mon_domain(struct rdt_resource *r, struct rdt_mon_domain *d
>  	}
>  
>  	domain_destroy_mon_state(d);
> -
> +done:
>  	mutex_unlock(&rdtgroup_mutex);
>  }
>  
> @@ -4104,12 +4107,15 @@ int resctrl_online_ctrl_domain(struct rdt_resource *r, struct rdt_ctrl_domain *d
>  int resctrl_online_mon_domain(struct rdt_resource *r, struct rdt_domain_hdr *hdr)
>  {
>  	struct rdt_mon_domain *d;
> -	int err;
> +	int err = 0;
>  
>  	WARN_ON_ONCE(hdr->type != DOMTYPE(r->rid, DOMTYPE_MON));
>  	d = container_of(hdr, struct rdt_mon_domain, hdr);
>  	mutex_lock(&rdtgroup_mutex);
>  
> +	if (r->rid == RDT_RESOURCE_PERF_PKG)
> +		goto do_mkdir;

same

> +
>  	err = domain_setup_mon_state(r, d);
>  	if (err)
>  		goto out_unlock;
> @@ -4123,6 +4129,7 @@ int resctrl_online_mon_domain(struct rdt_resource *r, struct rdt_domain_hdr *hdr
>  	if (resctrl_arch_is_llc_occupancy_enabled())
>  		INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);
>  
> +do_mkdir:
>  	/*
>  	 * If the filesystem is not mounted then only the default resource group
>  	 * exists. Creation of its directories is deferred until mount time

Reinette