Hi,


On 10/19/2017 12:18 AM, Tomasz Nowicki wrote:

On 18.10.2017 19:30, Jeremy Linton wrote:

quoted

On 10/18/2017 05:24 AM, Tomasz Nowicki wrote:

quoted

On 18.10.2017 07:39, Tomasz Nowicki wrote:

quoted

On 17.10.2017 17:22, Jeremy Linton wrote:

quoted

On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:

quoted

On 12.10.2017 21:48, Jeremy Linton wrote:

(trimming)

quoted

quoted

quoted

quoted

quoted

quoted

+            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.