[lkml]   [2011]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patch in this message
    SubjectRe: [PATCH 2/2] Add a thread cpu time implementation to vDSO
    On 12/12/11 12:15 PM, Andrew Lutomirski wrote:

    > This seems like a neat idea, but I'm a little worried about the
    > implementation. Surely someone with 4k+ cpus will complain when the
    > percpu vvar data exceeds a page and crashes. I have no personal
    > objection to a dynamically-sized vvar area, but it might need more
    > careful handling.

    I'm for disabling the vsyscall when the data doesn't fit in a couple of

    > I also wonder if there a problem with information leaking. If there
    > was an entire per-cpu vvar page (perhaps mapped differently on each
    > cpu) then nothing interesting would leak. But that could add
    > overhead.

    The timing based attacks depend on the granularity of timestamps. I feel
    what's available here is too coarse grained to be useful. Happy to
    disable the code at compile time for those cases. Are there
    CONFIG_HIGH_SECURITY type of options I could use for this purpose?

    >> diff --git a/arch/x86/include/asm/vvar.h b/arch/x86/include/asm/vvar.h
    >> index 0fd7a4a..e36e1c1 100644
    >> --- a/arch/x86/include/asm/vvar.h
    >> +++ b/arch/x86/include/asm/vvar.h
    >> @@ -47,5 +47,6 @@
    >> DECLARE_VVAR(0, volatile unsigned long, jiffies)
    >> DECLARE_VVAR(16, int, vgetcpu_mode)
    >> DECLARE_VVAR(128, struct vsyscall_gtod_data, vsyscall_gtod_data)
    >> +DECLARE_VVAR(2048, struct vcpu_data, vcpu_data)
    > Any reason this isn't page-aligned? Offset 2048 seems like an odd place.

    I meant it to be 128 + sizeof(struct vsyscall_gtod_data) so we fit
    everything in one page if CONFIG_NR_CPUS is small enough.

    I'll fix it up in the next rev.

    >> notrace static long vdso_fallback_gettime(long clock, struct timespec *ts)
    >> {
    >> long ret;
    >> - asm("syscall" : "=a" (ret) :
    >> - "0" (__NR_clock_gettime),"D" (clock), "S" (ts) : "memory");
    >> + asm volatile("syscall" : "=a" (ret) :
    >> + "0" (__NR_clock_gettime),"D" (clock), "S" (ts) : "memory");
    >> return ret;
    >> }
    > Huh? This should probably be a separate patch (and probably not a
    > -stable candidate, since it would take amazing compiler stupidity to
    > generate anything other than the obvious code). The memory clobber
    > may also be enough to make this officially safe.

    Yes - this should be a separate patch. gcc-4.4 likes to get rid of the
    instruction in __do_thread_cpu_time without the asm volatile (in spite
    of the memory clobber).

    >> + if (vp->tsc_unstable) {
    >> + struct timespec ts;
    >> + vdso_fallback_gettime(CLOCK_THREAD_CPUTIME_ID,&ts);
    >> + return timespec_to_ns(&ts);
    >> + }
    > Yuck -- another flag indicating whether we're using the tsc.

    I renamed it to thread_cputime_disabled to deal with NR_CPUS > 64.

    >> +
    >> + do {
    >> + native_read_tscp(&p);
    > Do all 64-bit cpus have rdtscp? ISTR older Intel machines don't have it.

    Yeah - I ran into this one when testing with KVM on a rdtscp capable
    CPU. Will disable the vsyscall when the CPU doesn't support rdtscp.

    >> --- a/arch/x86/vdso/
    >> +++ b/arch/x86/vdso/
    >> @@ -25,6 +25,8 @@ VERSION {
    >> __vdso_getcpu;
    >> time;
    >> __vdso_time;
    >> + thread_cpu_time;
    >> + __vdso_thread_cpu_time;
    >> local: *;
    >> };
    >> }
    > Why do we have the non-__vdso versions? IMO anything that actually
    > uses them is likely to be confused. They have the same names as the
    > glibc wrappers, but the glibc wrappers have different return value and
    > errno semantics. I'd say just use __vdso.

    I'm not familiar with the history of __vdso_foo vs foo. Happy to get rid
    of the non __vdso versions.

    > Also, what's wrong with just adding the functionality to __vdso_clock_gettime?

    Performance. Most of this is client dependent (whether it expects time
    in nanoseconds or timespec).

    Baseline: 19.34 secs
    vclock_gettime: 4.74 secs
    thread_cpu_time: 3.62 secs

    Our use case prefers nanoseconds, so the conversion to timespec and back
    adds overhead. Also, having a direct entry point vs going through the
    switch in clock_gettime() adds some cycles.

    I have some people here asking me if they could get CLOCK_REALTIME and
    CLOCK_MONOTONIC also in nanoseconds for the same reason.

    >> +struct vcpu_data {
    >> + struct vpercpu_data vpercpu[NR_CPUS];
    >> + unsigned int tsc_khz;
    > I think this also duplicates some of the gtod_data stuff, at least in
    > the case that TSC works for gtod.

    vgotd.h seems to be clock source independent. This vsyscall is usable
    only on systems with tsc_unstable == 0. Also, there is only one instance
    of mult and shift there, whereas we save it on a per-cpu basis in the
    VVAR page.

    Thanks for the review and comments!


     \ /
      Last update: 2011-12-12 23:53    [W:0.029 / U:31.928 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site