Re: [PATCH v8 8/8] x86/resctrl: Display hardware ids of resource groups

["Moger, Babu" <babu.moger@amd.com>]

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