[PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[Rakib Mullick]

 sched_rt: Removes extra checking for nr_cpus_allowed when calling
find_lowest_rq

Remove the check for nr_cpus_allowed of an RT task before calling
find_lowest_cpu() from select_task_rq_rt function. Cause
find_lowest_cpu checks for rt.nr_cpus_allowed of a process.
It would've been nice, if we make the check before calling
find_lowest_cpu. But its called from other places, so keep the
nr_cpus_allowed check into find_lowest_cpu().


Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
---

--- linus-rc8/kernel/sched_rt.c	2010-10-15 05:26:43.000000000 +0600
+++ rakib-rc8/kernel/sched_rt.c	2010-10-19 16:22:30.000000000 +0600
@@ -971,8 +971,7 @@ select_task_rq_rt(struct rq *rq, struct
 	 * that is just being woken and probably will have
 	 * cold cache anyway.
 	 */
-	if (unlikely(rt_task(rq->curr)) &&
-	    (p->rt.nr_cpus_allowed > 1)) {
+	if (unlikely(rt_task(rq->curr))) {
 		int cpu = find_lowest_rq(p);

 		return (cpu == -1) ? task_cpu(p) : cpu;

Re: [PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[Peter Zijlstra]

On Tue, 2010-10-19 at 16:57 +0600, Rakib Mullick wrote:
> sched_rt: Removes extra checking for nr_cpus_allowed when calling
> find_lowest_rq

Why repeat this subject?

> Remove the check for nr_cpus_allowed of an RT task before calling
> find_lowest_cpu() from select_task_rq_rt function. Cause
> find_lowest_cpu checks for rt.nr_cpus_allowed of a process.
> It would've been nice, if we make the check before calling
> find_lowest_cpu. But its called from other places, so keep the
> nr_cpus_allowed check into find_lowest_cpu().

That's just not explaining anything.. what can go wrong, how does this
fix it.

> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> ---
> 
> --- linus-rc8/kernel/sched_rt.c	2010-10-15 05:26:43.000000000 +0600
> +++ rakib-rc8/kernel/sched_rt.c	2010-10-19 16:22:30.000000000 +0600
> @@ -971,8 +971,7 @@ select_task_rq_rt(struct rq *rq, struct
>  	 * that is just being woken and probably will have
>  	 * cold cache anyway.
>  	 */
> -	if (unlikely(rt_task(rq->curr)) &&
> -	    (p->rt.nr_cpus_allowed > 1)) {
> +	if (unlikely(rt_task(rq->curr))) {
>  		int cpu = find_lowest_rq(p);
> 
>  		return (cpu == -1) ? task_cpu(p) : cpu;

Re: [PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[Rakib Mullick]

On Tue, Oct 19, 2010 at 5:02 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, 2010-10-19 at 16:57 +0600, Rakib Mullick wrote:
>> sched_rt: Removes extra checking for nr_cpus_allowed when calling
>> find_lowest_rq
>
> Why repeat this subject?

Just trying to give a good looking format :-). Not sure whether it
really is or not.
>
>
> That's just not explaining anything.. what can go wrong, how does this
> fix it.

Actually it's not a fix. It just removes a check. It checks whether
this process is movable to other processor or not. And this check is
made from inside the find_lowest_cpu function. Thats why, the check
before calling find_lowest_cpu has been removed. Is my explanation
okay?

Thanks,
Rakib


>
>> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
>> ---
>>
>> --- linus-rc8/kernel/sched_rt.c       2010-10-15 05:26:43.000000000 +0600
>> +++ rakib-rc8/kernel/sched_rt.c       2010-10-19 16:22:30.000000000 +0600
>> @@ -971,8 +971,7 @@ select_task_rq_rt(struct rq *rq, struct
>>        * that is just being woken and probably will have
>>        * cold cache anyway.
>>        */
>> -     if (unlikely(rt_task(rq->curr)) &&
>> -         (p->rt.nr_cpus_allowed > 1)) {
>> +     if (unlikely(rt_task(rq->curr))) {
>>               int cpu = find_lowest_rq(p);
>>
>>               return (cpu == -1) ? task_cpu(p) : cpu;
>
>

Re: [PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[Steven Rostedt]

On Tue, 2010-10-19 at 13:02 +0200, Peter Zijlstra wrote:

> > Remove the check for nr_cpus_allowed of an RT task before calling
> > find_lowest_cpu() from select_task_rq_rt function. Cause
> > find_lowest_cpu checks for rt.nr_cpus_allowed of a process.
> > It would've been nice, if we make the check before calling
> > find_lowest_cpu. But its called from other places, so keep the
> > nr_cpus_allowed check into find_lowest_cpu().
> 

Yes it is redundant, but I'm wondering if we should modify
find_lowest_rq() just to short cut this case.

That is, instead of assigning all the variables on the stack, which I'm
thinking __get_cpu_var() may act like a barrier() where gcc can't
optimize. We should probably also do the following:

static int find_lowest_rq(struct task_struct *task)
{
	struct sched_domain *sd;
-	struct cpumask *lowest_mask = __get_cpu_var(local_cpu_mask);
-	int this_cpu = smp_processor_id();
-	int cpu      = task_cpu(task);
+	struct cpumask *lowest_mask;
+	int this_cpu;
+	int cpu;

	if (task->rt.nr_cpus_allowed == 1)
		return -1; /* No other targets possible */

+	lowest_mask = __get_cpu_var(local_cpu_mask);
+	this_cpu = smp_processor_id();
+	cpu = task_cpu(task;


Then removing the redundant check may not hurt anything.

-- Steve


> > Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> > ---
> > 
> > --- linus-rc8/kernel/sched_rt.c	2010-10-15 05:26:43.000000000 +0600
> > +++ rakib-rc8/kernel/sched_rt.c	2010-10-19 16:22:30.000000000 +0600
> > @@ -971,8 +971,7 @@ select_task_rq_rt(struct rq *rq, struct
> >  	 * that is just being woken and probably will have
> >  	 * cold cache anyway.
> >  	 */
> > -	if (unlikely(rt_task(rq->curr)) &&
> > -	    (p->rt.nr_cpus_allowed > 1)) {
> > +	if (unlikely(rt_task(rq->curr))) {
> >  		int cpu = find_lowest_rq(p);
> > 
> >  		return (cpu == -1) ? task_cpu(p) : cpu;

Re: [PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[Gregory Haskins]

>>> On 10/19/2010 at 07:02 AM, in message <1287486167.1994.1.camel@twins>, Peter
Zijlstra <peterz@infradead.org> wrote: 
> On Tue, 2010-10-19 at 16:57 +0600, Rakib Mullick wrote:

[snip]

>> --- linus-rc8/kernel/sched_rt.c	2010-10-15 05:26:43.000000000 +0600
>> +++ rakib-rc8/kernel/sched_rt.c	2010-10-19 16:22:30.000000000 +0600
>> @@ -971,8 +971,7 @@ select_task_rq_rt(struct rq *rq, struct
>>  	 * that is just being woken and probably will have
>>  	 * cold cache anyway.
>>  	 */
>> -	if (unlikely(rt_task(rq->curr)) &&
>> -	    (p->rt.nr_cpus_allowed > 1)) {

I think the motivation here was that checking nr_cpus_allowed is far cheaper than taking the hit on a function call in this particularly hot path.  As Steven points out in a follow-up reply, the function call has additional overhead before the equivalent check is made again.  We could possibly optimize this with some of the suggestions he made, but I am not sure if it is worth it (alone) as the call overhead would still be present.  OTOH, the cases where nr_cpus_allowed <= 1 are probably rare in the grand scheme of things.

My opinion is the check should probably remain (if but perhaps with a comment to explain its existence) unless someone (Rakib, hint hint) is willing to do some benchmarking to demonstrate that it doesn't actually have any positive impact.  It probably also makes sense to take Steve's suggested changes to improve the places that use the function without external optimization.

Kind Regards,
-Greg

Re: [PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[Steven Rostedt]

On Tue, 2010-10-19 at 06:57 -0600, Gregory Haskins wrote:
> >>> On 10/19/2010 at 07:02 AM, in message <1287486167.1994.1.camel@twins>, Peter
> Zijlstra <peterz@infradead.org> wrote: 
> > On Tue, 2010-10-19 at 16:57 +0600, Rakib Mullick wrote:
> 
> [snip]
> 
> >> --- linus-rc8/kernel/sched_rt.c	2010-10-15 05:26:43.000000000 +0600
> >> +++ rakib-rc8/kernel/sched_rt.c	2010-10-19 16:22:30.000000000 +0600
> >> @@ -971,8 +971,7 @@ select_task_rq_rt(struct rq *rq, struct
> >>  	 * that is just being woken and probably will have
> >>  	 * cold cache anyway.
> >>  	 */
> >> -	if (unlikely(rt_task(rq->curr)) &&
> >> -	    (p->rt.nr_cpus_allowed > 1)) {
> 
> I think the motivation here was that checking nr_cpus_allowed is far
> cheaper than taking the hit on a function call in this particularly
> hot path.  As Steven points out in a follow-up reply, the function
> call has additional overhead before the equivalent check is made
> again.  We could possibly optimize this with some of the suggestions
> he made, but I am not sure if it is worth it (alone) as the call
> overhead would still be present.  OTOH, the cases where
> nr_cpus_allowed <= 1 are probably rare in the grand scheme of things.
> 
> My opinion is the check should probably remain (if but perhaps with a
> comment to explain its existence) unless someone (Rakib, hint hint) is
> willing to do some benchmarking to demonstrate that it doesn't
> actually have any positive impact.  It probably also makes sense to
> take Steve's suggested changes to improve the places that use the
> function without external optimization.

Yeah, it probably is not worth removing the check here, as a function
call will add overhead.

And do not think that it is a unlikely case to have an RT task pinned to
a CPU. In true RT systems, that should be the norm. Any benchmark should
test the impact on tasks that are pinned to a CPU, not a general
scenario.

-- Steve

Re: [PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[Gregory Haskins]

>>> On 10/19/2010 at 09:01 AM, in message
<1287493260.16971.364.camel@gandalf.stny.rr.com>, Steven Rostedt
<rostedt@goodmis.org> wrote: 
> On Tue, 2010-10-19 at 06:57 -0600, Gregory Haskins wrote:
>> >>> On 10/19/2010 at 07:02 AM, in message <1287486167.1994.1.camel@twins>, Peter
>> Zijlstra <peterz@infradead.org> wrote: 
>> > On Tue, 2010-10-19 at 16:57 +0600, Rakib Mullick wrote:
>> 
>> [snip]
>> 
>> >> --- linus-rc8/kernel/sched_rt.c	2010-10-15 05:26:43.000000000 +0600
>> >> +++ rakib-rc8/kernel/sched_rt.c	2010-10-19 16:22:30.000000000 +0600
>> >> @@ -971,8 +971,7 @@ select_task_rq_rt(struct rq *rq, struct
>> >>  	 * that is just being woken and probably will have
>> >>  	 * cold cache anyway.
>> >>  	 */
>> >> -	if (unlikely(rt_task(rq->curr)) &&
>> >> -	    (p->rt.nr_cpus_allowed > 1)) {
>> 
>> I think the motivation here was that checking nr_cpus_allowed is far
>> cheaper than taking the hit on a function call in this particularly
>> hot path.  As Steven points out in a follow-up reply, the function
>> call has additional overhead before the equivalent check is made
>> again.  We could possibly optimize this with some of the suggestions
>> he made, but I am not sure if it is worth it (alone) as the call
>> overhead would still be present.  OTOH, the cases where
>> nr_cpus_allowed <= 1 are probably rare in the grand scheme of things.
>> 
>> My opinion is the check should probably remain (if but perhaps with a
>> comment to explain its existence) unless someone (Rakib, hint hint) is
>> willing to do some benchmarking to demonstrate that it doesn't
>> actually have any positive impact.  It probably also makes sense to
>> take Steve's suggested changes to improve the places that use the
>> function without external optimization.
> 
> Yeah, it probably is not worth removing the check here, as a function
> call will add overhead.
> 
> And do not think that it is a unlikely case to have an RT task pinned to
> a CPU. In true RT systems, that should be the norm. Any benchmark should
> test the impact on tasks that are pinned to a CPU, not a general
> scenario.
> 

Yes, agreed and excellent point, Steve.

-Greg

Re: [PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[Rakib Mullick]

On 10/19/10, Gregory Haskins <ghaskins@novell.com> wrote:
>>>> On 10/19/2010 at 07:02 AM, in message <1287486167.1994.1.camel@twins>,
>>>> Peter
> Zijlstra <peterz@infradead.org> wrote:
>> On Tue, 2010-10-19 at 16:57 +0600, Rakib Mullick wrote:
>
> I think the motivation here was that checking nr_cpus_allowed is far cheaper
> than taking the hit on a function call in this particularly hot path.  As
> Steven points out in a follow-up reply, the function call has additional
> overhead before the equivalent check is made again.  We could possibly
> optimize this with some of the suggestions he made, but I am not sure if it
> is worth it (alone) as the call overhead would still be present.  OTOH, the
> cases where nr_cpus_allowed <= 1 are probably rare in the grand scheme of
> things.
>
I also think that checking nr_cpus_allowed before calling
find_lowest_rq is cheaper. We can take even cheaper way by introducing
the same check in find_lock_lowest_rq(), which also calls
find_lowest_rq. Scenario in find_lock_lowest_rq is even worse, a check
for nr_cpus_allowed (maybe even before for loop) will reduce more
overhead and removing the  nr_cpus_allowed check completely from
find_lowest_rq. And it will keep find_lowest_rq simple. We can also
made a comment whenever someone is calling find_lowest_rq, should
check nr_cpus_allowed first. (I didn't propose it in patch, cause it's
duplicate check. And it looks clean).

> My opinion is the check should probably remain (if but perhaps with a
> comment to explain its existence) unless someone (Rakib, hint hint) is
> willing to do some benchmarking to demonstrate that it doesn't actually have
> any positive impact.  It probably also makes sense to take Steve's suggested
> changes to improve the places that use the function without external
> optimization.
>
If we made explicit check before calling find_lowest_rq, then I don't
think we need the change that Steve's suggesting. I think explicitly
checking is much more easier and removes extra overhead of function
calling.

Thanks,
Rakib
> Kind Regards,
> -Greg
>
>
>

Re: [PATCH] sched_rt: Removes extra checking for nr_cpus_allowed when calling find_lowest_rq

[Rakib Mullick]

On Tue, Oct 19, 2010 at 8:40 PM, Rakib Mullick <rakib.mullick@gmail.com> wrote:
> On 10/19/10, Gregory Haskins <ghaskins@novell.com> wrote:
>>>>> On 10/19/2010 at 07:02 AM, in message <1287486167.1994.1.camel@twins>,
>>>>> Peter
>> Zijlstra <peterz@infradead.org> wrote:
>>> On Tue, 2010-10-19 at 16:57 +0600, Rakib Mullick wrote:
>>
> If we made explicit check before calling find_lowest_rq, then I don't
> think we need the change that Steve's suggesting. I think explicitly
> checking is much more easier and removes extra overhead of function
> calling.

The following patch shows what I was trying to say. Please check and
comment. Hopefully this looks clean.


--- linus-rc8/kernel/sched_rt.c	2010-10-19 16:42:05.000000000 +0600
+++ rakib-rc8/kernel/sched_rt.c	2010-10-20 10:04:08.000000000 +0600
@@ -1174,9 +1174,6 @@ static int find_lowest_rq(struct task_st
 	int this_cpu = smp_processor_id();
 	int cpu      = task_cpu(task);

-	if (task->rt.nr_cpus_allowed == 1)
-		return -1; /* No other targets possible */
-
 	if (!cpupri_find(&task_rq(task)->rd->cpupri, task, lowest_mask))
 		return -1; /* No targets found */

@@ -1238,6 +1235,9 @@ static struct rq *find_lock_lowest_rq(st
 	int tries;
 	int cpu;

+	if (task->rt.nr_cpus_allowed < 2)
+		goto out;
+
 	for (tries = 0; tries < RT_MAX_TRIES; tries++) {
 		cpu = find_lowest_rq(task);

@@ -1275,6 +1275,7 @@ static struct rq *find_lock_lowest_rq(st
 		lowest_rq = NULL;
 	}

+out:
 	return lowest_rq;
 }



Thanks,
Rakib

>
> Thanks,
> Rakib
>> Kind Regards,
>> -Greg
>>
>>
>>
>