linux-kernel - Re: [RFC PATCH 03/10] timer: Simplify LVL_START() and calc

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: <87pn9e9lgq.fsf@nanos.tec.linutronix.de>
Date:   Thu, 02 Jul 2020 13:59:17 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Frederic Weisbecker <frederic@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Anna-Maria Gleixner <anna-maria@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>
Subject: Re: [RFC PATCH 03/10] timer: Simplify LVL_START() and calc_index()

Frederic Weisbecker <frederic@...nel.org> writes:
> LVL_START() makes the first index of a level to start with what would be
> the value of all bits set of the previous level.
>
> For example level 1 starts at 63 instead of 64.
>
> To cope with that, calc_index() always adds one offset for the level
> granularity to the expiry passed in parameter.
>
> Yet there is no apparent reason for such fixups so simplify the whole
> thing.

You sure?

> @@ -158,7 +158,7 @@ EXPORT_SYMBOL(jiffies_64);
>   * The time start value for each level to select the bucket at enqueue
>   * time.
>   */
> -#define LVL_START(n)	((LVL_SIZE - 1) << (((n) - 1) * LVL_CLK_SHIFT))
> +#define LVL_START(n)	(LVL_SIZE << (((n) - 1) * LVL_CLK_SHIFT))

>  /* Size of each clock level */
>  #define LVL_BITS	6
> @@ -489,7 +489,7 @@ static inline void timer_set_idx(struct timer_list *timer, unsigned int idx)
>   */
>  static inline unsigned calc_index(unsigned expires, unsigned lvl)
>  {
> -	expires = (expires + LVL_GRAN(lvl)) >> LVL_SHIFT(lvl);
> +	expires >>= LVL_SHIFT(lvl);
>  	return LVL_OFFS(lvl) + (expires & LVL_MASK);
>  }

So with that you move the expiry of each timer one jiffie ahead vs. the
original code, which violates the guarantee that a timer sleeps at least
for one jiffie for real and not measured in jiffies.

base->clk = 1
jiffies = 0
 local_irq_disable()
                        -> timer interrupt is raised in HW
 timer->expires = 1
 add_timer(timer)
        ---> index == 1
 local_irq_enable()
 timer interrupt
    jiffies++;
    softirq()
        expire(timer);

So the off by one has a reason.

Thanks,

        tglx