From: Steven Rostedt <[email protected]>
The if (unlikely(!rt_rq->rt_nr_running)) test in pick_next_task_rt()
tests if there is another rt task ready to run. If so, then pick it.
In most systems, only one RT task runs at a time most of the time.
Running the branch unlikely annotator profiler on a system doing average
work "running firefox, evolution, xchat, distcc builds, etc", it showed the
following:
correct incorrect % Function File Line
------- --------- - -------- ---- ----
324344 135104992 99 _pick_next_task_rt sched_rt.c 1064
99% of the time the condition is true. When an RT task schedules out,
it is unlikely that another RT task is waiting to run on that same run queue.
Cc:Peter Zijlstra <[email protected]>
Cc: Gregory Haskins <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
kernel/sched_rt.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 7a5c4db..a249ae3 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -1062,7 +1062,7 @@ static struct task_struct *_pick_next_task_rt(struct rq *rq)
rt_rq = &rq->rt;
- if (unlikely(!rt_rq->rt_nr_running))
+ if (likely(!rt_rq->rt_nr_running))
return NULL;
if (rt_rq_throttled(rt_rq))
--
1.7.2.3
>>> On 12/6/2010 at 08:58 PM, in message <[email protected]>,
Steven Rostedt <[email protected]> wrote:
> From: Steven Rostedt <[email protected]>
>
> The if (unlikely(!rt_rq->rt_nr_running)) test in pick_next_task_rt()
> tests if there is another rt task ready to run. If so, then pick it.
>
> In most systems, only one RT task runs at a time most of the time.
> Running the branch unlikely annotator profiler on a system doing average
> work "running firefox, evolution, xchat, distcc builds, etc", it showed the
> following:
My feeling is that generally speaking, if the branch is workload dependent, we should probably not annotate it at all and let the CPUs branch-predictor do its thing. I guess what I am not 100% clear on is how these annotations affect the BPU. I.e. is it a static decision point or can the BPU still "learn" if the annotation is particularly wrong for a given workload? For the former, I think we should just remove this particular annotation (and there are others that need review). For the latter, this is obviously the right annotation we should be using in this particular case.
-Greg
>
> correct incorrect % Function File
> Line
> ------- --------- - -------- ---- ----
> 324344 135104992 99 _pick_next_task_rt sched_rt.c
> 1064
>
> 99% of the time the condition is true. When an RT task schedules out,
> it is unlikely that another RT task is waiting to run on that same run
> queue.
>
> Cc:Peter Zijlstra <[email protected]>
> Cc: Gregory Haskins <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> kernel/sched_rt.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> index 7a5c4db..a249ae3 100644
> --- a/kernel/sched_rt.c
> +++ b/kernel/sched_rt.c
> @@ -1062,7 +1062,7 @@ static struct task_struct *_pick_next_task_rt(struct rq
> *rq)
>
> rt_rq = &rq->rt;
>
> - if (unlikely(!rt_rq->rt_nr_running))
> + if (likely(!rt_rq->rt_nr_running))
> return NULL;
>
> if (rt_rq_throttled(rt_rq))
On Mon, 2010-12-06 at 19:46 -0700, Gregory Haskins wrote:
> >>> On 12/6/2010 at 08:58 PM, in message <[email protected]>,
> Steven Rostedt <[email protected]> wrote:
> > From: Steven Rostedt <[email protected]>
> >
> > The if (unlikely(!rt_rq->rt_nr_running)) test in pick_next_task_rt()
> > tests if there is another rt task ready to run. If so, then pick it.
> >
> > In most systems, only one RT task runs at a time most of the time.
> > Running the branch unlikely annotator profiler on a system doing average
> > work "running firefox, evolution, xchat, distcc builds, etc", it showed the
> > following:
>
> My feeling is that generally speaking, if the branch is workload
> dependent, we should probably not annotate it at all and let the CPUs
> branch-predictor do its thing. I guess what I am not 100% clear on is
> how these annotations affect the BPU. I.e. is it a static decision
> point or can the BPU still "learn" if the annotation is particularly
> wrong for a given workload? For the former, I think we should just
> remove this particular annotation (and there are others that need
> review). For the latter, this is obviously the right annotation we
> should be using in this particular case.
I'm fine with just removing the likely() here. It is a bit workload
dependent. We can't assume that we are running a lot of RT tasks on a
single CPU, but we also should not assume that we are not doing so.
Considering that we are just coming out of an RT task, I doubt it will
hurt anything to just keep the annotation off.
I don't think gcc does anything special for x86 with regards to the BPU.
But I do believe that gcc will add special ops for likely()'s on other
archs.
-- Steve
On Mon, 2010-12-06 at 19:46 -0700, Gregory Haskins wrote:
> My feeling is that generally speaking, if the branch is workload dependent, we should probably not annotate it at all and let the CPUs branch-predictor do its thing. I guess what I am not 100% clear on is how these annotations affect the BPU. I.e. is it a static decision point or can the BPU still "learn" if the annotation is particularly wrong for a given workload? For the former, I think we should just remove this particular annotation (and there are others that need review). For the latter, this is obviously the right annotation we should be using in this particular case.
>
You need to get a better email client ;-)
OK, if I just remove the likely() do you Ack it?
-- Steve
Sorry for toppost...mobile.
Re: mail client: ack
Re: patch: ack
;)
-Greg
-----Original Message-----
From: Steven Rostedt <[email protected]>
To: Gregory Haskins <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: <[email protected]>
Sent: 12/10/2010 7:07:29 PM
Subject: Re: [RFC][PATCH 04/10] sched: Change pick_next_task_rt from unlikely to likely
On Mon, 2010-12-06 at 19:46 -0700, Gregory Haskins wrote:
> My feeling is that generally speaking, if the branch is workload dependent, we should probably not annotate it at all and let the CPUs branch-predictor do its thing. I guess what I am not 100% clear on is how these annotations affect the BPU. I.e. is it a static decision point or can the BPU still "learn" if the annotation is particularly wrong for a given workload? For the former, I think we should just remove this particular annotation (and there are others that need review). For the latter, this is obviously the right annotation we should be using in this particular case.
>
You need to get a better email client ;-)
OK, if I just remove the likely() do you Ack it?
-- Steve