Messages in this thread | | | From | Jason Vas Dias <> | Date | Sun, 11 Mar 2018 22:03:29 +0000 | Subject | Re: [PATCH v4.16-rc4 1/1] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW |
| |
Thanks Thomas -
On 11/03/2018, Thomas Gleixner <tglx@linutronix.de> 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 [unhandled content-type:application/octet-stream] | |