Re: [PATCH v5 5/8] x86/resctrl: Introduce "-o debug" mount option

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 6/1/2023 12:01 PM, Babu Moger wrote:
> Add "-o debug" option to mount resctrl filesystem in debug mode.
> This option can be used for adding extra files to help debug
> resctrl issues.

Please avoid uncertainty in the changelog (re. "can be used"). I
think it will help to be more specific if the first and last
hunks of patch 8 is merged into this patch, making it clear
that the debug mount option is in support of debug files with
this changelog written to support that.

> For example, hardware uses CLOSID and RMIDs to control and monitor
> resctrl resources. These numbers are not visible to the user. These
> details can help to debug issues. Debug option provides a method to
> add these files to resctrl.

With the debug file support added here this extra motivation should
not be necessary (remaining hunks of patch 8 can be moved to where
these files are introduced).
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  Documentation/arch/x86/resctrl.rst     |    3 +++
>  arch/x86/kernel/cpu/resctrl/internal.h |    1 +
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   15 +++++++++++++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 290f01aa3002..afdee4d1f691 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
> +"debug":
> +	Make debug files accessible. Available debug files are annotated with
> +	"Available only with debug option".
>  

At the risk of becoming unreadable, the earlier documentation of the mount
command should also change.

>  L2 and L3 CDP are controlled separately.
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index c20cd6acb7a3..c07c6517d856 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				enable_debug;
>  };
>  
>  static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index baed20b2d788..be91dea5f927 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -56,6 +56,8 @@ static char last_cmd_status_buf[512];
>  
>  struct dentry *debugfs_resctrl;
>  
> +static bool resctrl_debug;
> +
>  void rdt_last_cmd_clear(void)
>  {
>  	lockdep_assert_held(&rdtgroup_mutex);
> @@ -2368,6 +2370,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>  	if (!ret && ctx->enable_mba_mbps)
>  		ret = set_mba_sc(true);
>  
> +	if (!ret && ctx->enable_debug)
> +		resctrl_debug = true;
> +
>  	return ret;
>  }

Looks like unwinding of rdt_enable_ctx() errors is done in the caller, this is
not ideal and additions like above cause some error unwinding to be omitted.
I cannot see why rdt_enable_ctx() cannot do its own error unwinding. This
may be better as a separate patch.

Reinette