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

[Jeremy Linton]

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.