sched: Reenable interrupts in do sched_yield()

sched: Reenable interrupts in do sched_yield()

[Thomas Gleixner]

do_sched_yield() invokes schedule() with interrupts disabled which is
not allowed. This goes back to the pre git era to commit a6efb709806c
("[PATCH] irqlock patch 2.5.27-H6") in the history tree.

Reenable interrupts and remove the misleading comment which "explains" it.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/sched/core.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6094,12 +6094,8 @@ static void do_sched_yield(void)
 	schedstat_inc(rq->yld_count);
 	current->sched_class->yield_task(rq);
 
-	/*
-	 * Since we are going to call schedule() anyway, there's
-	 * no need to preempt or enable interrupts:
-	 */
 	preempt_disable();
-	rq_unlock(rq, &rf);
+	rq_unlock_irq(rq, &rf);
 	sched_preempt_enable_no_resched();
 
 	schedule();

Re: sched: Reenable interrupts in do sched_yield()

[Steven Rostedt]

On Tue, 20 Oct 2020 16:46:55 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> -	/*
> -	 * Since we are going to call schedule() anyway, there's
> -	 * no need to preempt or enable interrupts:

I think the above comment still makes sense, just needs to be tweeked:

	/*
	 * Since we are going to call schedule() anyway, there's
	 * no need to allow preemption after releasing the rq lock.
> -	 */

Especially, since we are now enabling interrupts, which is likely to
trigger a preemption.

-- Steve

>  	preempt_disable();
> -	rq_unlock(rq, &rf);
> +	rq_unlock_irq(rq, &rf);
>  	sched_preempt_enable_no_resched();
>  
>  	schedule();

Re: sched: Reenable interrupts in do sched_yield()

[Thomas Gleixner]

On Tue, Oct 20 2020 at 11:38, Steven Rostedt wrote:
> On Tue, 20 Oct 2020 16:46:55 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
>
>> -	/*
>> -	 * Since we are going to call schedule() anyway, there's
>> -	 * no need to preempt or enable interrupts:
>
> I think the above comment still makes sense, just needs to be tweeked:
>
> 	/*
> 	 * Since we are going to call schedule() anyway, there's
> 	 * no need to allow preemption after releasing the rq lock.
>> -	 */
>
> Especially, since we are now enabling interrupts, which is likely to
> trigger a preemption.

sched_preempt_enable_no_resched() still enables preemption. It just
avoids the check. And it still allows preemption when an interrupt
triggering preemption happens between sched_preempt_enable_no_resched()
and __schedule() disabling preemption/interrupts.

So no, your new variant is just differently bogus and misleading.

Thanks,

        tglx

Re: sched: Reenable interrupts in do sched_yield()

[Steven Rostedt]

On Tue, 20 Oct 2020 20:02:55 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, Oct 20 2020 at 11:38, Steven Rostedt wrote:
> > On Tue, 20 Oct 2020 16:46:55 +0200
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> >  
> >> -	/*
> >> -	 * Since we are going to call schedule() anyway, there's
> >> -	 * no need to preempt or enable interrupts:  
> >
> > I think the above comment still makes sense, just needs to be tweeked:
> >
> > 	/*
> > 	 * Since we are going to call schedule() anyway, there's
> > 	 * no need to allow preemption after releasing the rq lock.  
> >> -	 */  
> >
> > Especially, since we are now enabling interrupts, which is likely to
> > trigger a preemption.  
> 
> sched_preempt_enable_no_resched() still enables preemption. It just
> avoids the check. And it still allows preemption when an interrupt
> triggering preemption happens between sched_preempt_enable_no_resched()
> and __schedule() disabling preemption/interrupts.
> 
> So no, your new variant is just differently bogus and misleading.

What I wrote wasn't exactly what I meant. What I meant to have:

	/*
	 * Since we are going to call schedule() anyways, there's
	 * no need to do the preemption check when the rq_lock is released.
	 */

That is, to document why we have the preempt_disable() before the unlock:

	preempt_disable();
	rq_unlock_irq(rq, &rf);
	sched_preempt_enable_no_resched();


-- Steve

Re: sched: Reenable interrupts in do sched_yield()

[Thomas Gleixner]

On Tue, Oct 20 2020 at 16:07, Steven Rostedt wrote:
> On Tue, 20 Oct 2020 20:02:55 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> What I wrote wasn't exactly what I meant. What I meant to have:
>
> 	/*
> 	 * Since we are going to call schedule() anyways, there's
> 	 * no need to do the preemption check when the rq_lock is released.
> 	 */
>
> That is, to document why we have the preempt_disable() before the unlock:

which is pretty obvious, but I let Peter decide on that.

Re: sched: Reenable interrupts in do sched_yield()

[Steven Rostedt]

On Wed, 21 Oct 2020 09:27:22 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, Oct 20 2020 at 16:07, Steven Rostedt wrote:
> > On Tue, 20 Oct 2020 20:02:55 +0200
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > What I wrote wasn't exactly what I meant. What I meant to have:
> >
> > 	/*
> > 	 * Since we are going to call schedule() anyways, there's
> > 	 * no need to do the preemption check when the rq_lock is released.
> > 	 */
> >
> > That is, to document why we have the preempt_disable() before the unlock:  
> 
> which is pretty obvious, but I let Peter decide on that.

To us maybe, but I like to have comments that explain why things are done to
average people. ;-)

If I go to another kernel developer outside the core kernel, would they know
why there's a preempt_disable() there?


 	preempt_disable();
	rq_unlock_irq(rq, &rf);
 	sched_preempt_enable_no_resched();
 
 	schedule();


Not everyone knows that the rq_unlock_irq() would trigger a schedule if an
interrupt happened as soon as irqs were enabled again and need_resched was
set.

-- Steve

[tip: sched/core] sched: Reenable interrupts in do_sched_yield()

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     345a957fcc95630bf5535d7668a59ed983eb49a7
Gitweb:        https://git.kernel.org/tip/345a957fcc95630bf5535d7668a59ed983eb49a7
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 20 Oct 2020 16:46:55 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 29 Oct 2020 11:00:31 +01:00

sched: Reenable interrupts in do_sched_yield()

do_sched_yield() invokes schedule() with interrupts disabled which is
not allowed. This goes back to the pre git era to commit a6efb709806c
("[PATCH] irqlock patch 2.5.27-H6") in the history tree.

Reenable interrupts and remove the misleading comment which "explains" it.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/87r1pt7y5c.fsf@nanos.tec.linutronix.de
---
 kernel/sched/core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d2003a7..6f533bb 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6094,12 +6094,8 @@ static void do_sched_yield(void)
 	schedstat_inc(rq->yld_count);
 	current->sched_class->yield_task(rq);
 
-	/*
-	 * Since we are going to call schedule() anyway, there's
-	 * no need to preempt or enable interrupts:
-	 */
 	preempt_disable();
-	rq_unlock(rq, &rf);
+	rq_unlock_irq(rq, &rf);
 	sched_preempt_enable_no_resched();
 
 	schedule();

Re: sched: Reenable interrupts in do sched_yield()

[Qais Yousef]

On 10/21/20 10:07, Steven Rostedt wrote:
> On Wed, 21 Oct 2020 09:27:22 +0200
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Tue, Oct 20 2020 at 16:07, Steven Rostedt wrote:
> > > On Tue, 20 Oct 2020 20:02:55 +0200
> > > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > What I wrote wasn't exactly what I meant. What I meant to have:
> > >
> > > 	/*
> > > 	 * Since we are going to call schedule() anyways, there's
> > > 	 * no need to do the preemption check when the rq_lock is released.
> > > 	 */
> > >
> > > That is, to document why we have the preempt_disable() before the unlock:  
> > 
> > which is pretty obvious, but I let Peter decide on that.
> 
> To us maybe, but I like to have comments that explain why things are done to
> average people. ;-)
> 
> If I go to another kernel developer outside the core kernel, would they know
> why there's a preempt_disable() there?
> 
> 
>  	preempt_disable();
> 	rq_unlock_irq(rq, &rf);
>  	sched_preempt_enable_no_resched();
>  
>  	schedule();
> 
> 
> Not everyone knows that the rq_unlock_irq() would trigger a schedule if an
> interrupt happened as soon as irqs were enabled again and need_resched was
> set.

Sorry a bit late to the party.

Personally, what actually is tripping me off is that rq_unlock_irq() will end
up calling preempt_enable(), and then we do sched_preempt_enable_no_resched().
Was there an earlier preempt_disable() called up in the chain that I couldn't
figure out that's why it's okay to do the 2? Otherwise I see we have imbalanced
preempt_disable/enable.

	preempt_disable()
	rq_unlock_irq()
		__raw_spin_unlock_irq()
			local_irq_enable()
			preempt_enable()	// first preempt_count_dec()
	sched_preempt_enable_no_resched()	// second preempt_count_dec()

Thanks

--
Qais Yousef