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
