linux-kernel - Re: 回复: [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: <YTBPR01MB326253A7B094A2F603488EA8C4629@YTBPR01MB3262.CANPRD01.PROD.OUTLOOK.COM>
Date:   Thu, 25 Mar 2021 19:18:08 +0000
From:   "Zhou Ti (x2019cwm)" <x2019cwm@...x.ca>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Frederic Weisbecker <frederic@...nel.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        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>,
        Linux PM <linux-pm@...r.kernel.org>
Subject: Re: 回复: [PATCH 01/10] tick/nohz: Prevent tick_nohz_get_sleep_length() from returning negative value

On March 25, 2021 14:56, Rafael J. Wysocki wrote:
> On Thursday, March 25, 2021 2:14:00 PM CET Frederic Weisbecker wrote:
> > On Tue, Mar 16, 2021 at 04:08:08PM +0000, Zhou Ti (x2019cwm) wrote:
> > > 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.
> >
> > Hmm, I'm going to leave the last word to Rafael since cpuidle are the only
> > callers of this. In any case we need to fix it.
> 
> Yes, we do.
> 
> So I said that I preferred to address this in the callers and the reason why
> is because, for example, for the teo governor it would be a matter of using
> a different data type to store the tick_nohz_get_sleep_length() return value,
> like in the (untested) patch below.
> 
> So at least in this case there is no need to add any new branches anywhere.
> 
> I'm still not sure about menu, because it is more complicated, but even if
> that one needs an extra branch, that is a win already.

I would like to point out the potential trouble that fixing this issue in the 
callers could cause.

1. This function is called multiple times in menu governor and TEO 
governor. I'm not sure that receiving results using signed integers is enough 
to solve all the problems, in the worst case it may require increasing 
the logical complexity of the code.

2. This function is important for developing idle governor. 
If the problem is not fixed in the function itself, then this potential 
pitfall should be explicitly stated in the documentation. This is because 
it is difficult to predict from the definition and naming of the function 
that it might return a negative number. I actually discovered this anomaly 
when I was doing data analysis on my own idle governor. For some idle control 
algorithms, this exception return could lead to serious consequences, 
because negative return logically won't happen.

> 
> ---
>  drivers/cpuidle/governors/teo.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Index: linux-pm/drivers/cpuidle/governors/teo.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/teo.c
> +++ linux-pm/drivers/cpuidle/governors/teo.c
> @@ -100,8 +100,8 @@ struct teo_idle_state {
>   * @intervals: Saved idle duration values.
>   */
>  struct teo_cpu {
> -       u64 time_span_ns;
> -       u64 sleep_length_ns;
> +       s64 time_span_ns;
> +       s64 sleep_length_ns;
>         struct teo_idle_state states[CPUIDLE_STATE_MAX];
>         int interval_idx;
>         u64 intervals[INTERVALS];
> @@ -216,7 +216,7 @@ static bool teo_time_ok(u64 interval_ns)
>   */
>  static int teo_find_shallower_state(struct cpuidle_driver *drv,
>                                     struct cpuidle_device *dev, int state_idx,
> -                                   u64 duration_ns)
> +                                   s64 duration_ns)
>  {
>         int i;
> 
> @@ -242,7 +242,7 @@ static int teo_select(struct cpuidle_dri
>  {
>         struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
>         s64 latency_req = cpuidle_governor_latency_req(dev->cpu);
> -       u64 duration_ns;
> +       s64 duration_ns;
>         unsigned int hits, misses, early_hits;
>         int max_early_idx, prev_max_early_idx, constraint_idx, idx, i;
>         ktime_t delta_tick;