linux-kernel - Re: [PATCH v5 5/8] x86/resctrl: Introduce "-o debug" mount option

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cc6ef418-3f7a-1bf7-b286-e3c534781740@amd.com>
Date:   Wed, 12 Jul 2023 11:40:19 -0500
From:   "Moger, Babu" <babu.moger@....com>
To:     Reinette Chatre <reinette.chatre@...el.com>, corbet@....net,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de
Cc:     fenghua.yu@...el.com, dave.hansen@...ux.intel.com, x86@...nel.org,
        hpa@...or.com, paulmck@...nel.org, akpm@...ux-foundation.org,
        quic_neeraju@...cinc.com, rdunlap@...radead.org,
        damien.lemoal@...nsource.wdc.com, songmuchun@...edance.com,
        peterz@...radead.org, jpoimboe@...nel.org, pbonzini@...hat.com,
        chang.seok.bae@...el.com, pawan.kumar.gupta@...ux.intel.com,
        jmattson@...gle.com, daniel.sneddon@...ux.intel.com,
        sandipan.das@....com, tony.luck@...el.com, james.morse@....com,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        bagasdotme@...il.com, eranian@...gle.com,
        christophe.leroy@...roup.eu, jarkko@...nel.org,
        adrian.hunter@...el.com, quic_jiles@...cinc.com,
        peternewman@...gle.com
Subject: Re: [PATCH v5 5/8] x86/resctrl: Introduce "-o debug" mount option

Hi Reinette,

On 7/7/23 16:42, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/1/2023 12:01 PM, Babu Moger wrote:
>> Add "-o debug" option to mount resctrl filesystem in debug mode.
>> This option can be used for adding extra files to help debug
>> resctrl issues.
> 
> Please avoid uncertainty in the changelog (re. "can be used"). I

ok Sure.

> think it will help to be more specific if the first and last
> hunks of patch 8 is merged into this patch, making it clear
> that the debug mount option is in support of debug files with
> this changelog written to support that.

Sure. Will do that.

> 
>> For example, hardware uses CLOSID and RMIDs to control and monitor
>> resctrl resources. These numbers are not visible to the user. These
>> details can help to debug issues. Debug option provides a method to
>> add these files to resctrl.
> 
> With the debug file support added here this extra motivation should
> not be necessary (remaining hunks of patch 8 can be moved to where
> these files are introduced).

Sure.

>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>>  Documentation/arch/x86/resctrl.rst     |    3 +++
>>  arch/x86/kernel/cpu/resctrl/internal.h |    1 +
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   15 +++++++++++++++
>>  3 files changed, 19 insertions(+)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 290f01aa3002..afdee4d1f691 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -46,6 +46,9 @@ mount options are:
>>  "mba_MBps":
>>  	Enable the MBA Software Controller(mba_sc) to specify MBA
>>  	bandwidth in MBps
>> +"debug":
>> +	Make debug files accessible. Available debug files are annotated with
>> +	"Available only with debug option".
>>  
> 
> At the risk of becoming unreadable, the earlier documentation of the mount
> command should also change.

Ok. Sure

> 
>>  L2 and L3 CDP are controlled separately.
>>  
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index c20cd6acb7a3..c07c6517d856 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -59,6 +59,7 @@ struct rdt_fs_context {
>>  	bool				enable_cdpl2;
>>  	bool				enable_cdpl3;
>>  	bool				enable_mba_mbps;
>> +	bool				enable_debug;
>>  };
>>  
>>  static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index baed20b2d788..be91dea5f927 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -56,6 +56,8 @@ static char last_cmd_status_buf[512];
>>  
>>  struct dentry *debugfs_resctrl;
>>  
>> +static bool resctrl_debug;
>> +
>>  void rdt_last_cmd_clear(void)
>>  {
>>  	lockdep_assert_held(&rdtgroup_mutex);
>> @@ -2368,6 +2370,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>>  	if (!ret && ctx->enable_mba_mbps)
>>  		ret = set_mba_sc(true);
>>  
>> +	if (!ret && ctx->enable_debug)
>> +		resctrl_debug = true;
>> +
>>  	return ret;
>>  }
> 
> Looks like unwinding of rdt_enable_ctx() errors is done in the caller, this is
> not ideal and additions like above cause some error unwinding to be omitted.
> I cannot see why rdt_enable_ctx() cannot do its own error unwinding. This
> may be better as a separate patch.
> 

Sure. Will do error unwinding of rdt_enable_ctx as a separate patch before
this patch. That seems more appropriate.
-- 
Thanks
Babu Moger