From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758330AbdEVTGK (ORCPT ); Mon, 22 May 2017 15:06:10 -0400 Received: from mail-wr0-f169.google.com ([209.85.128.169]:35097 "EHLO mail-wr0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751656AbdEVTGH (ORCPT ); Mon, 22 May 2017 15:06:07 -0400 MIME-Version: 1.0 In-Reply-To: <87y3tpso51.fsf@concordia.ellerman.id.au> References: <1494613450-4713-1-git-send-email-john.stultz@linaro.org> <87y3tpso51.fsf@concordia.ellerman.id.au> From: John Stultz Date: Mon, 22 May 2017 12:06:04 -0700 Message-ID: Subject: Re: [RFC][PATCH] time: Add warning about imminent deprecation of CONFIG_GENERIC_TIME_VSYSCALL_OLD To: Michael Ellerman Cc: lkml , Thomas Gleixner , Ingo Molnar , Miroslav Lichvar , Richard Cochran , Prarit Bhargava , Marcelo Tosatti , Paul Mackerras , Anton Blanchard , Benjamin Herrenschmidt , Tony Luck , Fenghua Yu Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, May 21, 2017 at 5:58 PM, Michael Ellerman wrote: > John Stultz writes: > >> CONFIG_GENERIC_TIME_VSYSCALL_OLD was introduced five years ago >> to allow a transition from the old vsyscall implementations to >> the new method (which simplified internal accounting and made >> timekeeping more precise). > > I'm sure it's completely obvious to everyone except me what needs to be > done, but can you spell it out for me? Keeping in mind that I don't know > anything about the time keeping code. No. Apologies, I probably should have included something like this. Basically long ago, timekeeping was handled (roughly) like: clock_gettime(): now = tk->clock->read() offset_ns = ((now - tk->cycle_last) * tk->clock->mult) >> tk->clock->shift; return timespec_add_ns(tk->xtime, offset_ns); But since for error handling use sub-ns precision, combined with that for update performance, we accumulate in fixed intervals, there are situations where in the update, we could accumulate half of a nanosecond into the base tk->xtime value and leaving half of a nanosecond in the offset. This caused the split nanosecond to be truncated out by the math, causing 1ns discontinuities. So to address this, we came up with sort of a hack, which when we accumulate rounds up that partial nanosecond, and adds the amount we rounded up to the error (which will cause the freq correction code to slow the clock down slightly). This is the code that is now done in the old_vsyscall_fixup() logic. Unfortunately this fix (which generates up to a nanosecond of error per tick) then made the freq correction code do more work and made it more difficult to have a stable clock. So we went for a more proper fix, which was to properly handle the sub-nanosecond portion of the timekeeping throughout the logic, doing the truncation last. clock_gettime(): now = tk->clock->read() ret.tv_sec = tk->xtime_sec; offset_sns = (now - tk->cycle_last) * tk->clock->mult; ret.tv_nsec = (offset_sns + tk->tkr_mono.xtime_nsec) >> tk->clock->shift; return ret; So in the above, we now use the tk->tkr_mono.xtime_nsec (which despite its unfortunate name, stores the accumulated shifted nanoseconds), and add it to the (current_cycle_delta * clock->mult), then we do the shift last to preserve as much precision as we can. Unfortunately we need all the reader code to do the same, which wasn't easy to transition in some cases. So we provided the CONFIG_GENERIC_TIME_VSYSCALL_OLD option to preserve the old round-up behavior while arch maintainers could make the transition. >> However, PPC and IA64 have yet to make the transition, despite >> in some cases me sending test patches to try to help it along. >> >> http://patches.linaro.org/patch/30501/ >> http://patches.linaro.org/patch/35412/ > > I've never seen a PPC patch, did you send one? Yea. The PPC patch I never felt comfortable enough with to send. >> If its helpful, my last pass at the patches can be found here: >> https://git.linaro.org/people/john.stultz/linux.git dev/oldvsyscall-cleanup > > Looks like it's just a draft for PPC. Do you think that should work and > it just needs testing? The comment about the vdso being "slightly > behind" is a little concerning. So, long ago I talked w/ Paul Mackerras about the ppc vdso code, as ppc has some other legacy "userspace time" code that has to be maintained as well (I believe there's not code page, just data page that userspace pulls directly from). So for that case, we have the problem where we can't do this sub-ns accounting, so the hack there is rather then rounding-up and adding to ntp_error in the accumulation code (which then causes the mult to slow), we're basically doing it in the reader, slowing down mult by one. This will cause userspace time to have "steps" where after an accumulation time jumps forward a bit, but avoids the possibility of a discontinuity where time jumps backwards. But again, I don't want to pretend I'm not an expert on the ppc side. This draft patch doesn't even touch the __kernel_clock_gettime() implementations, and was trying to preserve the existing ppc logic while transitioning the core code. Its likely that a better fix should be done deeper in the ppc side (likely splitting the legacy userspace data formats out so any hacks only apply to them). thanks -john