Hi Reinette,

On 8/31/23 12:42, Reinette Chatre wrote:

Hi Babu,

On 8/30/2023 4:19 PM, Moger, Babu wrote:

quoted

On 8/29/23 15:14, Reinette Chatre wrote:

quoted

On 8/21/2023 4:30 PM, Babu Moger wrote:

...

quoted

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 2fae6d9e24d3..3fa32c1c2677 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h

@@ -296,9 +296,15 @@ struct rdtgroup {
  *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
  *	    Files: cpus, cpus_list, tasks
  *
+ *		--> RFTYPE_DEBUG (Files to help resctrl debugging)
+ *		    File: mon_hw_id
+ *

This does not look right. I think mon_hw_id should have RFTYPE_MON
(more below).

I am not sure about this (more below).

quoted

quoted

  *		--> RFTYPE_CTRL (Files only for CTRL group)
  *		    Files: mode, schemata, size
  *
+ *			--> RFTYPE_DEBUG (Files to help resctrl debugging)
+ *			    File: ctrl_hw_id
+ *
  */
 #define RFTYPE_INFO			BIT(0)
 #define RFTYPE_BASE			BIT(1)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 94bd69f9964c..e0c2479acf49 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c

@@ -776,6 +776,38 @@ static int rdtgroup_tasks_show(struct kernfs_open_file *of,
 	return ret;
 }
 
+static int rdtgroup_closid_show(struct kernfs_open_file *of,
+				struct seq_file *s, void *v)
+{
+	struct rdtgroup *rdtgrp;
+	int ret = 0;
+
+	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	if (rdtgrp)
+		seq_printf(s, "%u\n", rdtgrp->closid);
+	else
+		ret = -ENOENT;
+	rdtgroup_kn_unlock(of->kn);
+
+	return ret;
+}
+
+static int rdtgroup_rmid_show(struct kernfs_open_file *of,
+			      struct seq_file *s, void *v)
+{
+	struct rdtgroup *rdtgrp;
+	int ret = 0;
+
+	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	if (rdtgrp)
+		seq_printf(s, "%u\n", rdtgrp->mon.rmid);
+	else
+		ret = -ENOENT;
+	rdtgroup_kn_unlock(of->kn);
+
+	return ret;
+}
+
 #ifdef CONFIG_PROC_CPU_RESCTRL
 
 /*

@@ -1837,6 +1869,13 @@ static struct rftype res_common_files[] = {
 		.seq_show	= rdtgroup_tasks_show,
 		.fflags		= RFTYPE_BASE,
 	},
+	{
+		.name		= "mon_hw_id",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdtgroup_rmid_show,
+		.fflags		= RFTYPE_BASE | RFTYPE_DEBUG,

I missed this earlier but I think this also needs a RFTYPE_MON.
Perhaps patch 3 can introduce a new RFTYPE_MON_BASE to not
have the flags of the two new files look too different?

We have two types of files in base directory structure.

 if (rtype == RDTCTRL_GROUP)
                files = RFTYPE_BASE | RFTYPE_CTRL;
        else
                files = RFTYPE_BASE | RFTYPE_MON;

1. RFTYPE_BASE | RFTYPE_CTRL;
   Files for the control group only.

2. RFTYPE_BASE | RFTYPE_MON;
   Files for both control and mon groups. However, RFTYPE_MON is not used
for any files. It is only RFTYPE_BASE.

Because of the check in rdtgroup_add_files it all works fine.
For the control group it creates files with RFTYPE_BASE | RFTYPE_CTRL and
RFTYPE_BASE.

For the mon group it creates files with RFTYPE_BASE only.

This describes current behavior because there are no resctrl
files in base that are specific to monitoring, mon_hw_id is the
first.

This does not mean that the new file mon_hw_id should just have
RFTYPE_BASE because that would result in mon_hw_id being created
for all control groups, even those that do not support monitoring
Having mon_hw_id in resctrl for a group that does not support
monitoring is not correct.

You should be able to reproduce this when booting your system
with rdt=!cmt,!mbmlocal,!mbmtotal.

You are right. I reproduced it.

quoted

Adding FTYPE_MON_BASE will be a problem.

Yes, this change does not just involve assigning the RFTYPE_MON
to mon_hw_id. As you describe mkdir_rdt_prepare() does not take
RFTYPE_MON into account when creating the files. Could this not just
be a straightforward change to have it append RFTYPE_MON to the flags
of files needing to be created for a CTRL_MON group? This would
support new resource groups and then the default resource group
would need to be taken into account also. What am I missing?

It is not straight forward. We have have to handle few more things.
1. Base directory creation.
2. Mon directory creation after the base.

I got it working with this patches.  We may be able to clean it little
more or we can split also.

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h

b/arch/x86/kernel/cpu/resctrl/internal.h
index 3fa32c1c2677..e2f3197f1c24 100644

--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h

@@ -318,6 +318,7 @@ struct rdtgroup {
 #define RFTYPE_MON_INFO                        (RFTYPE_INFO | RFTYPE_MON)
 #define RFTYPE_TOP_INFO                        (RFTYPE_INFO | RFTYPE_TOP)
 #define RFTYPE_CTRL_BASE               (RFTYPE_BASE | RFTYPE_CTRL)
+#define RFTYPE_MON_BASE                        (RFTYPE_BASE | RFTYPE_MON)

 /* List of all resource groups */
 extern struct list_head rdt_all_groups;

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c

b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e0c2479acf49..1f9abab7b9bd 100644

--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c

@@ -1874,7 +1874,7 @@ static struct rftype res_common_files[] = {
                .mode           = 0444,
                .kf_ops         = &rdtgroup_kf_single_ops,
                .seq_show       = rdtgroup_rmid_show,
-               .fflags         = RFTYPE_BASE | RFTYPE_DEBUG,
+               .fflags         = RFTYPE_MON_BASE | RFTYPE_DEBUG,
        },
        {
                .name           = "schemata",

@@ -2558,6 +2558,7 @@ static void schemata_list_destroy(void)
 static int rdt_get_tree(struct fs_context *fc)
 {
        struct rdt_fs_context *ctx = rdt_fc2context(fc);
+       uint flags = RFTYPE_CTRL_BASE;
        struct rdt_domain *dom;
        struct rdt_resource *r;
        int ret;

@@ -2588,7 +2589,10 @@ static int rdt_get_tree(struct fs_context *fc)

        closid_init();

-       ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
+       if (rdt_mon_capable)
+               flags |= RFTYPE_MON;
+
+       ret = rdtgroup_add_files(rdtgroup_default.kn, flags);
        if (ret)
                goto out_schemata_free;

@@ -3336,6 +3340,9 @@ static int mkdir_rdt_prepare(struct kernfs_node

*parent_kn,
        else
                files = RFTYPE_BASE | RFTYPE_MON;

+       if (rdt_mon_capable)
+               files |= RFTYPE_MON;
+
        ret = rdtgroup_add_files(kn, files);
        if (ret) {
                rdt_last_cmd_puts("kernfs fill error\n");


-- 
Thanks
Babu Moger