lkml.org 
[lkml]   [2010]   [Dec]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] fix unsafe operation in high resolution timer
    From
    On Sat, Dec 25, 2010 at 10:12 AM, Yong Zhang <yong.zhang0@gmail.com> wrote:
    > On Fri, Dec 24, 2010 at 10:28:52PM +0800, Hillf Danton wrote:
    >> On Fri, Dec 24, 2010 at 3:17 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
    >> > On Thu, Dec 23, 2010 at 9:29 PM, Hillf Danton <dhillf@gmail.com> wrote:
    >> >> After calling the callback function of hrtimer, the timer could become
    >> >> unreliable in corner cases where the timer will no longer be queued
    >> >> and the mm segment, in which the timer is embedded, could be reclaimed
    >> >> in the callback.
    >> >>
    >> >> The unreliability is fixed by checking the result of callback before
    >> >> operating the timer again.
    >> >
    >> > Though the patch is buggy. But it actually explores a real problem.
    >> >
    >>
    >> Would you please finger out why the patch is buggy?
    >
    > No problem.

    thanks, Yong, for your explanation, and merry Christmas.

    >
    > Actually the patch change the behavior of current hrtimer.
    > See comments below :)
    >
    >> >> ---
    >> >>
    >> >> --- a/kernel/hrtimer.c  2010-11-01 19:54:12.000000000 +0800
    >> >> +++ b/kernel/hrtimer.c  2010-12-23 21:17:02.000000000 +0800
    >> >> @@ -1225,6 +1225,7 @@ static void __run_hrtimer(struct hrtimer
    >> >>        raw_spin_unlock(&cpu_base->lock);
    >> >>        trace_hrtimer_expire_entry(timer, now);
    >> >>        restart = fn(timer);
    >> >> +       if (restart != HRTIMER_NORESTART)
    >> >>        trace_hrtimer_expire_exit(timer);
    >> >>        raw_spin_lock(&cpu_base->lock);
    >> >>
    >> >> @@ -1236,11 +1237,8 @@ static void __run_hrtimer(struct hrtimer
    >> >>        if (restart != HRTIMER_NORESTART) {
    >> >>                BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
    >> >>                enqueue_hrtimer(timer, base);
    >> >> +               timer->state &= ~HRTIMER_STATE_CALLBACK;
    >
    > HRTIMER_STATE_CALLBACK is only cleared for RESTART hrtimer with
    > your modification.
    >
    >> >>        }
    >> >> -
    >> >> -       WARN_ON_ONCE(!(timer->state & HRTIMER_STATE_CALLBACK));
    >> >> -
    >> >> -       timer->state &= ~HRTIMER_STATE_CALLBACK;
    >
    > But for a hrtimer which is not free in its callback, like a
    > static defined one. the hrtimer could be referenced at the same
    > time. So here you cann't just delete the two lines.

    After callback, as you agree, it is hard to determine in the current
    implementation if the hrtimer is static defined, though another bit
    could be added, say, in the flag word of hrtimer, so cutting the two
    lines off is deserved.
    And more, who will take care of the NORESTART again after callback?

    Cheers
    Hillf
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-12-25 15:21    [W:0.027 / U:30.348 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site