Re: [PATCH v7 5/6] powerpc/pseries: Add support for FORM2 associativity

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 8/12/21 7:11 AM, David Gibson wrote:
> On Wed, Aug 11, 2021 at 09:39:32AM +0530, Aneesh Kumar K.V wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>>
>>> On Mon, Aug 09, 2021 at 10:54:33AM +0530, Aneesh Kumar K.V wrote:
>>>> PAPR interface currently supports two different ways of communicating resource
>>>> grouping details to the OS. These are referred to as Form 0 and Form 1
>>>> associativity grouping. Form 0 is the older format and is now considered
>>>> deprecated. This patch adds another resource grouping named FORM2.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>>
>>> LGTM, with the exception of some minor nits noted below.
>> ...
>>
>>> +
>>>> +	for (i = 0; i < max_numa_index; i++)
>>>> +		/* +1 skip the max_numa_index in the property */
>>>> +		numa_id_index_table[i] = of_read_number(&numa_lookup_index[i + 1], 1);
>>>> +
>>>> +
>>>> +	if (numa_dist_table_length != max_numa_index * max_numa_index) {
>>>> +
>>>
>>> Stray extra whitespace line here.
>>>
>>>> +		WARN(1, "Wrong NUMA distance information\n");
>>>> +		/* consider everybody else just remote. */
>>>> +		for (i = 0;  i < max_numa_index; i++) {
>>>> +			for (j = 0; j < max_numa_index; j++) {
>>>> +				int nodeA = numa_id_index_table[i];
>>>> +				int nodeB = numa_id_index_table[j];
>>>> +
>>>> +				if (nodeA == nodeB)
>>>> +					numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
>>>> +				else
>>>> +					numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
>>>> +			}
>>>> +		}
>>>
>>> I don't think it's necessarily a problem, but something to consider is
>>> that this fallback will initialize distance for *all* node IDs,
>>> whereas the normal path will only initialize it for nodes that are in
>>> the index table.  Since some later error checks key off whether
>>> certain fields in the distance table are initialized, is that the
>>> outcome you want?
>>>
>>
>> With the device tree details not correct, one of the possible way to
>> make progress is to consider everybody remote. With new node hotplug
>> support we used to check whether the distance table entry is
>> initialized. With the updated spec, we expect all possible numa node
>> distance to be available during boot.
> 
> Sure.  But my main point here is that the fallback behaviour in this
> clause is different from the fallback behaviour if the table is there
> and parseable, but incomplete - which is also not expected.
> 

With FORM2 fallback with bad device tree details is to consider 
everybody REMOTE. With Form1, we leave the distance table not populated 
as it was with the current kernel versions.

-aneesh