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

[Fenghua Yu <fenghuay@nvidia.com>]

Hi, James, Reinette,

On 2/20/25 08:01, Reinette Chatre wrote:
> Hi Catalin,
>
> 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.
> 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.
>
> 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" and considering the number of call sites I do
> not see how bloating the kernel is justified.

Agree with Reinette.


Plus, resctrl_find_domain() is only called during setup and CPU hot plug 
which are not run time paths and won't impact run time performance. 
inline doesn't help the performance but makes the kernel bigger.

I can see the function is moved from arch/x86/kernel/cpu/resctrl/core.c 
and there is no corresponding fs/resctrl/core.c.

If your motivation is to avoid fs/resctrl/core.c (which is much small) 
to have one less file and just host the function in .h, please consider 
to create fs/resctrl/core.c and put the function in it and declare it in 
the .h file. So there won't be inline issue any more.

>> (drive-by comment, I don't really understand this subsystem to make a
>> meaningful contribution)
>>
> Thanks for taking a look. The idea is not unique to resctrl.
>
> Reinette
>
Thanks.


-Fenghua