Hi Reinette,
On 8/31/23 12:42, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/30/2023 4:19 PM, Moger, Babu wrote:
>> On 8/29/23 15:14, Reinette Chatre wrote:
>>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>
>>> ...
>>>
>>>> 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).
>>
>>>
>>>> * --> 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.
>
>>
>> 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
