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: <69950bd1-d0f8-42f1-9aeb-07afa2cfb155@amd.com>
Date: Fri, 20 Jun 2025 16:57:16 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, 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 Reinette,

On 6/20/2025 10:53 AM, Reinette Chatre wrote:
> 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?

I don't see any issue. It should be fine. Will test and verify it.

thanks
Babu