linux-kernel - Re: [RESEND][PATCH v18 6/8] sched: Add an initial sketch of the find_proxy

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: <CANDhNCoMvY0s0AyePNouWqxdRpXCYJE=28E_b8RdmJ_2px_OBA@mail.gmail.com>
Date: Thu, 10 Jul 2025 17:43:36 -0700
From: John Stultz <jstultz@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: LKML <linux-kernel@...r.kernel.org>, K Prateek Nayak <kprateek.nayak@....com>, 
	Joel Fernandes <joelagnelf@...dia.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>, 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>, Metin Kaya <Metin.Kaya@....com>, 
	Xuewen Yan <xuewen.yan94@...il.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Suleiman Souhlal <suleiman@...gle.com>, 
	kuyo chang <kuyo.chang@...iatek.com>, hupu <hupu.gm@...il.com>, kernel-team@...roid.com
Subject: Re: [RESEND][PATCH v18 6/8] sched: Add an initial sketch of the
 find_proxy_task() function

On Thu, Jul 10, 2025 at 3:02 AM Peter Zijlstra <peterz@...radead.org> wrote:
> On Mon, Jul 07, 2025 at 08:43:53PM +0000, John Stultz wrote:
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 853157b27f384..dc82d9b8bee2c 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -6614,7 +6614,8 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
> >   * Otherwise marks the task's __state as RUNNING
> >   */
> >  static bool try_to_block_task(struct rq *rq, struct task_struct *p,
> > -                           unsigned long *task_state_p)
> > +                           unsigned long *task_state_p,
> > +                           bool deactivate_cond)
> >  {
> >       unsigned long task_state = *task_state_p;
> >       int flags = DEQUEUE_NOCLOCK;
> > @@ -6625,6 +6626,9 @@ static bool try_to_block_task(struct rq *rq, struct task_struct *p,
> >               return false;
> >       }
> >
> > +     if (!deactivate_cond)
> > +             return false;
> > +
> >       p->sched_contributes_to_load =
> >               (task_state & TASK_UNINTERRUPTIBLE) &&
> >               !(task_state & TASK_NOLOAD) &&
>
> I'm struggling with this; @deactivate_cond doesn't seem to adequately
> cover what it actually does.
>

So initially, there was just this extra logic to exit early if the
prev task was blocked_on so we don't deactivate it (keeping it on the
rq). However, in cases (later in the series) where we later decide to
bail on proxying and just deactivate the task even if we are
blocked_on, I wanted to re-use this for that fallback case, so passing
the !blocked_on state as an argument (which could be forced to true
when we bail) made sense to me. Thus a "deactivation condition".

> So far what it seems to do is when true, don't block. It still does the
> signal thing -- but I can't tell if that is actually required or not.

So, preserving the signal check seemed important, as if we do get a
signal we probably want to make sure we don't deactivate and that the
task so it can be selected to run.
I'm curious as to your thinking that makes you skeptical that it is required?

That said, I appreciate you raising the question, as looking at and
thinking about it some more, I do see that we should be clearing the
blocked_on state when we do set ourselves as runnable in that case,
otherwise if we are blocked_on we might get stuck waiting for a wakeup
(likely from the lock release).

I'll fix that up here.

> Would 'should_block' be a better name? And maybe stick a little
> something in the comment above try_to_block_task() or near the:
>
>         if (!should_block)
>                 return false;
>
> lines about why the signal bits are important to have done.

Sure, I'm happy to use that if should_block is more clear. I'll try to
add some better comments as well.

>
> > @@ -6648,6 +6652,89 @@ static bool try_to_block_task(struct rq *rq, struct task_struct *p,
> >       return true;
> >  }
> >
> > +#ifdef CONFIG_SCHED_PROXY_EXEC
> > +static inline struct task_struct *proxy_resched_idle(struct rq *rq)
> > +{
> > +     put_prev_set_next_task(rq, rq->donor, rq->idle);
> > +     rq_set_donor(rq, rq->idle);
> > +     set_tsk_need_resched(rq->idle);
> > +     return rq->idle;
> > +}
>
> Nothing cares about the return value.

Well, nothing in this patch, but the last patch in this series I sent
here uses it.  But if you think it makes the series easier to grok and
I can drop it here and add the return in that later patch.

Thanks so much again for the review and feedback! I really appreciate it!
-john