Messages in this thread | | | Date | Wed, 14 Mar 2018 15:27:27 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH v4.16-rc5 1/3] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW |
| |
Jason,
On Wed, 14 Mar 2018, jason.vas.dias@gmail.com wrote:
this subject line is not really what it should be.
[PATCH v4.16-rc5 1/3] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW
Documentation clearly says:
The canonical patch subject line is::
Subject: [PATCH 001/123] subsystem: summary phrase
The ``summary phrase`` in the email's Subject should concisely describe the patch which that email contains. The ``summary phrase`` should not be a filename. Do not use the same ``summary phrase`` for every patch in a whole patch series (where a ``patch series`` is an ordered sequence of multiple, related patches).
Aside of that the text body of the patch lacks:
1) A description of the patch
2) Your Signed-off-by. Again: checkpatch.pl complains for a reason.
Is it really that hard to comply with the established and documented proceedures?
> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c > index f19856d..fbc7371 100644 > --- a/arch/x86/entry/vdso/vclock_gettime.c > +++ b/arch/x86/entry/vdso/vclock_gettime.c > @@ -182,6 +182,18 @@ notrace static u64 vread_tsc(void) > return last; > } > > +notrace static u64 vread_tsc_raw(void) > +{ > + u64 tsc > + , last = gtod->raw_cycle_last;
This is hardly kernel coding style.
> + > + tsc = rdtsc_ordered();
and these spaces are pointless.
> + if (likely(tsc >= last)) > + return tsc; > + asm volatile (""); > + return last; > +}
As I explained to you before: This function is not required because gtod->cycle_last and gtod->raw_cycle_last are the same value.
> notrace static inline u64 vgetsns(int *mode) > { > u64 v; > @@ -203,6 +215,27 @@ notrace static inline u64 vgetsns(int *mode) > return v * gtod->mult; > } > > +notrace static inline u64 vgetsns_raw(int *mode) > +{ > + u64 v; > + cycles_t cycles; > + > + if (gtod->vclock_mode == VCLOCK_TSC) > + cycles = vread_tsc_raw(); > +#ifdef CONFIG_PARAVIRT_CLOCK > + else if (gtod->vclock_mode == VCLOCK_PVCLOCK) > + cycles = vread_pvclock(mode); > +#endif > +#ifdef CONFIG_HYPERV_TSCPAGE > + else if (gtod->vclock_mode == VCLOCK_HVCLOCK) > + cycles = vread_hvclock(mode); > +#endif > + else > + return 0; > + v = (cycles - gtod->raw_cycle_last) & gtod->raw_mask;
gtod->raw_mask is the same as gtod->mask for obvious reasons. So the whole thing can be simplified by extending vgetns() with a mult argument, which is handed in from the call sites.
> > + vdata->raw_cycle_last = tk->tkr_raw.cycle_last; > + vdata->raw_mask = tk->tkr_raw.mask; > + vdata->raw_mult = tk->tkr_raw.mult; > + vdata->raw_shift = tk->tkr_raw.shift;
Only the raw_mult/shift value needs to be stored.
Thanks,
tglx
| |