linux-kernel - Re: [PATCH v2] freezer,sched: Use saved_state to reduce some spurious wakeups

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]

Date:   Fri, 8 Sep 2023 13:08:07 -0700
From:   Elliot Berman <quic_eberman@...cinc.com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     Ingo Molnar <mingo@...hat.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Pavel Machek <pavel@....cz>,
        Thomas Gleixner <tglx@...utronix.de>, <kernel@...cinc.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-pm@...r.kernel.org>,
        "Prakash Viswalingam" <quic_prakashv@...cinc.com>
Subject: Re: [PATCH v2] freezer,sched: Use saved_state to reduce some spurious
 wakeups



On 9/7/2023 2:46 AM, Peter Zijlstra wrote:
> On Mon, Sep 04, 2023 at 08:59:03PM -0700, Elliot Berman wrote:
>>
>>
>> On 9/4/2023 2:23 PM, Peter Zijlstra wrote:
>>> On Wed, Aug 30, 2023 at 10:42:39AM -0700, Elliot Berman wrote:
>>>
>>>> Avoid the spurious wakeups by saving the state of TASK_FREEZABLE tasks.
>>>> If the task was running before entering TASK_FROZEN state
>>>> (__refrigerator()) or if the task received a wake up for the saved
>>>> state, then the task is woken on thaw. saved_state from PREEMPT_RT locks
>>>> can be re-used because freezer would not stomp on the rtlock wait flow:
>>>> TASK_RTLOCK_WAIT isn't considered freezable.
>>>
>>> You don't actually assert that anywhere I think, so the moment someone
>>> makes that happen you crash and burn.
>>>
>>
>> I can certainly add an assertion on the freezer side.
> 
> I think the assertion we have in ttwu_state_match() might be sufficient.
> 

That assertion checks that you only try to wake up with only
TASK_RTLOCK_WAIT and no other bits. I think it is probably good to also
have assertions that check that TASK_RTLOCK_WAIT and TASK_FROZEN are
exclusive bits and. I can add these assertions (a separate patch?), but
I think those checks would impact the hot path to do the extra tests.

>>> Also:
>>>
>>>> -#ifdef CONFIG_PREEMPT_RT
>>>> +#if IS_ENABLED(CONFIG_PREEMPT_RT) || IS_ENABLED(CONFIG_FREEZER)
>>>
>>> That makes wakeup more horrible for everyone :/
>>
>> I don't think the hot wakeup path is significantly impacted because the
>> added checks come after the hot path is already not taken.
> 
> Perhaps we should start off by doing the below, instead of making it
> more complicated instead. I suppose you're right about the overhead, but
> run a hackbench just to make sure or something.
> 

I ran perf bench sched message -g 40 -l 40 with the v3 patch [1]. After 60
iterations each, I don't see a significant difference on my arm64 platform:
both samples ~normal and ~eq variance w/t-test p-value: 0.79.

We also ran typical high level benchmarks for our SoCs (antutu,
geekbench, et. al) and didn't see any regressions there.

[1]: https://lore.kernel.org/all/20230908-avoid-spurious-freezer-wakeups-v3-1-d49821fda04d@quicinc.com/

Thanks,
Elliot