linux-kernel - Re: [PATCH v6 12/42] x86/resctrl: Move rdt_find_domain() to be visible to arch and fs code

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: <0206076d-9ffa-4a7f-871e-94c4a8d517b5@arm.com>
Date: Fri, 28 Feb 2025 19:56:10 +0000
From: James Morse <james.morse@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 Catalin Marinas <catalin.marinas@....com>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, H Peter Anvin <hpa@...or.com>,
 Babu Moger <Babu.Moger@....com>, shameerali.kolothum.thodi@...wei.com,
 D Scott Phillips OS <scott@...amperecomputing.com>,
 carl@...amperecomputing.com, lcherian@...vell.com,
 bobo.shaobowang@...wei.com, tan.shaopeng@...itsu.com,
 baolin.wang@...ux.alibaba.com, Jamie Iles <quic_jiles@...cinc.com>,
 Xin Hao <xhao@...ux.alibaba.com>, peternewman@...gle.com,
 dfustini@...libre.com, amitsinght@...vell.com,
 David Hildenbrand <david@...hat.com>, Rex Nie <rex.nie@...uarmicro.com>,
 Dave Martin <dave.martin@....com>, Koba Ko <kobak@...dia.com>,
 Shanker Donthineni <sdonthineni@...dia.com>
Subject: Re: [PATCH v6 12/42] x86/resctrl: Move rdt_find_domain() to be
 visible to arch and fs code

Hi Reinette,

On 20/02/2025 16:01, Reinette Chatre wrote:
> On 2/20/25 2:58 AM, Catalin Marinas wrote:
>> On Wed, Feb 19, 2025 at 03:24:06PM -0800, Reinette Chatre wrote:
>>> On 2/7/25 10:17 AM, James Morse wrote:
>>>> rdt_find_domain() finds a domain given a resource and a cache-id.
>>>> This is used by both the architecture code and the filesystem code.
>>>>
>>>> After the filesystem code moves to live in /fs/, this helper will no
>>>> longer be visible.
>>>>
>>>> Move it to the global header file. As its now globally visible, and
>>>> has only a handful of callers, swap the 'rdt' for 'resctrl'.

>>>> --- a/include/linux/resctrl.h
>>>> +++ b/include/linux/resctrl.h
>>>> @@ -372,6 +372,40 @@ static inline void resctrl_arch_rmid_read_context_check(void)
>>>>  		might_sleep();
>>>>  }
>>>>  
>>>> +/**
>>>> + * resctrl_find_domain() - Search for a domain id in a resource domain list.
>>>> + * @h:		The domain list to search.
>>>> + * @id:		The domain id to search for.
>>>> + * @pos:	A pointer to position in the list id should be inserted.
>>>> + *
>>>> + * Search the domain list to find the domain id. If the domain id is
>>>> + * found, return the domain. NULL otherwise.  If the domain id is not
>>>> + * found (and NULL returned) then the first domain with id bigger than
>>>> + * the input id can be returned to the caller via @pos.
>>>> + */
>>>> +static inline struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h,
>>>> +							 int id,
>>>> +							 struct list_head **pos)
>>>
>>> Could you please provide a motivation for why this needs to be inline now?
>>
>> It's in a header now, to avoid the compiler complaining about unused
>> static functions wherever this file is included. The alternative is a
>> prototype declaration and the actual implementation in a .c file.

(it was this)


> resctrl_find_domain() is currently in a .c file (arch/x86/kernel/cpu/resctrl/core.c)
> with a prototype declaration (in arch/x86/kernel/cpu/resctrl/internal.h). This patch
> switches resctrl_find_domain() to be inline without a motivation.

Its not clear what side should own this function, both the architecture and filesystem
code need to make use of it. The majority of callers are in the arch code - but putting it
here means its duplicated between architectures.
Putting it in the filesystem code means calling out to it, and means the compiler can't
remove that extra NULL argument thing if its unused. Its also a hindrance to having the
arch code standalone - so we can in the future consider making the filesystem parts a
module. (Tony has picked at this and pointed out that kernfs not being exported is a
bigger problem). Its not needed now (and I haven't tried), but it seems a reasonable
direction of travel.


> After a fresh reading of "The inline disease" in Documentation/process/coding-style.rst 
> I do see a few red flags related to making this function inline. The function is certainly
> larger than the rule of thumb of "3 lines"

The thing about rules of thumb ...
The first example I looked at is __task_state_index(), which is equally longer than this
rule of thumb.


> and considering the number of call sites I do not see how bloating the kernel is justified.

I think this is several orders of magnitude below the point that this is something to care
about. If this were inlined in put_user() or something lots of drivers call, I'd agree.

I'll move it to live in the filesystem code - that saves 36 bytes.
(I'm honestly surprised its measurable!)


James