Messages in this thread | | | Date | Thu, 15 Aug 2019 16:16:09 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [RFC PATCH 1/5] x86: tsc: add tsc to art helpers |
| |
Felipe,
On Thu, 15 Aug 2019, Felipe Balbi wrote: > Thomas Gleixner <tglx@linutronix.de> writes: > > On Tue, 16 Jul 2019, Felipe Balbi wrote: > > > > So some information what those interfaces are used for and why they are > > needed would be really helpful. > > Okay, I have some more details about this. The TGPIO device itself uses > ART since TSC is not directly available to anything other than the > CPU. The 'problem' here is that reading ART incurs extra latency which > we would like to avoid. Therefore, we use TSC and scale it to > nanoseconds which, would be the same as ART to ns.
Fine. But that's not really correct:
TSC = art_to_tsc_offset + ART * scale;
> >> +void get_tsc_ns(struct system_counterval_t *tsc_counterval, u64 *tsc_ns)
Why is this not returning the result instead of having that pointer indirection?
> >> +{ > >> + u64 tmp, res, rem; > >> + u64 cycles; > >> + > >> + tsc_counterval->cycles = clocksource_tsc.read(NULL); > >> + cycles = tsc_counterval->cycles; > >> + tsc_counterval->cs = art_related_clocksource;
So this does more than returning the TSC time converted to nanoseconds. The function name should reflect this. Plus both functions want kernel-doc explaining what they do.
> >> + rem = do_div(cycles, tsc_khz); > >> + > >> + res = cycles * USEC_PER_SEC; > >> + tmp = rem * USEC_PER_SEC; > >> + > >> + do_div(tmp, tsc_khz); > >> + res += tmp; > >> + > >> + *tsc_ns = res; > >> +} > >> +EXPORT_SYMBOL(get_tsc_ns); > >> + > >> +u64 get_art_ns_now(void) > >> +{ > >> + struct system_counterval_t tsc_cycles; > >> + u64 tsc_ns; > >> + > >> + get_tsc_ns(&tsc_cycles, &tsc_ns); > >> + > >> + return tsc_ns; > >> +} > >> +EXPORT_SYMBOL(get_art_ns_now); > > > > While the changes look innocuous I'm missing the big picture why this needs > > to emulate ART instead of simply using TSC directly. > > i don't think we're emulating ART here (other than the name in the > function). We're just reading TSC and converting to nanoseconds, right?
Well, the function name says clearly: get_art_ns_now(). But you are not using ART, you use the TSC and derive the ART value from it (incorrectly).
Thanks,
tglx
| |