From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751011AbdAWLLB (ORCPT <rfc822;w@1wt.eu>);
        Mon, 23 Jan 2017 06:11:01 -0500
Received: from Galois.linutronix.de ([146.0.238.70]:38697 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750738AbdAWLLA (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 23 Jan 2017 06:11:00 -0500
Date: Mon, 23 Jan 2017 12:10:27 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Zhihui Zhang <zzhsuny@gmail.com>
cc: John Stultz <john.stultz@linaro.org>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] timers: Reconcile the code and the comment for the 250HZ
 case
In-Reply-To: <CADFvMY+A3GTz6XFr_EszT_dNf9sasQOeeskx0yST_mmf+h0r-Q@mail.gmail.com>
Message-ID: <alpine.DEB.2.20.1701231147080.3836@nanos>
References: <1483391659-9752-1-git-send-email-zzhsuny@gmail.com> <CALAqxLX-XGj+65RW9NBLfQum6X9q=+1=MvydSdUsrZWtEf_EmA@mail.gmail.com> <CADFvMY+A3GTz6XFr_EszT_dNf9sasQOeeskx0yST_mmf+h0r-Q@mail.gmail.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sat, 21 Jan 2017, Zhihui Zhang wrote:

> Sure, I believe that comments should always match the code. In this

That's fine.

> case, using either LVL_SIZE - 1 or LVL_SIZE is fine based on my
> understanding about 20 days ago. But I could be wrong and miss some
> subtle details. Anyway, my point is about readability.

Well, readability is one thing, but correctness is more important, right?

Let's assume we have 4 buckets per level and base->clk is 0. So Level 0
has the following expiry times:

Bucket 0:   base->clk + 0
Bucket 1:   base->clk + 1
Bucket 2:   base->clk + 2
Bucket 3:   base->clk + 3

So we can accomodate 4 timers here, but there is a nifty detail. We
guarantee that expiries are not short, so a timer armed for base->clk
will expire at base->clk + 1.

The reason for this is that we have no distinction between absolute and
relative timeouts. But for relative timeouts we have to guarantee that the
timeout does not expire before the number of jiffies has elapsed.

Now a timer armed with 1 jiffie relativ to now (jiffies) cannot be queued
to bucket 0 because jiffies can be incremented immediately after queueing
the timer which would expire it early. So it's queued to bucket 1 and
that's why we need to have LVL_SIZE - 1 and not LVL_SIZE. See also
calc_index().

Your change completely breaks the wheel. Let's assume the above and a
timer expiring at base->clk + 3.

With your change the timer would fall into Level 0. So no calc_index()
does:

	expires = (expires + LVL_GRAN(lvl)) >> LVL_SHIFT(lvl);
	return LVL_OFFS(lvl) + (expires & LVL_MASK);

Let's substitute that for the expires = base->clk + 3 case:

        expires = (base->clk + 3 + 1) >> 0;

--->	expires = 4;

	return 0 + (4 & 0x03);

--->	index = 0

So the timer gets queued into bucket 0 and expires 4 jiffies too early.

So using either LVL_SIZE - 1 or LVL_SIZE is _NOT_ fine.

Thanks,

	tglx