linux-kernel - 回复: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep

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: <YTBPR01MB3262D78448BD2FB3AD5EB4F9C46B9@YTBPR01MB3262.CANPRD01.PROD.OUTLOOK.COM>
Date:   Tue, 16 Mar 2021 16:08:08 +0000
From:   "Zhou Ti (x2019cwm)" <x2019cwm@...x.ca>
To:     "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>,
        Yunfeng Ye <yeyunfeng@...wei.com>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Marcelo Tosatti <mtosatti@...hat.com>,
        Ingo Molnar <mingo@...nel.org>,
        "rafael@...nel.org" <rafael@...nel.org>
Subject: 回复: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep_length() from returning negative value

But I don't think it's a good idea to handle this in callers, because logically the function shouldn't return negative values. Returning 0 directly would allow idle governors to get another chance to select again.

________________________________________
发件人: Zhou Ti (x2019cwm) <x2019cwm@...x.ca>
发送时间: 2021年3月16日 3:57
收件人: Rafael J. Wysocki; Frederic Weisbecker; Peter Zijlstra
抄送: Thomas Gleixner; LKML; Yunfeng Ye; Paul E . McKenney; Marcelo Tosatti; Ingo Molnar; rafael@...nel.org
主题: 回复: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep_length() from returning negative value

Yes, the return of a negative number results in a very large unsigned integer, which idle governors use as a baseline prediction for future interrupts and to correct their own parameters. This problem can lead to the selection of idle states that are too deep, which can be detrimental to both energy and performance.

________________________________________
发件人: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
发送时间: 2021年3月16日 3:26
收件人: Frederic Weisbecker; Peter Zijlstra
抄送: Thomas Gleixner; LKML; Zhou Ti (x2019cwm); Yunfeng Ye; Paul E . McKenney; Marcelo Tosatti; Ingo Molnar; rafael@...nel.org
主题: Re: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep_length() from returning negative value

On 3/16/2021 3:53 PM, Frederic Weisbecker wrote:
> On Tue, Mar 16, 2021 at 03:35:37PM +0100, Peter Zijlstra wrote:
>> On Tue, Mar 16, 2021 at 02:37:03PM +0100, Frederic Weisbecker wrote:
>>> On Tue, Mar 16, 2021 at 01:21:29PM +0100, Peter Zijlstra wrote:
>>>> On Thu, Mar 11, 2021 at 01:36:59PM +0100, Frederic Weisbecker wrote:
>>>>> From: "Zhou Ti (x2019cwm)" <x2019cwm@...x.ca>
>>>>>
>>>>> If the hardware clock happens to fire its interrupts late, two possible
>>>>> issues can happen while calling tick_nohz_get_sleep_length(). Either:
>>>>>
>>>>> 1) The next clockevent device event is due past the last idle entry time.
>>>>>
>>>>> or:
>>>>>
>>>>> 2) The last timekeeping update happened before the last idle entry time
>>>>>     and the next timer callback expires before the last idle entry time.
>>>>>
>>>>> Make sure that both cases are handled to avoid returning a negative
>>>>> duration to the cpuidle governors.
>>>> Why? ... and wouldn't it be cheaper the fix the caller to
>>>> check negative once, instead of adding two branches here?
>>> There are already two callers and potentially two return values to check
>>> for each because the function returns two values.
>>>
>>> I'd rather make the API more robust instead of fixing each callers and worrying
>>> about future ones.
>> But what's the actual problem? The Changelog doesn't say why returning a
>> negative value is a problem, and in fact the return value is explicitly
>> signed.
>>
>> Anyway, I don't terribly mind the patch, I was just confused by the lack
>> of actual justification.
> And you're right, the motivation is pure FUD: I don't know exactly
> how the cpuidle governors may react to such negative values and so this
> is just to prevent from potential accident.
>
> Rafael, does that look harmless to you?

No, this is a problem now.  Both governors using this assign the return
value of it to a u64 var and so negative values confuse them.

That said I think it's better to deal with the issue in the callers.

I can send a patch for that if needed.