Jason,
On Wed, 14 Mar 2018, jason.vas.d...@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
