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

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

Hi Nathan, 

Thanks for your reviews.

> > -	if ((nid = numa_cpu_lookup_table[lcpu]) >= 0) {
> > +	nid = numa_cpu_lookup_table[fcpu];
> > +	if (nid >= 0) {
> >  		map_cpu_to_node(lcpu, nid);
> >  		return nid;
> >  	}
> 
> Yes, we need to something like this to prevent a VPHN change that occurs
> concurrently with onlining a core's threads from messing us up.
> 
> Is it a good assumption that the first thread of a sibling group will
> have its mapping initialized first? I think the answer is yes for boot,
> but hotplug... not so sure.
> 
> 
> > @@ -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.

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?

-- 
Thanks and Regards
Srikar Dronamraju