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

[Steven Rostedt <rostedt@goodmis.org>]

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