by Gerlando Falauto

[permalink] [raw]

Subject: Re: kernel deadlock

Hi Peter,

sorry for the slow response...

On 09/09/2013 12:08 PM, Peter Zijlstra wrote:
> On Tue, Sep 03, 2013 at 10:26:19AM -0700, John Stultz wrote:
>> Enabling the SCHED_FEAT(HRTICK, true) bit tends to cause lots of issues
>> on the various hardware I have, tripping the lockdep warnings on various
>> other issues:
>
> Does whatever kernel you guys are running have this commit:

That didn't seem to make a difference (it had already been pointed out
by Stephen Boyd).
In any case, latest patch from John fixes the problem... Hooray! :-)

Thanks again John!
Gerlando

>
> ---
> commit 971ee28cbd1ccd87b3164facd9359a534c1d2892
> Author: Peter Zijlstra <[email protected]>
> Date: Fri Jun 28 11:18:53 2013 +0200
>
> sched: Fix HRTICK
>
> David reported that the HRTICK sched feature was borken; which was enough
> motivation for me to finally fix it ;-)
>
> We should not allow hrtimer code to do softirq wakeups while holding scheduler
> locks. The hrtimer code only needs this when we accidentally try to program an
> expired time. We don't much care about those anyway since we have the regular
> tick to fall back to.
>
> Reported-by: David Ahern <[email protected]>
> Tested-by: David Ahern <[email protected]>
> Signed-off-by: Peter Zijlstra <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: Ingo Molnar <[email protected]>
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9b1f2e5..0d8eb45 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -370,13 +370,6 @@ static struct rq *this_rq_lock(void)
> #ifdef CONFIG_SCHED_HRTICK
> /*
> * Use HR-timers to deliver accurate preemption points.
> - *
> - * Its all a bit involved since we cannot program an hrt while holding the
> - * rq->lock. So what we do is store a state in in rq->hrtick_* and ask for a
> - * reschedule event.
> - *
> - * When we get rescheduled we reprogram the hrtick_timer outside of the
> - * rq->lock.
> */
>
> static void hrtick_clear(struct rq *rq)
> @@ -404,6 +397,15 @@ static enum hrtimer_restart hrtick(struct hrtimer *timer)
> }
>
> #ifdef CONFIG_SMP
> +
> +static int __hrtick_restart(struct rq *rq)
> +{
> + struct hrtimer *timer = &rq->hrtick_timer;
> + ktime_t time = hrtimer_get_softexpires(timer);
> +
> + return __hrtimer_start_range_ns(timer, time, 0, HRTIMER_MODE_ABS_PINNED, 0);
> +}
> +
> /*
> * called from hardirq (IPI) context
> */
> @@ -412,7 +414,7 @@ static void __hrtick_start(void *arg)
> struct rq *rq = arg;
>
> raw_spin_lock(&rq->lock);
> - hrtimer_restart(&rq->hrtick_timer);
> + __hrtick_restart(rq);
> rq->hrtick_csd_pending = 0;
> raw_spin_unlock(&rq->lock);
> }
> @@ -430,7 +432,7 @@ void hrtick_start(struct rq *rq, u64 delay)
> hrtimer_set_expires(timer, time);
>
> if (rq == this_rq()) {
> - hrtimer_restart(timer);
> + __hrtick_restart(rq);
> } else if (!rq->hrtick_csd_pending) {
> __smp_call_function_single(cpu_of(rq), &rq->hrtick_csd, 0);
> rq->hrtick_csd_pending = 1;
>