Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 08/23] x86/resctrl: Create mba_sc configuration in the
 rdt_domain
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        Jamie Iles <jamie@nuviainc.com>,
        "D Scott Phillips OS" <scott@os.amperecomputing.com>,
        <lcherian@marvell.com>, <bobo.shaobowang@huawei.com>,
        <tan.shaopeng@fujitsu.com>
References: <20211001160302.31189-1-james.morse@arm.com>
 <20211001160302.31189-9-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
Message-ID: <facea348-2c54-fe52-09b9-8feccd15db2d@intel.com>
Date:   Fri, 15 Oct 2021 15:26:24 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.14.0
In-Reply-To: <20211001160302.31189-9-james.morse@arm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aGg3Zk5aUUxEZjljdXA1ZXZxWnI3RGVzT3ZmNElsRit4ci8vNEpLVTVPRkhE?=
 =?utf-8?B?Z3pBOEtuaUNuVk9ZVjdnU0wvS2NGYzZ1OURhb0tuQTY3YS90cll2b1VqUjQv?=
 =?utf-8?B?L2NRNFI4Yml2ejZmZ2ZuaFh3ZCtaaTBwakREc2Y3S2M0U3Vxblk3ekpSRVNL?=
 =?utf-8?B?bzNCVFlzYm5teklQUUtOWUtqakNISithY1lTOGVjN3NIN3lIak1kTnBnd0ZB?=
 =?utf-8?B?RkJuYjd5WkhXZUNPcThpb0RvaVBVTzBMS214VzZQTkVqR3NtVG1MRlBhRkZy?=
 =?utf-8?B?dGtzWHJoRnE1dDFXUlZ2N3JGTFNVSWllWDZTbCtobzkraGt2L29XNVZyZzhB?=
 =?utf-8?B?b3dEZTlUczk3Vy9nc2VBS0lMMFJMQk16ZzdxZE1VbUsrU3E4WVZsL0V2ai9V?=
 =?utf-8?B?ckJqWFUrUVJaZ1lLWStnck9kRGRKWHpwb3BqalptdnpEL1N2c0RjdVFha3Js?=
 =?utf-8?B?NEVhTjFGY1Jhdnl3SEt6Ym5KL2ZBdU9LaDNTNHA1VHFpLzNrbU1IMHB0NVhy?=
 =?utf-8?B?VUQrRDJERW9pU3pBZ1p3T0wwQ1Q0SVhRd1ZTRGVVc3VwVUdlK3dzWENuTWRv?=
 =?utf-8?B?NmhHSzZuTSt2WktTemVwQjBNT0J4eUdSSURjcHBBWlY0M2hUSWhQeVN5UTJ1?=
 =?utf-8?B?RjN0Mm9ZVVZJK3JEUjIvblVxUU9tSFRNUHhJWmRKMHhtYjV2cCtMQy9EeHE0?=
 =?utf-8?B?SllWbHJjSm1FT045Z1F3SWZXM0F4c1liQ2VkbTU1TklRR3hsVmJEVFdMU1Z1?=
 =?utf-8?B?N2ZkWHYvZmNBbnNjQWwxb09TS2JVN3dndVl1QlZ0U2pUTkMzZGUrZ3I1NVJt?=
 =?utf-8?B?bFJRYUtSMlhCbjFwWWxSckZqOEo0TW9GZkU4WUp0QlRocHZRM1N2NnkzRU14?=
 =?utf-8?B?Z1RxakVzbDMwbzRwODV6Vkw3TktvdlVnbUo0aDV4bVpOZXpHKy9jUnVXVWRq?=
 =?utf-8?B?cGtrUGYvdDV2Yk9HUzQrRVRiYmt3YkpFRmdKZU5IU2RqMmtYZ20rbUN1RzVR?=
 =?utf-8?B?ZjZ6Zk1DcnhnS1VGTHFIcHFNOW9TZWFGWFRsRGJtTzBpTklGenl0b3B5LzRD?=
 =?utf-8?B?VnNYemI4cDArNGhuY0hDREplakVnQXJ1U2c1RUo4WGkwY1NuMU9JSE9oMkFU?=
 =?utf-8?B?a1NYZEV4cjF3MzJZcmh2bHdlRE5LTk1NOC9JTWM5cytPT3VZMFd0N1hteEMz?=
 =?utf-8?B?Tk1FcVN5bnZDdGJFR2wzU3NXOGpFampNWkFnSmRNMjZKWTNBZDN1VEdoWnNH?=
 =?utf-8?B?aWdWOTg3dGNYb3JkdnVjWmlNTHRUa1J0VEZpRTRWUFlsR25tcERHMnpqREtz?=
 =?utf-8?B?VHI2SzQ3dmlEL2o0MzFsYjFKbFQ4bE1UYUlnY3gvMWZKeUFtTVhyeFg2ZXdC?=
 =?utf-8?B?UW1kUVZ5eXNYN09iUGNZb1lvekpialJ4UmpOVHVkbmhtZkQxVFk3cytCdE5j?=
 =?utf-8?B?endEYVJxbjlsMkFqM2pxUTBGb3lsaGxlTm5GdFBGVVNBYmgwVmFobTBOd215?=
 =?utf-8?B?ajFHTkFXekNTVzc1VVBtRVBqTldzWEJNbzFrRk14citlVUkyaGIxazV1aVJM?=
 =?utf-8?B?eWlCdFE3T1Q4ZmZLMWpTOHp2SVBJdUhsdlFSL0VnSGpOeENyQnM3QWJndXlW?=
 =?utf-8?B?Z0RFemdMMHVnbk1OOFIzUmxRMWJ0U0xiNHRBaHkyejJ6YmF0R1UrVGF3RWhL?=
 =?utf-8?B?bE00WGFWVFZQOTE5VzdnYlN4RC8zWFBuZUlsblNlK29KNjN6V1J3c21YQzRI?=
 =?utf-8?Q?uZC44KVAxYRUtXJedr7tUkWhpSVZsLYEgDJ5L6d?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ffcce775-60a9-4ecb-435a-08d9902ad519
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Oct 2021 22:26:29.0234
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QeHhbUcsn++jZrHkdcaqApZkRffPclVNfV825Q9bqyuzhApxPcNhLm7Uhn2OWK4Fkn0Jk/vQ+UMXj7qIXIGQqXZAzm/K+12nqojusyvD5hk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5387
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi James,

On 10/1/2021 9:02 AM, James Morse wrote:
> To support resctrl's MBA software controller, the architecture must provide
> a second configuration array to hold the mbps_val from user-space.
> 
> This complicates the interface between the architecture code.

This complicates the interface between the architecture code and ... ?

> 
> Make the filesystem parts of resctrl create an array for the mba_sc
> values when is_mba_sc() is set to true. The software controller
> can be changed to use this, allowing the architecture code to only
> consider the values configured in hardware.

This changes significantly more than just where the mbps_val array is 
hosted. It also changes how the life cycle of this array is managed. 
Previously it followed the domain, whether mba_sc was enabled or not. 
Now that it depends on mba_sc it is managed quite differently.

Could the changelog be upfront about this change and its motivation? 
Stating this would make this much easier to review and also the later 
patches where the original mbps_val initialization code is removed 
without replacement.

> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> Changes since v1:
>   * Added missing error handling to mba_sc_domain_allocate() in
>     domain_setup_mon_state()
>   * Added comment about mba_sc_domain_allocate() races
>   * Squashed out struct resctrl_mba_sc
>   * Moved mount time alloc/free calls to set_mba_sc().
>   * Removed mount check in resctrl_offline_domain()
>   * Reword commit message
> ---
>   arch/x86/kernel/cpu/resctrl/internal.h |  1 -
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 67 ++++++++++++++++++++++++++
>   include/linux/resctrl.h                |  6 +++
>   3 files changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index e12b55f815bf..a7e2cbce29d5 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -36,7 +36,6 @@
>   #define MBM_OVERFLOW_INTERVAL		1000
>   #define MAX_MBA_BW			100u
>   #define MBA_IS_LINEAR			0x4
> -#define MBA_MAX_MBPS			U32_MAX
>   #define MAX_MBA_BW_AMD			0x800
>   #define MBM_CNTR_WIDTH_OFFSET_AMD	20
>   
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 38670bb810cb..9d402bc8bdff 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1889,6 +1889,64 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r)
>   		l3_qos_cfg_update(&hw_res->cdp_enabled);
>   }
>   
> +static int mba_sc_domain_allocate(struct rdt_resource *r, struct rdt_domain *d)
> +{
> +	u32 num_closid = resctrl_arch_get_num_closid(r);
> +	int cpu = cpumask_any(&d->cpu_mask);
> +	int i;
> +
> +	/*
> +	 * d->mbps_val is allocated by a call to this function in set_mba_sc(),
> +	 * and domain_setup_mon_state(). Both calls are guarded by is_mba_sc(),
> +	 * which can only return true while the filesystem is mounted. The
> +	 * two calls are prevented from racing as rdt_get_tree() takes the
> +	 * cpuhp read lock before calling rdt_enable_ctx(ctx), which prevents
> +	 * it running concurrently with resctrl_online_domain().
> +	 */
> +	lockdep_assert_cpus_held();
> +
> +	d->mbps_val = kcalloc_node(num_closid, sizeof(*d->mbps_val),
> +				   GFP_KERNEL, cpu_to_node(cpu));
> +	if (!d->mbps_val)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_closid; i++)
> +		d->mbps_val[i] = MBA_MAX_MBPS;
> +
> +	return 0;
> +}
> +
> +static int mba_sc_allocate(struct rdt_resource *r)
> +{
> +	struct rdt_domain *d;
> +	int ret;
> +

Please initialize ret.

> +	list_for_each_entry(d, &r->domains, list) {
> +		ret = mba_sc_domain_allocate(r, d);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
> +static void mba_sc_domain_destroy(struct rdt_resource *r,
> +				  struct rdt_domain *d)
> +{
> +	kfree(d->mbps_val);
> +	d->mbps_val = NULL;
> +}
> +
> +static void mba_sc_destroy(struct rdt_resource *r)
> +{
> +	struct rdt_domain *d;
> +
> +	lockdep_assert_cpus_held();
> +
> +	list_for_each_entry(d, &r->domains, list)
> +		mba_sc_domain_destroy(r, d);
> +}
> +
>   /*
>    * Enable or disable the MBA software controller
>    * which helps user specify bandwidth in MBps.
> @@ -1911,6 +1969,10 @@ static int set_mba_sc(bool mba_sc)
>   		setup_default_ctrlval(r, hw_dom->ctrl_val, hw_dom->mbps_val);
>   	}
>   
> +	if (is_mba_sc(r))
> +		return mba_sc_allocate(r);
> +
> +	mba_sc_destroy(r);
>   	return 0;
>   }
>   
> @@ -3259,6 +3321,8 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
>   		__check_limbo(d, true);
>   		cancel_delayed_work(&d->cqm_limbo);
>   	}
> +	if (is_mba_sc(r))
> +		mba_sc_domain_destroy(r, d);
>   	bitmap_free(d->rmid_busy_llc);
>   	kfree(d->mbm_total);
>   	kfree(d->mbm_local);
> @@ -3291,6 +3355,9 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d)
>   		}
>   	}
>   
> +	if (is_mba_sc(r))
> +		return mba_sc_domain_allocate(r, d);
> +
>   	return 0;
>   }
>   

Could this be done symmetrically? That is, allocate in 
resctrl_online_domain() and free in resctrl_offline_domain().

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 5d283bdd6162..355660d46612 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -15,6 +15,9 @@ int proc_resctrl_show(struct seq_file *m,
>   
>   #endif
>   
> +/* max value for struct resctrl_mba_sc's mbps_val */
> +#define MBA_MAX_MBPS   U32_MAX

struct resctrl_mba_sc?

> +
>   /**
>    * enum resctrl_conf_type - The type of configuration.
>    * @CDP_NONE:	No prioritisation, both code and data are controlled or monitored.
> @@ -53,6 +56,8 @@ struct resctrl_staged_config {
>    * @cqm_work_cpu:	worker CPU for CQM h/w counters
>    * @plr:		pseudo-locked region (if any) associated with domain
>    * @staged_config:	parsed configuration to be applied
> + * @mbps_val:		Array of user specified control values for mba_sc,
> + *			indexed by closid

Could this inherit some of the useful kerneldoc associated with the 
mbps_val being replaced? That is, it exists when mba_sc is enabled and 
contains bandwidth values in MBps.

>    */
>   struct rdt_domain {
>   	struct list_head		list;
> @@ -67,6 +72,7 @@ struct rdt_domain {
>   	int				cqm_work_cpu;
>   	struct pseudo_lock_region	*plr;
>   	struct resctrl_staged_config	staged_config[CDP_NUM_TYPES];
> +	u32				*mbps_val;
>   };
>   
>   /**
> 

Reinette