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

[Gregory Haskins]

>>> On 10/19/2010 at 09:01 AM, in message
<1287493260.16971.364.camel@xxxxxxxxxxxxxxxxxxx>, Steven Rostedt
<rostedt@xxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/