[PATCH 3/20] powerpc/mm: Add HW threads support to no_hash TLB management

[Kumar Gala]

On Aug 2, 2009, at 9:03 PM, Michael Ellerman wrote:

> On Sat, 2009-08-01 at 08:29 +1000, Benjamin Herrenschmidt wrote:
>> On Thu, 2009-07-30 at 22:35 -0500, Kumar Gala wrote:
>>>>              /* XXX This clear should ultimately be part of
>>> local_flush_tlb_mm */
>>>> -             __clear_bit(id, stale_map[cpu]);
>>>> +             for (cpu = cpu_first_thread_in_core(cpu);
>>>> +                  cpu <= cpu_last_thread_in_core(cpu); cpu++)
>>>> +                     __clear_bit(id, stale_map[cpu]);
>>>>      }
>>>
>>> This looks a bit dodgy.  using 'cpu' as both the loop variable and
>>> what you are computing to determine loop start/end..
>>>
>> Hrm... I would have thought that it was still correct... do you see  
>> any
>> reason why the above code is wrong ? because if not we may be  
>> hitting a
>> gcc issue...
>>
>> IE. At loop init, cpu gets clamped down to the first thread in the  
>> core,
>> which should be fine. Then, we compare CPU to the last thread in core
>> for the current CPU which should always return the same value.
>>
>> So I'm very interested to know what is actually wrong, ie, either I'm
>> just missing something obvious, or you are just pushing a bug under  
>> the
>> carpet which could come back and bit us later :-)
>
> for (cpu = cpu_first_thread_in_core(cpu);
>     cpu <= cpu_last_thread_in_core(cpu); cpu++)
>        __clear_bit(id, stale_map[cpu]);
>
> ==
>
> cpu = cpu_first_thread_in_core(cpu);
> while (cpu <= cpu_last_thread_in_core(cpu)) {
> 	__clear_bit(id, stale_map[cpu]);
> 	cpu++;
> }
>
> cpu = 0
> cpu <= 1
> cpu++ (1)
> cpu <= 1
> cpu++ (2)
> cpu <= 3
> ...

Which is pretty much what I see, in a dual core setup, I get an oops  
because we are trying to clear cpu #2 (which clearly doesn't exist)

cpu = 1
(in loop)
	clearing 1
	clearing 2
OOPS

- k