Re: [PATCH v2 3/3] X86: Add a thread cpu time implementation to vDSO

[Shaohua Li <shli@fb.com>]

On Thu, Dec 18, 2014 at 04:22:59PM -0800, Andy Lutomirski wrote:
> On Thu, Dec 18, 2014 at 3:30 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Wed, Dec 17, 2014 at 3:12 PM, Shaohua Li <shli@fb.com> wrote:
> >> This primarily speeds up clock_gettime(CLOCK_THREAD_CPUTIME_ID, ..). We
> >> use the following method to compute the thread cpu time:
> >>
> >>     t0 = process start
> >>     t1 = most recent context switch time
> >>     t2 = time at which the vsyscall is invoked
> >>
> >>     thread_cpu_time = sum(time slices between t0 to t1) + (t2 - t1)
> >>                 = current->se.sum_exec_runtime + now - sched_clock()
> >>
> >> At context switch time We stash away
> >>
> >>     adj_sched_time = sum_exec_runtime - sched_clock()
> >>
> >> in a per-cpu struct in the VVAR page and then compute
> >>
> >>     thread_cpu_time = adj_sched_time + now
> >>
> >> All computations are done in nanosecs on systems where TSC is stable. If
> >> TSC is unstable, we fallback to a regular syscall.
> >>     Benchmark data:
> >>
> >>     for (i = 0; i < 100000000; i++) {
> >>             clock_gettime(CLOCK_THREAD_CPUTIME_ID, &ts);
> >>             sum += ts.tv_sec * NSECS_PER_SEC + ts.tv_nsec;
> >>     }
> >
> > A bunch of the time spent processing a CLOCK_THREAD_CPUTIME_ID syscall
> > is spent taking various locks, and I think it could be worth adding a
> > fast path for the read-my-own-clock case in which we just disable
> > preemption and read the thing without any locks.
> >
> > If we're actually going to go the vdso route, I'd like to make the
> > scheduler hooks clean.  Peterz and/or John, what's the right way to
> > get an arch-specific callback with sum_exec_runtime and an up to date
> > sched_clock value during a context switch?  I'd much rather not add
> > yet another rdtsc instruction to the scheduler.
> 
> Bad news: this patch is incorrect, I think.  Take a look at
> update_rq_clock -- it does fancy things involving irq time and
> paravirt steal time.  So this patch could result in extremely
> non-monotonic results.

Yes, it's not precise. But bear in mind, CONFIG_IRQ_TIME_ACCOUNTING is a
optional feature. Actually it's added not long time ago. I thought it's
acceptable the time isn't precise just like what we have before the
feature is added.

Thanks,
Shaohua