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 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