Message-ID: <99939c7d-3c8d-f1d4-9226-81f32925e045@intel.com>
Date:   Thu, 17 Aug 2023 11:34:39 -0700
From:   Fenghua Yu <fenghua.yu@...el.com>
To:     James Morse <james.morse@....com>, <x86@...nel.org>,
        <linux-kernel@...r.kernel.org>
CC:     Reinette Chatre <reinette.chatre@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        Babu Moger <Babu.Moger@....com>,
        <shameerali.kolothum.thodi@...wei.com>,
        "D Scott Phillips OS" <scott@...amperecomputing.com>,
        <carl@...amperecomputing.com>, <lcherian@...vell.com>,
        <bobo.shaobowang@...wei.com>, <tan.shaopeng@...itsu.com>,
        <xingxin.hx@...nanolis.org>, <baolin.wang@...ux.alibaba.com>,
        Jamie Iles <quic_jiles@...cinc.com>,
        Xin Hao <xhao@...ux.alibaba.com>, <peternewman@...gle.com>,
        <dfustini@...libre.com>
Subject: Re: [PATCH v5 19/24] x86/resctrl: Add helpers for system wide
 mon/alloc capable

Hi, James,

On 7/28/23 09:42, James Morse wrote:
> resctrl reads rdt_alloc_capable or rdt_mon_capable to determine
> whether any of the resources support the corresponding features.
> resctrl also uses the static-keys that affect the architecture's
> context-switch code to determine the same thing.
> 
> This forces another architecture to have the same static-keys.
> 
> As the static-key is enabled based on the capable flag, and none of
> the filesystem uses of these are in the scheduler path, move the
> capable flags behind helpers, and use these in the filesystem
> code instead of the static-key.
> 
> After this change, only the architecture code manages and uses
> the static-keys to ensure __resctrl_sched_in() does not need
> runtime checks.
> 
> This avoids multiple architectures having to define the same
> static-keys.
> 
> Cases where the static-key implicitly tested if the resctrl
> filesystem was mounted all have an explicit check added by a
> previous patch.
> 
> Tested-by: Shaopeng Tan <tan.shaopeng@...itsu.com>
> Reviewed-by: Shaopeng Tan <tan.shaopeng@...itsu.com>
> Signed-off-by: James Morse <james.morse@....com>
> 
> ---
> Changes since v1:
>   * Added missing conversion in mkdir_rdt_prepare_rmid_free()
> 
> Changes since v3:
>   * Expanded the commit message.
> ---
>   arch/x86/include/asm/resctrl.h            | 13 +++++++++
>   arch/x86/kernel/cpu/resctrl/internal.h    |  2 --
>   arch/x86/kernel/cpu/resctrl/monitor.c     |  4 +--
>   arch/x86/kernel/cpu/resctrl/pseudo_lock.c |  6 ++--
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 34 +++++++++++------------
>   5 files changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index 3876d4bb4bed..63a4a2332d61 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -38,10 +38,18 @@ struct resctrl_pqr_state {
>   
>   DECLARE_PER_CPU(struct resctrl_pqr_state, pqr_state);
>   
> +extern bool rdt_alloc_capable;
> +extern bool rdt_mon_capable;
> +
>   DECLARE_STATIC_KEY_FALSE(rdt_enable_key);
>   DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
>   DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
>   
> +static inline bool resctrl_arch_alloc_capable(void)
> +{
> +	return rdt_alloc_capable;
> +}
> +
>   static inline void resctrl_arch_enable_alloc(void)
>   {
>   	static_branch_enable_cpuslocked(&rdt_alloc_enable_key);
> @@ -54,6 +62,11 @@ static inline void resctrl_arch_disable_alloc(void)
>   	static_branch_dec_cpuslocked(&rdt_enable_key);
>   }
>   
> +static inline bool resctrl_arch_mon_capable(void)
> +{
> +	return rdt_mon_capable;
> +}
> +
>   static inline void resctrl_arch_enable_mon(void)
>   {
>   	static_branch_enable_cpuslocked(&rdt_mon_enable_key);
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index ac39fecba4ca..f99e0a1f39c8 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -136,8 +136,6 @@ struct rmid_read {
>   	void			*arch_mon_ctx;
>   };
>   
> -extern bool rdt_alloc_capable;
> -extern bool rdt_mon_capable;
>   extern unsigned int rdt_mon_features;
>   extern struct list_head resctrl_schema_all;
>   extern bool resctrl_mounted;
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 5350d44b16b6..c0b1ad8d8f6d 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -842,7 +842,7 @@ void mbm_handle_overflow(struct work_struct *work)
>   	 * If the filesystem has been unmounted this work no longer needs to
>   	 * run.
>   	 */
> -	if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))
> +	if (!resctrl_mounted || !resctrl_arch_mon_capable())
>   		goto out_unlock;
>   
>   	r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> @@ -879,7 +879,7 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms)
>   	 * When a domain comes online there is no guarantee the filesystem is
>   	 * mounted. If not, there is no need to catch counter overflow.
>   	 */
> -	if (!resctrl_mounted || !static_branch_likely(&rdt_mon_enable_key))
> +	if (!resctrl_mounted || !resctrl_arch_mon_capable())
>   		return;
>   	cpu = cpumask_any_housekeeping(&dom->cpu_mask);
>   	dom->mbm_work_cpu = cpu;
> diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> index 5ebd6e54c7f2..460421051abf 100644
> --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> @@ -567,7 +567,7 @@ static int rdtgroup_locksetup_user_restrict(struct rdtgroup *rdtgrp)
>   	if (ret)
>   		goto err_cpus;
>   
> -	if (rdt_mon_capable) {
> +	if (resctrl_arch_mon_capable()) {
>   		ret = rdtgroup_kn_mode_restrict(rdtgrp, "mon_groups");
>   		if (ret)
>   			goto err_cpus_list;
> @@ -614,7 +614,7 @@ static int rdtgroup_locksetup_user_restore(struct rdtgroup *rdtgrp)
>   	if (ret)
>   		goto err_cpus;
>   
> -	if (rdt_mon_capable) {
> +	if (resctrl_arch_mon_capable()) {
>   		ret = rdtgroup_kn_mode_restore(rdtgrp, "mon_groups", 0777);
>   		if (ret)
>   			goto err_cpus_list;
> @@ -762,7 +762,7 @@ int rdtgroup_locksetup_exit(struct rdtgroup *rdtgrp)
>   {
>   	int ret;
>   
> -	if (rdt_mon_capable) {
> +	if (resctrl_arch_mon_capable()) {
>   		ret = alloc_rmid(rdtgrp->closid);
>   		if (ret < 0) {
>   			rdt_last_cmd_puts("Out of RMIDs\n");
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index ce1ed485e4f7..fef78a3dc632 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -630,13 +630,13 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
>   
>   static bool is_closid_match(struct task_struct *t, struct rdtgroup *r)
>   {
> -	return (rdt_alloc_capable && (r->type == RDTCTRL_GROUP) &&
> +	return (resctrl_arch_alloc_capable() && (r->type == RDTCTRL_GROUP) &&
>   		resctrl_arch_match_closid(t, r->closid));
>   }
>   
>   static bool is_rmid_match(struct task_struct *t, struct rdtgroup *r)
>   {
> -	return (rdt_mon_capable && (r->type == RDTMON_GROUP) &&
> +	return (resctrl_arch_mon_capable() && (r->type == RDTMON_GROUP) &&
>   		resctrl_arch_match_rmid(t, r->mon.parent->closid,
>   					r->mon.rmid));
>   }
> @@ -2519,7 +2519,7 @@ static int rdt_get_tree(struct fs_context *fc)
>   	if (ret < 0)
>   		goto out_schemata_free;
>   
> -	if (rdt_mon_capable) {
> +	if (resctrl_arch_mon_capable()) {
>   		ret = mongroup_create_dir(rdtgroup_default.kn,
>   					  &rdtgroup_default, "mon_groups",
>   					  &kn_mongrp);
> @@ -2541,12 +2541,12 @@ static int rdt_get_tree(struct fs_context *fc)
>   	if (ret < 0)
>   		goto out_psl;
>   
> -	if (rdt_alloc_capable)
> +	if (resctrl_arch_alloc_capable())
>   		resctrl_arch_enable_alloc();
> -	if (rdt_mon_capable)
> +	if (resctrl_arch_mon_capable())
>   		resctrl_arch_enable_mon();
>   
> -	if (rdt_alloc_capable || rdt_mon_capable)
> +	if (resctrl_arch_alloc_capable() || resctrl_arch_mon_capable())
>   		resctrl_mounted = true;
>   
>   	if (is_mbm_enabled()) {
> @@ -2560,10 +2560,10 @@ static int rdt_get_tree(struct fs_context *fc)
>   out_psl:
>   	rdt_pseudo_lock_release();
>   out_mondata:
> -	if (rdt_mon_capable)
> +	if (resctrl_arch_mon_capable())
>   		kernfs_remove(kn_mondata);
>   out_mongrp:
> -	if (rdt_mon_capable)
> +	if (resctrl_arch_mon_capable())
>   		kernfs_remove(kn_mongrp);
>   out_info:
>   	kernfs_remove(kn_info);
> @@ -2815,9 +2815,9 @@ static void rdt_kill_sb(struct super_block *sb)
>   	rdt_pseudo_lock_release();
>   	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>   	schemata_list_destroy();
> -	if (rdt_alloc_capable)
> +	if (resctrl_arch_alloc_capable())
>   		resctrl_arch_disable_alloc();
> -	if (rdt_mon_capable)
> +	if (resctrl_arch_mon_capable())
>   		resctrl_arch_disable_mon();
>   	resctrl_mounted = false;
>   	kernfs_kill_sb(sb);
> @@ -3197,7 +3197,7 @@ static int mkdir_rdt_prepare_rmid_alloc(struct rdtgroup *rdtgrp)
>   {
>   	int ret;
>   
> -	if (!rdt_mon_capable)
> +	if (!resctrl_arch_mon_capable())
>   		return 0;
>   
>   	ret = alloc_rmid(rdtgrp->closid);
> @@ -3219,7 +3219,7 @@ static int mkdir_rdt_prepare_rmid_alloc(struct rdtgroup *rdtgrp)
>   
>   static void mkdir_rdt_prepare_rmid_free(struct rdtgroup *rgrp)
>   {
> -	if (rdt_mon_capable)
> +	if (resctrl_arch_mon_capable())
>   		free_rmid(rgrp->closid, rgrp->mon.rmid);
>   }
>   
> @@ -3385,7 +3385,7 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>   
>   	list_add(&rdtgrp->rdtgroup_list, &rdt_all_groups);
>   
> -	if (rdt_mon_capable) {
> +	if (resctrl_arch_mon_capable()) {
>   		/*
>   		 * Create an empty mon_groups directory to hold the subset
>   		 * of tasks and cpus to monitor.
> @@ -3440,14 +3440,14 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
>   	 * allocation is supported, add a control and monitoring
>   	 * subdirectory
>   	 */
> -	if (rdt_alloc_capable && parent_kn == rdtgroup_default.kn)
> +	if (resctrl_arch_alloc_capable() && parent_kn == rdtgroup_default.kn)
>   		return rdtgroup_mkdir_ctrl_mon(parent_kn, name, mode);
>   
>   	/*
>   	 * If RDT monitoring is supported and the parent directory is a valid
>   	 * "mon_groups" directory, add a monitoring subdirectory.
>   	 */
> -	if (rdt_mon_capable && is_mon_groups(parent_kn, name))
> +	if (resctrl_arch_mon_capable() && is_mon_groups(parent_kn, name))
>   		return rdtgroup_mkdir_mon(parent_kn, name, mode);
>   
>   	return -EPERM;
> @@ -3779,7 +3779,7 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
>   	 * If resctrl is mounted, remove all the
>   	 * per domain monitor data directories.
>   	 */
> -	if (resctrl_mounted && static_branch_unlikely(&rdt_mon_enable_key))
> +	if (resctrl_mounted && resctrl_arch_mon_capable())
>   		rmdir_mondata_subdir_allrdtgrp(r, d->id);
>   
>   	if (is_mbm_enabled())
> @@ -3862,7 +3862,7 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>   	 * by rdt_get_tree() calling mkdir_mondata_all().
>   	 * If resctrl is mounted, add per domain monitor data directories.
>   	 */
> -	if (resctrl_mounted && static_branch_unlikely(&rdt_mon_enable_key))
> +	if (resctrl_mounted && resctrl_arch_mon_capable())
>   		mkdir_mondata_subdir_allrdtgrp(r, d);
>   
>   	return 0;

Why isn't rdt_alloc_capable in get_rdt_alloc_resources() replaced by the 
helper?

static __init bool get_rdt_alloc_resources(void)
{
...
         if (rdt_alloc_capable)
...

Thanks.

-Fenghua

Powered by blists - more mailing lists