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

Thomas Gleixner
2018-03-11T18:01:47Z

On Sun, 11 Mar 2018, Jason Vas Dias wrote:

This looks better now. Though running that patch through checkpatch.pl
results in:

total: 28 errors, 20 warnings, 139 lines checked

....

> +notrace static u64 vread_tsc_raw(void)

Why do you need a separate function? I asked you to use vread_tsc(). So you
might have reasons for doing that, but please then explain WHY and not just
throw the stuff in my direction w/o any comment.

> +{
> + u64 tsc, last=gtod->raw_cycle_last;
> + if( likely( gtod->has_rdtscp ) ) {
> + u32 tsc_lo, tsc_hi,
> + tsc_cpu __attribute__((unused));
> + asm volatile
> + ( "rdtscp"
> + /* ^- has built-in cancellation point / pipeline stall "barrier" */
> + : "=a" (tsc_lo)
> + , "=d" (tsc_hi)
> + , "=c" (tsc_cpu)
> + ); // since all variables 32-bit, eax, edx, ecx used - NOT rax, rdx, rcx
> + tsc = ((((u64)tsc_hi) & 0xffffffffUL) << 32) | (((u64)tsc_lo) & 0xffffffffUL);

This is not required to make the vdso accessor for monotonic raw work.

If at all then the rdtscp support wants to be in a separate patch with a
proper explanation.

Aside of that the code for rdtscp wants to be in a proper inline helper in
the relevant header file and written according to the coding style the
kernel uses for asm inlines.

> + } else {
> + tsc = rdtsc_ordered();
> + }
> + if (likely(tsc >= last))
> + return tsc;
> + asm volatile ("");
> + return last;
> +}

The rest looks ok.

Thanks,

tglx