Re: [patch, 2.6.10-rc2] sched: fix ->nr_uninterruptible handling bugs

[Nick Piggin <nickpiggin@yahoo.com.au>]

Peter Williams wrote:
> Nick Piggin wrote:
> 
>> Ingo Molnar wrote:
>>
>>> PREEMPT_RT on SMP systems triggered weird (very high) load average
>>> values rather easily, which turned out to be a mainline kernel
>>> ->nr_uninterruptible handling bug in try_to_wake_up().
>>>
>>> the following code:
>>>
>>>         if (old_state == TASK_UNINTERRUPTIBLE) {
>>>                 old_rq->nr_uninterruptible--;
>>>
>>> potentially executes with old_rq potentially being != rq, and hence
>>> updating ->nr_uninterruptible without the lock held. Given a
>>> sufficiently concurrent preemption workload the count can get out of
>>> whack and updates might get lost, permanently skewing the global 
>>> count. Nothing except the load-average uses nr_uninterruptible() so this
>>> condition can go unnoticed quite easily.
>>>
>>
>> Hi Ingo,
>> Yes you're right.
>>
>> I have another idea. Revert back to the old code, then just transfer
>> the nr_uninterruptible count when migrating a task. That way, the
> 
> 
> I presume that you mean adjust rather than transfer.
> 
>> rq's nr_uninterruptible field always is a measure of the number of
>> uninterruptible tasks on it. What do you think?
> 
> 
> To make this work you need to do the adjustment every where that a task 
> changes CPU while in the UNINTERRUPTIBLE state.  Are both run queue 
> locks always held in these circumstances?  I don't think that they are 
> in try_to_wake_up() but it may be possible to work around that.
> 

Yeah this won't actually work of course, because a task can set itself
UNINTERRUPTIBLE and subsequently get preempted then moved CPUs before
calling schedule() itself.

And yeah I missed the original point of your fix which was due to the
task moving runqueues in try_to_wake_up. Sorry, forget about the patch :P