1. Patchew
  2. linux
  3. [PATCH 0/6] locking/rtmutex: Avoid PI state recursion through sched_submit_work()
  4. View patch

[PATCH 3/6] sched: Extract __schedule_loop()

Peter Zijlstra posted 6 patches 2 years, 4 months ago
There is a newer version of this series
[PATCH 3/6] sched: Extract __schedule_loop()
Posted by Peter Zijlstra 2 years, 4 months ago 
From: Thomas Gleixner <tglx@linutronix.de>

There are currently two implementations of this basic __schedule()
loop, and there is soon to be a third.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20230427111937.2745231-2-bigeasy@linutronix.de
---
 kernel/sched/core.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6787,16 +6787,21 @@ static void sched_update_worker(struct t
 	}
 }
 
-asmlinkage __visible void __sched schedule(void)
+static __always_inline void __schedule_loop(unsigned int sched_mode)
 {
-	struct task_struct *tsk = current;
-
-	sched_submit_work(tsk);
 	do {
 		preempt_disable();
-		__schedule(SM_NONE);
+		__schedule(sched_mode);
 		sched_preempt_enable_no_resched();
 	} while (need_resched());
+}
+
+asmlinkage __visible void __sched schedule(void)
+{
+	struct task_struct *tsk = current;
+
+	sched_submit_work(tsk);
+	__schedule_loop(SM_NONE);
 	sched_update_worker(tsk);
 }
 EXPORT_SYMBOL(schedule);
@@ -6860,11 +6865,7 @@ void __sched schedule_preempt_disabled(v
 #ifdef CONFIG_PREEMPT_RT
 void __sched notrace schedule_rtlock(void)
 {
-	do {
-		preempt_disable();
-		__schedule(SM_RTLOCK_WAIT);
-		sched_preempt_enable_no_resched();
-	} while (need_resched());
+	__schedule_loop(SM_RTLOCK_WAIT);
 }
 NOKPROBE_SYMBOL(schedule_rtlock);
 #endif
Re: [PATCH 3/6] sched: Extract __schedule_loop()
Posted by Phil Auld 2 years, 4 months ago 
Hi Peter,

On Tue, Aug 15, 2023 at 01:01:24PM +0200 Peter Zijlstra wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> There are currently two implementations of this basic __schedule()
> loop, and there is soon to be a third.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lkml.kernel.org/r/20230427111937.2745231-2-bigeasy@linutronix.de
> ---
>  kernel/sched/core.c |   21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6787,16 +6787,21 @@ static void sched_update_worker(struct t
>  	}
>  }
>  
> -asmlinkage __visible void __sched schedule(void)
> +static __always_inline void __schedule_loop(unsigned int sched_mode)

I think this needs __sched or it's the only thing that ever shows up
in wchan. E.g.

  16995 0 bash     S __schedule_loop.constprop.0
  17036 1 kworker/ I __schedule_loop.constprop.0
  17151 1 kworker/ I __schedule_loop.constprop.0
  17235 0 sleep    S __schedule_loop.constprop.0
  17236 4 ps       R -

vs

  10417 1 kworker/ I worker_thread
  10665 1 kworker/ I worker_thread
  10670 4 sshd     D -            
  10674 4 bash     S do_wait      
  10701 0 sleep    S hrtimer_nanosleep
  10702 4 ps       R -

Otherwise, looking forward to getting this and the rest of RT in...

Cheers,
Phil

>  {
> -	struct task_struct *tsk = current;
> -
> -	sched_submit_work(tsk);
>  	do {
>  		preempt_disable();
> -		__schedule(SM_NONE);
> +		__schedule(sched_mode);
>  		sched_preempt_enable_no_resched();
>  	} while (need_resched());
> +}
> +
> +asmlinkage __visible void __sched schedule(void)
> +{
> +	struct task_struct *tsk = current;
> +
> +	sched_submit_work(tsk);
> +	__schedule_loop(SM_NONE);
>  	sched_update_worker(tsk);
>  }
>  EXPORT_SYMBOL(schedule);
> @@ -6860,11 +6865,7 @@ void __sched schedule_preempt_disabled(v
>  #ifdef CONFIG_PREEMPT_RT
>  void __sched notrace schedule_rtlock(void)
>  {
> -	do {
> -		preempt_disable();
> -		__schedule(SM_RTLOCK_WAIT);
> -		sched_preempt_enable_no_resched();
> -	} while (need_resched());
> +	__schedule_loop(SM_RTLOCK_WAIT);
>  }
>  NOKPROBE_SYMBOL(schedule_rtlock);
>  #endif
> 
> 

--
Re: [PATCH 3/6] sched: Extract __schedule_loop()
Posted by Sebastian Andrzej Siewior 2 years, 4 months ago 
On 2023-08-15 18:33:01 [-0400], Phil Auld wrote:
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -6787,16 +6787,21 @@ static void sched_update_worker(struct t
> >  	}
> >  }
> >  
> > -asmlinkage __visible void __sched schedule(void)
> > +static __always_inline void __schedule_loop(unsigned int sched_mode)
> 
> I think this needs __sched or it's the only thing that ever shows up
> in wchan. E.g.
> 
>   16995 0 bash     S __schedule_loop.constprop.0

I don't see __schedule_loop in my RT and !RT build. I tried gcc and
clang.

Sebastian
Re: [PATCH 3/6] sched: Extract __schedule_loop()
Posted by Phil Auld 2 years, 4 months ago 
On Wed, Aug 16, 2023 at 12:01:54PM +0200 Sebastian Andrzej Siewior wrote:
> On 2023-08-15 18:33:01 [-0400], Phil Auld wrote:
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -6787,16 +6787,21 @@ static void sched_update_worker(struct t
> > >  	}
> > >  }
> > >  
> > > -asmlinkage __visible void __sched schedule(void)
> > > +static __always_inline void __schedule_loop(unsigned int sched_mode)
> > 
> > I think this needs __sched or it's the only thing that ever shows up
> > in wchan. E.g.
> > 
> >   16995 0 bash     S __schedule_loop.constprop.0
> 
> I don't see __schedule_loop in my RT and !RT build. I tried gcc and
> clang.
>

I do.  Admittedly I'm not an expert in how the wchan unwinding works but
we have a slightly older version of this patch in our kernel (schedule_loop
not __schedule_loop). When I added __sched it fixed it.   Maybe there
is something else but that seemed pretty obvious. 


/* Attach to any functions which should be ignored in wchan output. */
#define __sched		__section(".sched.text")

I can't explain why you are not seeing it.


Cheers,
Phil


> Sebastian
> 

--
Re: [PATCH 3/6] sched: Extract __schedule_loop()
Posted by Sebastian Andrzej Siewior 2 years, 4 months ago 
On 2023-08-16 07:39:45 [-0400], Phil Auld wrote:
> I do.  Admittedly I'm not an expert in how the wchan unwinding works but
> we have a slightly older version of this patch in our kernel (schedule_loop
> not __schedule_loop). When I added __sched it fixed it.   Maybe there
> is something else but that seemed pretty obvious. 
> 
> 
> /* Attach to any functions which should be ignored in wchan output. */
> #define __sched		__section(".sched.text")
> 
> I can't explain why you are not seeing it.

as peterz pointed out, it is marked __always_inline so the compiler
shouldn't make a separate function out of it.
Could you check with _this_ series? The schedule_loop variant is in RT
and does not have this inline thingy. So it would be good if the issue
you report actually exists in the series that has been posted.

> Cheers,
> Phil

Sebastian
Re: [PATCH 3/6] sched: Extract __schedule_loop()
Posted by Phil Auld 2 years, 4 months ago 
On Wed, Aug 16, 2023 at 02:20:07PM +0200 Sebastian Andrzej Siewior wrote:
> On 2023-08-16 07:39:45 [-0400], Phil Auld wrote:
> > I do.  Admittedly I'm not an expert in how the wchan unwinding works but
> > we have a slightly older version of this patch in our kernel (schedule_loop
> > not __schedule_loop). When I added __sched it fixed it.   Maybe there
> > is something else but that seemed pretty obvious. 
> > 
> > 
> > /* Attach to any functions which should be ignored in wchan output. */
> > #define __sched		__section(".sched.text")
> > 
> > I can't explain why you are not seeing it.
> 
> as peterz pointed out, it is marked __always_inline so the compiler
> shouldn't make a separate function out of it.
> Could you check with _this_ series? The schedule_loop variant is in RT
> and does not have this inline thingy. So it would be good if the issue
> you report actually exists in the series that has been posted.
>

Hhm, yes.  I was looking at the issue in our tree when these patches
came by. Sorry... I seem to have glossed over the __always_inline.
That would certainly work as well and, of course, does explain why you
aren't seeing it.

read more. talk less :)


Cheers,
Phil

> > Cheers,
> > Phil
> 
> Sebastian
> 

--
Re: [PATCH 3/6] sched: Extract __schedule_loop()
Posted by Phil Auld 2 years, 4 months ago 
On Tue, Aug 15, 2023 at 06:33:01PM -0400 Phil Auld wrote:
> Hi Peter,
> 
> On Tue, Aug 15, 2023 at 01:01:24PM +0200 Peter Zijlstra wrote:
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > There are currently two implementations of this basic __schedule()
> > loop, and there is soon to be a third.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Link: https://lkml.kernel.org/r/20230427111937.2745231-2-bigeasy@linutronix.de
> > ---
> >  kernel/sched/core.c |   21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -6787,16 +6787,21 @@ static void sched_update_worker(struct t
> >  	}
> >  }
> >  
> > -asmlinkage __visible void __sched schedule(void)
> > +static __always_inline void __schedule_loop(unsigned int sched_mode)
> 
> I think this needs __sched or it's the only thing that ever shows up
> in wchan. E.g.
>

Using this command:

    ps -Ao pid,f,fname,s,wchan 


>   16995 0 bash     S __schedule_loop.constprop.0
>   17036 1 kworker/ I __schedule_loop.constprop.0
>   17151 1 kworker/ I __schedule_loop.constprop.0
>   17235 0 sleep    S __schedule_loop.constprop.0
>   17236 4 ps       R -
> 
> vs
> 
>   10417 1 kworker/ I worker_thread
>   10665 1 kworker/ I worker_thread
>   10670 4 sshd     D -            
>   10674 4 bash     S do_wait      
>   10701 0 sleep    S hrtimer_nanosleep
>   10702 4 ps       R -
> 
> Otherwise, looking forward to getting this and the rest of RT in...
> 
> Cheers,
> Phil
> 
> >  {
> > -	struct task_struct *tsk = current;
> > -
> > -	sched_submit_work(tsk);
> >  	do {
> >  		preempt_disable();
> > -		__schedule(SM_NONE);
> > +		__schedule(sched_mode);
> >  		sched_preempt_enable_no_resched();
> >  	} while (need_resched());
> > +}
> > +
> > +asmlinkage __visible void __sched schedule(void)
> > +{
> > +	struct task_struct *tsk = current;
> > +
> > +	sched_submit_work(tsk);
> > +	__schedule_loop(SM_NONE);
> >  	sched_update_worker(tsk);
> >  }
> >  EXPORT_SYMBOL(schedule);
> > @@ -6860,11 +6865,7 @@ void __sched schedule_preempt_disabled(v
> >  #ifdef CONFIG_PREEMPT_RT
> >  void __sched notrace schedule_rtlock(void)
> >  {
> > -	do {
> > -		preempt_disable();
> > -		__schedule(SM_RTLOCK_WAIT);
> > -		sched_preempt_enable_no_resched();
> > -	} while (need_resched());
> > +	__schedule_loop(SM_RTLOCK_WAIT);
> >  }
> >  NOKPROBE_SYMBOL(schedule_rtlock);
> >  #endif
> > 
> > 
> 
> -- 
> 

--
Re: [PATCH 3/6] sched: Extract __schedule_loop()
Posted by Peter Zijlstra 2 years, 4 months ago 
On Tue, Aug 15, 2023 at 06:33:01PM -0400, Phil Auld wrote:
> Hi Peter,
> 
> On Tue, Aug 15, 2023 at 01:01:24PM +0200 Peter Zijlstra wrote:
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > There are currently two implementations of this basic __schedule()
> > loop, and there is soon to be a third.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Link: https://lkml.kernel.org/r/20230427111937.2745231-2-bigeasy@linutronix.de
> > ---
> >  kernel/sched/core.c |   21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -6787,16 +6787,21 @@ static void sched_update_worker(struct t
> >  	}
> >  }
> >  
> > -asmlinkage __visible void __sched schedule(void)
> > +static __always_inline void __schedule_loop(unsigned int sched_mode)
> 
> I think this needs __sched or it's the only thing that ever shows up
> in wchan. E.g.
> 
>   16995 0 bash     S __schedule_loop.constprop.0
>   17036 1 kworker/ I __schedule_loop.constprop.0
>   17151 1 kworker/ I __schedule_loop.constprop.0
>   17235 0 sleep    S __schedule_loop.constprop.0
>   17236 4 ps       R -

But but but __always_inline... ?!?
Re: [PATCH 3/6] sched: Extract __schedule_loop()
Posted by Phil Auld 2 years, 4 months ago 
On Wed, Aug 16, 2023 at 12:39:26AM +0200 Peter Zijlstra wrote:
> On Tue, Aug 15, 2023 at 06:33:01PM -0400, Phil Auld wrote:
> > Hi Peter,
> > 
> > On Tue, Aug 15, 2023 at 01:01:24PM +0200 Peter Zijlstra wrote:
> > > From: Thomas Gleixner <tglx@linutronix.de>
> > > 
> > > There are currently two implementations of this basic __schedule()
> > > loop, and there is soon to be a third.
> > > 
> > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > Link: https://lkml.kernel.org/r/20230427111937.2745231-2-bigeasy@linutronix.de
> > > ---
> > >  kernel/sched/core.c |   21 +++++++++++----------
> > >  1 file changed, 11 insertions(+), 10 deletions(-)
> > > 
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -6787,16 +6787,21 @@ static void sched_update_worker(struct t
> > >  	}
> > >  }
> > >  
> > > -asmlinkage __visible void __sched schedule(void)
> > > +static __always_inline void __schedule_loop(unsigned int sched_mode)
> > 
> > I think this needs __sched or it's the only thing that ever shows up
> > in wchan. E.g.
> > 
> >   16995 0 bash     S __schedule_loop.constprop.0
> >   17036 1 kworker/ I __schedule_loop.constprop.0
> >   17151 1 kworker/ I __schedule_loop.constprop.0
> >   17235 0 sleep    S __schedule_loop.constprop.0
> >   17236 4 ps       R -
> 
> But but but __always_inline... ?!?
> 

Yep, sorry. Totally missed those characters (and this reply of yours).

The older version from the rt tree had neither. 


Cheers,
Phil
--

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.