Hi Reinette,
On 8/31/23 19:43, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/31/2023 4:58 PM, Moger, Babu wrote:
>> On 8/31/23 12:42, Reinette Chatre wrote:
>>> 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.
>>
>
> heh ... these are not a "few more things" ... these are exactly
> the items I mentioned: "base directory creation" is taking into account
> the default resource group and "mon directory creation after the
> base" are the changes needed in mkdir_rdt_prepare() where RFTYPE_MON
> is appended to the flags.
Ok. Got it.
>
>> I got it working with this patches. We may be able to clean it little
>> more or we can split also.
>
> I think it would make things easier to understand if there
> is a separate patch that adds support for files with
> RFTYPE_MON flag.
Ok. Sure,
>
>>
>> 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;
>
> I assume that you may have just copied this from mkdir_rdt_prepare() but
> I think this should rather match the type as this is used (unsigned long).
Yes. Will correct it.
>
>> 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;
>> +
>
> Is this not redundant considering what just happened a few lines above?
Yea. Right. I will change the previous line to
files = RFTYPE_BASE;
Thanks
Babu Moger
