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

[Reinette Chatre]

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