Hi Tony, On 11/28/2023 3:14 PM, Tony Luck wrote: > Add a "total" mount option to be used in conjunction with "mba_MBps" > to request use of the total memory bandwidth event as the feedback > input to the control loop. "total" is very generic. It is also not clear to me why users would need to use two mount options. What if the new mount option is "mba_MBps_total" instead, without user needing to also provide "mba_MBps"? > > Also fall back to using the total event if the local event is not > supported by the CPU. > > Update the once-per-second polling code to use the event (local > or total memory bandwidth). Please take care to describe why this change is needed, not just what it does. This is required by x86. For confirmation: https://lore.kernel.org/lkml/20231009172517.GRZSQ3fT05LGgpcW35@fat_crate.local/ > > Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx> > --- > > Changes since v3: > > Reinette suggested that users might like the option to use the total > memory bandwidth event. I tried out some code to make the event runtime > selectable via a r/w file in the resctrl/info directories. But that > got complicated because of the amount of state that needs to be updated > when switching events. Since there isn't a firm use case for user > selectable event, this latest version falls back to the far simpler > case of using a mount option. (I did not realize that that discussion was over.) > > Documentation/arch/x86/resctrl.rst | 3 +++ > arch/x86/kernel/cpu/resctrl/internal.h | 3 +++ > arch/x86/kernel/cpu/resctrl/monitor.c | 20 +++++++++----------- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 15 ++++++++++++++- > 4 files changed, 29 insertions(+), 12 deletions(-) > > diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst > index a6279df64a9d..29c3e7137eb8 100644 > --- a/Documentation/arch/x86/resctrl.rst > +++ b/Documentation/arch/x86/resctrl.rst > @@ -46,6 +46,9 @@ mount options are: > "mba_MBps": > Enable the MBA Software Controller(mba_sc) to specify MBA > bandwidth in MBps > +"total": > + Use total instead of local memory bandwidth to drive the > + MBA Software Controller > "debug": > Make debug files accessible. Available debug files are annotated with > "Available only with debug option". > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index a4f1aa15f0a2..f98fc9adc2da 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -59,6 +59,7 @@ struct rdt_fs_context { > bool enable_cdpl2; > bool enable_cdpl3; > bool enable_mba_mbps; > + bool use_mbm_total; > bool enable_debug; > }; Why did you choose new member to not follow existing custom of having an enable_ prefix? > > @@ -428,6 +429,8 @@ extern struct rdt_hw_resource rdt_resources_all[]; > extern struct rdtgroup rdtgroup_default; > DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key); > > +extern enum resctrl_event_id mba_mbps_evt_id; > + This global seems unnecessary. struct resctrl_membw.mba_sc indicates if the software controller is enabled. Creating this global fragments related information. One option could be to change the type of struct resctrl_membw.mba_sc to enum resctrl_event_id. I assume that 0 would never be a valid event ID and can thus be used to know if the software controller is disabled. If this is done then enum resctrl_event_id's documentation should be updated with this assumption/usage. Reinette
