Re: [PATCH v4.16-rc5 1/3] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW

[Thomas Gleixner]

Jason,

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