linux-kernel - Re: [RESEND][PATCH v21 2/6] sched/locking: Add blocked_on_state to provide necessary tri-state for proxy return-migration

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: <d70e7e50-d5c3-4689-b91f-9dce0f1a0424@amd.com>
Date: Fri, 19 Sep 2025 08:57:47 +0530
From: K Prateek Nayak <kprateek.nayak@....com>
To: John Stultz <jstultz@...gle.com>
CC: LKML <linux-kernel@...r.kernel.org>, Joel Fernandes
	<joelagnelf@...dia.com>, Qais Yousef <qyousef@...alina.io>, Ingo Molnar
	<mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>, 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 v21 2/6] sched/locking: Add blocked_on_state to
 provide necessary tri-state for proxy return-migration

Hello John,

On 9/19/2025 4:27 AM, John Stultz wrote:
>> Of the three {set,clear}_task_blcoked_on() usage:
>>
>>     $ grep -r "\(set\|clear\)_task_blocked_on" include/
>>     kernel/locking/mutex.c: __set_task_blocked_on(current, lock);
>>     kernel/locking/mutex.c: __clear_task_blocked_on(current, lock);
>>     kernel/locking/mutex.c: clear_task_blocked_on(current, lock);
>>
>> two can be converted directly and perhaps clear_task_blocked_on() can be
>> renamed as clear_task_blocked_on_set_runnable()?
>>
>> This is just me rambling on so feel free to ignore. I probably have to
>> train my mind enough to see __clear_task_blocked_on() not only clears
>> "blocked_on" but also sets task to runnable :)
> 
> Yeah, the case where we don't already hold the lock and want to do
> both gets more complex in that case.
> 
> Hrm. Maybe just the way the functions are organized in the header make
> it seem like we're managing two separate bits of state, where really
> they are ordered.
> I'll try to re-arrange that introducing the
> set_task_blocked_on/clear_task_blocked_on first, then the transition
> set_blocked_on_<state>() helpers after?
> Maybe that and  some comments will make that clearer?

Again that was me rambling. Even a small comment above
clear_task_blocked_on() would also be sufficient if the whole rework
turns out to be more extensive. 

> 
>>> @@ -6749,6 +6776,15 @@ find_proxy_task(struct rq *rq, struct task_struct *donor, struct rq_flags *rf)
>>>
>>>       WARN_ON_ONCE(owner && !owner->on_rq);
>>>       return owner;
>>> +
>>> +     /*
>>> +      * NOTE: This logic is down here, because we need to call
>>> +      * the functions with the mutex wait_lock and task
>>> +      * blocked_lock released, so we have to get out of the
>>> +      * guard() scope.
>>> +      */
>>
>> I didn't know that was possible! Neat. Since cleanup.h has a note
>> reading:
>>
>>     ... the expectation is that usage of "goto" and cleanup helpers is
>>     never mixed in the same function.
>>
>> are there any concerns w.r.t. compiler versions etc. or am I just being
>> paranoid?
> 
> Hrrrrmmmm.  I hadn't seen that detail. :/   I guess I was just lucky
> it worked with my toolchain.

I have been too. Maybe it is okay to use a goto if folks know what
they are doing ¯\_(ツ)_/¯

Another idea is to have:

    bool deactivate_donor = false;

    for (p = donor; task_is_blocked(p); p = owner) {
        guard(raw_spinlock)(...);
        ...
        if (<condition> {
            deactivate_donor = true;
            break;
        }
        ...
    }
    if (deactivate_donor)
        return proxy_deactivate(rq, donor);

Can that work?

> 
> Oof. That may require reworking all this logic back to explicit
> lock/unlock calls, away from the guard() usage.
> 
> Let me think on if there's a better way.
> 
> Thanks so much again for pointing this out!
> -john

-- 
Thanks and Regards,
Prateek