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

[Jason Vas Dias]

Thanks Thomas -

On 11/03/2018, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 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
>

Hmm, I was unaware of that script, I'll run and find out why -
probably because whitespace is not visible in emacs with
my monospace font and it is very difficult to see if tabs
are used if somehow a '\t\ ' or ' \t' has slipped in .

I'll run the script, fix the errors. and repost.

> ....
>
>> +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.
>

mainly, because vread_tsc() makes its comparison against gtod->cycles_last ,
a copy of tk->tkr_mono.cycle_last, while vread_tsc_raw() uses
gtod->raw_cycle_last, a copy of tk->tkr_raw.cycle_last .

And rdtscp has a built-in "barrier", as the comments explain, making
rdtsc_ordered()'s 'barrier()' unnecessary .


>> +{
>> +        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.
>

Sorry, I will put the function in the same header as rdtsc_ordered () ,
in a separate patch.

> The rest looks ok.
>
> Thanks,
>
> 	tglx
>

I'll re-generate patches and resend .

A complete patch , against 4.15.9, is attached , that I am using ,
including a suggested '__vdso_linux_tsc_calibration()'
function and arch/x86/include/uapi/asm/vdso_tsc_calibration.h file
that does not return any pointers into the VDSO .

Presuming this was split into separate patches as you suggest,
and was against the latest HEAD branch (4.16-rcX), would it be OK to
include the vdso_linux_tsc_calibration() work ?
It does enable user space code to develop accurate TSC readers
which are free to use different structures and pico-second resolution.
The actual user-space clock_gettime(CLOCK_MONOTONIC_RAW)
replacement I am using for work just reads the TSC ,  with a latency of
< 8ns, and uses the linux_tsc_calibration to convert using
floating-point as required.

Thanks & Regards,
Jason

Attachment: vdso_gettime_monotonic_raw-4.15.9.patch
Description: Binary data