Message-ID: <fef12c9e-7d6f-bcd4-f92e-e776eb9e673b@intel.com>
Date:   Wed, 15 Mar 2023 11:45:32 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.8.0
Subject: Re: [PATCH v3 7/7] x86/resctrl: Add debug files when mounted with
 debug option
Content-Language: en-US
To:     Babu Moger <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: <167778850105.1053859.14596357862185564029.stgit@bmoger-ubuntu>
 <167778870823.1053859.9940104139782066465.stgit@bmoger-ubuntu>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <167778870823.1053859.9940104139782066465.stgit@bmoger-ubuntu>
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?STk5SGFxY2RHWW50UXVXZ242UkRrRm9DYis2MFAxaERYMXFwL0w0dG5sZE5U?=
 =?utf-8?B?eWVzMFhCVTV6dnVUc2NKcUVIUjhETE1vVlVvOWxSQi93ZkR2dnBCMW1PSUls?=
 =?utf-8?B?bVBiU3NQSmdLZlJPeHh6MTlIb0swU053ZHVaYmhkaGZNZE0wWWZQQUVEN1RP?=
 =?utf-8?B?eDlLYkpVempnQW9zRUZ6ME9CUTZ6Vm8yZVcxbm8xT2dXOGpRQmpRMXV0OWpE?=
 =?utf-8?B?azdQQWppOVhZazJFbTdJYkZkbjIvZ05xTU9XNlZlTnRzU2V6MG5ZTUlibGV0?=
 =?utf-8?B?aUtCaCtDWmZwaGI5MXZXa1NGYW5ZZmlxcUZ4U3NvZXJQVFAzNERJOWlxQi9q?=
 =?utf-8?B?RDZWcWZ1aHVMdEFnb0Z3blVFOC81WkVSZ1lwdWJORjJkMWdIazRPejlTemNk?=
 =?utf-8?B?TWIreStsWm5RRGY4bWhQdkpCMDcwcm9LZXEyRnQrY21PMnl1dW5oV1V6R3NI?=
 =?utf-8?B?K0lJbEdnOC9JdW1tZG5Ga3NOWFR5ejJoa3pGaG1wb3dUU01ZQndoOHFYWFR3?=
 =?utf-8?B?WmJSelFRTmtQV0ExMzlsUXo4TnZkMTQ4cDFjUVNRbnlNYnl4Nm1hdjhPY2FS?=
 =?utf-8?B?TFZUdjl1eEFDSERJcnNEa1MwUTc3MHJyQjdCSUYxWnJnVHAvalVQbDJRYUd1?=
 =?utf-8?B?elRQUUlaemVJUlFhNkx1S0FUbkxxbHNCVm1JSTVhQytER2JIOG1vbDJ3dmI4?=
 =?utf-8?B?MWd5aFZFWC9KZWRNTkRScVBzTXFIbzJueDIrdWwwWWp6VVdzSnBuSStxbnFQ?=
 =?utf-8?B?R0w4VXJGVmhnZE9JaFg0TFVnUXBrV1E2U1dTMEh2UHo4bldsRThDZnlMQjBs?=
 =?utf-8?B?enV4bThUczcyK0dtdFp2bXE4RVpkWFFWV3pIU09NZ2VnWUljZUF1b1RKTXMv?=
 =?utf-8?B?Q1Vvb1AxbURPOEtLNkZtcW1uN050b05qbkx0RzZEVTl4WUM5ZjJMY1pjayt1?=
 =?utf-8?B?cjh1VUNMbm5lbDNwQkxrU2syR1gyRDE3Q1h4N1d1NVBVajQxWGRmV25sbk4w?=
 =?utf-8?B?b21TUDlzWU5FMXJ2cTJja2NTYnhJem5ScW15QVVadTA0UUk1dmw5UGUvR3RL?=
 =?utf-8?B?TFBmemE3cGx2bE02eXJHSVZsRElOVkZpczhGRlVab2JTS0pEdU1wektsZVpQ?=
 =?utf-8?B?aGR4c1Noa3M1S2pZb1doN2VPT0Y2MnB5QnlUMkYwanNKSUdEZkgyRm84T0NS?=
 =?utf-8?B?a0RLVHVJa3JmL2s3NTE5U3krQWI2b0xjMGhOL3EwQ1Z4Z0JJY3ZtdGVaNFg2?=
 =?utf-8?B?TURYcXRSaFlWY2h4TW1RbWkybVBtZ1g1U1ltT2VlZzJ4TVRTL05BVXdGM0F0?=
 =?utf-8?B?ektlQkpjaEZqL3FwS1dvUlJodTJ2UnAzbjBVYWU3Tm1yT1hvMnJaUlFOWUFx?=
 =?utf-8?B?dVBtclVhZTZLbW8yaFZ0YlIrTDJaV2NXZ3d4WExaM1pIeDlZRHNObXFZcFNz?=
 =?utf-8?B?U0svT0J1cDFZMForQ0FkMm9MSkhtVmtYcDFUeE1Hd1FDKzAyU1V0Y1BzWGc2?=
 =?utf-8?B?SUJZQ09oeEF5Q0ZXU2ExNWg4V1VIYWxZT0pwYXRwekJpMXFYMG40WVpaVG9H?=
 =?utf-8?B?SC9YUXZUTm1GNlhodCtQWDBqYnNrdTVpQ0orQzZZRytsekdTQ0ZJaFBUMmJ6?=
 =?utf-8?B?UWNEZUx0SDZkM2ZvS3JqdFVDd0dFZ2hFQk5MVGpaU0FtOFhPQnRRUS9rLzZS?=
 =?utf-8?B?dnM1VUF2MTNxTWQ3VEIwQnp3eW84dlFpdlhGUWpvU0ZXdWc1MWIxUkorK2wv?=
 =?utf-8?B?dDBuSDhnREJRSXgxMXUyTzl4QWI0ZHp6Z1MwQ0o0eU9ZK0pzSGhRQWpLNVlL?=
 =?utf-8?B?SVdqV0JKalh0T1dVUUYyY2pNWThkUDRqT2JpRkkrWVpBUlNXSnQxUXh5anNT?=
 =?utf-8?B?WmdFWC9xdTFrZ2Irak50Y1FaTkVZWHZXWFRNbWdBaFpmWEVNalliMkJPREE5?=
 =?utf-8?B?VTdPaEE2Zzc2ck9WN0dOdUduNEwxekF0dzlWVFZFY2JBQ1Bmc3NOS2RDWENl?=
 =?utf-8?B?eU5HOXp0UUNIS1lhcUpkbzJNUFBkYlB4dGFDNnBFeDlPNFA1d3o3b3lIUHZF?=
 =?utf-8?B?Zm9pdXNUOUk3eDV5b1puelZVTVNiYzd6RmdrMWo5WkxuVU5XREpMTFQ4Unpl?=
 =?utf-8?B?YVpiZmRJVUVyclp3K1pYY0hEOFJrNU1CeGpHWHZ4dnFBNHBuVFBLZkVyajNO?=
 =?utf-8?B?U0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 424be8f6-bb13-47c6-e1a5-08db25857748
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Mar 2023 18:45:36.8793
 (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: TMGPD3rIsxlMv+GU77QLrY1dAuB2KYn1eG/MQMfvV5j+oWfPdn2A8tWI9v0/5cLIz4q2xID5Rh5/HaYhhjy0yDcz7JU2Rml97MNbmfZFGos=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR11MB6250
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Babu,

On 3/2/2023 12:25 PM, Babu Moger wrote:
> Add the debug files to the resctrl hierarchy.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   48 +++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index a1f13715a65c..790c6b9f9031 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2400,6 +2400,45 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>  			     struct rdtgroup *prgrp,
>  			     struct kernfs_node **mon_data_kn);
>  
> +void resctrl_add_debug_file(struct kernfs_node *parent_kn, const char *config,
> +			    unsigned long fflags)
> +{
> +	struct rftype *rft;
> +
> +	rft = rdtgroup_get_rftype_by_name(config);
> +	if (rft) {
> +		rft->fflags |= fflags;
> +		rdtgroup_add_file(parent_kn, rft);
> +	}
> +}
> +
> +static void resctrl_add_debug_files(void)
> +{
> +	resctrl_add_debug_file(rdtgroup_default.kn, "rmid", RFTYPE_BASE);
> +	resctrl_add_debug_file(rdtgroup_default.kn, "closid", RFTYPE_BASE_CTRL);
> +	kernfs_activate(rdtgroup_default.kn);
> +}
> +

I think that separating this from all the other resctrl file creation
can be a source of confusion and bugs. Why not add the debug files
at the same time as all the other files belonging to a resource group?

How about introducing new flags, perhaps RFTYPE_MON_DEBUG and
RFTYPE_CTRL_DEBUG. When the debug option is enabled (when resctrl_debug
is true) then the appropriate flag can be OR'd with the other flags
before rdtgroup_add_files() is called.

It sounds to me if there are plans to add more of these files. A function
like resctrl_add_debug_files() requires a lot of changes and care (and thus
potential for errors) every time a new debug file is added.

On another note ... what are the plans with this debug area? At some
point it may be better to expand resctrl debugfs.

Reinette