Re: [PATCH v2 05/23] x86/resctrl: Add domain online callback for resctrl work

[Babu Moger]

On 10/1/21 11:02 AM, James Morse wrote:
> Because domains are exposed to user-space via resctrl, the filesystem
> must update its state when CPU hotplug callbacks are triggered.
> 
> Some of this work is common to any architecture that would support
> resctrl, but the work is tied up with the architecture code to
> allocate the memory.
> 
> Move domain_setup_mon_state(), the monitor subdir creation call and the
> mbm/limbo workers into a new resctrl_online_domain() call. These bits
> are not specific to the architecture. Grouping them in one function
> allows that code to be moved to /fs/ and re-used by another architecture.
> 
> Signed-off-by: James Morse <james.morse@xxxxxxx>
> ---
> Changes since v1:
>  * Capitalisation
>  * Removed inline comment
>  * Added to the commit message
> ---
>  arch/x86/kernel/cpu/resctrl/core.c     | 57 ++++------------------
>  arch/x86/kernel/cpu/resctrl/internal.h |  2 -
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 65 ++++++++++++++++++++++++--
>  include/linux/resctrl.h                |  1 +
>  4 files changed, 69 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 2f87177f1f69..f1fa54de8136 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -443,42 +443,6 @@ static int domain_setup_ctrlval(struct rdt_resource *r, struct rdt_domain *d)
>  	return 0;
>  }
>  
> -static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d)
> -{
> -	size_t tsize;
> -
> -	if (is_llc_occupancy_enabled()) {
> -		d->rmid_busy_llc = bitmap_zalloc(r->num_rmid, GFP_KERNEL);
> -		if (!d->rmid_busy_llc)
> -			return -ENOMEM;
> -		INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);
> -	}
> -	if (is_mbm_total_enabled()) {
> -		tsize = sizeof(*d->mbm_total);
> -		d->mbm_total = kcalloc(r->num_rmid, tsize, GFP_KERNEL);
> -		if (!d->mbm_total) {
> -			bitmap_free(d->rmid_busy_llc);
> -			return -ENOMEM;
> -		}
> -	}
> -	if (is_mbm_local_enabled()) {
> -		tsize = sizeof(*d->mbm_local);
> -		d->mbm_local = kcalloc(r->num_rmid, tsize, GFP_KERNEL);
> -		if (!d->mbm_local) {
> -			bitmap_free(d->rmid_busy_llc);
> -			kfree(d->mbm_total);
> -			return -ENOMEM;
> -		}
> -	}
> -
> -	if (is_mbm_enabled()) {
> -		INIT_DELAYED_WORK(&d->mbm_over, mbm_handle_overflow);
> -		mbm_setup_overflow_handler(d, MBM_OVERFLOW_INTERVAL);
> -	}
> -
> -	return 0;
> -}
> -
>  /*
>   * domain_add_cpu - Add a cpu to a resource's domain list.
>   *
> @@ -498,6 +462,7 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>  	struct list_head *add_pos = NULL;
>  	struct rdt_hw_domain *hw_dom;
>  	struct rdt_domain *d;
> +	int err;
>  
>  	d = rdt_find_domain(r, id, &add_pos);
>  	if (IS_ERR(d)) {
> @@ -527,21 +492,15 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>  		return;
>  	}
>  
> -	if (r->mon_capable && domain_setup_mon_state(r, d)) {
> -		kfree(hw_dom->ctrl_val);
> -		kfree(hw_dom->mbps_val);
> -		kfree(hw_dom);
> -		return;
> -	}
> -
>  	list_add_tail(&d->list, add_pos);
>  
> -	/*
> -	 * If resctrl is mounted, add
> -	 * per domain monitor data directories.
> -	 */
> -	if (static_branch_unlikely(&rdt_mon_enable_key))
> -		mkdir_mondata_subdir_allrdtgrp(r, d);
> +	err = resctrl_online_domain(r, d);
> +	if (err) {
> +		list_del(&d->list);
> +		kfree(hw_dom->ctrl_val);
> +		kfree(hw_dom->mbps_val);
> +		kfree(d);
> +	}
>  }
>  
>  static void domain_remove_cpu(int cpu, struct rdt_resource *r)
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 8828b5c1b6d2..be48a682dbdb 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -524,8 +524,6 @@ void mon_event_count(void *info);
>  int rdtgroup_mondata_show(struct seq_file *m, void *arg);
>  void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
>  				    unsigned int dom_id);
> -void mkdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
> -				    struct rdt_domain *d);
>  void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>  		    struct rdt_domain *d, struct rdtgroup *rdtgrp,
>  		    int evtid, int first);
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index e243c7d15b81..19691f9ab061 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2565,16 +2565,13 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn,
>   * Add all subdirectories of mon_data for "ctrl_mon" groups
>   * and "monitor" groups with given domain id.
>   */
> -void mkdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
> -				    struct rdt_domain *d)
> +static void mkdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
> +					   struct rdt_domain *d)
>  {
>  	struct kernfs_node *parent_kn;
>  	struct rdtgroup *prgrp, *crgrp;
>  	struct list_head *head;
>  
> -	if (!r->mon_capable)
> -		return;
> -
>  	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
>  		parent_kn = prgrp->mon.mon_data_kn;
>  		mkdir_mondata_subdir(parent_kn, d, r, prgrp);
> @@ -3236,6 +3233,64 @@ static int __init rdtgroup_setup_root(void)
>  	return ret;
>  }
>  
> +static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d)
> +{
> +	size_t tsize;
> +
> +	if (is_llc_occupancy_enabled()) {
> +		d->rmid_busy_llc = bitmap_zalloc(r->num_rmid, GFP_KERNEL);
> +		if (!d->rmid_busy_llc)
> +			return -ENOMEM;
> +	}
> +	if (is_mbm_total_enabled()) {
> +		tsize = sizeof(*d->mbm_total);
> +		d->mbm_total = kcalloc(r->num_rmid, tsize, GFP_KERNEL);
> +		if (!d->mbm_total) {
> +			bitmap_free(d->rmid_busy_llc);
> +			return -ENOMEM;
> +		}
> +	}
> +	if (is_mbm_local_enabled()) {
> +		tsize = sizeof(*d->mbm_local);
> +		d->mbm_local = kcalloc(r->num_rmid, tsize, GFP_KERNEL);
> +		if (!d->mbm_local) {
> +			bitmap_free(d->rmid_busy_llc);
> +			kfree(d->mbm_total);
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
> +{
> +	int err;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);

Looks like lockdep_assert_held was not there in this sequence.
Are you concerned about this lock not being held?
Thanks
Babu Moger