linux-kernel - Re: [PATCH v9 2/9] x86/resctrl: Prepare for per-ctrl_mon group mba

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZzvSFm-dcZwdK9cO@agluck-desk3>
Date: Mon, 18 Nov 2024 15:47:34 -0800
From: "Luck, Tony" <tony.luck@...el.com>
To: babu.moger@....com
Cc: Fenghua Yu <fenghua.yu@...el.com>,
	Reinette Chatre <reinette.chatre@...el.com>,
	Peter Newman <peternewman@...gle.com>,
	Jonathan Corbet <corbet@....net>, x86@...nel.org,
	James Morse <james.morse@....com>,
	Jamie Iles <quic_jiles@...cinc.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	"Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	patches@...ts.linux.dev
Subject: Re: [PATCH v9 2/9] x86/resctrl: Prepare for per-ctrl_mon group
 mba_MBps control

On Fri, Nov 15, 2024 at 10:20:34AM -0600, Moger, Babu wrote:

Thanks for looking. Comments below.

> Hi Tony,
> 
> On 11/13/2024 6:17 PM, Tony Luck wrote:
> > Resctrl uses local memory bandwidth event as input to the feedback
> > loop when the mba_MBps mount option is used. This means that this
> > mount option cannot be used on systems that only support monitoring
> > of total bandwidth.
> > 
> > Prepare to allow users to choose the input event independently for
> > each ctrl_mon group.
> 
> How about this?
> 
> Provide users with the ability to select the input event independently for
> each ctrl_mon group.

That's a description for the series as a whole. This patch doesn't
do all the things in that sentence.

> 
> > 
> > Signed-off-by: Tony Luck <tony.luck@...el.com>
> > ---
> >   include/linux/resctrl.h                | 2 ++
> >   arch/x86/kernel/cpu/resctrl/internal.h | 2 ++
> >   arch/x86/kernel/cpu/resctrl/core.c     | 3 +++
> >   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 6 ++++++
> >   4 files changed, 13 insertions(+)
> > 
> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index d94abba1c716..fd05b937e2f4 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -49,6 +49,8 @@ enum resctrl_event_id {
> >   	QOS_L3_MBM_LOCAL_EVENT_ID	= 0x03,
> >   };
> > +extern enum resctrl_event_id mba_mbps_default_event;
> > +
> >   /**
> >    * struct resctrl_staged_config - parsed configuration to be applied
> >    * @new_ctrl:		new ctrl value to be loaded
> > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> > index faaff9d64102..485800055a7d 100644
> > --- a/arch/x86/kernel/cpu/resctrl/internal.h
> > +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> > @@ -283,6 +283,7 @@ struct pseudo_lock_region {
> >    *				monitor only or ctrl_mon group
> >    * @mon:			mongroup related data
> >    * @mode:			mode of resource group
> > + * @mba_mbps_event:		input monitoring event id when mba_sc is enabled
> >    * @plr:			pseudo-locked region
> >    */
> >   struct rdtgroup {
> > @@ -295,6 +296,7 @@ struct rdtgroup {
> >   	enum rdt_group_type		type;
> >   	struct mongroup			mon;
> >   	enum rdtgrp_mode		mode;
> > +	enum resctrl_event_id		mba_mbps_event;
> >   	struct pseudo_lock_region	*plr;
> >   };
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index f3ee5859b69d..94bf559966d6 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -963,6 +963,9 @@ static __init bool get_rdt_mon_resources(void)
> >   	if (!rdt_mon_features)
> >   		return false;
> > +	if (is_mbm_local_enabled())
> > +		mba_mbps_default_event = QOS_L3_MBM_LOCAL_EVENT_ID;
> 
> 
> Any reason to separate this patch and patch 8?  I feel it can be combined.

patch 8 will set mba_mbps_default_event to QOS_L3_MBM_TOTAL_EVENT_ID
on systems witout support for local memory bandwidth monitoring.

The rest of the code isn't ready for that until midway through this
series when other code has been updated to handle total bandwidth
correctly.

I may have gone to extremes moving that part out all the way to patch
8. It could potentially happen earlier in the series.

> 
> > +
> >   	return !rdt_get_mon_l3_config(r);
> >   }
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 2b198ef95e1e..a8022bddf9f7 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -65,6 +65,8 @@ static void rdtgroup_destroy_root(void);
> >   struct dentry *debugfs_resctrl;
> > +enum resctrl_event_id mba_mbps_default_event;
> > +
> >   static bool resctrl_debug;
> >   void rdt_last_cmd_clear(void)
> > @@ -3611,6 +3613,8 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
> >   			rdt_last_cmd_puts("kernfs subdir error\n");
> >   			goto out_del_list;
> >   		}
> > +		if (is_mba_sc(NULL))
> > +			rdtgrp->mba_mbps_event = mba_mbps_default_event;
> >   	}
> >   	goto out_unlock;
> > @@ -3970,6 +3974,8 @@ static void __init rdtgroup_setup_default(void)
> >   	rdtgroup_default.closid = RESCTRL_RESERVED_CLOSID;
> >   	rdtgroup_default.mon.rmid = RESCTRL_RESERVED_RMID;
> >   	rdtgroup_default.type = RDTCTRL_GROUP;
> > +	if (supports_mba_mbps())
> > +		rdtgroup_default.mba_mbps_event = mba_mbps_default_event;
> >   	INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
> >   	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
> 
> -- 
> - Babu Moger

-Tony