Re: [PATCH] sched: Fix lost reschedule in __cond_resched()

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@xxxxxxxxx> wrote:

> On Sat, Dec 13, 2014 at 08:36:34AM +0100, Ingo Molnar wrote:
> > 
> > * Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > 
> > > I'm also not sure if the bug ever happens with preemption 
> > > disabled. Sasha, was that you who reported that you cannot 
> > > reproduce it without preemption? It strikes me that there's a 
> > > race condition in __cond_resched() wrt preemption, for example: 
> > > we do
> > > 
> > >         __preempt_count_add(PREEMPT_ACTIVE);
> > >         __schedule();
> > >         __preempt_count_sub(PREEMPT_ACTIVE);
> > > 
> > > and in between the __schedule() and __preempt_count_sub(), if 
> > > an interrupt comes in and wakes up some important process, it 
> > > won't reschedule (because preemption is active), but then we 
> > > enable preemption again and don't check whether we should 
> > > reschedule (again), and we just go on our merry ways.
> > 
> > Indeed, that's a really good find regardless of whether it's the 
> > source of these lockups - the (untested) patch below ought to 
> > cure that.
> > 
> > > Now, I don't see how that could really matter for a long time - 
> > > returning to user space will check need_resched, and sleeping 
> > > will obviously force a reschedule anyway, so these kinds of 
> > > races should at most delay things by just a tiny amount, but 
> > > maybe there is some case where we screw up in a bigger way. So 
> > > I do *not* believe that the one in __cond_resched() matters, 
> > > but I'm giving it as an example of the kind of things that 
> > > could go wrong.
> > 
> > (as you later note) NOHZ is somewhat special in this regard, 
> > because there we try really hard not to run anything 
> > periodically, so a lost reschedule will matter more.
> > 
> > But ... I'd be surprised if this patch made a difference: it 
> > should normally not be possible to go idle with tasks on the 
> > runqueue (even with this bug present), and with at least one busy 
> > task on the CPU we get the regular scheduler tick which ought to 
> > hide such latencies.
> > 
> > It's nevertheless a good thing to fix, I'm just not sure it's the 
> > root cause of the observed lockup here.
> > 
> > Thanks,
> > 
> > 	Ingo
> > 
> > --
> > 
> > Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index bb398c0c5f08..532809aa0544 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -4207,6 +4207,8 @@ static void __cond_resched(void)
> >  	__preempt_count_add(PREEMPT_ACTIVE);
> >  	__schedule();
> >  	__preempt_count_sub(PREEMPT_ACTIVE);
> > +	if (need_resched())
> > +		__schedule();
> >  }
> 
> Nice catch! This indeed matters a lot for full nohz where a lost reschedule
> interrupt might be ignored and not fixed with a near tick. Although even if
> it is fixed by a tick, a missed reschedule delayed by HZ involves latency issue.
> 
> Anyway, probably the above __schedule() should stay as a preemption point
> to make sure that a TASK_[UN]INTERRUPTIBLE is handled as expected and avoids
> early task deactivation.
> 
> Such as:
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@xxxxxxxxx>
> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 240157c..6e942f3 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2922,6 +2922,21 @@ void __sched schedule_preempt_disabled(void)
>  	preempt_disable();
>  }
>  
> +static void __preempt_schedule(void)
> +{
> +	do {
> +		__preempt_count_add(PREEMPT_ACTIVE);
> +		__schedule();
> +		__preempt_count_sub(PREEMPT_ACTIVE);
> +
> +		/*
> +		 * Check again in case we missed a preemption opportunity
> +		 * between schedule and now.
> +		 */
> +		barrier();
> +	} while (need_resched());
> +}
> +
>  #ifdef CONFIG_PREEMPT
>  /*
>   * this is the entry point to schedule() from in-kernel preemption
> @@ -2937,17 +2952,7 @@ asmlinkage __visible void __sched notrace preempt_schedule(void)
>  	if (likely(!preemptible()))
>  		return;
>  
> -	do {
> -		__preempt_count_add(PREEMPT_ACTIVE);
> -		__schedule();
> -		__preempt_count_sub(PREEMPT_ACTIVE);
> -
> -		/*
> -		 * Check again in case we missed a preemption opportunity
> -		 * between schedule and now.
> -		 */
> -		barrier();
> -	} while (need_resched());
> +	__preempt_schedule();
>  }
>  NOKPROBE_SYMBOL(preempt_schedule);
>  EXPORT_SYMBOL(preempt_schedule);
> @@ -4249,9 +4254,7 @@ SYSCALL_DEFINE0(sched_yield)
>  
>  static void __cond_resched(void)
>  {
> -	__preempt_count_add(PREEMPT_ACTIVE);
> -	__schedule();
> -	__preempt_count_sub(PREEMPT_ACTIVE);
> +	__preempt_schedule();
>  }

Yeah, agreed, your variant is even nicer.

Thanks,

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