Re: [PATCH v6 1/3] x86/resctrl: Add mount option "mba_MBps_event"

["Moger, Babu" <bmoger@amd.com>]

Hi Tony,

On 12/7/2023 1:56 PM, Tony Luck wrote:
> The MBA Software Controller(mba_sc) is a feedback loop that uses
> measurements of local memory bandwidth to adjust MBA throttling levels
> to keep workloads in a resctrl group within a target bandwidth set in
> the schemata file.
>
> Users may want to use total memory bandwidth instead of local to handle
> workloads that have poor NUMA localization.
>
> Add a new mount option "mba_MBps_event={event_name}" where event_name
> is one of "mbm_Local_bytes" or "mbm_total_bytes" that allows a user to
> specify which monitoring event to use.
>
> Update the once-per-second polling code to use the chosen event (local
> or total memory bandwidth).
>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>   include/linux/resctrl.h                |  2 +
>   arch/x86/kernel/cpu/resctrl/internal.h |  3 +-
>   arch/x86/kernel/cpu/resctrl/monitor.c  | 21 +++++----
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 +++++++++++++++++++++-----
>   4 files changed, 63 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 66942d7fba7f..1feb3b2e64fa 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -129,6 +129,7 @@ enum membw_throttle_mode {
>    * @throttle_mode:	Bandwidth throttling mode when threads request
>    *			different memory bandwidths
>    * @mba_sc:		True if MBA software controller(mba_sc) is enabled
> + * @mba_mbps_event:	Event (local or total) for mba_sc
>    * @mb_map:		Mapping of memory B/W percentage to memory B/W delay
>    */
>   struct resctrl_membw {
> @@ -138,6 +139,7 @@ struct resctrl_membw {
>   	bool				arch_needs_linear;
>   	enum membw_throttle_mode	throttle_mode;
>   	bool				mba_sc;
> +	enum resctrl_event_id		mba_mbps_event;
>   	u32				*mb_map;
>   };
>   
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index a4f1aa15f0a2..8b9b8f664324 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -58,7 +58,8 @@ struct rdt_fs_context {
>   	struct kernfs_fs_context	kfc;
>   	bool				enable_cdpl2;
>   	bool				enable_cdpl3;
> -	bool				enable_mba_mbps;
> +	bool				enable_mba_mbps_local;
> +	bool				enable_mba_mbps_total;
>   	bool				enable_debug;
>   };
>   
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index f136ac046851..d9e590f1cbc3 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -431,9 +431,10 @@ static int __mon_event_count(u32 rmid, struct rmid_read *rr)
>    */
>   static void mbm_bw_count(u32 rmid, struct rmid_read *rr)
>   {
> -	struct mbm_state *m = &rr->d->mbm_local[rmid];
>   	u64 cur_bw, bytes, cur_bytes;
> +	struct mbm_state *m;
>   
> +	m = get_mbm_state(rr->d, rmid, rr->evtid);
>   	cur_bytes = rr->val;
>   	bytes = cur_bytes - m->prev_bw_bytes;
>   	m->prev_bw_bytes = cur_bytes;
> @@ -521,19 +522,21 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm)
>   	u32 closid, rmid, cur_msr_val, new_msr_val;
>   	struct mbm_state *pmbm_data, *cmbm_data;
>   	u32 cur_bw, delta_bw, user_bw;
> +	enum resctrl_event_id evt_id;
>   	struct rdt_resource *r_mba;
>   	struct rdt_domain *dom_mba;
>   	struct list_head *head;
>   	struct rdtgroup *entry;
>   
> -	if (!is_mbm_local_enabled())
> +	if (!is_mbm_enabled())
>   		return;
>   
>   	r_mba = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
> +	evt_id = r_mba->membw.mba_mbps_event;
>   
>   	closid = rgrp->closid;
>   	rmid = rgrp->mon.rmid;
> -	pmbm_data = &dom_mbm->mbm_local[rmid];
> +	pmbm_data = get_mbm_state(dom_mbm, rmid, evt_id);
>   
>   	dom_mba = get_domain_from_cpu(smp_processor_id(), r_mba);
>   	if (!dom_mba) {
> @@ -553,7 +556,7 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_domain *dom_mbm)
>   	 */
>   	head = &rgrp->mon.crdtgrp_list;
>   	list_for_each_entry(entry, head, mon.crdtgrp_list) {
> -		cmbm_data = &dom_mbm->mbm_local[entry->mon.rmid];
> +		cmbm_data = get_mbm_state(dom_mbm, entry->mon.rmid, evt_id);
>   		cur_bw += cmbm_data->prev_bw;
>   		delta_bw += cmbm_data->delta_bw;
>   	}
> @@ -616,18 +619,14 @@ static void mbm_update(struct rdt_resource *r, struct rdt_domain *d, int rmid)
>   		rr.evtid = QOS_L3_MBM_TOTAL_EVENT_ID;
>   		rr.val = 0;
>   		__mon_event_count(rmid, &rr);
> +		if (is_mba_sc(NULL) && rr.evtid == r->membw.mba_mbps_event)
> +			mbm_bw_count(rmid, &rr);
>   	}
>   	if (is_mbm_local_enabled()) {
>   		rr.evtid = QOS_L3_MBM_LOCAL_EVENT_ID;
>   		rr.val = 0;
>   		__mon_event_count(rmid, &rr);
> -
> -		/*
> -		 * Call the MBA software controller only for the
> -		 * control groups and when user has enabled
> -		 * the software controller explicitly.
> -		 */
> -		if (is_mba_sc(NULL))
> +		if (is_mba_sc(NULL) && rr.evtid == r->membw.mba_mbps_event)
>   			mbm_bw_count(rmid, &rr);
>   	}
>   }
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 69a1de92384a..5f64a0b2597c 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2294,7 +2294,7 @@ static bool supports_mba_mbps(void)
>   {
>   	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
>   
> -	return (is_mbm_local_enabled() &&
> +	return (is_mbm_enabled() &&
>   		r->alloc_capable && is_mba_linear());
>   }
>   
> @@ -2302,7 +2302,7 @@ static bool supports_mba_mbps(void)
>    * Enable or disable the MBA software controller
>    * which helps user specify bandwidth in MBps.
>    */
> -static int set_mba_sc(bool mba_sc)
> +static int set_mba_sc(bool mba_sc, enum resctrl_event_id mba_mbps_event)
>   {
>   	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
>   	u32 num_closid = resctrl_arch_get_num_closid(r);
> @@ -2313,6 +2313,7 @@ static int set_mba_sc(bool mba_sc)
>   		return -EINVAL;
>   
>   	r->membw.mba_sc = mba_sc;
> +	r->membw.mba_mbps_event = mba_mbps_event;
>   
>   	list_for_each_entry(d, &r->domains, list) {
>   		for (i = 0; i < num_closid; i++)
> @@ -2445,13 +2446,14 @@ static void rdt_disable_ctx(void)
>   {
>   	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>   	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
> -	set_mba_sc(false);
> +	set_mba_sc(false, QOS_L3_MBM_LOCAL_EVENT_ID);

This is kind of miss leading. Why do you pass 
"QOS_L3_MBM_LOCAL_EVENT_ID" here?

If you move the following initialization to rdt_enable_ctx, then you 
don't need to pass the second argument.

r->membw.mba_mbps_event = mba_mbps_event;

Thanks
Babu

>   
>   	resctrl_debug = false;
>   }
>   
>   static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>   {
> +	enum resctrl_event_id mba_mbps_event;
>   	int ret = 0;
>   
>   	if (ctx->enable_cdpl2) {
> @@ -2466,8 +2468,12 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>   			goto out_cdpl2;
>   	}
>   
> -	if (ctx->enable_mba_mbps) {
> -		ret = set_mba_sc(true);
> +	if (ctx->enable_mba_mbps_local || ctx->enable_mba_mbps_total) {
> +		if (ctx->enable_mba_mbps_total)
> +			mba_mbps_event = QOS_L3_MBM_TOTAL_EVENT_ID;
> +		else
> +			mba_mbps_event = QOS_L3_MBM_LOCAL_EVENT_ID;
> +		ret = set_mba_sc(true, mba_mbps_event);
>   		if (ret)
>   			goto out_cdpl3;
>   	}
> @@ -2683,15 +2689,17 @@ enum rdt_param {
>   	Opt_cdp,
>   	Opt_cdpl2,
>   	Opt_mba_mbps,
> +	Opt_mba_mbps_event,
>   	Opt_debug,
>   	nr__rdt_params
>   };
>   
>   static const struct fs_parameter_spec rdt_fs_parameters[] = {
> -	fsparam_flag("cdp",		Opt_cdp),
> -	fsparam_flag("cdpl2",		Opt_cdpl2),
> -	fsparam_flag("mba_MBps",	Opt_mba_mbps),
> -	fsparam_flag("debug",		Opt_debug),
> +	fsparam_flag("cdp",			Opt_cdp),
> +	fsparam_flag("cdpl2",			Opt_cdpl2),
> +	fsparam_flag("mba_MBps",		Opt_mba_mbps),
> +	fsparam_string("mba_MBps_event",	Opt_mba_mbps_event),
> +	fsparam_flag("debug",			Opt_debug),
>   	{}
>   };
>   
> @@ -2715,7 +2723,25 @@ static int rdt_parse_param(struct fs_context *fc, struct fs_parameter *param)
>   	case Opt_mba_mbps:
>   		if (!supports_mba_mbps())
>   			return -EINVAL;
> -		ctx->enable_mba_mbps = true;
> +		if (is_mbm_local_enabled())
> +			ctx->enable_mba_mbps_local = true;
> +		else
> +			return -EINVAL;
> +		return 0;
> +	case Opt_mba_mbps_event:
> +		if (!supports_mba_mbps())
> +			return -EINVAL;
> +		if (!strcmp("mbm_local_bytes", param->string)) {
> +			if (!is_mbm_local_enabled())
> +				return -EINVAL;
> +			ctx->enable_mba_mbps_local = true;
> +		} else if (!strcmp("mbm_total_bytes", param->string)) {
> +			if (!is_mbm_total_enabled())
> +				return -EINVAL;
> +			ctx->enable_mba_mbps_total = true;
> +		} else {
> +			return -EINVAL;
> +		}
>   		return 0;
>   	case Opt_debug:
>   		ctx->enable_debug = true;
> @@ -3780,16 +3806,27 @@ static int rdtgroup_rename(struct kernfs_node *kn,
>   	return ret;
>   }
>   
> +static char *mba_sc_event_opt_name(struct rdt_resource *r)
> +{
> +	if (r->membw.mba_mbps_event == QOS_L3_MBM_LOCAL_EVENT_ID)
> +		return ",mba_MBps_event=mbm_local_bytes";
> +	else if (r->membw.mba_mbps_event == QOS_L3_MBM_TOTAL_EVENT_ID)
> +		return ",mba_MBps_event=mbm_total_bytes";
> +	return "";
> +}
> +
>   static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
>   {
> +	struct rdt_resource *r_mba = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
> +
>   	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
>   		seq_puts(seq, ",cdp");
>   
>   	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
>   		seq_puts(seq, ",cdpl2");
>   
> -	if (is_mba_sc(&rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl))
> -		seq_puts(seq, ",mba_MBps");
> +	if (is_mba_sc(r_mba))
> +		seq_puts(seq, mba_sc_event_opt_name(r_mba));
>   
>   	if (resctrl_debug)
>   		seq_puts(seq, ",debug");