Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton <jeremy.linton@arm.com>]

Hi,


On 10/19/2017 12:18 AM, Tomasz Nowicki wrote:
> On 18.10.2017 19:30, Jeremy Linton wrote:
>> On 10/18/2017 05:24 AM, Tomasz Nowicki wrote:
>>> On 18.10.2017 07:39, Tomasz Nowicki wrote:
>>>> On 17.10.2017 17:22, Jeremy Linton wrote:
>>>>> On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:
>>>>>> On 12.10.2017 21:48, Jeremy Linton wrote:
(trimming)
>>>>>>> +            if (*found != NULL)
>>>>>>> +                pr_err("Found duplicate cache level/type unable 
>>>>>>> to determine uniqueness\n");
>>>
>>> Actually I still see this error messages in my dmesg. It is because 
>>> the following ThunderX2 per-core L1 and L2 cache hierarchy:
>>>
>>> Core
>>>   ------------------
>>> |                  |
>>> | L1i -----        |
>>> |         |        |
>>> |          ----L2  |
>>> |         |        |
>>> | L1d -----        |
>>> |                  |
>>>   ------------------
>>>
>>> In this case we have two paths which lead to L2 cache and hit above 
>>> case. Is it really error case?
>>
>> No, but its not deterministic unless we mark the node, which doesn't 
>> solve the problem of a table constructed like
>>
>> L1i->L2 (unified)
>> L1d->L2 (unified)
>>
>> or various other structures which aren't disallowed by the spec and 
>> have non-deterministic real world meanings, anymore than constructing 
>> the table like:
>>
>> L1i
>> Lid->L2(unified)
>>
>> which I tend to prefer because with a structuring like that it can be 
>> deterministic (and in a way actually represents the non-coherent 
>> behavior of (most?) ARM64 core's i-caches, as could be argued the 
>> first example if the allocation policies are varied between the L2 
>> nodes).
>>
>> The really ugly bits here happen if you add another layer:
>>
>> L1i->L2i-L3
>> L1d------^
>>
>> which is why I made that an error message, not including the fact that 
>> since the levels aren't tagged the numbering and meaning isn't clear.
>>
>> (the L1i in the above example might be better called an L0i to avoid 
>> throwing off the reset of the hierarchy numbering, also so it could be 
>> ignored).
>>
>> Summary:
>>
>> I'm not at all happy with this specification's attempt to leave out 
>> pieces of information which make parsing things more deterministic. In 
>> this case I'm happy to demote the message level, but not remove it 
>> entirely but I do think the obvious case you list shouldn't be the 
>> default one.
>>
>> Lastly:
>>
>> I'm assuming the final result is that the table is actually being 
>> parsed correctly despite the ugly message?
> 
> Indeed, the ThunderX2 PPTT table is being parsed so that topology shown 
> in lstopo and lscpu is correct.

Great.

Also, I think this is a better change:

      if ((*found != NULL) && (*found != cache))
          pr_err("Found duplicate cache level/type unable to determine 
uniqueness\n");

Which if its a duplicate node/type at the given level the message is 
just suppressed. It will of course still trigger in cases like:

L1d->L2
l1i->L2

or other odd cases.