lkml.org 
[lkml]   [2009]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch for 2.6.32? 1/3] hrtimers: remove the "timer_stats_active" check when setting the start info
    On Tue, 17 Nov 2009, akpm@linux-foundation.org wrote:

    Back to LKML with some more cc's.

    > From: Feng Tang <feng.tang@intel.com>
    >
    > Recent hrtimer code will set the start info to a hrtimer only when that
    > flag is set, then the start info of all hrtimers will always be
    > uninitialised before a "echo 1 > /proc/timer_stats", thus the
    > /proc/timer_lists will have something like:
    >
    > active timers:
    > #0: <c27d46b0>, tick_sched_timer, S:01, <(null)>, /-1
    > # expires at 91062000000-91062000000 nsecs [in 156071 to 156071 nsecs]
    > #1: <efb81b6c>, hrtimer_wakeup, S:01, <(null)>, /-1
    > # expires at 91062300331-91062350331 nsecs [in 456402 to 506402 nsecs]
    > #2: <efac9b6c>, hrtimer_wakeup, S:01, <(null)>, /-1
    > # expires at 91068699811-91068749811 nsecs [in 6855882 to 6905882 nsecs]
    > #3: <efacdb6c>, hrtimer_wakeup, S:01, <(null)>, /-1
    > # expires at 91068755511-91068805511 nsecs [in 6911582 to 6961582 nsecs]
    > #4: <efa95b6c>, hrtimer_wakeup, S:01, <(null)>, /-1
    > # expires at 91068806066-91068856066 nsecs [in 6962137 to 7012137 nsecs]
    > .....
    >
    > This patch fixes it.

    This patch does more than that and it does not mention it in the
    change log. The change log is also missing the context which
    introduced this regression.

    Just to get the context straight:

    commit 507e1231 (timer stats: Optimize by adding quick check to avoid
    function calls) commit 507e1231 added the timer_stats_active check in
    timer_stats_hrtimer_set_start_info() to reduce the overhead when timer
    stats are inactive.

    As an unintentional side effect it introduced the above regression in
    /proc/timer_list.

    > static inline void timer_stats_hrtimer_set_start_info(struct hrtimer *timer)
    > {
    > - if (likely(!timer_stats_active))
    > - return;

    This is fine. It reverts the hrtimer part of commit 507e1231 and fixes
    the /proc/timer_list regression for the price of the same overhead
    which we had before that commit.

    That's the immediate fix which needs to go to Linus and stable.

    > __timer_stats_hrtimer_set_start_info(timer, __builtin_return_address(0));
    > }
    >
    > diff -puN kernel/hrtimer.c~hrtimers-remove-the-timer_stats_active-check-when-setting-the-start-info kernel/hrtimer.c
    > --- a/kernel/hrtimer.c~hrtimers-remove-the-timer_stats_active-check-when-setting-the-start-info
    > +++ a/kernel/hrtimer.c
    > @@ -750,7 +750,7 @@ static inline void hrtimer_init_timer_hr
    > #ifdef CONFIG_TIMER_STATS
    > void __timer_stats_hrtimer_set_start_info(struct hrtimer *timer, void *addr)
    > {
    > - if (timer->start_site)
    > + if (timer->start_site == addr && timer->start_pid == current->pid)

    This part is unrelated to the above regression and is wrong for the
    following reasons:

    1) it runs the memcpy when the start_site changes even when the pid
    is the same, which is extremly bad for tick->sched_timer as this
    timer is re(armed) frequently from different places when NOHZ=y.

    2) it runs the memcpy when the pid changes which is even worse for
    tick->sched_timer as this timer is re(armed) frequently from
    hrtimer_interrupt which hits random pids and would therefor
    report completely wrong pid/comm info.

    This needs more thought and is definitely neither -rc7 nor stable
    material.

    Thanks,

    tglx


    \
     
     \ /
      Last update: 2009-11-18 20:33    [W:2.962 / U:0.208 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site