Hi Jeremy,
On 2017/9/22 2:48, Jeremy Linton wrote:
> Hi,
>
> On 09/20/2017 02:15 AM, Xiongfeng Wang wrote:
>> Hi Jeremy,
>>
>> On 2017/9/20 2:47, 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.
>>>
>>> +
>>> +static int topology_setup_acpi_cpu(struct acpi_table_header *table,
>>> + unsigned int cpu, int level)
>>> +{
>>> + struct acpi_pptt_processor *cpu_node;
>>> + u32 acpi_cpu_id = acpi_cpu_get_madt_gicc(cpu)->uid;
>>> +
>>> + cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
>>> + if (cpu_node) {
>>> + cpu_node = acpi_find_processor_package_id(table, cpu_node, level);
>>> + return (int)((u8 *)cpu_node - (u8 *)table);
>>> + }
>>> + pr_err_once("PPTT table found, but unable to locate core for %d\n",
>>> + cpu);
>>> + return -ENOENT;
>>
>> Can we return -1 when PPTT doesn't exist? So that we can still get topo info
>> from MPIDR.
>> 'store_cpu_topology()' determine whether cpu topology has been populated by
>> checking
>> whether cluster_id is -1. If cluster_id is not -1, it won't read cpu topo
>> info from MPIDR.
>> Or maybe we can change 'store_cpu_topology()' as well. If cluster_id is less
>> than zero,
>> we read cpu topo info from MPIDR.
>
> ? I will retest this, but any negative return from setup_acpi_topology() for
> the initial node (subsequent requests should minimally duplicate the cpu node
> rather than failing) request, should result in a call to
> reset_cpu_topology(), and the information being sourced from the MPIDR in
> store_cpu_topology(). There are various ways the tree could be "damaged" but
> if all the system cpus have matching valid acpi_id/cpu nodes, then that
> reflects a valid topology and there really isn't enough information to decide
> if its damaged. The one case where this isn't accurate is missing socket
> identifiers, but the code actually handles this as well as the lack of
> missing cluster/thread ids (which don't exist in the standard).
>
Yes, you are right. I didn't notice the function reset_cpu_topology() before.
Thanks for your explanation.
>>
>>> +}
>>> +
>>> +/*
>>> + * simply assign a ACPI cache entry to each known CPU cache entry
>>> + * determining which entries are shared is done later.
>>> + */
>>> +int cache_setup_acpi(unsigned int cpu)
>>> +{
>>> + struct acpi_table_header *table;
>>> + acpi_status status;
>>> +
>>> + pr_debug("Cache Setup ACPI cpu %d\n", cpu);
>>> +
>>> + status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
>>> + if (ACPI_FAILURE(status)) {
>>> + pr_err_once("No PPTT table found, cache topology may be
>>> inaccurate\n");
>>> + return -ENOENT;
>>> + }
>>> +
>>> + cache_setup_acpi_cpu(table, cpu);
>>> + acpi_put_table(table);
>>> +
>>> + return status;
>>> +}
>>> +
>>> +/*
>>> + * Determine a topology unique ID for each thread/core/cluster/socket/etc.
>>> + * This ID can then be used to group peers.
>>> + */
>>> +int setup_acpi_cpu_topology(unsigned int cpu, int level)
>>> +{
>>> + struct acpi_table_header *table;
>>> + acpi_status status;
>>> + int retval;
>>
>> Can we add a static int array to record already assigned id for each level?
>> So that we can count the id starting from zero. And also the id can be
>> successive.
>
> I don't like the idea of a node<->generated_id array/map in this module,
> although I've considered a number of ways to create more normalized values.
> Particularly, the one area that might be useful is utilizing the acpi id for
> the cores, which is problematic in the MT case. One of my other alternative
> ideas was to plug a unique node id into a reserved field in the table as the
> node is traversed. That idea is a little evil, and really should be part of
> the ACPI standard so the firmware is providing the ID's rather than dependent
> on the traversal order of the tree.
>
> If it turns out that these ID's need to be zero based, or some other
> limitation I would prefer just renumbering them in update_siblings_mask() or
> parse_acpi_topology(). I hacked together a patch to renumber the package_id
> yesterday, but i'm pretty sure I've seen (non arm) machines with odd
> package/core ids in the past, and I don't really see anything in the ABI the
> dictating this. Let me clean it up a bit and I can post it as an additional
> patch on top of the PPTT patches.
>
Yes, I agree. Renumbering the IDs in parse_acpi_topology() is better.
> So, is this an actual use case/problem, or its just an "appearance" type of
> thing?
>
It is not an actual use problem, just an "appearance".
Thanks,
Xiongfeng Wang
