diff mbox series

[v3,7/7] x86/resctrl: Add debug files when mounted with debug option

Message ID 167778870823.1053859.9940104139782066465.stgit@bmoger-ubuntu
State New
Headers
Series x86/resctrl: Miscellaneous resctrl features |

Commit Message

Moger, Babu March 2, 2023, 8:25 p.m. UTC 
  Add the debug files to the resctrl hierarchy.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |   48 +++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

Comments

Reinette Chatre March 15, 2023, 6:45 p.m. UTC | #1 
Hi Babu,

On 3/2/2023 12:25 PM, Babu Moger wrote:
> Add the debug files to the resctrl hierarchy.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   48 +++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index a1f13715a65c..790c6b9f9031 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2400,6 +2400,45 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>  			     struct rdtgroup *prgrp,
>  			     struct kernfs_node **mon_data_kn);
>  
> +void resctrl_add_debug_file(struct kernfs_node *parent_kn, const char *config,
> +			    unsigned long fflags)
> +{
> +	struct rftype *rft;
> +
> +	rft = rdtgroup_get_rftype_by_name(config);
> +	if (rft) {
> +		rft->fflags |= fflags;
> +		rdtgroup_add_file(parent_kn, rft);
> +	}
> +}
> +
> +static void resctrl_add_debug_files(void)
> +{
> +	resctrl_add_debug_file(rdtgroup_default.kn, "rmid", RFTYPE_BASE);
> +	resctrl_add_debug_file(rdtgroup_default.kn, "closid", RFTYPE_BASE_CTRL);
> +	kernfs_activate(rdtgroup_default.kn);
> +}
> +

I think that separating this from all the other resctrl file creation
can be a source of confusion and bugs. Why not add the debug files
at the same time as all the other files belonging to a resource group?

How about introducing new flags, perhaps RFTYPE_MON_DEBUG and
RFTYPE_CTRL_DEBUG. When the debug option is enabled (when resctrl_debug
is true) then the appropriate flag can be OR'd with the other flags
before rdtgroup_add_files() is called.

It sounds to me if there are plans to add more of these files. A function
like resctrl_add_debug_files() requires a lot of changes and care (and thus
potential for errors) every time a new debug file is added.

On another note ... what are the plans with this debug area? At some
point it may be better to expand resctrl debugfs.

Reinette
Moger, Babu March 22, 2023, 3:09 p.m. UTC | #2 
On 3/15/23 13:45, Reinette Chatre wrote:
> Hi Babu,
> 
> On 3/2/2023 12:25 PM, Babu Moger wrote:
>> Add the debug files to the resctrl hierarchy.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   48 +++++++++++++++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index a1f13715a65c..790c6b9f9031 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2400,6 +2400,45 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>>  			     struct rdtgroup *prgrp,
>>  			     struct kernfs_node **mon_data_kn);
>>  
>> +void resctrl_add_debug_file(struct kernfs_node *parent_kn, const char *config,
>> +			    unsigned long fflags)
>> +{
>> +	struct rftype *rft;
>> +
>> +	rft = rdtgroup_get_rftype_by_name(config);
>> +	if (rft) {
>> +		rft->fflags |= fflags;
>> +		rdtgroup_add_file(parent_kn, rft);
>> +	}
>> +}
>> +
>> +static void resctrl_add_debug_files(void)
>> +{
>> +	resctrl_add_debug_file(rdtgroup_default.kn, "rmid", RFTYPE_BASE);
>> +	resctrl_add_debug_file(rdtgroup_default.kn, "closid", RFTYPE_BASE_CTRL);
>> +	kernfs_activate(rdtgroup_default.kn);
>> +}
>> +
> 
> I think that separating this from all the other resctrl file creation
> can be a source of confusion and bugs. Why not add the debug files
> at the same time as all the other files belonging to a resource group?
> 
> How about introducing new flags, perhaps RFTYPE_MON_DEBUG and
> RFTYPE_CTRL_DEBUG. When the debug option is enabled (when resctrl_debug
> is true) then the appropriate flag can be OR'd with the other flags
> before rdtgroup_add_files() is called.

Yes. I could try that.
> 
> It sounds to me if there are plans to add more of these files. A function
> like resctrl_add_debug_files() requires a lot of changes and care (and thus
> potential for errors) every time a new debug file is added.
> 
> On another note ... what are the plans with this debug area? At some
> point it may be better to expand resctrl debugfs.

Hmm.. I have not thought about that.
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index a1f13715a65c..790c6b9f9031 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2400,6 +2400,45 @@  static int mkdir_mondata_all(struct kernfs_node *parent_kn,
 			     struct rdtgroup *prgrp,
 			     struct kernfs_node **mon_data_kn);
 
+void resctrl_add_debug_file(struct kernfs_node *parent_kn, const char *config,
+			    unsigned long fflags)
+{
+	struct rftype *rft;
+
+	rft = rdtgroup_get_rftype_by_name(config);
+	if (rft) {
+		rft->fflags |= fflags;
+		rdtgroup_add_file(parent_kn, rft);
+	}
+}
+
+static void resctrl_add_debug_files(void)
+{
+	resctrl_add_debug_file(rdtgroup_default.kn, "rmid", RFTYPE_BASE);
+	resctrl_add_debug_file(rdtgroup_default.kn, "closid", RFTYPE_BASE_CTRL);
+	kernfs_activate(rdtgroup_default.kn);
+}
+
+void resctrl_remove_debug_file(struct kernfs_node *parent_kn,
+			       const char *config, unsigned long fflags)
+{
+	struct rftype *rft;
+
+	rft = rdtgroup_get_rftype_by_name(config);
+	if (rft) {
+		rft->fflags &= ~fflags;
+		kernfs_remove_by_name(parent_kn, config);
+	}
+}
+
+static void resctrl_remove_debug_files(void)
+{
+	resctrl_remove_debug_file(rdtgroup_default.kn, "rmid", RFTYPE_BASE);
+	resctrl_remove_debug_file(rdtgroup_default.kn, "closid",
+				  RFTYPE_BASE_CTRL);
+	kernfs_activate(rdtgroup_default.kn);
+}
+
 static int rdt_enable_ctx(struct rdt_fs_context *ctx)
 {
 	int ret = 0;
@@ -2413,8 +2452,10 @@  static int rdt_enable_ctx(struct rdt_fs_context *ctx)
 	if (!ret && ctx->enable_mba_mbps)
 		ret = set_mba_sc(true);
 
-	if (!ret && ctx->debug)
+	if (!ret && ctx->debug) {
 		resctrl_debug = true;
+		resctrl_add_debug_files();
+	}
 
 	return ret;
 }
@@ -2831,6 +2872,11 @@  static void rdt_kill_sb(struct super_block *sb)
 
 	set_mba_sc(false);
 
+	if (resctrl_debug) {
+		resctrl_remove_debug_files();
+		resctrl_debug = false;
+	}
+
 	/*Put everything back to default values. */
 	for_each_alloc_capable_rdt_resource(r)
 		reset_all_ctrls(r);