Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5b75506b-80ac-a89d-4b70-f775c3b354d3@intel.com>
Date:   Thu, 31 Aug 2023 10:42:39 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.14.0
Subject: Re: [PATCH v8 8/8] x86/resctrl: Display hardware ids of resource
 groups
To:     <babu.moger@amd.com>, <corbet@lwn.net>, <tglx@linutronix.de>,
        <mingo@redhat.com>, <bp@alien8.de>
CC:     <fenghua.yu@intel.com>, <dave.hansen@linux.intel.com>,
        <x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>,
        <akpm@linux-foundation.org>, <quic_neeraju@quicinc.com>,
        <rdunlap@infradead.org>, <damien.lemoal@opensource.wdc.com>,
        <songmuchun@bytedance.com>, <peterz@infradead.org>,
        <jpoimboe@kernel.org>, <pbonzini@redhat.com>,
        <chang.seok.bae@intel.com>, <pawan.kumar.gupta@linux.intel.com>,
        <jmattson@google.com>, <daniel.sneddon@linux.intel.com>,
        <sandipan.das@amd.com>, <tony.luck@intel.com>,
        <james.morse@arm.com>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <bagasdotme@gmail.com>,
        <eranian@google.com>, <christophe.leroy@csgroup.eu>,
        <jarkko@kernel.org>, <adrian.hunter@intel.com>,
        <quic_jiles@quicinc.com>, <peternewman@google.com>
References: <20230821233048.434531-1-babu.moger@amd.com>
 <20230821233048.434531-9-babu.moger@amd.com>
 <4019a73a-8478-f18b-7f34-b8d838dfc7fe@intel.com>
 <da47698a-aa9e-3760-1321-5ec3ea1a14e6@amd.com>
Content-Language: en-US
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <da47698a-aa9e-3760-1321-5ec3ea1a14e6@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N1lvV08rT0o3ZnFoTVI0cEtvU1Z3MXhxYmdCbjdNNVlNdVVWRUVqWTllYURy?=
 =?utf-8?B?b1NGNElWUmxVTmtlMVQrSFhoUDE3RXZ5bDhYQzlBNmFJc0FDbzhmSVNDS1g5?=
 =?utf-8?B?dThrbGkvblJSY21LT3U1ZkF6aDByemhQcXJkMW9uUmt3ZTBkR2I5cjllRjNV?=
 =?utf-8?B?Z25nSmxrYU5ETFR6eGlQTW1Wc3hhNDlKNUFxMVRWTEFNL1c4Y0hVT3RIWm5x?=
 =?utf-8?B?L0s1TlUzRkFwNVdLejFrSzZOR1BZZk5EYkYwTnd1Rno4bXA1d3dybXlSc2Uy?=
 =?utf-8?B?dmNTMW55bGR5QXB1ZTZQRDFUQjVHVkxWWXV2M3A0TmEwdjlXRmI4NXk1M1JE?=
 =?utf-8?B?VEp4dXBSVHZDdHgrV0RLbVFINU1UZ3BhTTlCVkNrYTNRS2p2SFdFOGZ2OURk?=
 =?utf-8?B?NnJvMWxmcTkwSzdaYWVKVW15b2w2SkhTRlBRd3pnQm9yb0RNd1o2cDJ0emFM?=
 =?utf-8?B?QWVuN3FHb0FqS0haTTlycGZWNFFXVWtKekVCdW5JdFgydExOYWdINnJ5STdL?=
 =?utf-8?B?WGkvMWx1V0QvNHFZenVOUHQ0VUpSV3VlSVlDWWx0NTBsZTlLU1Yvai9xS0Z0?=
 =?utf-8?B?KzZ5VEdWQ0VaMjE5ZUtOUmh6YkRiYkc3ejJOMGpGdFJTVkRndDUvU0dDSjN4?=
 =?utf-8?B?REJqNkhJa28xRXNaK2E3anZTUlpnYkFDSldwRFhibE9VWnZGa3g2T2tXTGo2?=
 =?utf-8?B?K0pIWUxOMWdvN2FuaU9XRzJGaXVsU2lOQnF5cU9aVjFWRi9sbWF4V2x5OUhz?=
 =?utf-8?B?VCs3QmY5QlZsa1UraGZ6ZHIrTEkzRGJRVEVOelIwZFpnMUZmalZhSmZBcUYr?=
 =?utf-8?B?T055bWNXMlQybkJiLzBzWEw0N2N6WG91eWhjTHBxSWo5dnpacVFnMUsvTzlF?=
 =?utf-8?B?TDNDbHRrVTc5ZEVtVVYxcnhmeUtIZ2NsTFN6aUU0elNicHpjYUpTUUVvck9v?=
 =?utf-8?B?V1pCQlorcVpUMy9xNjU0a0liNW5zenhQcURVRGI1YkFlUmFYcGYydlViVUla?=
 =?utf-8?B?YVQrbnZWOTFDZmdkd0RXNlVBZFI0amNvVExqYWo2VC94VEFORXJ1R3FZNW8x?=
 =?utf-8?B?QUhqaGdIclNLUWwvdXlTRTRkYlFPb2JHNjVHeXVoQkplMTlUVFMzMUIvZHlj?=
 =?utf-8?B?NmRZS011K0NxZTdlY1czZmRWRFhVVFB2R1k2UFl4L2M4bHdYanJXM3JydE8z?=
 =?utf-8?B?SlEveHREQUN2MmdUdlVSc1hLTFlhY0JSZmRFUTQ2dHYrSEkrNWx3cm9JcjZ4?=
 =?utf-8?B?R0xRdHhUZVloNTNnM3IrYmh2SjZSbmZVSjVUZFdkYy9JSFVjSXlQR0paM000?=
 =?utf-8?B?L2l1Q0RGS1g1TzRBMVRWOHpoY3Y3cXh5Q1B1U2s3SFBrRmFKSlFONFVhL3lv?=
 =?utf-8?B?S20zd1pROGhiYWRpWU5nQVA1ODZsVDVRenpUTHBvZEpPbTJUUGVsakZXemJ3?=
 =?utf-8?B?RzZTQTFTTkRMVWhqeW9ZcUw5WEtZa1ZCVnJjeEYwL1JsUUtnMmlrdnlqRjRC?=
 =?utf-8?B?dE9EZWJ4dGc0cGsrKzhRVTlFQUkzMVJxWkpDbjRDZXNWbVY2eFcxT0ZWejk2?=
 =?utf-8?B?YUpTOWs2N3RPdUtNM2kzblBxdk1QOUc2cWtZOW5hbWY1SkdoVldkcjg3clRq?=
 =?utf-8?B?QU00YURNK1c2aU41WFFscUJFaEY5YVlSU0lyWEZqdVhzY2hoWWlsbjNTTzU2?=
 =?utf-8?B?dUhiQmorMzE3bmhsNCsyQWNjdW5vWjMwZGdCRkYyaEFBZGUyaDFQdVZrR2Vx?=
 =?utf-8?B?ckpLT09IK0hLS3o1emFjTnBuemx4djY3aVZIZElkZGZRMENNL3NkbFB4TnpB?=
 =?utf-8?B?QmZCR0FWTDlROG1lankzM3NsRFQwRDVJNzlKYjh5ZGVCQXhaWUUvWFFXMEpK?=
 =?utf-8?B?Z1QzVUFSckk5MFh3c1R6T0dRbXdHSU9hVUNQSUVFWjVWOE1FSzArS29mczAz?=
 =?utf-8?B?dnJYekRJU1l5VkxCZ2JWa0J6WjlLUEl1N1MwRldOOTNBWU02THlsR1hXaDlL?=
 =?utf-8?B?UUVYQlIrZGp4WFRwRTFxcVpYd1daT1VYRGZDQWZ6RVJwRlZTSkNFbFJhRSta?=
 =?utf-8?B?OVdaYklDWXFEdW1qaVlzMWZSVlRSSUZ4SEdLSFNuWStGdk5iVkplYzZIVXMr?=
 =?utf-8?B?VTVRdFpqaWgwRVhFVmhFQXY0eFM3cHlYSEI4QTZyK1pzREd0MFU2V2dSL0t3?=
 =?utf-8?B?bnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3c70b46b-f8b8-491e-d306-08dbaa49ade5
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Aug 2023 17:42:43.2123
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SLaPPunawtVF3G098hs35c1X7P14B9Nh+Eie6wgdsgD2eMLHKjFPBD5zciAykjMIQHK6fJFe5FxMTfyNalQWhvDQqiC1Uj45DGgtszauc8k=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYXPR11MB8690
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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