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

[Dave Kleikamp]

On Mon, 2009-08-03 at 11:21 -0500, Kumar Gala wrote:
> 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_last_thread_in_core(cpu) is a moving target.  You want something
like:

cpu = cpu_first_thread_in_core(cpu);
last = cpu_last_thread_in_core(cpu);
while (cpu <= last) {
	__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

-- 
David Kleikamp
IBM Linux Technology Center