linux-kernel - Re: [PATCH v10 7/7] sched: Split scheduler and execution contexts

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: <20240604141103.idwodwyeecml4keh@airbuntu>
Date: Tue, 4 Jun 2024 15:11:03 +0100
From: Qais Yousef <qyousef@...alina.io>
To: John Stultz <jstultz@...gle.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Joel Fernandes <joelaf@...gle.com>, Ingo Molnar <mingo@...hat.com>,
	Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Valentin Schneider <vschneid@...hat.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ben Segall <bsegall@...gle.com>,
	Zimuzo Ezeozue <zezeozue@...gle.com>,
	Youssef Esmat <youssefesmat@...gle.com>,
	Mel Gorman <mgorman@...e.de>,
	Daniel Bristot de Oliveira <bristot@...hat.com>,
	Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>,
	Boqun Feng <boqun.feng@...il.com>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Xuewen Yan <xuewen.yan94@...il.com>,
	K Prateek Nayak <kprateek.nayak@....com>,
	Metin Kaya <Metin.Kaya@....com>,
	Thomas Gleixner <tglx@...utronix.de>, kernel-team@...roid.com,
	Connor O'Brien <connoro@...gle.com>
Subject: Re: [PATCH v10 7/7] sched: Split scheduler and execution contexts

On 05/06/24 21:54, John Stultz wrote:
> From: Peter Zijlstra <peterz@...radead.org>
> 
> Let's define the scheduling context as all the scheduler state
> in task_struct for the task selected to run, and the execution
> context as all state required to actually run the task.
> 
> Currently both are intertwined in task_struct. We want to
> logically split these such that we can use the scheduling
> context of the task selected to be scheduled, but use the
> execution context of a different task to actually be run.
> 
> To this purpose, introduce rq_selected() macro to point to the
> task_struct selected from the runqueue by the scheduler, and
> will be used for scheduler state, and preserve rq->curr to
> indicate the execution context of the task that will actually be
> run.

This is subjective opinion of course. But I am not a fan of rq_selected().
I think we are better with more explicit

	rq->currs	/* current sched context */
	rq->currx	/* current execution context */

and the helpers

	task_curr_sctx()
	task_curr_xctx()

This will ensure all current users of rq->curr will generate a build error and
be better audited what they are supposed to be. And I think the code is more
readable this way.

Another way to do it is to split task_struct to sched and exec context (rather
than control the reference to curr as done here). Then curr would be always the
same, but reference to its sched context would change with inheritance.

ie:

	struct task_sctx {
		...
	};

	struct task_struct {
		struct task_sctx *sctx;
		/* exec context is embedded as-is */
		...
	};

Which means would just have to update all accessors to sched context to do
extra dereference. Which I am not sure if a problematic extra level of
indirection.

I see the latter approach  more intuitive and explicit about what is a sched
context that is supposed to be inherited and what is not.

I generally think breaking down task structure would be good. Hopefully the new
data perf can help us make better decision on what attributes to group
together. And generally this might be a good exercise to rethink its content
which grown organicaly over the year. Maybe we want to create similar such
smaller wrapper structs for other fields too.

I **think** with this approach we would just need go fix compilation errors to
do p->sctx->{attribute}.

Proxy exec later would only update the reference to this struct to enforce
inheritance for rq->curr->sctx to point to the donor's context.