09 Dec, 2013

10 commits

  • The posix cpu timers code makes a heavy use of BUG_ON()
    but none of these concern fatal issues that require
    to stop the machine. So let's just warn the user when
    some internal state slips out of our hands.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     
  • The remaining uses of tasklist_lock were mostly about synchronizing
    against sighand modifications, getting coherent and safe group samples
    and also thread/process wide timers list handling.

    All of this is already safely synchronizable with the target's
    sighand lock. Let's use it on these places instead.

    Also update the comments about locking.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     
  • Timer deletion doesn't need the tasklist lock.
    We need to protect against:

    * concurrent access to the lists p->cputime_expires and
    p->sighand->cputime_expires

    * task reaping that may also delete the timer list entry

    * timer firing

    We already hold the timer lock which protects us against concurrent
    timer firing.

    The rest only need the targets sighand to be locked.
    So hold it and drop the use of tasklist_lock there.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     
  • There is no need for the tasklist_lock just to take a process
    wide clock sample.

    All we need is to get a coherent sample that doesn't race with
    exit() and exec():

    * exit() may be concurrently reaping a task and flushing its time

    * sighand is unstable under exit() and exec(), and the latter also
    result in group leader that can change

    To protect against these, locking the target's sighand is enough.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     
  • Consolidate the clock sampling common code used for both local
    and remote targets.

    Note that this introduces a tiny user ABI change: if a
    PID is passed to clock_gettime() along the clockid,
    we used to forbid a process wide clock sample when that
    PID doesn't belong to a group leader. Now after this patch
    we allow process wide clock samples if that PID belongs to
    the current task, even if the current task is not the
    group leader.

    But local process wide clock samples are allowed if PID == 0
    (current task) even if the current task is not the group leader.
    So in the end this should be no big deal as this actually harmonize
    the behaviour when the remote sample is actually a local one.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     
  • a0b2062b0904ef07944c4a6e4d0f88ee44f1e9f2
    ("posix_timers: fix racy timer delta caching on task exit") forgot
    to remove the arguments used for timer caching.

    Fix this leftover.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     
  • Now that we've removed all the optimizations that could
    result in NULL timer's targets, we can remove all the
    associated special case handling.

    Also add some warnings on NULL targets to spot any possible
    leftover.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     
  • When a timer's target is seen to be buried, for example on calls
    to timer_gettime(), the posix cpu timers code behaves a bit
    like a garbage collector and releases early the reference to the
    task.

    Then again, this optimization complicates the code for no much
    value: it's up to the user to release the timer and its associated
    ressources by calling timer_delete() after it buries the target
    tasks.

    Remove this to simplify the code.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     
  • Now that we removed dead thread posix cpu timers caching,
    lets remove the dead process wide version. This caching
    is similar to the per thread version but it should be even
    more rare:

    * If the process id dead, we are not reading its timers
    status from a thread belonging to its group since they
    are all dead. So this caching only concern remote process
    timers reads. Now posix cpu timers using itimers or timer_settime()
    can't do remote process timers anyway so it's not even clear if there
    is actually a user for this caching.

    * Unlike per thread timers caching, this only applies to
    zombies targets. Buried targets' process wide timers return
    0 values. But then again, timer_gettime() can't read remote
    process timers, so if the process is dead, there can't be
    any reader left anyway.

    Then again this caching seem to complicate the code for
    corner cases that are probably not worth it. So lets get
    rid of it.

    Also remove the sample snapshot on dying process timer
    that is now useless, as suggested by Kosaki.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     
  • When a task is exiting or has exited, its posix cpu timers
    don't tick anymore and won't elapse further. It's too late
    for them to expire.

    So any further call to timer_gettime() on these timers will
    return the same remaining expiry time.

    The current code optimize this by caching the remaining delta
    and storing it where we use to save the absolute expiration time.
    This way, the future calls to timer_gettime() won't need to
    compute the difference between the absolute expiration time and
    the current time anymore.

    Now this optimization doesn't seem to bring much value. Computing
    the timer remaining delta is not very costly. Fetching the timer
    value OTOH can be costly in two ways:

    * CPUCLOCK_SCHED read requires to lock the target's rq. But some
    optimizations are on the way to make task_sched_runtime() not holding
    the rq lock of a non-running target.

    * CPUCLOCK_VIRT/CPUCLOCK_PROF read simply consist in fetching
    current->utime/current->stime except when the system uses full
    dynticks cputime accounting. The latter requires a per task lock
    in order to correctly compute user and system time. But once the
    target is dead, this lock shouldn't be contended anyway.

    All in one this caching doesn't seem to be justified.
    Given that it complicates the code significantly for
    few wins, let's remove it on single thread timers.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Kosaki Motohiro
    Cc: Andrew Morton

    Frederic Weisbecker
     

03 Dec, 2013

2 commits

  • A posix CPU timer can be rearmed while it is firing or after it is
    notified with a signal. This can happen for example with timers that
    were set with a non zero interval in timer_settime().

    This rearming can happen in two places:

    1) On timer firing time, which happens on the target's tick. If the timer
    can't trigger a signal because it is ignored, it reschedules itself
    to honour the timer interval.

    2) On signal handling from the timer's notification target. This one
    can be a different task than the timer's target itself. Once the
    signal is notified, the notification target rearms the timer, again
    to honour the timer interval.

    When a timer is rearmed, we need to notify the full dynticks CPUs
    such that they restart their tick in case they are running tasks that
    may have a share in elapsing this timer.

    Now the 1st case above handles full dynticks CPUs with a call to
    posix_cpu_timer_kick_nohz() from the posix cpu timer firing code. But
    the second case ignores the fact that some CPUs may run non-idle tasks
    with their tick off. As a result, when a timer is resheduled after its signal
    notification, the full dynticks CPUs may completely ignore it and not
    tick on the timer as expected

    This patch fixes this bug by handling both cases in one. All we need
    is to move the kick to the rearming common code in posix_cpu_timer_schedule().

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Steven Rostedt
    Cc: Olivier Langlois

    Frederic Weisbecker
     
  • After a posix cpu timer is set, a workqueue is scheduled in order to
    kick the full dynticks CPUs and let them restart their tick if
    necessary in case the task they are running is concerned by the
    new timer.

    This kick is implemented by way of IPIs, which require interrupts
    to be enabled, hence the need for a workqueue to raise them because
    the posix cpu timer set path has interrupts disabled.

    Now if there is no full dynticks CPU on the system, the workqueue is
    still scheduled but it simply won't send any IPI and return immediately.

    So lets spare that worqueue when it is not needed.

    Signed-off-by: Frederic Weisbecker
    Cc: Thomas Gleixner
    Cc: Ingo Molnar
    Cc: Peter Zijlstra
    Cc: Oleg Nesterov
    Cc: Steven Rostedt

    Frederic Weisbecker
     

03 Jul, 2013

5 commits

  • When a task exits, we perform a caching of the remaining cputime delta
    before expiring of its timers.

    This is done from the following places:

    * When the task is reaped. We iterate through its list of
    posix cpu timers and store the remaining timer delta to
    the timer struct instead of the absolute value.
    (See posix_cpu_timers_exit() / posix_cpu_timers_exit_group() )

    * When we call posix_cpu_timer_get() or posix_cpu_timer_schedule().
    If the timer's task is considered dying when watched from these
    places, the same conversion from absolute to relative expiry time
    is performed. Then the given task's reference is released.
    (See clear_dead_task() ).

    The relevance of this caching is questionable but this is another
    and deeper debate.

    The big issue here is that these two sources of caching don't mix
    up very well together.

    More specifically, the caching can easily be done twice, resulting
    in a wrong delta as it gets spuriously substracted a second time by
    the elapsed clock. This can happen in the following scenario:

    1) The task exits and gets reaped: we call posix_cpu_timers_exit()
    and the absolute timer expiry values are converted to a relative
    delta.

    2) timer_gettime() -> posix_cpu_timer_get() is called and relies on
    clear_dead_task() because tsk->exit_state == EXIT_DEAD.
    The delta gets substracted again by the elapsed clock and we return
    a wrong result.

    To fix this, just remove the caching done on task reaping time. It
    doesn't bring much value on its own. The caching done from
    posix_cpu_timer_get/schedule is enough.

    And it would also be hard to get it really right: we could make it put and
    clear the target task in the timer struct so that readers know if they are
    dealing with a relative cached of absolute value. But it would be racy.
    The only safe way to do it would be to lock the itimer->it_lock so that we
    know nobody reads the cputime expiry value while we modify it and its
    target task reference. Doing so would involve some funny workarounds to
    avoid circular lock against the sighand lock. There is just no reason to
    maintain this.

    The user visible effect of this patch can be observed by running the
    following code: it creates a subthread that launches a posix cputimer
    which expires after 10 seconds. But then the subthread only busy loops for 2
    seconds and exits. The parent reaps the subthread and read the timer value.
    Its expected value should the be the initial timer's expiration value
    minus the cputime elapsed in the subthread. Roughly 10 - 2 = 8 seconds:

    #include
    #include
    #include
    #include
    #include

    static timer_t id;
    static struct itimerspec val = { .it_value.tv_sec = 10, }, new;

    static void *thread(void *unused)
    {
    int err;
    struct timeval start, end, diff;

    timer_create(CLOCK_THREAD_CPUTIME_ID, NULL, &id);
    if (err < 0) {
    perror("Can't create timer\n");
    return NULL;
    }

    /* Arm 10 sec timer */
    err = timer_settime(id, 0, &val, NULL);
    if (err < 0) {
    perror("Can't set timer\n");
    return NULL;
    }

    /* Exit after 2 seconds of execution */
    gettimeofday(&start, NULL);
    do {
    gettimeofday(&end, NULL);
    timersub(&end, &start, &diff);
    } while (diff.tv_sec < 2);

    return NULL;
    }

    int main(int argc, char **argv)
    {
    pthread_t pthread;
    int err;

    err = pthread_create(&pthread, NULL, thread, NULL);
    if (err) {
    perror("Can't create thread\n");
    return -1;
    }
    pthread_join(pthread, NULL);
    /* Just wait a little bit to make sure the child got reaped */
    sleep(1);
    err = timer_gettime(id, &new);
    if (err)
    perror("Can't get timer value\n");
    printf("%d %ld\n", new.it_value.tv_sec, new.it_value.tv_nsec);

    return 0;
    }

    Before the patch:

    $ ./posix_cpu_timers
    6 2278074

    After the patch:

    $ ./posix_cpu_timers
    8 1158766

    Before the patch, the elapsed time got two more seconds spuriously accounted.

    Signed-off-by: Frederic Weisbecker
    Cc: Stanislaw Gruszka
    Cc: Thomas Gleixner
    Cc: Peter Zijlstra
    Cc: Ingo Molnar
    Cc: Oleg Nesterov
    Cc: KOSAKI Motohiro
    Cc: Olivier Langlois
    Signed-off-by: Andrew Morton

    Frederic Weisbecker
     
  • In order to re-arm a timer after it fired, we take a sample of the current
    process or thread cputime.

    If the task is dying though, we don't arm anything but we cache the
    remaining timer expiration delta for further reads.

    Something similar is performed in posix_cpu_timer_get() but here we forget
    to take the process wide cputime sample before caching it.

    As a result we are storing random stack content, leading every further
    reads of that timer to return junk values.

    Fix this by taking the appropriate sample in the case of process wide
    timers.

    This probably doesn't matter much in practice because, at this stage, the
    thread is the last one in the group and we reached exit_notify(). This
    implies that we called exit_itimers() and there should be no more timers
    to handle for that task.

    So this is likely dead code anyway but let's fix the current logic
    and the warning that came along:

    kernel/posix-cpu-timers.c: In function 'posix_cpu_timer_schedule':
    kernel/posix-cpu-timers.c:1127: warning: 'now' may be used uninitialized in this function

    Then we can start to think further about cleaning up that code.

    Reported-by: Andrew Morton
    Reported-by: Chen Gang
    Signed-off-by: Frederic Weisbecker
    Cc: Stanislaw Gruszka
    Cc: Thomas Gleixner
    Cc: Peter Zijlstra
    Cc: Ingo Molnar
    Cc: Oleg Nesterov
    Cc: Chen Gang
    Cc: KOSAKI Motohiro
    Cc: Olivier Langlois
    Signed-off-by: Andrew Morton

    Frederic Weisbecker
     
  • Consolidate the common code amongst per thread and per process timers list
    on tick time.

    List traversal, expiry check and subsequent updates can be shared in a
    common helper.

    Signed-off-by: Frederic Weisbecker
    Cc: Stanislaw Gruszka
    Cc: Thomas Gleixner
    Cc: Peter Zijlstra
    Cc: Ingo Molnar
    Cc: Oleg Nesterov
    Cc: KOSAKI Motohiro
    Cc: Olivier Langlois
    Signed-off-by: Andrew Morton

    Frederic Weisbecker
     
  • Cleaning up the posix cpu timers on task exit shares some common code
    among timer list types, most notably the list traversal and expiry time
    update.

    Unify this in a common helper.

    Signed-off-by: Frederic Weisbecker
    Cc: Stanislaw Gruszka
    Cc: Thomas Gleixner
    Cc: Peter Zijlstra
    Cc: Ingo Molnar
    Cc: Oleg Nesterov
    Cc: KOSAKI Motohiro
    Cc: Olivier Langlois
    Signed-off-by: Andrew Morton

    Frederic Weisbecker
     
  • The posix cpu timer expiry time is stored in a union of two types: a 64
    bits field if we rely on scheduler precise accounting, or a cputime_t if
    we rely on jiffies.

    This results in quite some duplicate code and special cases to handle the
    two types.

    Just unify this into a single 64 bits field. cputime_t can always fit
    into it.

    Signed-off-by: Frederic Weisbecker
    Cc: Stanislaw Gruszka
    Cc: Thomas Gleixner
    Cc: Peter Zijlstra
    Cc: Ingo Molnar
    Cc: Oleg Nesterov
    Cc: KOSAKI Motohiro
    Cc: Olivier Langlois
    Signed-off-by: Andrew Morton

    Frederic Weisbecker
     

23 Apr, 2013

1 commit

  • The test that checks if a CPU can stop its tick from posix CPU
    timers angle was mistakenly inverted.

    What we want is to prevent the tick from being stopped as long
    as the current CPU's task runs a posix CPU timer.

    Fix this.

    Signed-off-by: Frederic Weisbecker
    Cc: Chris Metcalf
    Cc: Christoph Lameter
    Cc: Geoff Levand
    Cc: Gilad Ben Yossef
    Cc: Hakan Akkan
    Cc: Ingo Molnar
    Cc: Kevin Hilman
    Cc: Li Zhong
    Cc: Oleg Nesterov
    Cc: Paul E. McKenney
    Cc: Paul Gortmaker
    Cc: Peter Zijlstra
    Cc: Steven Rostedt
    Cc: Thomas Gleixner

    Frederic Weisbecker
     

19 Apr, 2013

2 commits

  • Bring a new helper that the full dynticks infrastructure can
    call in order to know if it can safely stop the tick from
    the posix cpu timers subsystem point of view.

    Signed-off-by: Frederic Weisbecker
    Cc: Chris Metcalf
    Cc: Christoph Lameter
    Cc: Geoff Levand
    Cc: Gilad Ben Yossef
    Cc: Hakan Akkan
    Cc: Ingo Molnar
    Cc: Kevin Hilman
    Cc: Li Zhong
    Cc: Oleg Nesterov
    Cc: Paul E. McKenney
    Cc: Paul Gortmaker
    Cc: Peter Zijlstra
    Cc: Steven Rostedt
    Cc: Thomas Gleixner

    Frederic Weisbecker
     
  • Kick the full dynticks CPUs when a posix cpu timer is enqueued by
    way of a standard call to posix_cpu_timer_set() or set_process_cpu_timer().
    This also include rescheduled firing timers.

    This way they can re-evaluate the state of (and possibly restart) their
    tick against the new expiry modification.

    Signed-off-by: Frederic Weisbecker
    Cc: Chris Metcalf
    Cc: Christoph Lameter
    Cc: Geoff Levand
    Cc: Gilad Ben Yossef
    Cc: Hakan Akkan
    Cc: Ingo Molnar
    Cc: Kevin Hilman
    Cc: Li Zhong
    Cc: Oleg Nesterov
    Cc: Paul E. McKenney
    Cc: Paul Gortmaker
    Cc: Peter Zijlstra
    Cc: Steven Rostedt
    Cc: Thomas Gleixner

    Frederic Weisbecker
     

20 Feb, 2013

1 commit

  • Pull timer changes from Ingo Molnar:
    "Main changes:

    - ntp: Add CONFIG_RTC_SYSTOHC: a generic RTC driver facility
    complementing the existing CONFIG_RTC_HCTOSYS, which uses NTP to
    keep the hardware clock updated.

    - posix-timers: Fix clock_adjtime to always return timex data on
    success. This is changing the ABI, but no breakage was expected
    and found - caution is warranted nevertheless.

    - platform persistent clock improvements/cleanups.

    - clockevents: refactor timer broadcast handling to be more generic
    and less duplicated with matching architecture code (mostly ARM
    motivated.)

    - various fixes and cleanups"

    * 'timers-core-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip:
    timers/x86/hpet: Use HPET_COUNTER to specify the hpet counter in vread_hpet()
    posix-cpu-timers: Fix nanosleep task_struct leak
    clockevents: Fix generic broadcast for FEAT_C3STOP
    time, Fix setting of hardware clock in NTP code
    hrtimer: Prevent hrtimer_enqueue_reprogram race
    clockevents: Add generic timer broadcast function
    clockevents: Add generic timer broadcast receiver
    timekeeping: Switch HAS_PERSISTENT_CLOCK to ALWAYS_USE_PERSISTENT_CLOCK
    x86/time/rtc: Don't print extended CMOS year when reading RTC
    x86: Select HAS_PERSISTENT_CLOCK on x86
    timekeeping: Add CONFIG_HAS_PERSISTENT_CLOCK option
    rtc: Skip the suspend/resume handling if persistent clock exist
    timekeeping: Add persistent_clock_exist flag
    posix-timers: Fix clock_adjtime to always return timex data on success
    Round the calculated scale factor in set_cyc2ns_scale()
    NTP: Add a CONFIG_RTC_SYSTOHC configuration
    MAINTAINERS: Update John Stultz's email
    time: create __getnstimeofday for WARNless calls

    Linus Torvalds
     

15 Feb, 2013

1 commit

  • The trinity fuzzer triggered a task_struct reference leak via
    clock_nanosleep with CPU_TIMERs. do_cpu_nanosleep() calls
    posic_cpu_timer_create(), but misses a corresponding
    posix_cpu_timer_del() which leads to the task_struct reference leak.

    Reported-and-tested-by: Tommi Rantala
    Signed-off-by: Stanislaw Gruszka
    Cc: Dave Jones
    Cc: John Stultz
    Cc: Oleg Nesterov
    Cc: stable@vger.kernel.org
    Link: http://lkml.kernel.org/r/20130215100810.GF4392@redhat.com
    Signed-off-by: Thomas Gleixner

    Stanislaw Gruszka
     

28 Jan, 2013

1 commit

  • This is in preparation for the full dynticks feature. While
    remotely reading the cputime of a task running in a full
    dynticks CPU, we'll need to do some extra-computation. This
    way we can account the time it spent tickless in userspace
    since its last cputime snapshot.

    Signed-off-by: Frederic Weisbecker
    Cc: Andrew Morton
    Cc: Ingo Molnar
    Cc: Li Zhong
    Cc: Namhyung Kim
    Cc: Paul E. McKenney
    Cc: Paul Gortmaker
    Cc: Peter Zijlstra
    Cc: Steven Rostedt
    Cc: Thomas Gleixner

    Frederic Weisbecker
     

20 Dec, 2012

1 commit


17 Dec, 2012

1 commit


29 Nov, 2012

1 commit


15 Dec, 2011

1 commit


26 Oct, 2011

1 commit

  • * 'core-locking-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip: (27 commits)
    rtmutex: Add missing rcu_read_unlock() in debug_rt_mutex_print_deadlock()
    lockdep: Comment all warnings
    lib: atomic64: Change the type of local lock to raw_spinlock_t
    locking, lib/atomic64: Annotate atomic64_lock::lock as raw
    locking, x86, iommu: Annotate qi->q_lock as raw
    locking, x86, iommu: Annotate irq_2_ir_lock as raw
    locking, x86, iommu: Annotate iommu->register_lock as raw
    locking, dma, ipu: Annotate bank_lock as raw
    locking, ARM: Annotate low level hw locks as raw
    locking, drivers/dca: Annotate dca_lock as raw
    locking, powerpc: Annotate uic->lock as raw
    locking, x86: mce: Annotate cmci_discover_lock as raw
    locking, ACPI: Annotate c3_lock as raw
    locking, oprofile: Annotate oprofilefs lock as raw
    locking, video: Annotate vga console lock as raw
    locking, latencytop: Annotate latency_lock as raw
    locking, timer_stats: Annotate table_lock as raw
    locking, rwsem: Annotate inner lock as raw
    locking, semaphores: Annotate inner lock as raw
    locking, sched: Annotate thread_group_cputimer as raw
    ...

    Fix up conflicts in kernel/posix-cpu-timers.c manually: making
    cputimer->cputime a raw lock conflicted with the ABBA fix in commit
    bcd5cff7216f ("cputimer: Cure lock inversion").

    Linus Torvalds
     

18 Oct, 2011

1 commit

  • There's a lock inversion between the cputimer->lock and rq->lock;
    notably the two callchains involved are:

    update_rlimit_cpu()
    sighand->siglock
    set_process_cpu_timer()
    cpu_timer_sample_group()
    thread_group_cputimer()
    cputimer->lock
    thread_group_cputime()
    task_sched_runtime()
    ->pi_lock
    rq->lock

    scheduler_tick()
    rq->lock
    task_tick_fair()
    update_curr()
    account_group_exec()
    cputimer->lock

    Where the first one is enabling a CLOCK_PROCESS_CPUTIME_ID timer, and
    the second one is keeping up-to-date.

    This problem was introduced by e8abccb7193 ("posix-cpu-timers: Cure
    SMP accounting oddities").

    Cure the problem by removing the cputimer->lock and rq->lock nesting,
    this leaves concurrent enablers doing duplicate work, but the time
    wasted should be on the same order otherwise wasted spinning on the
    lock and the greater-than assignment filter should ensure we preserve
    monotonicity.

    Reported-by: Dave Jones
    Reported-by: Simon Kirby
    Signed-off-by: Peter Zijlstra
    Cc: stable@kernel.org
    Cc: Linus Torvalds
    Cc: Martin Schwidefsky
    Link: http://lkml.kernel.org/r/1318928713.21167.4.camel@twins
    Signed-off-by: Thomas Gleixner

    Peter Zijlstra
     

30 Sep, 2011

1 commit

  • David reported:

    Attached below is a watered-down version of rt/tst-cpuclock2.c from
    GLIBC. Just build it with "gcc -o test test.c -lpthread -lrt" or
    similar.

    Run it several times, and you will see cases where the main thread
    will measure a process clock difference before and after the nanosleep
    which is smaller than the cpu-burner thread's individual thread clock
    difference. This doesn't make any sense since the cpu-burner thread
    is part of the top-level process's thread group.

    I've reproduced this on both x86-64 and sparc64 (using both 32-bit and
    64-bit binaries).

    For example:

    [davem@boricha build-x86_64-linux]$ ./test
    process: before(0.001221967) after(0.498624371) diff(497402404)
    thread: before(0.000081692) after(0.498316431) diff(498234739)
    self: before(0.001223521) after(0.001240219) diff(16698)
    [davem@boricha build-x86_64-linux]$

    The diff of 'process' should always be >= the diff of 'thread'.

    I make sure to wrap the 'thread' clock measurements the most tightly
    around the nanosleep() call, and that the 'process' clock measurements
    are the outer-most ones.

    ---
    #include
    #include
    #include
    #include
    #include
    #include
    #include
    #include

    static pthread_barrier_t barrier;

    static void *chew_cpu(void *arg)
    {
    pthread_barrier_wait(&barrier);
    while (1)
    __asm__ __volatile__("" : : : "memory");
    return NULL;
    }

    int main(void)
    {
    clockid_t process_clock, my_thread_clock, th_clock;
    struct timespec process_before, process_after;
    struct timespec me_before, me_after;
    struct timespec th_before, th_after;
    struct timespec sleeptime;
    unsigned long diff;
    pthread_t th;
    int err;

    err = clock_getcpuclockid(0, &process_clock);
    if (err)
    return 1;

    err = pthread_getcpuclockid(pthread_self(), &my_thread_clock);
    if (err)
    return 1;

    pthread_barrier_init(&barrier, NULL, 2);
    err = pthread_create(&th, NULL, chew_cpu, NULL);
    if (err)
    return 1;

    err = pthread_getcpuclockid(th, &th_clock);
    if (err)
    return 1;

    pthread_barrier_wait(&barrier);

    err = clock_gettime(process_clock, &process_before);
    if (err)
    return 1;

    err = clock_gettime(my_thread_clock, &me_before);
    if (err)
    return 1;

    err = clock_gettime(th_clock, &th_before);
    if (err)
    return 1;

    sleeptime.tv_sec = 0;
    sleeptime.tv_nsec = 500000000;
    nanosleep(&sleeptime, NULL);

    err = clock_gettime(th_clock, &th_after);
    if (err)
    return 1;

    err = clock_gettime(my_thread_clock, &me_after);
    if (err)
    return 1;

    err = clock_gettime(process_clock, &process_after);
    if (err)
    return 1;

    diff = process_after.tv_nsec - process_before.tv_nsec;
    printf("process: before(%lu.%.9lu) after(%lu.%.9lu) diff(%lu)\n",
    process_before.tv_sec, process_before.tv_nsec,
    process_after.tv_sec, process_after.tv_nsec, diff);
    diff = th_after.tv_nsec - th_before.tv_nsec;
    printf("thread: before(%lu.%.9lu) after(%lu.%.9lu) diff(%lu)\n",
    th_before.tv_sec, th_before.tv_nsec,
    th_after.tv_sec, th_after.tv_nsec, diff);
    diff = me_after.tv_nsec - me_before.tv_nsec;
    printf("self: before(%lu.%.9lu) after(%lu.%.9lu) diff(%lu)\n",
    me_before.tv_sec, me_before.tv_nsec,
    me_after.tv_sec, me_after.tv_nsec, diff);

    return 0;
    }

    This is due to us using p->se.sum_exec_runtime in
    thread_group_cputime() where we iterate the thread group and sum all
    data. This does not take time since the last schedule operation (tick
    or otherwise) into account. We can cure this by using
    task_sched_runtime() at the cost of having to take locks.

    This also means we can (and must) do away with
    thread_group_sched_runtime() since the modified thread_group_cputime()
    is now more accurate and would deadlock when called from
    thread_group_sched_runtime().

    Aside of that it makes the function safe on 32 bit systems. The old
    code added t->se.sum_exec_runtime unprotected. sum_exec_runtime is a
    64bit value and could be changed on another cpu at the same time.

    Reported-by: David Miller
    Signed-off-by: Peter Zijlstra
    Cc: stable@kernel.org
    Link: http://lkml.kernel.org/r/1314874459.7945.22.camel@twins
    Tested-by: David Miller
    Signed-off-by: Thomas Gleixner

    Peter Zijlstra
     

13 Sep, 2011

1 commit

  • The thread_group_cputimer lock can be taken in atomic context and therefore
    cannot be preempted on -rt - annotate it.

    In mainline this change documents the low level nature of
    the lock - otherwise there's no functional difference. Lockdep
    and Sparse checking will work as usual.

    Signed-off-by: Thomas Gleixner
    Signed-off-by: Ingo Molnar

    Thomas Gleixner
     

23 May, 2011

1 commit


31 Mar, 2011

1 commit


02 Feb, 2011

7 commits