On Mon, Aug 07, 2017 at 09:52:10AM -0700, John Stultz wrote:
> On Mon, Aug 7, 2017 at 8:52 AM, Prarit Bhargava <prarit@xxxxxxxxxx> wrote:
> > +u64 ktime_get_real_log_ts(u64 *offset_real)
> > +{
> > + *offset_real = ktime_to_ns(tk_core.timekeeper.offs_real);
> > +
> > + if (timekeeping_active)
> > + return ktime_get_mono_fast_ns();
> > + else
> > + return local_clock();
> > +}
> > +
> > +u64 ktime_get_boot_log_ts(void)
> > +{
> > + if (timekeeping_active)
> > + return ktime_get_boot_fast_ns();
> > + else
> > + return local_clock();
> > +}
>
> This feels a little tacked on and duplicative. I'd suggest having one
> function that returns the offset_real and offset_boot or have a
> separate get_mono_log_ts() so its at least consistent. Additionally,
> in the commit message, you call out the lack of locking between
> grabing the offs_real and calling get_mono_fast_ns(), but I worry it
> may be particularly problematic on 32bit systems, and you don't have
> any notes in the actual code about it (it looks like an oversight).
>
> Also, when timekeeping_active flips over, and we change from local
> clock to the specified clock, do we see a discontinuity in the log? I
> know folks use to gripe about that back in the day.
Yeah, yuck, this smells. Please don't mix clocks like this.
I expect all you want is to avoid the explosion you get from calling the
fast things too early, right? Please use the below, which should result
in it returning 0.
---
kernel/time/timekeeping.c | 47 +++++++++++++++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index cedafa008de5..d111039e0245 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -60,8 +60,39 @@ struct tk_fast {
struct tk_read_base base[2];
};
-static struct tk_fast tk_fast_mono ____cacheline_aligned;
-static struct tk_fast tk_fast_raw ____cacheline_aligned;
+/* Suspend-time cycles value for halted fast timekeeper. */
+static u64 cycles_at_suspend;
+
+static u64 dummy_clock_read(struct clocksource *cs)
+{
+ return cycles_at_suspend;
+}
+
+static struct clocksource dummy_clock = {
+ .read = dummy_clock_read,
+};
+
+static struct tk_fast tk_fast_mono ____cacheline_aligned = {
+ .base = {
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ },
+};
+
+static struct tk_fast tk_fast_raw ____cacheline_aligned = {
+ .base = {
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ (struct tk_read_base){
+ .clock = &dummy_clock,
+ },
+ },
+};
/* flag for if timekeeping is suspended */
int __read_mostly timekeeping_suspended;
@@ -477,18 +508,6 @@ u64 notrace ktime_get_boot_fast_ns(void)
}
EXPORT_SYMBOL_GPL(ktime_get_boot_fast_ns);
-/* Suspend-time cycles value for halted fast timekeeper. */
-static u64 cycles_at_suspend;
-
-static u64 dummy_clock_read(struct clocksource *cs)
-{
- return cycles_at_suspend;
-}
-
-static struct clocksource dummy_clock = {
- .read = dummy_clock_read,
-};
-
/**
* halt_fast_timekeeper - Prevent fast timekeeper from accessing clocksource.
* @tk: Timekeeper to snapshot.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
