Message-ID: <2605aa54-95f7-4619-e632-0a95a68e5fb0@caviumnetworks.com>
Date:   Thu, 19 Oct 2017 07:18:43 +0200
From:   Tomasz Nowicki <tnowicki@...iumnetworks.com>
To:     Jeremy Linton <jeremy.linton@....com>,
        Tomasz Nowicki <tnowicki@...iumnetworks.com>,
        linux-acpi@...r.kernel.org
Cc:     mark.rutland@....com, Jonathan.Zhang@...ium.com,
        Jayachandran.Nair@...ium.com, lorenzo.pieralisi@....com,
        austinwc@...eaurora.org, linux-pm@...r.kernel.org,
        jhugo@...eaurora.org, gregkh@...uxfoundation.org,
        sudeep.holla@....com, rjw@...ysocki.net,
        linux-kernel@...r.kernel.org, will.deacon@....com,
        wangxiongfeng2@...wei.com, viresh.kumar@...aro.org,
        hanjun.guo@...aro.org, catalin.marinas@....com, ahs3@...hat.com,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table
 parsing

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:
>>> Hi,
>>>
>>> On 17.10.2017 17:22, Jeremy Linton wrote:
>>>> Hi,
>>>>
>>>> On 10/17/2017 08:25 AM, Tomasz Nowicki wrote:
>>>>> Hi Jeremy,
>>>>>
>>>>> I did second round of review and have some more comments, please 
>>>>> see below:
>>>>>
>>>>> On 12.10.2017 21:48, Jeremy Linton wrote:
>>>>>> ACPI 6.2 adds a new table, which describes how processing units
>>>>>> are related to each other in tree like fashion. Caches are
>>>>>> also sprinkled throughout the tree and describe the properties
>>>>>> of the caches in relation to other caches and processing units.
>>>>>>
>>>>>> Add the code to parse the cache hierarchy and report the total
>>>>>> number of levels of cache for a given core using
>>>>>> acpi_find_last_cache_level() as well as fill out the individual
>>>>>> cores cache information with cache_setup_acpi() once the
>>>>>> cpu_cacheinfo structure has been populated by the arch specific
>>>>>> code.
>>>>>>
>>>>>> Further, report peers in the topology using setup_acpi_cpu_topology()
>>>>>> to report a unique ID for each processing unit at a given level
>>>>>> in the tree. These unique id's can then be used to match related
>>>>>> processing units which exist as threads, COD (clusters
>>>>>> on die), within a given package, etc.
>>>>>>
>>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@....com>
>>>>>> ---
>>>>>>   drivers/acpi/pptt.c | 485 
>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>   1 file changed, 485 insertions(+)
>>>>>>   create mode 100644 drivers/acpi/pptt.c
>>>>>>
>>>>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..c86715fed4a7
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/acpi/pptt.c
>>>>>> @@ -0,1 +1,485 @@
>>>>>> +/*
>>>>>> + * Copyright (C) 2017, ARM
>>>>>> + *
>>>>>> + * This program is free software; you can redistribute it and/or 
>>>>>> modify it
>>>>>> + * under the terms and conditions of the GNU General Public License,
>>>>>> + * version 2, as published by the Free Software Foundation.
>>>>>> + *
>>>>>> + * This program is distributed in the hope it will be useful, but 
>>>>>> WITHOUT
>>>>>> + * ANY WARRANTY; without even the implied warranty of 
>>>>>> MERCHANTABILITY or
>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
>>>>>> License for
>>>>>> + * more details.
>>>>>> + *
>>>>>> + * This file implements parsing of Processor Properties Topology 
>>>>>> Table (PPTT)
>>>>>> + * which is optionally used to describe the processor and cache 
>>>>>> topology.
>>>>>> + * Due to the relative pointers used throughout the table, this 
>>>>>> doesn't
>>>>>> + * leverage the existing subtable parsing in the kernel.
>>>>>> + */
>>>>>> +#define pr_fmt(fmt) "ACPI PPTT: " fmt
>>>>>> +
>>>>>> +#include <linux/acpi.h>
>>>>>> +#include <linux/cacheinfo.h>
>>>>>> +#include <acpi/processor.h>
>>>>>> +
>>>>>> +/*
>>>>>> + * Given the PPTT table, find and verify that the subtable entry
>>>>>> + * is located within the table
>>>>>> + */
>>>>>> +static struct acpi_subtable_header *fetch_pptt_subtable(
>>>>>> +    struct acpi_table_header *table_hdr, u32 pptt_ref)
>>>>>> +{
>>>>>> +    struct acpi_subtable_header *entry;
>>>>>> +
>>>>>> +    /* there isn't a subtable at reference 0 */
>>>>>> +    if (!pptt_ref)
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    if (pptt_ref + sizeof(struct acpi_subtable_header) > 
>>>>>> table_hdr->length)
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + 
>>>>>> pptt_ref);
>>>>>> +
>>>>>> +    if (pptt_ref + entry->length > table_hdr->length)
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    return entry;
>>>>>> +}
>>>>>> +
>>>>>> +static struct acpi_pptt_processor *fetch_pptt_node(
>>>>>> +    struct acpi_table_header *table_hdr, u32 pptt_ref)
>>>>>> +{
>>>>>> +    return (struct acpi_pptt_processor 
>>>>>> *)fetch_pptt_subtable(table_hdr, pptt_ref);
>>>>>> +}
>>>>>> +
>>>>>> +static struct acpi_pptt_cache *fetch_pptt_cache(
>>>>>> +    struct acpi_table_header *table_hdr, u32 pptt_ref)
>>>>>> +{
>>>>>> +    return (struct acpi_pptt_cache 
>>>>>> *)fetch_pptt_subtable(table_hdr, pptt_ref);
>>>>>> +}
>>>>>> +
>>>>>> +static struct acpi_subtable_header *acpi_get_pptt_resource(
>>>>>> +    struct acpi_table_header *table_hdr,
>>>>>> +    struct acpi_pptt_processor *node, int resource)
>>>>>> +{
>>>>>> +    u32 ref;
>>>>>> +
>>>>>> +    if (resource >= node->number_of_priv_resources)
>>>>>> +        return NULL;
>>>>>> +
>>>>>> +    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) +
>>>>>> +              sizeof(u32) * resource);
>>>>>> +
>>>>>> +    return fetch_pptt_subtable(table_hdr, ref);
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * given a pptt resource, verify that it is a cache node, then walk
>>>>>> + * down each level of caches, counting how many levels are found
>>>>>> + * as well as checking the cache type (icache, dcache, unified). 
>>>>>> If a
>>>>>> + * level & type match, then we set found, and continue the search.
>>>>>> + * Once the entire cache branch has been walked return its max
>>>>>> + * depth.
>>>>>> + */
>>>>>> +static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
>>>>>> +                int local_level,
>>>>>> +                struct acpi_subtable_header *res,
>>>>>> +                struct acpi_pptt_cache **found,
>>>>>> +                int level, int type)
>>>>>> +{
>>>>>> +    struct acpi_pptt_cache *cache;
>>>>>> +
>>>>>> +    if (res->type != ACPI_PPTT_TYPE_CACHE)
>>>>>> +        return 0;
>>>>>> +
>>>>>> +    cache = (struct acpi_pptt_cache *) res;
>>>>>> +    while (cache) {
>>>>>> +        local_level++;
>>>>>> +
>>>>>> +        if ((local_level == level) &&
>>>>>> +            (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
>>>>>> +            ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == 
>>>>>> type)) {
>>>>>
>>>>> Attributes have to be shifted:
>>>>>
>>>>> (cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2
>>>>
>>>> Hmmm, I'm not sure that is true, the top level function in this 
>>>> routine convert the "linux" constant to the ACPI version of that 
>>>> constant. In that case the "type" field is pre-shifted, so that it 
>>>> matches the result of just anding against the field... That is 
>>>> unless I messed something up, which I don't see at the moment (and 
>>>> the code of course has been tested with PPTT's from multiple people 
>>>> at this point).
>>>
>>> For ThunderX2 I got lots of errors in dmesg:
>>> Found duplicate cache level/type unable to determine uniqueness
>>>
>>> So I fixed "type" macros definitions (without shifting) and shift it 
>>> here which fixes the issue. As you said, it can be pre-shifted as well.
> 
> Ah, yah right... If you removed the shift per your original comment then 
> it breaks this. Yes, and the type definitions for cache type aren't 
> wrong in this version because the unified state has the 3rd bit set for 
> both the 0x3 and 0x2 values and its only used to covert from the linux 
> type to the ACPI type (and not back because we don't mess with whatever 
> the original "detection" was). I'm not really planning on changing that 
> because I don't think it helps "readability" (and it converts a compile 
> time constant to a runtime shift).
> 
>>>
>>>>
>>>>
>>>>>
>>>>>> +            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.

Thanks,
Tomasz

Powered by blists - more mailing lists