Date: Thu, 1 Dec 2016 00:21:02 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: David Gibson <david@gibson.dropbear.id.au>
cc: John Stultz <john.stultz@linaro.org>,
        lkml <linux-kernel@vger.kernel.org>, Liav Rehana <liavr@mellanox.com>,
        Chris Metcalf <cmetcalf@mellanox.com>,
        Richard Cochran <richardcochran@gmail.com>,
        Ingo Molnar <mingo@kernel.org>, Prarit Bhargava <prarit@redhat.com>,
        Laurent Vivier <lvivier@redhat.com>,
        "Christopher S . Hall" <christopher.s.hall@intel.com>,
        "4.6+" <stable@vger.kernel.org>
Subject: Re: [PATCH] timekeeping: Change type of nsec variable to unsigned
 in its calculation.
In-Reply-To: <20161129235727.GA19891@umbus>
Message-ID: <alpine.DEB.2.20.1611302355070.3619@nanos>
References: <1479531216-25361-1-git-send-email-john.stultz@linaro.org> <alpine.DEB.2.20.1611291520070.4358@nanos> <20161129235727.GA19891@umbus>
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
Content-Length: 2366
Lines: 61

On Wed, 30 Nov 2016, David Gibson wrote:
> On Tue, Nov 29, 2016 at 03:22:17PM +0100, Thomas Gleixner wrote:
> > If we have legitimate use cases with a negative delta, then this patch
> > breaks them no matter what. See the basic C course section in the second
> > link.
> 
> So, fwiw, when I first wrote a variant on this, I wasn't trying to fix
> every case - just to make the consequences less bad if something goes
> wrong.  An overflow here can still mess up timekeeping, it's true, but
> time going backwards tends to cause things to go horribly, horribly
> wrong - which was why I spotted this in the first place.

I completely understand the intention.

We _cannot_ make that whole thing unsigned when it is not 100% clear
that there is no legitimate caller which hands in a negative delta and
rightfully expects to get a negative nanoseconds value handed back.

If someone sits down and proves that this cannot happen there is no reason
to hold that off.

But that still does not solve the underlying root cause. Assume the
following:

  T1 = base + to_nsec(delta1)

       where delta1 is big, but the multiplication does not overflow 64bit

  Now wait a bit and do:
      
  T2 = base + to_nsec(delta2)

       now delta2 is big enough, so the  multiplication does overflow 64bit
       now delta2 is big enough to overflow 64bit with the multiplication.

  The result is T2 < T1, i.e. time goes backwards.

All what the unsigned conversion does is to procrastinate the problem by a
factor of 2. So instead of failing after 10 seconds we fail after 20
seconds. And just because you never observed the 20 seconds problem it does
not go away magically.

The proper solution is to figure out WHY we are running into that situation
at all. So far all I have seen are symptom reports and fairy tales about
ftp connections, but no real root cause analysis.

The only reason for this to happen is that 'base' does not get updated for
a too long time, so the delta grows into the overflow range.

We already have protection against idle sleeping too long for this to
happen. If the idle protection is not working then it needs to be fixed.

if some other situation can cause the base not to be updated for a long
time, then this needs to be fixed.

Curing the symptom is a guarantee that the root cause will show another
symptom sooner than later.

Thanks,

	tglx