linux-kernel - Re: [PATCH v1 1/6] x86/resctrl: Move __resctrl_sched

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: <d3ef1a5a-a7ee-477c-8697-d64b91726d91@intel.com>
Date: Sun, 7 Apr 2024 12:21:17 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Peter Newman <peternewman@...gle.com>
CC: Fenghua Yu <fenghua.yu@...el.com>, James Morse <james.morse@....com>,
	Stephane Eranian <eranian@...gle.com>, Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, Dave Hansen
	<dave.hansen@...ux.intel.com>, <x86@...nel.org>, "H. Peter Anvin"
	<hpa@...or.com>, Peter Zijlstra <peterz@...radead.org>, Juri Lelli
	<juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt
	<rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, Mel Gorman
	<mgorman@...e.de>, Daniel Bristot de Oliveira <bristot@...hat.com>, "Valentin
 Schneider" <vschneid@...hat.com>, Uros Bizjak <ubizjak@...il.com>, "Mike
 Rapoport" <rppt@...nel.org>, "Kirill A. Shutemov"
	<kirill.shutemov@...ux.intel.com>, Rick Edgecombe
	<rick.p.edgecombe@...el.com>, Xin Li <xin3.li@...el.com>, Babu Moger
	<babu.moger@....com>, Shaopeng Tan <tan.shaopeng@...itsu.com>, "Maciej
 Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Jens Axboe
	<axboe@...nel.dk>, Christian Brauner <brauner@...nel.org>, Oleg Nesterov
	<oleg@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>, Tycho Andersen
	<tandersen@...flix.com>, Nicholas Piggin <npiggin@...il.com>, Beau Belgrave
	<beaub@...ux.microsoft.com>, "Matthew Wilcox (Oracle)" <willy@...radead.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/6] x86/resctrl: Move __resctrl_sched_in() out-of-line

Hi Peter,

On 4/5/2024 3:04 PM, Peter Newman wrote:
> Hi Reinette,
> 
> On Thu, Apr 4, 2024 at 4:09 PM Reinette Chatre
> <reinette.chatre@...el.com> wrote:
>>
>> Hi Peter,
>>
>> On 3/25/2024 10:27 AM, Peter Newman wrote:
>>> __resctrl_sched_in() is unable to dereference a struct rdtgroup pointer
>>> when defined inline because rdtgroup is a private structure defined in
>>> internal.h.
>>
>> Being inline has nothing to do with whether it can reference a struct rdtgroup
>> pointer, no?
> 
> No, but it has a lot to do with whether it can de-reference a struct
> rdtgroup pointer in order to obtain a CLOSID or RMID, as doing so
> would pull the definitions for struct rdtgroup and struct mongroup
> into an external header. Before doing so, I would want to make sure
> implementing  __resctrl_sched_in() inline is actually adding value.

I expect that each architecture would need architecture specific task
switching code and by pointing to rdtgroup from the task_struct every
architecture would need to know how to reach the CLOSID/RMID within.

Having architectures reach into the private fs data is not ideal
so we should consider to make just a portion of rdtgroup information
required to support switching code accessible to the architectures, not the
entire rdtgroup and mongroup structures.

..

>>> +static inline void resctrl_sched_in(struct task_struct *tsk)
>>> +{
>>> +#ifdef CONFIG_X86_CPU_RESCTRL
>>> +     if (static_branch_likely(&rdt_enable_key))
>>> +             __resctrl_sched_in(tsk);
>>> +#endif
>>> +}
>>> +
>>
>> include/linux/resctrl.h should rather be divided to accommodate code
>> as below:
>>
>> #ifdef CONFIG_X86_CPU_RESCTRL
>>
>> static inline void resctrl_sched_in(struct task_struct *tsk)
>> {
>>         if (static_branch_likely(&rdt_enable_key))
>>                 __resctrl_sched_in(tsk);
>> }
>>
>> #else
>>
>> static inline void resctrl_sched_in(struct task_struct *tsk) {}
>>
>> #endif
>>
>> so that core code does not get anything unnecessary when CONFIG_X86_CPU_RESCTRL
>> is not set.
> 
> Will do.

I think this needs more thought. rdt_enable_key is x86 specific now and should not
be in the fs code. Every architecture will have its own task switch code, with
__resctrl_sched_in() belonging to x86 and thus not something to be directly called
from the fs code.

Reinette