lkml.org 
[lkml]   [2015]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: Another SCHED_DEADLINE bug (with bisection and possible fix)
    Date
    Hi, Luca,

    I've just notived this.

    30.12.2014, 02:27, "luca abeni" <luca.abeni@unitn.it>:
    > Hi all,
    >
    > when running some experiments on current git master, I noticed a
    > regression respect to version 3.18 of the kernel: when invoking
    > sched_setattr() to change the SCHED_DEADLINE parameters of a task that
    > is already scheduled by SCHED_DEADLINE, it is possible to crash the
    > system.
    >
    > The bug can be reproduced with this testcase:
    > http://disi.unitn.it/~abeni/reclaiming/bug-test.tgz
    > Uncompress it, enter the "Bug-Test" directory, and type "make test".
    > After few cycles, my test machine (a laptop with an intel i7 CPU)
    > becomes unusable, and freezes.
    >
    > Since I know that 3.18 is not affected by this bug, I tried a bisect,
    > that pointed to commit 67dfa1b756f250972bde31d65e3f8fde6aeddc5b
    > (sched/deadline: Implement cancel_dl_timer() to use in
    > switched_from_dl()).
    > By looking at that commit, I suspect the problem is that it removes the
    > following lines from init_dl_task_timer():
    > -       if (hrtimer_active(timer)) {
    > -               hrtimer_try_to_cancel(timer);
    > -               return;
    > -       }
    >
    > As a result, when changing the parameters of a SCHED_DEADLINE task
    > init_dl_task_timer() is invoked again, and it can initialize a pending
    > timer (not sure why, but this seems to be the cause of the freezes I am
    > seeing).
    >
    > So, I modified core.c::__setparam_dl() to invoke init_dl_task_timer()
    > only if the task is not already scheduled by SCHED_DEADLINE...
    > Basically, I changed
    >         init_dl_task_timer(dl_se);
    > into
    >         if (p->sched_class != &dl_sched_class) {
    >                 init_dl_task_timer(dl_se);
    >         }
    >
    > I am not sure if this is the correct fix, but with this change the
    > kernel survives my test script (mentioned above), and arrives to 500
    > cycles (without my patch, it crashes after 2 or 3 cycles).
    >
    > What do you think? Is my patch correct, or should I fix the issue in a
    > different way?

    I think we should do something like below.

    hrtimer_init() is already called in sched_fork(), so we shouldn't do this
    twice. Also, we shouldn't reinit dl_throttled etc if timer is pending,
    and we should prevent a race here.

    This passes your test (I waited for 30 cycles), but there's no SOB,
    because I need a little time to check everything once again.

    include/linux/sched/deadline.h | 2 ++
    kernel/sched/core.c | 12 ++++++++----
    kernel/sched/deadline.c | 10 +---------
    kernel/sched/sched.h | 1 -
    4 files changed, 11 insertions(+), 14 deletions(-)
    diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
    index 9d303b8..c70a7fc 100644
    --- a/include/linux/sched/deadline.h
    +++ b/include/linux/sched/deadline.h
    @@ -21,4 +21,6 @@ static inline int dl_task(struct task_struct *p)
    return dl_prio(p->prio);
    }

    +extern enum hrtimer_restart dl_task_timer(struct hrtimer *timer);
    +
    #endif /* _SCHED_DEADLINE_H */
    diff --git a/kernel/sched/core.c b/kernel/sched/core.c
    index c0accc0..a388cc8 100644
    --- a/kernel/sched/core.c
    +++ b/kernel/sched/core.c
    @@ -1840,6 +1840,7 @@ static void __sched_fork(unsigned long clone_flags, struct task_struct *p)

    RB_CLEAR_NODE(&p->dl.rb_node);
    hrtimer_init(&p->dl.dl_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
    + p->dl.dl_timer.function = dl_task_timer;
    __dl_clear_params(p);

    INIT_LIST_HEAD(&p->rt.run_list);
    @@ -3250,16 +3251,19 @@ static void
    __setparam_dl(struct task_struct *p, const struct sched_attr *attr)
    {
    struct sched_dl_entity *dl_se = &p->dl;
    + struct hrtimer *timer = &dl_se->dl_timer;
    +
    + if (!hrtimer_active(timer) || hrtimer_try_to_cancel(timer) != -1) {
    + dl_se->dl_throttled = 0;
    + dl_se->dl_new = 1;
    + dl_se->dl_yielded = 0;
    + }

    - init_dl_task_timer(dl_se);
    dl_se->dl_runtime = attr->sched_runtime;
    dl_se->dl_deadline = attr->sched_deadline;
    dl_se->dl_period = attr->sched_period ?: dl_se->dl_deadline;
    dl_se->flags = attr->sched_flags;
    dl_se->dl_bw = to_ratio(dl_se->dl_period, dl_se->dl_runtime);
    - dl_se->dl_throttled = 0;
    - dl_se->dl_new = 1;
    - dl_se->dl_yielded = 0;
    }

    /*
    diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
    index e5db8c6..8649bd6 100644
    --- a/kernel/sched/deadline.c
    +++ b/kernel/sched/deadline.c
    @@ -500,7 +500,7 @@ static int start_dl_timer(struct sched_dl_entity *dl_se, bool boosted)
    * updating (and the queueing back to dl_rq) will be done by the
    * next call to enqueue_task_dl().
    */
    -static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
    +enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
    {
    struct sched_dl_entity *dl_se = container_of(timer,
    struct sched_dl_entity,
    @@ -559,14 +559,6 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
    return HRTIMER_NORESTART;
    }

    -void init_dl_task_timer(struct sched_dl_entity *dl_se)
    -{
    - struct hrtimer *timer = &dl_se->dl_timer;
    -
    - hrtimer_init(timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
    - timer->function = dl_task_timer;
    -}
    -
    static
    int dl_runtime_exceeded(struct rq *rq, struct sched_dl_entity *dl_se)
    {
    diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
    index 9a2a45c..ad3a2f0 100644
    --- a/kernel/sched/sched.h
    +++ b/kernel/sched/sched.h
    @@ -1257,7 +1257,6 @@ extern void init_rt_bandwidth(struct rt_bandwidth *rt_b, u64 period, u64 runtime

    extern struct dl_bandwidth def_dl_bandwidth;
    extern void init_dl_bandwidth(struct dl_bandwidth *dl_b, u64 period, u64 runtime);
    -extern void init_dl_task_timer(struct sched_dl_entity *dl_se);

    unsigned long to_ratio(u64 period, u64 runtime);


    \
     
     \ /
      Last update: 2015-01-05 00:21    [W:4.758 / U:0.376 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site