| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120719093305.GA27086@gmail.com>
Date: Thu, 19 Jul 2012 11:33:05 +0200
From: Ingo Molnar <mingo@...nel.org>
To: John Stultz <john.stultz@...aro.org>
Cc: lkml <linux-kernel@...r.kernel.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Richard Cochran <richardcochran@...il.com>,
Prarit Bhargava <prarit@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/2] time: Cleanup offs_real/wall_to_mono and
offs_boot/total_sleep_time updates
* John Stultz <john.stultz@...aro.org> wrote:
> For performance reasons, we maintain ktime_t based duplicates of
> wall_to_monotonic (offs_real) and total_sleep_time (offs_boot).
>
> Since large problems could occur (such as the resume regression
> on 3.5-rc7, or the leapsecond hrtimer issue) if these value pairs
> were to be inconsistently updated, this patch this cleans up how
> we modify these value pairs to ensure we are always consistent.
>
> As a side-effect this is also more efficient as we only
> caulculate the duplicate values when they are changed,
> rather then every update_wall_time call.
Makes sense.
> This also provides WARN_ONs to detect if future changes break
> the invariants.
>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Cc: Richard Cochran <richardcochran@...il.com>
> Cc: Prarit Bhargava <prarit@...hat.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
> kernel/time/timekeeping.c | 94 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 59 insertions(+), 35 deletions(-)
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index f045cc5..0b780bd 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -65,14 +65,14 @@ struct timekeeper {
> * used instead.
> */
> struct timespec wall_to_monotonic;
> - /* time spent in suspend */
> - struct timespec total_sleep_time;
> - /* The raw monotonic time for the CLOCK_MONOTONIC_RAW posix clock. */
> - struct timespec raw_time;
> /* Offset clock monotonic -> clock realtime */
> ktime_t offs_real;
> + /* time spent in suspend */
> + struct timespec total_sleep_time;
> /* Offset clock monotonic -> clock boottime */
> ktime_t offs_boot;
> + /* The raw monotonic time for the CLOCK_MONOTONIC_RAW posix clock. */
> + struct timespec raw_time;
> /* Seqlock for all timekeeper values */
> seqlock_t lock;
> };
> @@ -117,6 +117,36 @@ static void tk_xtime_add(struct timekeeper *tk, const struct timespec *ts)
> tk->xtime_nsec += ts->tv_nsec << tk->shift;
> }
>
> +
Stray newline.
> +static void tk_set_wall_to_mono(struct timekeeper *tk, struct timespec wtm)
> +{
> + struct timespec tmp;
> +
> + /*
> + * Verify consistency of: offset_real = -wall_to_monotonic
> + * before modifying anything
> + */
> + set_normalized_timespec(&tmp, -tk->wall_to_monotonic.tv_sec,
> + -tk->wall_to_monotonic.tv_nsec);
> + WARN_ON_ONCE(tk->offs_real.tv64 != timespec_to_ktime(tmp).tv64);
> + tk->wall_to_monotonic = wtm;
> + set_normalized_timespec(&tmp, -wtm.tv_sec, -wtm.tv_nsec);
> + tk->offs_real = timespec_to_ktime(tmp);
> +}
> +
> +
Stray newline.
> +static void tk_set_sleep_time(struct timekeeper *tk, struct timespec t)
> +{
> + /* Verify consistency before modifying */
> + WARN_ON_ONCE(tk->offs_boot.tv64 !=
> + timespec_to_ktime(tk->total_sleep_time).tv64);
asserts like this can be put into a single line - we rarely need
to read them if they don't trigger - and making them
non-intrusive oneliners is a bonus.
> +
> + tk->total_sleep_time = t;
> + tk->offs_boot = timespec_to_ktime(t);
> +}
> +
> +
> +
Stray newlines.
> /**
> * timekeeper_setup_internals - Set up internals to use clocksource clock.
> *
> @@ -217,13 +247,6 @@ static inline s64 timekeeping_get_ns_raw(struct timekeeper *tk)
> return nsec + arch_gettimeoffset();
> }
>
> -static void update_rt_offset(struct timekeeper *tk)
> -{
> - struct timespec tmp, *wtm = &tk->wall_to_monotonic;
> -
> - set_normalized_timespec(&tmp, -wtm->tv_sec, -wtm->tv_nsec);
> - tk->offs_real = timespec_to_ktime(tmp);
> -}
>
> /* must hold write on timekeeper.lock */
> static void timekeeping_update(struct timekeeper *tk, bool clearntp)
> @@ -234,7 +257,6 @@ static void timekeeping_update(struct timekeeper *tk, bool clearntp)
> tk->ntp_error = 0;
> ntp_clear();
> }
> - update_rt_offset(tk);
> xt = tk_xtime(tk);
> update_vsyscall(&xt, &tk->wall_to_monotonic, tk->clock, tk->mult);
> }
> @@ -420,8 +442,8 @@ int do_settimeofday(const struct timespec *tv)
> ts_delta.tv_sec = tv->tv_sec - xt.tv_sec;
> ts_delta.tv_nsec = tv->tv_nsec - xt.tv_nsec;
>
> - timekeeper.wall_to_monotonic =
> - timespec_sub(timekeeper.wall_to_monotonic, ts_delta);
> + tk_set_wall_to_mono(&timekeeper,
> + timespec_sub(timekeeper.wall_to_monotonic, ts_delta));
>
> tk_set_xtime(&timekeeper, tv);
>
> @@ -456,8 +478,8 @@ int timekeeping_inject_offset(struct timespec *ts)
>
>
> tk_xtime_add(&timekeeper, ts);
> - timekeeper.wall_to_monotonic =
> - timespec_sub(timekeeper.wall_to_monotonic, *ts);
> + tk_set_wall_to_mono(&timekeeper,
> + timespec_sub(timekeeper.wall_to_monotonic, *ts));
There's a *lot* of unnecessary ugliness here and in many other
places in kernel/time/ due to repeating patterns of
"timekeeper.", which gets repeated many times and blows up line
length.
Please add this to the highest level (system call, irq handler,
etc.) code:
struct timekeeper *tk = &timekeeper;
and pass that down to lower levels. The tk-> pattern will be the
obvious thing to type in typical time handling functions.
This increases readability and clarifies the data flow and might
bring other advantages in the future.
> timekeeping_update(&timekeeper, true);
>
> @@ -624,7 +646,7 @@ void __init timekeeping_init(void)
> {
> struct clocksource *clock;
> unsigned long flags;
> - struct timespec now, boot;
> + struct timespec now, boot, tmp;
>
> read_persistent_clock(&now);
> read_boot_clock(&boot);
> @@ -645,23 +667,19 @@ void __init timekeeping_init(void)
> if (boot.tv_sec == 0 && boot.tv_nsec == 0)
> boot = tk_xtime(&timekeeper);
>
> - set_normalized_timespec(&timekeeper.wall_to_monotonic,
> - -boot.tv_sec, -boot.tv_nsec);
> - update_rt_offset(&timekeeper);
> - timekeeper.total_sleep_time.tv_sec = 0;
> - timekeeper.total_sleep_time.tv_nsec = 0;
> + set_normalized_timespec(&tmp, -boot.tv_sec, -boot.tv_nsec);
> + tk_set_wall_to_mono(&timekeeper, tmp);
> +
> + tmp.tv_sec = 0;
> + tmp.tv_nsec = 0;
> + tk_set_sleep_time(&timekeeper, tmp);
> +
> write_sequnlock_irqrestore(&timekeeper.lock, flags);
> }
>
> /* time in seconds when suspend began */
> static struct timespec timekeeping_suspend_time;
>
> -static void update_sleep_time(struct timespec t)
> -{
> - timekeeper.total_sleep_time = t;
> - timekeeper.offs_boot = timespec_to_ktime(t);
> -}
> -
> /**
> * __timekeeping_inject_sleeptime - Internal function to add sleep interval
> * @delta: pointer to a timespec delta value
> @@ -677,10 +695,9 @@ static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
> "sleep delta value!\n");
> return;
> }
> -
> tk_xtime_add(tk, delta);
> - tk->wall_to_monotonic = timespec_sub(tk->wall_to_monotonic, *delta);
> - update_sleep_time(timespec_add(tk->total_sleep_time, *delta));
> + tk_set_wall_to_mono(tk, timespec_sub(tk->wall_to_monotonic, *delta));
> + tk_set_sleep_time(tk, timespec_add(tk->total_sleep_time, *delta));
> }
>
>
Stray newline.
I see a pattern with the newlines there - and this isnt the
first patch from you that has that problem.
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists