Re: [PATCH v4.16-rc5 (3)] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW
Good day - RE: On 15/03/2018, Thomas Gleixner wrote: > On Thu, 15 Mar 2018, Jason Vas Dias wrote: >> On 15/03/2018, Thomas Gleixner wrote: >> > On Thu, 15 Mar 2018, jason.vas.d...@gmail.com wrote: >> > >> >> Resent to address reviewer comments. >> > >> > I was being patient so far and tried to guide you through the patch >> > submission process, but unfortunately this turns out to be just waste of >> > my >> > time. >> > >> > You have not addressed any of the comments I made here: >> > >> > [1] >> > https://lkml.kernel.org/r/alpine.deb.2.21.1803141511340.2...@nanos.tec.linutronix.de >> > [2] >> > https://lkml.kernel.org/r/alpine.deb.2.21.1803141527300.2...@nanos.tec.linutronix.de >> > >> >> I'm really sorry about that - I did not see those mails , >> and have searched for them in my inbox - > > That's close to the 'my dog ate the homework' excuse. > Nevertheless, those messages are NOT in my inbox, nor can I find them on the list - a google search for 'alpine.DEB.2.21.1803141511340.2481' or 'alpine.DEB.2.21.1803141527300.2481' returns only the last two mails on the subject , where you included the links to https://lkml.kernel.org. I don't know what went wrong here, but I did not receive those mails until you informed me of them yesterday evening, when I immediately regenerated the Patch #1 incorporating fixes for your comments, and sent it with Subject: '[PATCH v4.16-rc5 1/1] x86/vdso: VDSO should handle\ clock_gettime(CLOCK_MONOTONIC_RAW) without syscall ' This version re-uses the 'gtod->cycles' value, which as you point out, is the same as 'tk->tkr_raw.cycle_last' - so I removed vread_tsc_raw() . > Of course they were sent to the list and to you personally as I used > reply-all. From the mail server log: > > 2018-03-14 15:27:27 1ew7NH-00039q-Hv <= t...@linutronix.de > id=alpine.deb.2.21.1803141511340.2...@nanos.tec.linutronix.de > > 2018-03-14 15:27:30 1ew7NH-00039q-Hv => jason.vas.d...@gmail.com R=dnslookup > T=remote_smtp H=gmail-smtp-in.l.google.com [2a00:1450:4013:c01::1a] > X=TLS1.2:RSA_AES_128_CBC_SHA1:128 DN="C=US,ST=California,L=Mountain > View,O=Google Inc,CN=mx.google.com" > > 2018-03-14 15:27:31 1ew7NH-00039q-Hv => linux-kernel@vger.kernel.org > R=dnslookup T=remote_smtp H=vger.kernel.org [209.132.180.67] > > > > 2018-03-14 15:27:47 1ew7NH-00039q-Hv Completed > > If those messages would not have been delivered to > linux-kernel@vger.kernel.org they would hardly be on the mailing list > archive, right? > Yes, I cannot explain why I did not receive them . I guess I should consider gmail an unreliable delivery method and use the lkml.org web interface to check for replies - I will do this from now one. > And they both got delivered to your gmail account as well. > No, they are not in my gmail account Inbox or folders. > ERROR: Missing Signed-off-by: line(s) > total: 1 errors, 0 warnings, 71 lines checked > I do not know how to fix this error - I was hoping someone on the list might enlighten me. > > WARNING: externs should be avoided in .c files > #24: FILE: arch/x86/entry/vdso/vclock_gettime.c:31: > +extern unsigned int __vdso_tsc_calibration( > I thought that must be a script bug, since no extern is being declared by that line; it is an external function declaration, just like the unmodified line that precedes it. > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? > #93: > new file mode 100644 > > ERROR: Missing Signed-off-by: line(s) > > total: 1 errors, 2 warnings, 143 lines checked > > It reports an error for every single patch of your latest submission. > >> And I did send the test results in a previous mail - > > In private mail which I ignore if there is no real good reason. And just > for the record. This private mail contains the following headers: > > In-Reply-To: > References: <1521001222-10712-1-git-send-email-jason.vas.d...@gmail.com> > <1521001222-10712-3-git-send-email-jason.vas.d...@gmail.com> > > From: Jason Vas Dias > Date: Wed, 14 Mar 2018 15:08:55 + > Message-ID: > > Subject: Re: [PATCH v4.16-rc5 2/3] x86/vdso: on Intel, VDSO should handle > CLOCK_MONOTONIC_RAW > > So now, if you take the message ID which is in the In-Reply-To: field and > compare it to the message ID which I used for link [2]: > > In-Reply-To: >> > https://lkml.kernel.org/r/alpine.deb.2.21.1803141527300.2...@nanos.tec.linutronix.de > > you might notice that these are identical. So how did you end up replying > to a mail which you never received? > > Nice try. I'm really fed up with this. > The only thing I'm trying to do is fix the implementation of clock_gettime(CLOCK_MONOTONIC_RAW) . I was replying to the message you sent , which contained a reference header for the messages you sent but which I did not receive . I am sorry this happened, but it is beyond my control. As soon as I can, I am going to rent a host in a data-center and set up my own email server. If this experience has taught me anything, it is that gmail is not of suffic
Re: [PATCH v4.16-rc5 (3)] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW
On Thu, 15 Mar 2018, Jason Vas Dias wrote: > On 15/03/2018, Thomas Gleixner wrote: > > On Thu, 15 Mar 2018, jason.vas.d...@gmail.com wrote: > > > >> Resent to address reviewer comments. > > > > I was being patient so far and tried to guide you through the patch > > submission process, but unfortunately this turns out to be just waste of my > > time. > > > > You have not addressed any of the comments I made here: > > > > [1] > > https://lkml.kernel.org/r/alpine.deb.2.21.1803141511340.2...@nanos.tec.linutronix.de > > [2] > > https://lkml.kernel.org/r/alpine.deb.2.21.1803141527300.2...@nanos.tec.linutronix.de > > > > I'm really sorry about that - I did not see those mails , > and have searched for them in my inbox - That's close to the 'my dog ate the homework' excuse. Of course they were sent to the list and to you personally as I used reply-all. From the mail server log: 2018-03-14 15:27:27 1ew7NH-00039q-Hv <= t...@linutronix.de id=alpine.deb.2.21.1803141511340.2...@nanos.tec.linutronix.de 2018-03-14 15:27:30 1ew7NH-00039q-Hv => jason.vas.d...@gmail.com R=dnslookup T=remote_smtp H=gmail-smtp-in.l.google.com [2a00:1450:4013:c01::1a] X=TLS1.2:RSA_AES_128_CBC_SHA1:128 DN="C=US,ST=California,L=Mountain View,O=Google Inc,CN=mx.google.com" 2018-03-14 15:27:31 1ew7NH-00039q-Hv => linux-kernel@vger.kernel.org R=dnslookup T=remote_smtp H=vger.kernel.org [209.132.180.67] 2018-03-14 15:27:47 1ew7NH-00039q-Hv Completed If those messages would not have been delivered to linux-kernel@vger.kernel.org they would hardly be on the mailing list archive, right? And they both got delivered to your gmail account as well. > are you sure they were sent to 'linux-kernel@vger.kernel.org' ? > That is the only list I am subscribed to . > I clicked on the links , but the 'To:' field is just > 'linux-kernel' . There is no 'To' field on that page. [prev in list] [next in list] [prev in thread] [next in thread] List: linux-kernel List != To. As any other sane archive it strips the To and Cc fields for obvious reasons. > If I had seen those messages before I re-submitted, > those issues would have been fixed. > > checkpatch.pl did not report them - > I ran it with all patches and it reported > no errors . patch 1/3: ERROR: Missing Signed-off-by: line(s) total: 1 errors, 0 warnings, 119 lines checked patch 2/3: ERROR: Missing Signed-off-by: line(s) total: 1 errors, 0 warnings, 71 lines checked patch 3/3: WARNING: externs should be avoided in .c files #24: FILE: arch/x86/entry/vdso/vclock_gettime.c:31: +extern unsigned int __vdso_tsc_calibration( WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #93: new file mode 100644 ERROR: Missing Signed-off-by: line(s) total: 1 errors, 2 warnings, 143 lines checked It reports an error for every single patch of your latest submission. > And I did send the test results in a previous mail - In private mail which I ignore if there is no real good reason. And just for the record. This private mail contains the following headers: In-Reply-To: References: <1521001222-10712-1-git-send-email-jason.vas.d...@gmail.com> <1521001222-10712-3-git-send-email-jason.vas.d...@gmail.com> From: Jason Vas Dias Date: Wed, 14 Mar 2018 15:08:55 + Message-ID: Subject: Re: [PATCH v4.16-rc5 2/3] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW So now, if you take the message ID which is in the In-Reply-To: field and compare it to the message ID which I used for link [2]: In-Reply-To: > > https://lkml.kernel.org/r/alpine.deb.2.21.1803141527300.2...@nanos.tec.linutronix.de you might notice that these are identical. So how did you end up replying to a mail which you never received? Nice try. I'm really fed up with this. Thanks, tglx
Re: [PATCH v4.16-rc5 (3)] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW
Hi Thomas - RE: On 15/03/2018, Thomas Gleixner wrote: > Jason, > > On Thu, 15 Mar 2018, jason.vas.d...@gmail.com wrote: > >> Resent to address reviewer comments. > > I was being patient so far and tried to guide you through the patch > submission process, but unfortunately this turns out to be just waste of my > time. > > You have not addressed any of the comments I made here: > > [1] > https://lkml.kernel.org/r/alpine.deb.2.21.1803141511340.2...@nanos.tec.linutronix.de > [2] > https://lkml.kernel.org/r/alpine.deb.2.21.1803141527300.2...@nanos.tec.linutronix.de > I'm really sorry about that - I did not see those mails , and have searched for them in my inbox - are you sure they were sent to 'linux-kernel@vger.kernel.org' ? That is the only list I am subscribed to . I clicked on the links , but the 'To:' field is just 'linux-kernel' . If I had seen those messages before I re-submitted, those issues would have been fixed. checkpatch.pl did not report them - I ran it with all patches and it reported no errors . And I did send the test results in a previous mail - $ gcc -m64 -o timer timer.c ( must be compiled in 64-bit mode). This is using the new rdtscp() function : $ ./timer -r 100 ... Total time: 0.02806S - Average Latency: 0.00028S N zero deltas: 0 N inconsistent deltas: 0 Average of 100 average latencies of 100 samples : 0.00027S This is using the rdtsc_ordered() function: $ ./timer -m -r 100 Total time: 0.05269S - Average Latency: 0.00052S N zero deltas: 0 N inconsistent deltas: 0 Average of 100 average latencies of 100 samples : 0.00047S timer.c is a very short program that just reads N_SAMPLES (a compile-time option) timespecs using either CLOCK_MONOTONIC_RAW (no -m) or CLOCK_MONOTONIC first parameter to clock_gettime(), then computes the deltas as long long, then averages them , counting any zero deltas, or deltas where the previous timespec is somehow greater than the current timespec, which are reported as inconsitencies (note 'inconistent deltas:0' and 'zero deltas: 0' in output). So my initial claim that rdtscp() can be twice as fast as rdtsc_ordered() was not far-fetched - this is what I am seeing . I think this is because of the explicit barrier() call in rdtsc_ordered() . This must be slower than than the internal processor pipeline "cancellation point" (barrier) used by the rdtscp instruction itself. This is the only reason for the rdtscp call - plus all modern Intel & AMD CPUs support it, and it DOES solve the ordering problem, whereby instructions in one pipeline of a task can get different rdtsc() results than instructions in another pipeline. I will document the results better in the ChangeLog , fix all issues you identified, and resend . I did not mean to ignore your comments - those mails are nowhere in my Inbox - please , confirm the actual email address they are getting sent to. Thanks & Regards, Jason /* * Program to measure high-res timer latency. * */ #include #include #include #include #include #include #include #include #include #include #ifndef N_SAMPLES #define N_SAMPLES 100 #endif #define _STR(_S_) #_S_ #define STR(_S_) _STR(_S_) #define TS2NS(_TS_) unsigned long long)(_TS_).tv_sec)*10ULL) + (((unsigned long long)((_TS_).tv_nsec int main(int argc, char *const* argv, char *const* envp) { struct timespec sample[N_SAMPLES+1]; unsigned int cnt=N_SAMPLES, s=0 , avg_n=0; unsigned long long deltas [ N_SAMPLES ] , t1, t2, sum=0, zd=0, ic=0, d , t_start, avg_ns, *avgs=0; clockid_t clk = CLOCK_MONOTONIC_RAW; bool do_dump = false; int argn=1, repeat=1; for(; argn < argc; argn+=1) if( argv[argn] != NULL ) if( *(argv[argn]) == '-') switch( *(argv[argn]+1) ) { case 'm': case 'M': clk = CLOCK_MONOTONIC; break; case 'd': case 'D': do_dump = true; break; case 'r': case 'R': if( (argn < argc) && (argv[argn+1] != NULL)) repeat = atoi(argv[argn+=1]); break; case '?': case 'h': case 'u': case 'U': case 'H': fprintf(stderr,"Usage: timer_latency [\n\t-m : use CLOCK_MONOTONIC clock (not CLOCK_MONOTONIC_RAW)\n\t-d : dump timespec contents. N_SAMPLES: " STR(N_SAMPLES) "\n\t" "-r \n]\t" "Calculates average timer latency (minimum time that can be measured) over N_SAMPLES.\n" ); return 0; } if( repeat > 1 ) { avgs=alloca(sizeof(unsigned long long) * (N_SAMPLES + 1)); if( ((unsigned long) avgs) & 7 ) avgs = ((unsigned long long*)(((unsigned char*)avgs)+(8-((unsigned long) avgs) & 7))); } do { cnt=N_SAMPLES; s=0; do { if( 0 != clock_gettime(clk, &sample[s++]) ) { fprintf(stderr,"oops, clock_gettime() failed: %d: '%s'.\n", errno, strerror(errno)); return 1; } }while( --cnt ); clock_gettime(clk, &sample[s]); for(s=1; s < (N_SAMPLES+1); s+=1) { t1 = TS2NS(sample[s-1]); t2 = TS2NS(sample[s]); if ( (t1 > t2)
Re: [PATCH v4.16-rc5 (3)] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW
Jason, On Thu, 15 Mar 2018, jason.vas.d...@gmail.com wrote: > Resent to address reviewer comments. I was being patient so far and tried to guide you through the patch submission process, but unfortunately this turns out to be just waste of my time. You have not addressed any of the comments I made here: [1] https://lkml.kernel.org/r/alpine.deb.2.21.1803141511340.2...@nanos.tec.linutronix.de [2] https://lkml.kernel.org/r/alpine.deb.2.21.1803141527300.2...@nanos.tec.linutronix.de But still you claim that you addressed reviewer comments. The delta between the two patch series shows clearly that you ignored both review mails completely. You merily changed the implementation of a function, which I not even reviewed, and fiddled in the big fat comment. Feel free to ignore me, but rest assured that this is the last chance before you end up in the /dev/null mail filter. I recommend you to try to fixup only the first patch and once that is good to go you might try to submit the rest. But please take your time and address _ALL_ of my comments and follow the documented rules. If you have questions regarding my comments or the process after working through them and the Documentation I pointed to, feel free to ask them on the list and not in private mail. If you think that my comments or recommendations are wrong, discuss them on list instead of silently ignoring them. Yours grumpy tglx
[PATCH v4.16-rc5 (3)] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW
Resent to address reviewer comments. Currently, the VDSO does not handle clock_gettime( CLOCK_MONOTONIC_RAW, &ts ) on Intel / AMD - it calls vdso_fallback_gettime() for this clock, which issues a syscall, having an unacceptably high latency (minimum measurable time or time between measurements) of 300-700ns on 2 2.8-3.9ghz Haswell x86_64 Family'_'Model : 06_3C machines under various versions of Linux. Sometimes, particularly when correlating elapsed time to performance counter values, user-space code needs to know elapsed time from the perspective of the CPU no matter how "hot" / fast or "cold" / slow it might be running wrt NTP / PTP "real" time; when code needs this, the latencies associated with a syscall are often unacceptably high. I reported this as Bug #198161 : 'https://bugzilla.kernel.org/show_bug.cgi?id=198961' and in previous posts with subjects matching 'CLOCK_MONOTONIC_RAW' . This patch handles CLOCK_MONOTONIC_RAW clock_gettime() in the VDSO , by exporting the raw clock calibration, last cycles, last xtime_nsec, and last raw_sec value in the vsyscall_gtod_data during vsyscall_update() . Now the new do_monotonic_raw() function in the vDSO has a latency of @ 24ns on average, and the test program: tools/testing/selftest/timers/inconsistency-check.c succeeds with arguments: '-c 4 -t 120' or any arbitrary -t value. The patch is against Linus' latest 4.16-rc5 tree, current HEAD of : git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git . This patch affects only files: arch/x86/include/asm/vgtod.h arch/x86/entry/vdso/vclock_gettime.c arch/x86/entry/vdso/vdso.lds.S arch/x86/entry/vdso/vdsox32.lds.S arch/x86/entry/vdso/vdso32/vdso32.lds.S arch/x86/entry/vsyscall/vsyscall_gtod.c There are 3 patches in the series : Patch #1 makes the VDSO handle clock_gettime(CLOCK_MONOTONIC_RAW) with rdtsc_ordered() Patches #2 & #3 should be considered "optional" : Patch #2 makes the VDSO handle clock_gettime(CLOCK_MONOTONIC_RAW) with a new rdtscp() function in msr.h Patch #3 makes the VDSO export TSC calibration data via a new function in the vDSO: unsigned int __vdso_linux_tsc_calibration ( struct linux_tsc_calibration *tsc_cal ) that user code can optionally call. Patch #2 makes clock_gettime(CLOCK_MONOTONIC_RAW) calls somewhat faster than clock_gettime(CLOCK_MONOTONIC) calls. I think something like Patch #3 is necessary to export TSC calibration data to user-space TSC readers. It is entirely up to the kernel developers whether they want to include patches #2 and #3, but I think something like Patch #1 really needs to get into a future Linux release, as an unecessary latency of 200-1000ns for a timer that can tick 3 times per nanosecond is unacceptable. Patches for kernels 3.10.0-21 and 4.9.65-rt23 (ARM) are attached to bug #198161. Thanks & Best Regards, Jason Vas Dias
[PATCH v4.16-rc5 (3)] x86/vdso: on Intel, VDSO should handle CLOCK_MONOTONIC_RAW
Currently the VDSO does not handle clock_gettime( CLOCK_MONOTONIC_RAW, &ts ) on Intel / AMD - it calls vdso_fallback_gettime() for this clock, which issues a syscall, having an unacceptably high latency (minimum measurable time or time between measurements) of 300-700ns on 2 2.8-3.9ghz Haswell x86_64 Family'_'Model : 06_3C machines under various versions of Linux. Sometimes, particularly when correlating elapsed time to performance counter values, user-space code needs to know elapsed time from the perspective of the CPU no matter how "hot" / fast or "cold" / slow it might be running wrt NTP / PTP "real" time; when code needs this, the latencies associated with a syscall are often unacceptably high. I reported this as Bug #198161 : 'https://bugzilla.kernel.org/show_bug.cgi?id=198961' and in previous posts with subjects matching 'CLOCK_MONOTONIC_RAW' . This patch handles CLOCK_MONOTONIC_RAW clock_gettime() in the VDSO , by exporting the raw clock calibration, last cycles, last xtime_nsec, and last raw_sec value in the vsyscall_gtod_data during vsyscall_update() . Now the new do_monotonic_raw() function in the vDSO has a latency of @ 24ns on average, and the test program: tools/testing/selftest/timers/inconsistency-check.c succeeds with arguments: '-c 4 -t 120' or any arbitrary -t value. The patch is against Linus' latest 4.16-rc5 tree, current HEAD of : git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git . This patch affects only files: arch/x86/include/asm/vgtod.h arch/x86/entry/vdso/vclock_gettime.c arch/x86/entry/vdso/vdso.lds.S arch/x86/entry/vdso/vdsox32.lds.S arch/x86/entry/vdso/vdso32/vdso32.lds.S arch/x86/entry/vsyscall/vsyscall_gtod.c There are 3 patches in the series : Patch #1 makes the VDSO handle clock_gettime(CLOCK_MONOTONIC_RAW) with rdtsc_ordered() Patch #2 makes the VDSO handle clock_gettime(CLOCK_MONOTONIC_RAW) with a new rdtscp() function in msr.h Patch #3 makes the VDSO export TSC calibration data via a new function in the vDSO: unsigned int __vdso_linux_tsc_calibration ( struct linux_tsc_calibration *tsc_cal ) that user code can optionally call. Patches #2 & #3 should be considered "optional" . Patch #2 makes clock_gettime(CLOCK_MONOTONIC_RAW) calls have @ half the latency of clock_gettime(CLOCK_MONOTONIC) calls. I think something like Patch #3 is necessary to export TSC calibration data to user-space TSC readers. Best Regards, Jason Vas Dias