[PATCH] sched: select eligible run-queue for RT task

[PATCH] sched: select eligible run-queue for RT task

[Hillf Danton]

When selecting run-queue for a given task, eligible run-queue should be
returned by checking the CPU affinity of task.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---
 kernel/sched_rt.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 88725c9..45b3e0a 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -1006,7 +1006,8 @@ select_task_rq_rt(struct task_struct *p, int
sd_flag, int flags)
 	int cpu;

 	if (sd_flag != SD_BALANCE_WAKE)
-		return smp_processor_id();
+		return cpumask_test_cpu(smp_processor_id(), &p->cpus_allowed) ?
+			smp_processor_id() : task_cpu(p);

 	cpu = task_cpu(p);
 	rq = cpu_rq(cpu);

Re: [PATCH] sched: select eligible run-queue for RT task

[Steven Rostedt]

On Fri, 2011-06-03 at 22:06 +0800, Hillf Danton wrote:
> When selecting run-queue for a given task, eligible run-queue should be
> returned by checking the CPU affinity of task.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
>  kernel/sched_rt.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> index 88725c9..45b3e0a 100644
> --- a/kernel/sched_rt.c
> +++ b/kernel/sched_rt.c
> @@ -1006,7 +1006,8 @@ select_task_rq_rt(struct task_struct *p, int
> sd_flag, int flags)
>  	int cpu;
> 
>  	if (sd_flag != SD_BALANCE_WAKE)
> -		return smp_processor_id();
> +		return cpumask_test_cpu(smp_processor_id(), &p->cpus_allowed) ?
> +			smp_processor_id() : task_cpu(p);

I wonder if we should bother even dhoing a test here. Perhaps a better
solution is just:

	if (sd_flag != SD_BALANCE_WAKE)
		return task_cpu(p);

-- Steve

Re: [PATCH] sched: select eligible run-queue for RT task

[Mike Galbraith]

On Wed, 2011-06-15 at 13:50 -0400, Steven Rostedt wrote:
> On Fri, 2011-06-03 at 22:06 +0800, Hillf Danton wrote:
> > When selecting run-queue for a given task, eligible run-queue should be
> > returned by checking the CPU affinity of task.
> > 
> > Signed-off-by: Hillf Danton <dhillf@gmail.com>
> > ---
> >  kernel/sched_rt.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> > index 88725c9..45b3e0a 100644
> > --- a/kernel/sched_rt.c
> > +++ b/kernel/sched_rt.c
> > @@ -1006,7 +1006,8 @@ select_task_rq_rt(struct task_struct *p, int
> > sd_flag, int flags)
> >  	int cpu;
> > 
> >  	if (sd_flag != SD_BALANCE_WAKE)
> > -		return smp_processor_id();
> > +		return cpumask_test_cpu(smp_processor_id(), &p->cpus_allowed) ?
> > +			smp_processor_id() : task_cpu(p);
> 
> I wonder if we should bother even dhoing a test here. Perhaps a better
> solution is just:
> 
> 	if (sd_flag != SD_BALANCE_WAKE)
> 		return task_cpu(p);

Hm.  We shouldn't need to check the mask here for SD_BALANCE_WAKE, since
it will be checked when we return to select_task_rq().

For exec, it doesn't matter which we return, but for a preempted and
migrated parent waking it's child, it could.  task_cpu(parent) seems
better than task_cpu(child), since that is likely where the parent was
preempted, and may still be occupied by a higher priority task.  We'll
subsequently try to push, but we then fiddle with a higher priority rq
needlessly, no?

So to me, it looks like things are better as is.. but we could perhaps
do better by handling SD_BALANCE_FORK here as well, to avoid some 'queue
the child locally (overload) then push it away' overhead.

	-Mike

Re: [PATCH] sched: select eligible run-queue for RT task

[Steven Rostedt]

On Thu, 2011-06-16 at 05:19 +0200, Mike Galbraith wrote:
> On Wed, 2011-06-15 at 13:50 -0400, Steven Rostedt wrote:
> > On Fri, 2011-06-03 at 22:06 +0800, Hillf Danton wrote:
> > > When selecting run-queue for a given task, eligible run-queue should be
> > > returned by checking the CPU affinity of task.
> > > 
> > > Signed-off-by: Hillf Danton <dhillf@gmail.com>
> > > ---
> > >  kernel/sched_rt.c |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> > > index 88725c9..45b3e0a 100644
> > > --- a/kernel/sched_rt.c
> > > +++ b/kernel/sched_rt.c
> > > @@ -1006,7 +1006,8 @@ select_task_rq_rt(struct task_struct *p, int
> > > sd_flag, int flags)
> > >  	int cpu;
> > > 
> > >  	if (sd_flag != SD_BALANCE_WAKE)
> > > -		return smp_processor_id();
> > > +		return cpumask_test_cpu(smp_processor_id(), &p->cpus_allowed) ?
> > > +			smp_processor_id() : task_cpu(p);
> > 
> > I wonder if we should bother even dhoing a test here. Perhaps a better
> > solution is just:
> > 
> > 	if (sd_flag != SD_BALANCE_WAKE)
> > 		return task_cpu(p);
> 
> Hm.  We shouldn't need to check the mask here for SD_BALANCE_WAKE, since
> it will be checked when we return to select_task_rq().

How does that matter if it will be checked in select_task_rq(). The
point is that we want to avoid this work when not needed. But that's
more for exec.

> 
> For exec, it doesn't matter which we return, but for a preempted and
> migrated parent waking it's child, it could.  task_cpu(parent) seems
> better than task_cpu(child), since that is likely where the parent was
> preempted, and may still be occupied by a higher priority task.  We'll
> subsequently try to push, but we then fiddle with a higher priority rq
> needlessly, no?

I'm a bit confused by what you mean here. Actually, I have a patch to
have it do the same work for both WAKE and FORK. The reason is that we
will do this work in wake_up_new regardless. The question is when? If we
do it now, we push the task before it has been queued. If we do it later
(as it is done now), if we push the task, we have to first dequeue it,
because it has already been queued.

commit 318e0893ce3f524ca045f9fd9dfd567c0a6f9446 explains this nicely ;)


> 
> So to me, it looks like things are better as is.. but we could perhaps
> do better by handling SD_BALANCE_FORK here as well, to avoid some 'queue
> the child locally (overload) then push it away' overhead.

Maybe you just said what I said, but I'm still confused by what you
said. It's late for me, and I think it's time for bed ;)

Night!

-- Steve

Re: [PATCH] sched: select eligible run-queue for RT task

[Mike Galbraith]

On Wed, 2011-06-15 at 23:51 -0400, Steven Rostedt wrote:

> Maybe you just said what I said, but I'm still confused by what you
> said. It's late for me, and I think it's time for bed ;)

I _think_ we said the same, but I've been smoking a UV+RT mix, and might
be completely incapable of coherent speech.

> Night!

Sleep well.

	-Mike