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

[Thomas Gleixner]

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