linux-kernel - Re: [PATCH v11 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: <CANDhNCrkf1Uz42V3vMFChp1nKnkeHH7ZPxd_gC4KOMmWPcRVgQ@mail.gmail.com>
Date: Fri, 12 Jul 2024 12:10:03 -0700
From: John Stultz <jstultz@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Joel Fernandes <joelaf@...gle.com>, 
	Qais Yousef <qyousef@...alina.io>, 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>, 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>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, kernel-team@...roid.com, 
	"Connor O'Brien" <connoro@...gle.com>
Subject: Re: [PATCH v11 7/7] sched: Split scheduler and execution contexts

On Fri, Jul 12, 2024 at 8:02 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> On Tue, Jul 09, 2024 at 01:31:50PM -0700, 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.
>
> > * Swapped proxy for selected for clarity
>
> I'm not loving this naming...  what does selected even mean? What was
> wrong with proxy? -- (did we have this conversation before?)

So yeah, this came up earlier:
https://lore.kernel.org/lkml/CANDhNCr3acrEpBYd2LVkY3At=HCDZxGWqbMMwzVJ-Mn--dv3DA@mail.gmail.com/

My big concern is that the way proxy was used early in the series
seemed to be inverted from how the term is commonly used.

A proxy is one who takes an action on behalf of someone else.

In this case we have a blocked task that was picked to run, but then
we run another task in its place. Intuitively, this makes the proxy
the one that actually runs, not the one that was picked. But the
earliest versions of the patch had this flipped, and caused lots of
conceptual confusion in the discussions I had with folks about what
the patch was doing (as well as my own confusion initially working on
the patch).

So to avoid this, I've minimized the use of the term proxy in the
code, trying to use less confusing terms.

To me, selected is a more straightforward term, as it's the task the
scheduler chose to run (but not necessarily the one that actually
runs).
And curr is fine enough, as it is the currently running task.

But I'm not wedded to any particular name.  "selected", "chosen",
(Valentin suggested "picked" earlier, which I'd be ok with, but wanted
some sense of consensus before I go through to rework it all), but I
do think "proxy" for the scheduler context would make the code
unnecessarily difficult for others to understand.

> > +static inline int task_current_selected(struct rq *rq, struct task_struct *p)
> > +{
> > +     return rq_selected(rq) == p;
> > +}
>
>
> And I think I hated on the macros before, and you said you needed them
> to to allow !PROXY builds.
>
> But what about something like:
>
> #ifdef CONFIG_PROXY_EXEC
>         struct task_struct __rcu *proxy;
>         struct task_struct __rcu *curr;
> #else
>         union {
>                 struct task_struct __rcu *proxy;
>                 struct task_struct __rcu *curr;
>         };
> #endif
>
>
> And then we can use rq->proxy and rq->curr like the good old days?

Ok, yeah, we can use a union for that. I tend to think of unions as a
bit overly subtle for this sort of trick (the wrapper functions make
it more clear there's indirection going on), but that's a taste issue
and I'm not picky.

> I realize this is going to be a lot of typing to fix up, and perhaps
> there's a reason to not do this. But...

I have no problems reworking this if it helps get this series unstuck!

I might strongly push back against "proxy" as the name for the
scheduler context, but if it really is your favorite, I'll hold my
nose to move this along.

Do let me know your final preference and I'll go rework it all asap.

Thanks again so much for providing some feedback here! I really appreciate it!
-john