lkml.org 
[lkml]   [2014]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC 0/7] hrtimer: drop active hrtimer checks after adding it
    From
    Hi Thomas,

    On 10 July 2014 07:04, Frederic Weisbecker <fweisbec@gmail.com> wrote:
    > On Wed, Jul 09, 2014 at 11:30:41PM +0200, Thomas Gleixner wrote:
    >> On Wed, 9 Jul 2014, Viresh Kumar wrote:
    >>
    >> So your patch series drops active hrtimer checks after adding it,
    >> according to your subject line.
    >>
    >> Quite useeul to drop something after adding it, right?

    I meant "hrtimer" by "it". Will fix it in case this patchset is still required.

    >> > As hrtimer_start*() never fails, hrtimer_active() is guaranteed to return '1'.
    >> > So, there is no point calling hrtimer_active().
    >>
    >> Wrong as usual.

    I cross-checked this with Frederic and Preeti before reaching out to
    you, to make sure its not 'obviously stupid'. And still couldn't get it
    right. :(

    >> It's a common pattern that short timeouts are given which lead to
    >> immediate expiry so the extra round through schedule is even more
    >> pointless than the extra check.

    Just wanted to confirm it again, you are talking about CPU being
    interrupted by clockevent device's interrupt right after hrtimer_start*()
    returns and before calling hrtimer_active()?

    > It may be a common pattern but it's not obvious at all as is in the code
    > except for timers gurus.
    >
    > It looks like error handling while it's actually an optimization.
    >
    > Also what about this pattern when it's used in interrupt or interrupt-disabled code?
    > In this case the handler is not going to fire right away, unless it's enqueued
    > on another CPU for unpinned timers.
    >
    > For example this code in tick_nohz_stop_sched_tick():
    >
    > hrtimer_start(&ts->sched_timer, expires, HRTIMER_MODE_ABS_PINNED);
    > /* Check, if the timer was already in the past */
    > if (hrtimer_active(&ts->sched_timer))
    > goto out;
    >
    > It's not clear what this is handling. Concurrent immediate callback expiration from another CPU?
    > But the timer is pinned local so it can't execute right away between hrtimer_start() and hrtimer_active()
    > check...

    Actually I was concerned about other cases as well.

    - Timeouts

    I do agree that an extra check is better than an extra round of schedule().
    But this is already achieved without calling hrtimer_active(), isn't it?

    All these timeout hrtimers have hrtimer_wakeup() as there handler (as
    these are initialized with: hrtimer_init_sleeper()).

    And on expiration hrtimer_wakeup() does this: t->task = NULL;

    So would this extra call to hrtimer_active() make any difference?

    - Process-context: sched changes

    I am not sure if scheduler routines: start_bandwidth_timer() and
    start_dl_timer() would get called *only* with interrupts disabled.

    But, it doesn't look obvious that the optimization Thomas mentioned
    earlier is relevant here as well. These might be added here for error
    checking.

    I might be wrong here as I don't have any understanding of this code
    and so sorry in advance.


    Note: My tree is monitored by kbuild-bot and these changes are under
    testing for over a week now. And I haven't received any reports of the
    WARN() firing in __hrtimer_start_range_ns().. Probably these short
    timeouts aren't getting hit at all by bot's tests.

    --
    viresh


    \
     
     \ /
      Last update: 2014-07-14 07:21    [W:5.886 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site