Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 2 Jul 2020 14:27:01 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Anna-Maria Gleixner <anna-maria@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>
Subject: Re: [RFC PATCH 03/10] timer: Simplify LVL_START() and calc_index()
Message-ID: <20200702122659.GB17670@lenoir>
References: <20200701011030.14324-1-frederic@kernel.org>
 <20200701011030.14324-4-frederic@kernel.org>
 <87pn9e9lgq.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87pn9e9lgq.fsf@nanos.tec.linutronix.de>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 02, 2020 at 01:59:17PM +0200, Thomas Gleixner wrote:
> 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?

No :o)

> 
> > @@ -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.

Fair enough. I didn't know about the one jiffy sleep guarantee.
I'll convert this patch to a comment explaining that off by one,
using your above example.

Thanks!