Re: [PATCH] timekeeping: Change type of nsec variable to unsigned in its calculation.

[Thomas Gleixner]

On Thu, 1 Dec 2016, David Gibson wrote:
> On Thu, Dec 01, 2016 at 12:21:02AM +0100, Thomas Gleixner wrote:
> > 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.
> 
> But.. delta is a cycle_t, which is typedef'd to u64, so how could it
> be negative?

Indeed. To be honest I did not bother to look that up and for some reason I
was assuming that it's a s64. :(

So yes, we can make all this unsigned and all worries about negative deltas
are moot.

But we really should get rid of that cycle_t typedef and simply use u64 and
be done with it. All this typedeffery for no value is just causing
confusion. I'm very well able to confuse myself, so I don't need extra
stimulus.

> This is why I believed my original version (35a4933) to be safe - it
> was merely removing a signed intermediate from what was essentially an
> unsigned calculation (technically the output was signed, but the right
> shift means that's not relevant).
> 
> > 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.
> 
> Hm, I see.  Do we ever actually update time that way (at least core
> system time), rather than using the last result as a base?

It's:

    delta = read(clocksoure) - last_update;
    T = base + to_nsec(delta)

and in the update function we do:

    now = read(clocksoure);
    delta = now - last_update;
    last_update = now;
    base += to_ns(delta);

Usually the update happens once per tick and if all cpus are idle we have a
safe guard which makes sure that the update happens _before_ we run into
the overflow situation or into a wraparound of the clocksource, which ever
comes first.

> It does seem like the safer approach might be to clamp the result in
> case of overflow, though.

Right, but clamping has its own issues. See below.

> > 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.
> 
> At least in the case I was observing I'm pretty sure we weren't
> updating time that way - we always used a delta from the last value,
> so to_nsec() returning always positive was enough to make time not go
> backwards.

See above. But in case of failure the delta to the last update value was
big enough to overflow into negative space. Making it unsigned just
happened to 'work' because the stop time of the VM was not large enough to
trigger the unsigned mult overflow.

> > 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.
> 
> In the case I hit, it was due to running in a VM that had been stopped
> for a substantial amount of time, so nothing that's actually under the
> guest kernel's control.  The bug-as-reported was that if the VM was
> suspended for too long it would blow up immediately upon resume.

Suspended as in suspend/resume? The timekeeping_resume() code path does not
use the conversion function, it has already it's own algorithm to protect
against the potential overflow.

So I assume that you are talking about a VM which was not scheduled by the
host due to overcommitment (who ever thought that this is a good idea) or
whatever other reason (yes, people were complaining about wreckage caused
by stopping kernels with debuggers) for a long enough time to trigger that
overflow situation. If that's the case then the unsigned conversion will
just make it more unlikely but it still will happen.

I agree that clamping the result would prevent the time going backwards
issue for clocksources which have a wide enough counter (x86 TSC, powerpc
incrementer, ...), but it won't prevent problems with clocksources which
wrap around due to a small bit width of the counter.

We have two options to deal with the issue for wide enough clocksources:

1) Checking that delta is less than clocksource->max_cycles.

   I really hate this because we invested a lot of thoughts to squeeze
   everything we need for the time getters hotpath into a single cache
   line. Accessing clocksource->max_cycles forces us to touch another
   one. Bah!

   Aside of that what guarantees that we never run into a situation where
   something doing timekeeping updates (NTP, PTP, PPS ...) uses such a
   clamped value and comes to completely bogus conclusions? Are we going to
   analyze and fixup all of that in order to prevent such wreckage?
   
   I seriously doubt that given the fact, that nobody sat down and analyzed
   the signed/unsigned issue proper, which is way less complex.

2) Use mul_u64_u32_shr()

   That works without an extra cache line, but it's more expensive in terms
   of text size especially on architectures which do not support the mul64
   expansion to 128bit natively.

   But that seems like the most robust solution. We can be clever and make
   this conditional on both a configuration switch and a static key which
   can be turned on by guests. I'll send out a RFC series later today.

Yet another proof that virtualization is creating more problems than it
solves.

Thanks,

	tglx