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.
>
> 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?
Reinette
