linux-kernel - Re: [PATCH v6 5/8] fs/resctrl: Add user interface to enable/disable io

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <93df95f2-b37d-436e-9872-b64f14256914@intel.com>
Date: Fri, 20 Jun 2025 08:53:21 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: <babu.moger@....com>, <corbet@....net>, <tony.luck@...el.com>,
	<Dave.Martin@....com>, <james.morse@....com>, <tglx@...utronix.de>,
	<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>
CC: <x86@...nel.org>, <hpa@...or.com>, <akpm@...ux-foundation.org>,
	<paulmck@...nel.org>, <rostedt@...dmis.org>, <thuth@...hat.com>,
	<ardb@...nel.org>, <gregkh@...uxfoundation.org>, <seanjc@...gle.com>,
	<thomas.lendacky@....com>, <pawan.kumar.gupta@...ux.intel.com>,
	<perry.yuan@....com>, <yosry.ahmed@...ux.dev>, <kai.huang@...el.com>,
	<xiaoyao.li@...el.com>, <peterz@...radead.org>, <kan.liang@...ux.intel.com>,
	<mario.limonciello@....com>, <xin3.li@...el.com>, <sohil.mehta@...el.com>,
	<chang.seok.bae@...el.com>, <andrew.cooper3@...rix.com>,
	<ebiggers@...gle.com>, <ak@...ux.intel.com>, <xin@...or.com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 5/8] fs/resctrl: Add user interface to enable/disable
 io_alloc feature

Hi Babu,

On 6/19/25 11:41 AM, Moger, Babu wrote:
> On 6/17/25 22:59, Reinette Chatre wrote:
>> On 6/11/25 2:23 PM, Babu Moger wrote:

...

>>> + */
>>> +static int resctrl_io_alloc_closid_get(struct rdt_resource *r)
>>> +{
>>> +	int num_closids = closids_supported();
>>> +
>>> +	if (resctrl_arch_get_cdp_enabled(r->rid))
>>> +		num_closids *= 2;
>>> +
>>> +	if (num_closids != resctrl_arch_get_num_closid(r))
>>> +		return -ENOSPC;
>>> +
>>> +	return closids_supported() - 1;
>>> +}
>>
>> resctrl_io_alloc_closid_get() seems to be trying to do two things: 
>> - determine what the io_alloc_closid is
>> - make sure the io_alloc_closid is supported
>>
>> I think this should be split into two functions. Once the
>> io_alloc_closid is determined to be supported and io_alloc
>> enabled then there is no reason to keep checking if it is
>> supported whenever the io_alloc_closid is queried.
>>
>> How about simplifying this to:
>>
>> /*
>>  * note how this returns u32 that will eliminate
>>  * unnecessary error checking in usages where io_alloc_closid
>>  * needs to be determined after an resctrl_arch_get_io_alloc_enabled(r)
>>  * already confirmed io_alloc is enabled
>>  * function comment could note that this returns the CLOSID
>>  * required by io_alloc but not whether the CLOSID can
>>  * be supported, for this resctrl_io_alloc_closid_supported() should
>>  * be used.
>>  * Can also note that returned value will always be valid if
>>  * resctrl_arch_get_io_alloc_enabled(r) is true.
>>  */
>> u32 resctrl_io_alloc_closid(struct rdt_resource *r) {
>> 	if (resctrl_arch_get_cdp_enabled(r->rid))
>> 		return resctrl_arch_get_num_closid(r)/2  - 1
>> 	else
>> 		return resctrl_arch_get_num_closid(r) -1
>> }
>>
>> /*
>>  * note how below already makes resctrl's io_alloc implementation
>>  * more generic
>>  */
>> resctrl_io_alloc_closid_supported(u32 io_alloc_closid) {
>> 	return io_alloc_closid <  closids_supported()
>> }
>>
> 
> Sure.
>    Changed the check to
> 
>     return io_alloc_closid == (closids_supported() -1)
> 

resctrl_io_alloc_closid_supported() is not intended to reflect what the
value is but just check if provided value is supported. By changing the
check to above resctrl_io_alloc_closid_supported() does two things again
(what the move to new functions aimed to avoid): checking that the CLOSID
is supported while requiring that it is the highest supported CLOSID.
What issue(s) do you see with using "io_alloc_closid <  closids_supported()"
as the check?

Reinette