Re: [PATCH v3 3/5] powerpc/numa: Use cpu node map of first sibling thread

[Nathan Lynch <nathanl@linux.ibm.com>]

Hi Srikar,

Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
>> > @@ -496,6 +501,16 @@ static int numa_setup_cpu(unsigned long lcpu)
>> >  	if (nid < 0 || !node_possible(nid))
>> >  		nid = first_online_node;
>> >  
>> > +	/*
>> > +	 * Update for the first thread of the core. All threads of a core
>> > +	 * have to be part of the same node. This not only avoids querying
>> > +	 * for every other thread in the core, but always avoids a case
>> > +	 * where virtual node associativity change causes subsequent threads
>> > +	 * of a core to be associated with different nid.
>> > +	 */
>> > +	if (fcpu != lcpu)
>> > +		map_cpu_to_node(fcpu, nid);
>> > +
>> 
>> OK, I see that this somewhat addresses my concern above. But changing
>> this mapping for a remote cpu is unsafe except under specific
>> circumstances. I think this should first assert:
>> 
>> * numa_cpu_lookup_table[fcpu] == NUMA_NO_NODE
>> * cpu_online(fcpu) == false
>> 
>> to document and enforce the conditions that must hold for this to be OK.
>
> I do understand that we shouldn't be modifying the nid for a different cpu.
>
> We just checked above that the mapping for the first cpu doesnt exist.
> If the first cpu (or remote cpu as you coin it) was online, then its
> mapping should have existed and we return even before we come here.

I agree that is how the code will work with your change, and I'm fine
with simply warning if fcpu is offline.

The point is to make this rule more explicit in the code for the benefit
of future readers and to catch violations of it by future changes. There
is a fair amount of code remaining in this file and elsewhere in
arch/powerpc that was written under the impression that changing the
cpu-node relationship at runtime is OK.


> nid = numa_cpu_lookup_table[fcpu];
> if (nid >= 0) {
> 	map_cpu_to_node(lcpu, nid);
> 	return nid;
> }
>
> Currently numa_setup_cpus is only called at very early boot and in cpu
> hotplug. At hotplug time, the oneline of cpus is serialized. Right? Do we 
> see a chance of remote cpu changing its state as we set its nid here?
>
> Also lets say if we assert and for some unknown reason the assertion fails.
> How do we handle the failure case?  We cant get out without setting
> the nid. We cant continue setting the nid. Should we panic the system given
> that the check a few lines above is now turning out to be false? Probably
> no, as I think we can live with it.
>
> Any thoughts?

I think just WARN_ON(cpu_online(fcpu)) would be satisfactory. In my
experience, the downstream effects of violating this condition are
varied and quite difficult to debug. Seems only appropriate to emit a
warning and stack trace before the OS inevitably becomes unstable.