Re: [RFC PATCH 03/10] timer: Simplify LVL_START() and calc_index()

Thomas Gleixner
2020-07-02T11:59:24Z

Frederic Weisbecker <frederic@kernel.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