Re: [PATCH v4] x86/resctrl: Add mount option to pick total MBM event

[Reinette Chatre]

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