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

From: Jeremy Linton
Date: Thu Oct 19 2017 - 10:24:22 EST


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.