LinuxLists.cc - [PATCH v5 0/9] Support PPTT for ARM64

[permalink] [raw]

Subject: [PATCH v5 2/9] ACPI/PPTT: Add Processor Properties Topology Table parsing

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.

An additional patch later in the set adds the ability to report
peers in the topology using find_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 <[email protected]>
---
drivers/acpi/pptt.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 476 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..0f8a1631af33
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,0 +1,476 @@
+/*
+ * 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.
+ *
+ * The PPTT structure is an inverted tree, with each node potentially
+ * holding one or two inverted tree data structures describing
+ * the caches available at that level. Each cache structure optionally
+ * contains properties describing the cache at a given level which can be
+ * used to override hardware probed values.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include <linux/acpi.h>
+#include <linux/cacheinfo.h>
+#include <acpi/processor.h>
+
+/* total number of attributes checked by the properties code */
+#define PPTT_CHECKED_ATTRIBUTES 6
+
+/*
+ * 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 < sizeof(struct acpi_subtable_header))
+ return NULL;
+
+ if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+ return NULL;
+
+ entry = ACPI_ADD_PTR(struct acpi_subtable_header, 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 = ACPI_ADD_PTR(u32, node, sizeof(struct acpi_pptt_processor));
+ ref += resource;
+
+ return fetch_pptt_subtable(table_hdr, *ref);
+}
+
+/*
+ * Attempt to find a given cache level, while counting the max number
+ * of cache levels for the cache node.
+ *
+ * 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)) {
+ if ((*found != NULL) && (cache != *found))
+ pr_err("Found duplicate cache level/type unable to determine uniqueness\n");
+
+ pr_debug("Found cache @ level %d\n", level);
+ *found = cache;
+ /*
+ * continue looking at this node's resource list
+ * to verify that we don't find a duplicate
+ * cache node.
+ */
+ }
+ cache = fetch_pptt_cache(table_hdr, cache->next_level_of_cache);
+ }
+ return local_level;
+}
+
+/*
+ * Given a CPU node look for cache levels that exist at this level, and then
+ * for each cache node, count how many levels exist below (logically above) it.
+ * If a level and type are specified, and we find that level/type, abort
+ * processing and return the acpi_pptt_cache structure.
+ */
+static struct acpi_pptt_cache *acpi_find_cache_level(
+ struct acpi_table_header *table_hdr,
+ struct acpi_pptt_processor *cpu_node,
+ int *starting_level, int level, int type)
+{
+ struct acpi_subtable_header *res;
+ int number_of_levels = *starting_level;
+ int resource = 0;
+ struct acpi_pptt_cache *ret = NULL;
+ int local_level;
+
+ /* walk down from processor node */
+ while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) {
+ resource++;
+
+ local_level = acpi_pptt_walk_cache(table_hdr, *starting_level,
+ res, &ret, level, type);
+ /*
+ * we are looking for the max depth. Since its potentially
+ * possible for a given node to have resources with differing
+ * depths verify that the depth we have found is the largest.
+ */
+ if (number_of_levels < local_level)
+ number_of_levels = local_level;
+ }
+ if (number_of_levels > *starting_level)
+ *starting_level = number_of_levels;
+
+ return ret;
+}
+
+/*
+ * Given a processor node containing a processing unit, walk into it and count
+ * how many levels exist solely for it, and then walk up each level until we hit
+ * the root node (ignore the package level because it may be possible to have
+ * caches that exist across packages). Count the number of cache levels that
+ * exist at each level on the way up.
+ */
+static int acpi_process_node(struct acpi_table_header *table_hdr,
+ struct acpi_pptt_processor *cpu_node)
+{
+ int total_levels = 0;
+
+ do {
+ acpi_find_cache_level(table_hdr, cpu_node, &total_levels, 0, 0);
+ cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
+ } while (cpu_node);
+
+ return total_levels;
+}
+
+/*
+ * Determine if the *node parameter is a leaf node by iterating the
+ * PPTT table, looking for nodes which reference it.
+ * Return 0 if we find a node referencing the passed node,
+ * or 1 if we don't.
+ */
+static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
+ struct acpi_pptt_processor *node)
+{
+ struct acpi_subtable_header *entry;
+ unsigned long table_end;
+ u32 node_entry;
+ struct acpi_pptt_processor *cpu_node;
+
+ table_end = (unsigned long)table_hdr + table_hdr->length;
+ node_entry = ACPI_PTR_DIFF(node, table_hdr);
+ entry = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
+ sizeof(struct acpi_table_pptt));
+
+ while ((unsigned long)(entry + 1) < table_end) {
+ cpu_node = (struct acpi_pptt_processor *)entry;
+ if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
+ (cpu_node->parent == node_entry))
+ return 0;
+ entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
+ entry->length);
+ }
+ return 1;
+}
+
+/*
+ * Find the subtable entry describing the provided processor.
+ * This is done by iterating the PPTT table looking for processor nodes
+ * which have an acpi_processor_id that matches the acpi_cpu_id parameter
+ * passed into the function. If we find a node that matches this criteria
+ * we verify that its a leaf node in the topology rather than depending
+ * on the valid flag, which doesn't need to be set for leaf nodes.
+ */
+static struct acpi_pptt_processor *acpi_find_processor_node(
+ struct acpi_table_header *table_hdr,
+ u32 acpi_cpu_id)
+{
+ struct acpi_subtable_header *entry;
+ unsigned long table_end;
+ struct acpi_pptt_processor *cpu_node;
+
+ table_end = (unsigned long)table_hdr + table_hdr->length;
+ entry = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
+ sizeof(struct acpi_table_pptt));
+
+ /* find the processor structure associated with this cpuid */
+ while ((unsigned long)(entry + 1) < table_end) {
+ cpu_node = (struct acpi_pptt_processor *)entry;
+
+ if (entry->length == 0) {
+ pr_err("Invalid zero length subtable\n");
+ break;
+ }
+ if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
+ (acpi_cpu_id == cpu_node->acpi_processor_id) &&
+ acpi_pptt_leaf_node(table_hdr, cpu_node)) {
+ return (struct acpi_pptt_processor *)entry;
+ }
+
+ entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
+ entry->length);
+ }
+
+ return NULL;
+}
+
+static int acpi_find_cache_levels(struct acpi_table_header *table_hdr,
+ u32 acpi_cpu_id)
+{
+ int number_of_levels = 0;
+ struct acpi_pptt_processor *cpu;
+
+ cpu = acpi_find_processor_node(table_hdr, acpi_cpu_id);
+ if (cpu)
+ number_of_levels = acpi_process_node(table_hdr, cpu);
+
+ return number_of_levels;
+}
+
+/* Convert the linux cache_type to a ACPI PPTT cache type value */
+static u8 acpi_cache_type(enum cache_type type)
+{
+ switch (type) {
+ case CACHE_TYPE_DATA:
+ pr_debug("Looking for data cache\n");
+ return ACPI_PPTT_CACHE_TYPE_DATA;
+ case CACHE_TYPE_INST:
+ pr_debug("Looking for instruction cache\n");
+ return ACPI_PPTT_CACHE_TYPE_INSTR;
+ default:
+ case CACHE_TYPE_UNIFIED:
+ pr_debug("Looking for unified cache\n");
+ /*
+ * It is important that ACPI_PPTT_CACHE_TYPE_UNIFIED
+ * contains the bit pattern that will match both
+ * ACPI unified bit patterns because we use it later
+ * to match both cases.
+ */
+ return ACPI_PPTT_CACHE_TYPE_UNIFIED;
+ }
+}
+
+/* find the ACPI node describing the cache type/level for the given CPU */
+static struct acpi_pptt_cache *acpi_find_cache_node(
+ struct acpi_table_header *table_hdr, u32 acpi_cpu_id,
+ enum cache_type type, unsigned int level,
+ struct acpi_pptt_processor **node)
+{
+ int total_levels = 0;
+ struct acpi_pptt_cache *found = NULL;
+ struct acpi_pptt_processor *cpu_node;
+ u8 acpi_type = acpi_cache_type(type);
+
+ pr_debug("Looking for CPU %d's level %d cache type %d\n",
+ acpi_cpu_id, level, acpi_type);
+
+ cpu_node = acpi_find_processor_node(table_hdr, acpi_cpu_id);
+
+ while ((cpu_node) && (!found)) {
+ found = acpi_find_cache_level(table_hdr, cpu_node,
+ &total_levels, level, acpi_type);
+ *node = cpu_node;
+ cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
+ }
+
+ return found;
+}
+
+/*
+ * The ACPI spec implies that the fields in the cache structures are used to
+ * extend and correct the information probed from the hardware. In the case
+ * of arm64 the CCSIDR probing has been removed because it might be incorrect.
+ */
+static void update_cache_properties(struct cacheinfo *this_leaf,
+ struct acpi_pptt_cache *found_cache,
+ struct acpi_pptt_processor *cpu_node)
+{
+ int valid_flags = 0;
+
+ if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
+ this_leaf->size = found_cache->size;
+ valid_flags++;
+ }
+ if (found_cache->flags & ACPI_PPTT_LINE_SIZE_VALID) {
+ this_leaf->coherency_line_size = found_cache->line_size;
+ valid_flags++;
+ }
+ if (found_cache->flags & ACPI_PPTT_NUMBER_OF_SETS_VALID) {
+ this_leaf->number_of_sets = found_cache->number_of_sets;
+ valid_flags++;
+ }
+ if (found_cache->flags & ACPI_PPTT_ASSOCIATIVITY_VALID) {
+ this_leaf->ways_of_associativity = found_cache->associativity;
+ valid_flags++;
+ }
+ if (found_cache->flags & ACPI_PPTT_WRITE_POLICY_VALID) {
+ switch (found_cache->attributes & ACPI_PPTT_MASK_WRITE_POLICY) {
+ case ACPI_PPTT_CACHE_POLICY_WT:
+ this_leaf->attributes = CACHE_WRITE_THROUGH;
+ break;
+ case ACPI_PPTT_CACHE_POLICY_WB:
+ this_leaf->attributes = CACHE_WRITE_BACK;
+ break;
+ }
+ valid_flags++;
+ }
+ if (found_cache->flags & ACPI_PPTT_ALLOCATION_TYPE_VALID) {
+ switch (found_cache->attributes & ACPI_PPTT_MASK_ALLOCATION_TYPE) {
+ case ACPI_PPTT_CACHE_READ_ALLOCATE:
+ this_leaf->attributes |= CACHE_READ_ALLOCATE;
+ break;
+ case ACPI_PPTT_CACHE_WRITE_ALLOCATE:
+ this_leaf->attributes |= CACHE_WRITE_ALLOCATE;
+ break;
+ case ACPI_PPTT_CACHE_RW_ALLOCATE:
+ case ACPI_PPTT_CACHE_RW_ALLOCATE_ALT:
+ this_leaf->attributes |=
+ CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE;
+ break;
+ }
+ valid_flags++;
+ }
+ /*
+ * If all the above flags are valid, and the cache type is NOCACHE
+ * update the cache type as well.
+ */
+ if ((this_leaf->type == CACHE_TYPE_NOCACHE) &&
+ (valid_flags == PPTT_CHECKED_ATTRIBUTES))
+ this_leaf->type = CACHE_TYPE_UNIFIED;
+}
+
+/*
+ * Update the kernel cache information for each level of cache
+ * associated with the given acpi cpu.
+ */
+static void cache_setup_acpi_cpu(struct acpi_table_header *table,
+ unsigned int cpu)
+{
+ struct acpi_pptt_cache *found_cache;
+ struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
+ u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
+ struct cacheinfo *this_leaf;
+ unsigned int index = 0;
+ struct acpi_pptt_processor *cpu_node = NULL;
+
+ while (index < get_cpu_cacheinfo(cpu)->num_leaves) {
+ this_leaf = this_cpu_ci->info_list + index;
+ found_cache = acpi_find_cache_node(table, acpi_cpu_id,
+ this_leaf->type,
+ this_leaf->level,
+ &cpu_node);
+ pr_debug("found = %p %p\n", found_cache, cpu_node);
+ if (found_cache)
+ update_cache_properties(this_leaf,
+ found_cache,
+ cpu_node);
+
+ index++;
+ }
+}
+
+/**
+ * acpi_find_last_cache_level() - Determines the number of cache levels for a PE
+ * @cpu: Kernel logical cpu number
+ *
+ * Given a logical cpu number, returns the number of levels of cache represented
+ * in the PPTT. Errors caused by lack of a PPTT table, or otherwise, return 0
+ * indicating we didn't find any cache levels.
+ *
+ * Return: Cache levels visible to this core.
+ */
+int acpi_find_last_cache_level(unsigned int cpu)
+{
+ u32 acpi_cpu_id;
+ struct acpi_table_header *table;
+ int number_of_levels = 0;
+ acpi_status status;
+
+ pr_debug("Cache Setup find last level cpu=%d\n", cpu);
+
+ acpi_cpu_id = get_acpi_id_for_cpu(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");
+ } else {
+ number_of_levels = acpi_find_cache_levels(table, acpi_cpu_id);
+ acpi_put_table(table);
+ }
+ pr_debug("Cache Setup find last level level=%d\n", number_of_levels);
+
+ return number_of_levels;
+}
+
+/**
+ * cache_setup_acpi() - Override CPU cache topology with data from the PPTT
+ * @cpu: Kernel logical cpu number
+ *
+ * Updates the global cache info provided by cpu_get_cacheinfo()
+ * when there are valid properties in the acpi_pptt_cache nodes. A
+ * successful parse may not result in any updates if none of the
+ * cache levels have any valid flags set. Futher, a unique value is
+ * associated with each known CPU cache entry. This unique value
+ * can be used to determine whether caches are shared between cpus.
+ *
+ * Return: -ENOENT on failure to find table, or 0 on success
+ */
+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;
+}
--
2.13.5

2017-12-01 22:24:02

[permalink] [raw]

Subject: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

Add a entry to to struct cacheinfo to maintain a reference to the PPTT
node which can be used to match identical caches across cores. Also
stub out cache_setup_acpi() so that individual architectures can
enable ACPI topology parsing.

Signed-off-by: Jeremy Linton <[email protected]>
---
drivers/acpi/pptt.c | 1 +
drivers/base/cacheinfo.c | 20 ++++++++++++++------
include/linux/cacheinfo.h | 13 ++++++++++++-
3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index 0f8a1631af33..a35e457cefb7 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
{
int valid_flags = 0;

+ this_leaf->firmware_node = cpu_node;
if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
this_leaf->size = found_cache->size;
valid_flags++;
diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index eb3af2739537..ba89f9310e6f 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
struct cacheinfo *sib_leaf)
{
- return sib_leaf->of_node == this_leaf->of_node;
+ if (acpi_disabled)
+ return sib_leaf->of_node == this_leaf->of_node;
+ else
+ return sib_leaf->firmware_node == this_leaf->firmware_node;
}

/* OF properties to query for a given cache type */
@@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
}
#endif

+int __weak cache_setup_acpi(unsigned int cpu)
+{
+ return -ENOTSUPP;
+}
+
static int cache_shared_cpu_map_setup(unsigned int cpu)
{
struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
@@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
if (this_cpu_ci->cpu_map_populated)
return 0;

- if (of_have_populated_dt())
+ if (!acpi_disabled)
+ ret = cache_setup_acpi(cpu);
+ else if (of_have_populated_dt())
ret = cache_setup_of_node(cpu);
- else if (!acpi_disabled)
- /* No cache property/hierarchy support yet in ACPI */
- ret = -ENOTSUPP;
+
if (ret)
return ret;

@@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu)

static void cache_override_properties(unsigned int cpu)
{
- if (of_have_populated_dt())
+ if (acpi_disabled && of_have_populated_dt())
return cache_of_override_properties(cpu);
}

diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 3d9805297cda..7ebff157ae6c 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -37,6 +37,8 @@ enum cache_type {
* @of_node: if devicetree is used, this represents either the cpu node in
* case there's no explicit cache node or the cache node itself in the
* device tree
+ * @firmware_node: When not using DT, this may contain pointers to other
+ * firmware based values. Particularly ACPI/PPTT unique values.
* @disable_sysfs: indicates whether this node is visible to the user via
* sysfs or not
* @priv: pointer to any private data structure specific to particular
@@ -65,8 +67,8 @@ struct cacheinfo {
#define CACHE_ALLOCATE_POLICY_MASK \
(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
#define CACHE_ID BIT(4)
-
struct device_node *of_node;
+ void *firmware_node;
bool disable_sysfs;
void *priv;
};
@@ -99,6 +101,15 @@ int func(unsigned int cpu) \
struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu);
int init_cache_level(unsigned int cpu);
int populate_cache_leaves(unsigned int cpu);
+int cache_setup_acpi(unsigned int cpu);
+int acpi_find_last_cache_level(unsigned int cpu);
+#ifndef CONFIG_ACPI
+int acpi_find_last_cache_level(unsigned int cpu)
+{
+ /*ACPI kernels should be built with PPTT support*/
+ return 0;
+}
+#endif

const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);

--
2.13.5

2017-12-01 22:24:06

[permalink] [raw]

Subject: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

The PPTT can be used to determine the groupings of CPU's at
given levels in the system. Lets add a few routines to the PPTT
parsing code to return a unique id for each unique level in the
processor hierarchy. This can then be matched to build
thread/core/cluster/die/package/etc mappings for each processing
element in the system.

Signed-off-by: Jeremy Linton <[email protected]>
---
drivers/acpi/pptt.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 115 insertions(+)

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index a35e457cefb7..b9b7b8b8ad21 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -412,6 +412,79 @@ static void cache_setup_acpi_cpu(struct acpi_table_header *table,
}
}

+/* Passing level values greater than this will result in search termination */
+#define PPTT_ABORT_PACKAGE 0xFF
+
+/*
+ * Given an acpi_pptt_processor node, walk up until we identify the
+ * package that the node is associated with, or we run out of levels
+ * to request or the search is terminated with a flag match
+ * The level parameter also serves to limit possible loops within the tree.
+ */
+static struct acpi_pptt_processor *acpi_find_processor_package_id(
+ struct acpi_table_header *table_hdr,
+ struct acpi_pptt_processor *cpu,
+ int level, int flag)
+{
+ struct acpi_pptt_processor *prev_node;
+
+ while (cpu && level) {
+ if (cpu->flags & flag)
+ break;
+ pr_debug("level %d\n", level);
+ prev_node = fetch_pptt_node(table_hdr, cpu->parent);
+ if (prev_node == NULL)
+ break;
+ cpu = prev_node;
+ level--;
+ }
+ return cpu;
+}
+
+/*
+ * Get a unique value given a cpu, and a topology level, that can be
+ * matched to determine which cpus share common topological features
+ * at that level.
+ */
+static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
+ unsigned int cpu, int level, int flag)
+{
+ struct acpi_pptt_processor *cpu_node;
+ u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
+
+ cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
+ if (cpu_node) {
+ cpu_node = acpi_find_processor_package_id(table, cpu_node,
+ level, flag);
+ /* Only the first level has a guaranteed id */
+ if (level == 0)
+ return cpu_node->acpi_processor_id;
+ return (int)((u8 *)cpu_node - (u8 *)table);
+ }
+ pr_err_once("PPTT table found, but unable to locate core for %d\n",
+ cpu);
+ return -ENOENT;
+}
+
+static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ int retval;
+
+ status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+ if (ACPI_FAILURE(status)) {
+ pr_err_once("No PPTT table found, cpu topology may be inaccurate\n");
+ return -ENOENT;
+ }
+ retval = topology_get_acpi_cpu_tag(table, cpu, level, flag);
+ pr_debug("Topology Setup ACPI cpu %d, level %d ret = %d\n",
+ cpu, level, retval);
+ acpi_put_table(table);
+
+ return retval;
+}
+
/**
* acpi_find_last_cache_level() - Determines the number of cache levels for a PE
* @cpu: Kernel logical cpu number
@@ -475,3 +548,45 @@ int cache_setup_acpi(unsigned int cpu)

return status;
}
+
+/**
+ * find_acpi_cpu_topology() - Determine a unique topology value for a given cpu
+ * @cpu: Kernel logical cpu number
+ * @level: The topological level for which we would like a unique ID
+ *
+ * Determine a topology unique ID for each thread/core/cluster/mc_grouping
+ * /socket/etc. This ID can then be used to group peers, which will have
+ * matching ids.
+ *
+ * The search terminates when either the requested level is found or
+ * we reach a root node. Levels beyond the termination point will return the
+ * same unique ID. The unique id for level 0 is the acpi processor id. All
+ * other levels beyond this use a generated value to uniquely identify
+ * a topological feature.
+ *
+ * Return: -ENOENT if the PPTT doesn't exist, or the cpu cannot be found.
+ * Otherwise returns a value which represents a unique topological feature.
+ */
+int find_acpi_cpu_topology(unsigned int cpu, int level)
+{
+ return find_acpi_cpu_topology_tag(cpu, level, 0);
+}
+
+/**
+ * find_acpi_cpu_topology_package() - Determine a unique cpu package value
+ * @cpu: Kernel logical cpu number
+ *
+ * Determine a topology unique package ID for the given cpu.
+ * This ID can then be used to group peers, which will have matching ids.
+ *
+ * The search terminates when either a level is found with the PHYSICAL_PACKAGE
+ * flag set or we reach a root node.
+ *
+ * Return: -ENOENT if the PPTT doesn't exist, or the cpu cannot be found.
+ * Otherwise returns a value which represents the package for this cpu.
+ */
+int find_acpi_cpu_topology_package(unsigned int cpu)
+{
+ return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
+ ACPI_PPTT_PHYSICAL_PACKAGE);
+}
--
2.13.5

2017-12-01 22:24:10

[permalink] [raw]

Subject: [PATCH v5 9/9] ACPI: Add PPTT to injectable table list

Add ACPI_SIG_PPTT to the table so initrd's can override the
system topology.

Signed-off-by: Geoffrey Blake <[email protected]>
Signed-off-by: Jeremy Linton <[email protected]>
---
drivers/acpi/tables.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 80ce2a7d224b..6d254450115b 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -456,7 +456,8 @@ static const char * const table_sigs[] = {
ACPI_SIG_SLIC, ACPI_SIG_SPCR, ACPI_SIG_SPMI, ACPI_SIG_TCPA,
ACPI_SIG_UEFI, ACPI_SIG_WAET, ACPI_SIG_WDAT, ACPI_SIG_WDDT,
ACPI_SIG_WDRT, ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_PSDT,
- ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
+ ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, ACPI_SIG_PPTT,
+ NULL };

#define ACPI_HEADER_SIZE sizeof(struct acpi_table_header)

--
2.13.5

2017-12-01 22:24:39

[permalink] [raw]

Subject: [PATCH v5 8/9] arm64: topology: Enable ACPI/PPTT based CPU topology.

Propagate the topology information from the PPTT tree to the
cpu_topology array. We can get the thread id, core_id and
cluster_id by assuming certain levels of the PPTT tree correspond
to those concepts. The package_id is flagged in the tree and can be
found by calling find_acpi_cpu_topology_package() which terminates
its search when it finds an ACPI node flagged as the physical
package. If the tree doesn't contain enough levels to represent
all of the requested levels then the root node will be returned
for all subsequent levels.

Signed-off-by: Jeremy Linton <[email protected]>
---
arch/arm64/kernel/topology.c | 47 +++++++++++++++++++++++++++++++++++++++++++-
include/linux/topology.h | 2 ++
2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 74a8a5173a35..198714aca9e8 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -11,6 +11,7 @@
* for more details.
*/

+#include <linux/acpi.h>
#include <linux/arch_topology.h>
#include <linux/cpu.h>
#include <linux/cpumask.h>
@@ -22,6 +23,7 @@
#include <linux/sched.h>
#include <linux/sched/topology.h>
#include <linux/slab.h>
+#include <linux/smp.h>
#include <linux/string.h>

#include <asm/cpu.h>
@@ -300,6 +302,47 @@ static void __init reset_cpu_topology(void)
}
}

+#ifdef CONFIG_ACPI
+/*
+ * Propagate the topology information of the processor_topology_node tree to the
+ * cpu_topology array.
+ */
+static int __init parse_acpi_topology(void)
+{
+ u64 is_threaded;
+ int cpu;
+ int topology_id;
+
+ is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
+
+ for_each_possible_cpu(cpu) {
+ topology_id = find_acpi_cpu_topology(cpu, 0);
+ if (topology_id < 0)
+ return topology_id;
+
+ if (is_threaded) {
+ cpu_topology[cpu].thread_id = topology_id;
+ topology_id = find_acpi_cpu_topology(cpu, 1);
+ cpu_topology[cpu].core_id = topology_id;
+ topology_id = find_acpi_cpu_topology_package(cpu);
+ cpu_topology[cpu].physical_id = topology_id;
+ } else {
+ cpu_topology[cpu].thread_id = -1;
+ cpu_topology[cpu].core_id = topology_id;
+ topology_id = find_acpi_cpu_topology_package(cpu);
+ cpu_topology[cpu].physical_id = topology_id;
+ }
+ }
+ return 0;
+}
+
+#else
+static int __init parse_acpi_topology(void)
+{
+ /*ACPI kernels should be built with PPTT support*/
+ return -EINVAL;
+}
+#endif

void __init init_cpu_topology(void)
{
@@ -309,6 +352,8 @@ void __init init_cpu_topology(void)
* Discard anything that was parsed if we hit an error so we
* don't use partial information.
*/
- if (of_have_populated_dt() && parse_dt_topology())
+ if ((!acpi_disabled) && parse_acpi_topology())
+ reset_cpu_topology();
+ else if (of_have_populated_dt() && parse_dt_topology())
reset_cpu_topology();
}
diff --git a/include/linux/topology.h b/include/linux/topology.h
index cb0775e1ee4b..170ce87edd88 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -43,6 +43,8 @@
if (nr_cpus_node(node))

int arch_update_cpu_topology(void);
+int find_acpi_cpu_topology(unsigned int cpu, int level);
+int find_acpi_cpu_topology_package(unsigned int cpu);

/* Conform to ACPI 2.0 SLIT distance definitions */
#define LOCAL_DISTANCE 10
--
2.13.5

2017-12-01 22:24:57

[permalink] [raw]

Subject: [PATCH v5 7/9] arm64: Topology, rename cluster_id

Lets match the name of the arm64 topology field
to the kernel macro that uses it.

Signed-off-by: Jeremy Linton <[email protected]>
---
arch/arm64/include/asm/topology.h | 4 ++--
arch/arm64/kernel/topology.c | 27 ++++++++++++++-------------
2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index c4f2d50491eb..118136268f66 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -7,14 +7,14 @@
struct cpu_topology {
int thread_id;
int core_id;
- int cluster_id;
+ int physical_id;
cpumask_t thread_sibling;
cpumask_t core_sibling;
};

extern struct cpu_topology cpu_topology[NR_CPUS];

-#define topology_physical_package_id(cpu) (cpu_topology[cpu].cluster_id)
+#define topology_physical_package_id(cpu) (cpu_topology[cpu].physical_id)
#define topology_core_id(cpu) (cpu_topology[cpu].core_id)
#define topology_core_cpumask(cpu) (&cpu_topology[cpu].core_sibling)
#define topology_sibling_cpumask(cpu) (&cpu_topology[cpu].thread_sibling)
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 8d48b233e6ce..74a8a5173a35 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -51,7 +51,7 @@ static int __init get_cpu_for_node(struct device_node *node)
return -1;
}

-static int __init parse_core(struct device_node *core, int cluster_id,
+static int __init parse_core(struct device_node *core, int physical_id,
int core_id)
{
char name[10];
@@ -67,7 +67,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
leaf = false;
cpu = get_cpu_for_node(t);
if (cpu >= 0) {
- cpu_topology[cpu].cluster_id = cluster_id;
+ cpu_topology[cpu].physical_id = physical_id;
cpu_topology[cpu].core_id = core_id;
cpu_topology[cpu].thread_id = i;
} else {
@@ -89,7 +89,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
return -EINVAL;
}

- cpu_topology[cpu].cluster_id = cluster_id;
+ cpu_topology[cpu].physical_id = physical_id;
cpu_topology[cpu].core_id = core_id;
} else if (leaf) {
pr_err("%pOF: Can't get CPU for leaf core\n", core);
@@ -105,7 +105,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
bool leaf = true;
bool has_cores = false;
struct device_node *c;
- static int cluster_id __initdata;
+ static int physical_id __initdata;
int core_id = 0;
int i, ret;

@@ -144,7 +144,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
}

if (leaf) {
- ret = parse_core(c, cluster_id, core_id++);
+ ret = parse_core(c, physical_id, core_id++);
} else {
pr_err("%pOF: Non-leaf cluster with core %s\n",
cluster, name);
@@ -162,7 +162,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
pr_warn("%pOF: empty cluster\n", cluster);

if (leaf)
- cluster_id++;
+ physical_id++;

return 0;
}
@@ -198,7 +198,7 @@ static int __init parse_dt_topology(void)
* only mark cores described in the DT as possible.
*/
for_each_possible_cpu(cpu)
- if (cpu_topology[cpu].cluster_id == -1)
+ if (cpu_topology[cpu].physical_id == -1)
ret = -EINVAL;

out_map:
@@ -228,7 +228,7 @@ static void update_siblings_masks(unsigned int cpuid)
for_each_possible_cpu(cpu) {
cpu_topo = &cpu_topology[cpu];

- if (cpuid_topo->cluster_id != cpu_topo->cluster_id)
+ if (cpuid_topo->physical_id != cpu_topo->physical_id)
continue;

cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
@@ -249,7 +249,7 @@ void store_cpu_topology(unsigned int cpuid)
struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
u64 mpidr;

- if (cpuid_topo->cluster_id != -1)
+ if (cpuid_topo->physical_id != -1)
goto topology_populated;

mpidr = read_cpuid_mpidr();
@@ -263,19 +263,19 @@ void store_cpu_topology(unsigned int cpuid)
/* Multiprocessor system : Multi-threads per core */
cpuid_topo->thread_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
- cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
+ cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
MPIDR_AFFINITY_LEVEL(mpidr, 3) << 8;
} else {
/* Multiprocessor system : Single-thread per core */
cpuid_topo->thread_id = -1;
cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
- cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
+ cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
MPIDR_AFFINITY_LEVEL(mpidr, 2) << 8 |
MPIDR_AFFINITY_LEVEL(mpidr, 3) << 16;
}

pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n",
- cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
+ cpuid, cpuid_topo->physical_id, cpuid_topo->core_id,
cpuid_topo->thread_id, mpidr);

topology_populated:
@@ -291,7 +291,7 @@ static void __init reset_cpu_topology(void)

cpu_topo->thread_id = -1;
cpu_topo->core_id = 0;
- cpu_topo->cluster_id = -1;
+ cpu_topo->physical_id = -1;

cpumask_clear(&cpu_topo->core_sibling);
cpumask_set_cpu(cpu, &cpu_topo->core_sibling);
@@ -300,6 +300,7 @@ static void __init reset_cpu_topology(void)
}
}

+
void __init init_cpu_topology(void)
{
reset_cpu_topology();
--
2.13.5

2017-12-01 22:25:33

[permalink] [raw]

Subject: [PATCH v5 5/9] arm64: Add support for ACPI based firmware tables

The /sys cache entries should support ACPI/PPTT generated cache
topology information. Lets detect ACPI systems and call
an arch specific cache_setup_acpi() routine to update the hardware
probed cache topology.

For arm64, if ACPI is enabled, determine the max number of cache
levels and populate them using the PPTT table if one is available.

Signed-off-by: Jeremy Linton <[email protected]>
---
arch/arm64/kernel/cacheinfo.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
index 380f2e2fbed5..0bf0a835122f 100644
--- a/arch/arm64/kernel/cacheinfo.c
+++ b/arch/arm64/kernel/cacheinfo.c
@@ -17,6 +17,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

+#include <linux/acpi.h>
#include <linux/cacheinfo.h>
#include <linux/of.h>

@@ -46,7 +47,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,

static int __init_cache_level(unsigned int cpu)
{
- unsigned int ctype, level, leaves, of_level;
+ unsigned int ctype, level, leaves, fw_level;
struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);

for (level = 1, leaves = 0; level <= MAX_CACHE_LEVEL; level++) {
@@ -59,15 +60,19 @@ static int __init_cache_level(unsigned int cpu)
leaves += (ctype == CACHE_TYPE_SEPARATE) ? 2 : 1;
}

- of_level = of_find_last_cache_level(cpu);
- if (level < of_level) {
+ if (acpi_disabled)
+ fw_level = of_find_last_cache_level(cpu);
+ else
+ fw_level = acpi_find_last_cache_level(cpu);
+
+ if (level < fw_level) {
/*
* some external caches not specified in CLIDR_EL1
* the information may be available in the device tree
* only unified external caches are considered here
*/
- leaves += (of_level - level);
- level = of_level;
+ leaves += (fw_level - level);
+ level = fw_level;
}

this_cpu_ci->num_levels = level;
--
2.13.5

2017-12-01 22:23:55

[permalink] [raw]

Subject: [PATCH v5 1/9] arm64/acpi: Create arch specific cpu to acpi id helper

Its helpful to be able to lookup the acpi_processor_id associated
with a logical cpu. Provide an arm64 helper to do this.

Signed-off-by: Jeremy Linton <[email protected]>
---
arch/arm64/include/asm/acpi.h | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 32f465a80e4e..0db62a4cbce2 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -86,6 +86,10 @@ static inline bool acpi_has_cpu_in_madt(void)
}

struct acpi_madt_generic_interrupt *acpi_cpu_get_madt_gicc(int cpu);
+static inline u32 get_acpi_id_for_cpu(unsigned int cpu)
+{
+ return acpi_cpu_get_madt_gicc(cpu)->uid;
+}

static inline void arch_fix_phys_package_id(int num, u32 slot) { }
void __init acpi_init_cpus(void);
--
2.13.5

2017-12-01 22:26:15

[permalink] [raw]

Subject: [PATCH v5 3/9] ACPI: Enable PPTT support on ARM64

Now that we have a PPTT parser, in preparation for its use
on arm64, lets build it.

Signed-off-by: Jeremy Linton <[email protected]>
---
arch/arm64/Kconfig | 1 +
drivers/acpi/Kconfig | 3 +++
drivers/acpi/Makefile | 1 +
3 files changed, 5 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index a93339f5178f..e62fd1e08c1f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -7,6 +7,7 @@ config ARM64
select ACPI_REDUCED_HARDWARE_ONLY if ACPI
select ACPI_MCFG if ACPI
select ACPI_SPCR_TABLE if ACPI
+ select ACPI_PPTT if ACPI
select ARCH_CLOCKSOURCE_DATA
select ARCH_HAS_DEBUG_VIRTUAL
select ARCH_HAS_DEVMEM_IS_ALLOWED
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 46505396869e..df7aebf0af0e 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -545,6 +545,9 @@ config ACPI_CONFIGFS

if ARM64
source "drivers/acpi/arm64/Kconfig"
+
+config ACPI_PPTT
+ bool
endif

config TPS68470_PMIC_OPREGION
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 41954a601989..b6056b566df4 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_ACPI_BGRT) += bgrt.o
obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o
obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o
obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
+obj-$(CONFIG_ACPI_PPTT) += pptt.o

# processor has its own "processor." module_param namespace
processor-y := processor_driver.o
--
2.13.5

2017-12-12 01:11:00

[permalink] [raw]

Subject: Re: [PATCH v5 2/9] ACPI/PPTT: Add Processor Properties Topology Table parsing

On Friday, December 1, 2017 11:23:23 PM CET 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.
>
> An additional patch later in the set adds the ability to report
> peers in the topology using find_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 <[email protected]>

This is only going to be used by ARM64 for the time being, so I need
someone from that camp to review this.

Sudeep, Hanjun, Lorenzo?

Thanks,
Rafael

2017-12-12 01:11:52

[permalink] [raw]

Subject: Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote:
> Add a entry to to struct cacheinfo to maintain a reference to the PPTT
> node which can be used to match identical caches across cores. Also
> stub out cache_setup_acpi() so that individual architectures can
> enable ACPI topology parsing.
>
> Signed-off-by: Jeremy Linton <[email protected]>
> ---
> drivers/acpi/pptt.c | 1 +
> drivers/base/cacheinfo.c | 20 ++++++++++++++------
> include/linux/cacheinfo.h | 13 ++++++++++++-
> 3 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index 0f8a1631af33..a35e457cefb7 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
> {
> int valid_flags = 0;
>
> + this_leaf->firmware_node = cpu_node;
> if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
> this_leaf->size = found_cache->size;
> valid_flags++;
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index eb3af2739537..ba89f9310e6f 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
> static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
> struct cacheinfo *sib_leaf)
> {
> - return sib_leaf->of_node == this_leaf->of_node;
> + if (acpi_disabled)
> + return sib_leaf->of_node == this_leaf->of_node;
> + else
> + return sib_leaf->firmware_node == this_leaf->firmware_node;
> }
>
> /* OF properties to query for a given cache type */
> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
> }
> #endif
>
> +int __weak cache_setup_acpi(unsigned int cpu)
> +{
> + return -ENOTSUPP;
> +}
> +
> static int cache_shared_cpu_map_setup(unsigned int cpu)
> {
> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
> if (this_cpu_ci->cpu_map_populated)
> return 0;
>
> - if (of_have_populated_dt())
> + if (!acpi_disabled)
> + ret = cache_setup_acpi(cpu);
> + else if (of_have_populated_dt())
> ret = cache_setup_of_node(cpu);
> - else if (!acpi_disabled)
> - /* No cache property/hierarchy support yet in ACPI */
> - ret = -ENOTSUPP;
> +
> if (ret)
> return ret;
>
> @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu)
>
> static void cache_override_properties(unsigned int cpu)
> {
> - if (of_have_populated_dt())
> + if (acpi_disabled && of_have_populated_dt())
> return cache_of_override_properties(cpu);
> }
>
> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
> index 3d9805297cda..7ebff157ae6c 100644
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -37,6 +37,8 @@ enum cache_type {
> * @of_node: if devicetree is used, this represents either the cpu node in
> * case there's no explicit cache node or the cache node itself in the
> * device tree
> + * @firmware_node: When not using DT, this may contain pointers to other
> + * firmware based values. Particularly ACPI/PPTT unique values.
> * @disable_sysfs: indicates whether this node is visible to the user via
> * sysfs or not
> * @priv: pointer to any private data structure specific to particular
> @@ -65,8 +67,8 @@ struct cacheinfo {
> #define CACHE_ALLOCATE_POLICY_MASK \
> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
> #define CACHE_ID BIT(4)
> -
> struct device_node *of_node;
> + void *firmware_node;

What about converting this to using struct fwnode instead of adding
fields to it?

> bool disable_sysfs;
> void *priv;
> };
> @@ -99,6 +101,15 @@ int func(unsigned int cpu) \
> struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu);
> int init_cache_level(unsigned int cpu);
> int populate_cache_leaves(unsigned int cpu);
> +int cache_setup_acpi(unsigned int cpu);
> +int acpi_find_last_cache_level(unsigned int cpu);
> +#ifndef CONFIG_ACPI
> +int acpi_find_last_cache_level(unsigned int cpu)
> +{
> + /*ACPI kernels should be built with PPTT support*/
> + return 0;
> +}
> +#endif
>
> const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
>
>

Thanks,
Rafael

2017-12-12 01:12:54

[permalink] [raw]

Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
> The PPTT can be used to determine the groupings of CPU's at
> given levels in the system. Lets add a few routines to the PPTT
> parsing code to return a unique id for each unique level in the
> processor hierarchy. This can then be matched to build
> thread/core/cluster/die/package/etc mappings for each processing
> element in the system.
>
> Signed-off-by: Jeremy Linton <[email protected]>

Why can't this be folded into patch [2/9]?

Thanks,
Rafael

2017-12-12 16:13:14

[permalink] [raw]

Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

Hi,

First, thanks for taking a look at this.

On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>> The PPTT can be used to determine the groupings of CPU's at
>> given levels in the system. Lets add a few routines to the PPTT
>> parsing code to return a unique id for each unique level in the
>> processor hierarchy. This can then be matched to build
>> thread/core/cluster/die/package/etc mappings for each processing
>> element in the system.
>>
>> Signed-off-by: Jeremy Linton <[email protected]>
>
> Why can't this be folded into patch [2/9]?

It can, and I will be happy squash it.

It was requested that the topology portion of the parser be split out
back in v3.

https://www.spinics.net/lists/linux-acpi/msg78487.html

2017-12-12 17:03:33

[permalink] [raw]

Subject: Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

Hi,

On 12/11/2017 07:11 PM, Rafael J. Wysocki wrote:
> On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote:
>> Add a entry to to struct cacheinfo to maintain a reference to the PPTT
>> node which can be used to match identical caches across cores. Also
>> stub out cache_setup_acpi() so that individual architectures can
>> enable ACPI topology parsing.
>>
>> Signed-off-by: Jeremy Linton <[email protected]>
>> ---
>> drivers/acpi/pptt.c | 1 +
>> drivers/base/cacheinfo.c | 20 ++++++++++++++------
>> include/linux/cacheinfo.h | 13 ++++++++++++-
>> 3 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 0f8a1631af33..a35e457cefb7 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
>> {
>> int valid_flags = 0;
>>
>> + this_leaf->firmware_node = cpu_node;
>> if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
>> this_leaf->size = found_cache->size;
>> valid_flags++;
>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>> index eb3af2739537..ba89f9310e6f 100644
>> --- a/drivers/base/cacheinfo.c
>> +++ b/drivers/base/cacheinfo.c
>> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
>> static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>> struct cacheinfo *sib_leaf)
>> {
>> - return sib_leaf->of_node == this_leaf->of_node;
>> + if (acpi_disabled)
>> + return sib_leaf->of_node == this_leaf->of_node;
>> + else
>> + return sib_leaf->firmware_node == this_leaf->firmware_node;
>> }
>>
>> /* OF properties to query for a given cache type */
>> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>> }
>> #endif
>>
>> +int __weak cache_setup_acpi(unsigned int cpu)
>> +{
>> + return -ENOTSUPP;
>> +}
>> +
>> static int cache_shared_cpu_map_setup(unsigned int cpu)
>> {
>> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
>> if (this_cpu_ci->cpu_map_populated)
>> return 0;
>>
>> - if (of_have_populated_dt())
>> + if (!acpi_disabled)
>> + ret = cache_setup_acpi(cpu);
>> + else if (of_have_populated_dt())
>> ret = cache_setup_of_node(cpu);
>> - else if (!acpi_disabled)
>> - /* No cache property/hierarchy support yet in ACPI */
>> - ret = -ENOTSUPP;
>> +
>> if (ret)
>> return ret;
>>
>> @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu)
>>
>> static void cache_override_properties(unsigned int cpu)
>> {
>> - if (of_have_populated_dt())
>> + if (acpi_disabled && of_have_populated_dt())
>> return cache_of_override_properties(cpu);
>> }
>>
>> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
>> index 3d9805297cda..7ebff157ae6c 100644
>> --- a/include/linux/cacheinfo.h
>> +++ b/include/linux/cacheinfo.h
>> @@ -37,6 +37,8 @@ enum cache_type {
>> * @of_node: if devicetree is used, this represents either the cpu node in
>> * case there's no explicit cache node or the cache node itself in the
>> * device tree
>> + * @firmware_node: When not using DT, this may contain pointers to other
>> + * firmware based values. Particularly ACPI/PPTT unique values.
>> * @disable_sysfs: indicates whether this node is visible to the user via
>> * sysfs or not
>> * @priv: pointer to any private data structure specific to particular
>> @@ -65,8 +67,8 @@ struct cacheinfo {
>> #define CACHE_ALLOCATE_POLICY_MASK \
>> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
>> #define CACHE_ID BIT(4)
>> -
>> struct device_node *of_node;
>> + void *firmware_node;
>
> What about converting this to using struct fwnode instead of adding
> fields to it?

I didn't really want to add another field here, but I've also pointed
out how I thought converting it to a fwnode wasn't a good choice.

https://lkml.org/lkml/2017/11/20/502

Mostly because IMHO its even more misleading (lacking any
fwnode_operations) than misusing the of_node as a void *.

Given that I'm in the minority thinking this, how far down the fwnode
path on the ACPI side do we want to go? Is simply treating it as a void
pointer sufficient for the ACPI side, considering all the PPTT code
needs is a unique token?

>
>> bool disable_sysfs;
>> void *priv;
>> };
>> @@ -99,6 +101,15 @@ int func(unsigned int cpu) \
>> struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu);
>> int init_cache_level(unsigned int cpu);
>> int populate_cache_leaves(unsigned int cpu);
>> +int cache_setup_acpi(unsigned int cpu);
>> +int acpi_find_last_cache_level(unsigned int cpu);
>> +#ifndef CONFIG_ACPI
>> +int acpi_find_last_cache_level(unsigned int cpu)
>> +{
>> + /*ACPI kernels should be built with PPTT support*/
>> + return 0;
>> +}
>> +#endif
>>
>> const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
>>
>>
>
> Thanks,
> Rafael
>

2017-12-12 17:25:27

[permalink] [raw]

Subject: Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

On Tue, Dec 12, 2017 at 6:03 PM, Jeremy Linton <[email protected]> wrote:
> Hi,
>
>
> On 12/11/2017 07:11 PM, Rafael J. Wysocki wrote:
>>
>> On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote:
>>>
>>> Add a entry to to struct cacheinfo to maintain a reference to the PPTT
>>> node which can be used to match identical caches across cores. Also
>>> stub out cache_setup_acpi() so that individual architectures can
>>> enable ACPI topology parsing.
>>>
>>> Signed-off-by: Jeremy Linton <[email protected]>
>>> ---
>>> drivers/acpi/pptt.c | 1 +
>>> drivers/base/cacheinfo.c | 20 ++++++++++++++------
>>> include/linux/cacheinfo.h | 13 ++++++++++++-
>>> 3 files changed, 27 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>>> index 0f8a1631af33..a35e457cefb7 100644
>>> --- a/drivers/acpi/pptt.c
>>> +++ b/drivers/acpi/pptt.c
>>> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo
>>> *this_leaf,
>>> {
>>> int valid_flags = 0;
>>> + this_leaf->firmware_node = cpu_node;
>>> if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
>>> this_leaf->size = found_cache->size;
>>> valid_flags++;
>>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>>> index eb3af2739537..ba89f9310e6f 100644
>>> --- a/drivers/base/cacheinfo.c
>>> +++ b/drivers/base/cacheinfo.c
>>> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
>>> static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>>> struct cacheinfo *sib_leaf)
>>> {
>>> - return sib_leaf->of_node == this_leaf->of_node;
>>> + if (acpi_disabled)
>>> + return sib_leaf->of_node == this_leaf->of_node;
>>> + else
>>> + return sib_leaf->firmware_node ==
>>> this_leaf->firmware_node;
>>> }
>>> /* OF properties to query for a given cache type */
>>> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct
>>> cacheinfo *this_leaf,
>>> }
>>> #endif
>>> +int __weak cache_setup_acpi(unsigned int cpu)
>>> +{
>>> + return -ENOTSUPP;
>>> +}
>>> +
>>> static int cache_shared_cpu_map_setup(unsigned int cpu)
>>> {
>>> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int
>>> cpu)
>>> if (this_cpu_ci->cpu_map_populated)
>>> return 0;
>>> - if (of_have_populated_dt())
>>> + if (!acpi_disabled)
>>> + ret = cache_setup_acpi(cpu);
>>> + else if (of_have_populated_dt())
>>> ret = cache_setup_of_node(cpu);
>>> - else if (!acpi_disabled)
>>> - /* No cache property/hierarchy support yet in ACPI */
>>> - ret = -ENOTSUPP;
>>> +
>>> if (ret)
>>> return ret;
>>> @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned
>>> int cpu)
>>> static void cache_override_properties(unsigned int cpu)
>>> {
>>> - if (of_have_populated_dt())
>>> + if (acpi_disabled && of_have_populated_dt())
>>> return cache_of_override_properties(cpu);
>>> }
>>> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
>>> index 3d9805297cda..7ebff157ae6c 100644
>>> --- a/include/linux/cacheinfo.h
>>> +++ b/include/linux/cacheinfo.h
>>> @@ -37,6 +37,8 @@ enum cache_type {
>>> * @of_node: if devicetree is used, this represents either the cpu node
>>> in
>>> * case there's no explicit cache node or the cache node itself in
>>> the
>>> * device tree
>>> + * @firmware_node: When not using DT, this may contain pointers to other
>>> + * firmware based values. Particularly ACPI/PPTT unique values.
>>> * @disable_sysfs: indicates whether this node is visible to the user
>>> via
>>> * sysfs or not
>>> * @priv: pointer to any private data structure specific to particular
>>> @@ -65,8 +67,8 @@ struct cacheinfo {
>>> #define CACHE_ALLOCATE_POLICY_MASK \
>>> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
>>> #define CACHE_ID BIT(4)
>>> -
>>> struct device_node *of_node;
>>> + void *firmware_node;
>>
>>
>> What about converting this to using struct fwnode instead of adding
>> fields to it?
>
>
> I didn't really want to add another field here, but I've also pointed out
> how I thought converting it to a fwnode wasn't a good choice.
>
> https://lkml.org/lkml/2017/11/20/502
>
> Mostly because IMHO its even more misleading (lacking any fwnode_operations)
> than misusing the of_node as a void *.

I'm not sure what you mean.

Anyway, the idea is to have one pointer in there instead of two that
cannot be used at the same time and there's no reason why of_node
should be special.

of_node should just be one of multiple choices.

> Given that I'm in the minority thinking this, how far down the fwnode path
> on the ACPI side do we want to go?

I have no idea. :-)

> Is simply treating it as a void pointer
> sufficient for the ACPI side, considering all the PPTT code needs is a
> unique token?

I guess you can think about it as of_node under a different name, but
whether or not this is sufficient depends on what you need it for.

Thanks,
Rafael

2017-12-12 22:55:12

[permalink] [raw]

Subject: Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

Hi,

On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2017 at 6:03 PM, Jeremy Linton <[email protected]> wrote:
>> Hi,
>>
>>
>> On 12/11/2017 07:11 PM, Rafael J. Wysocki wrote:
>>>
>>> On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote:
>>>>
>>>> Add a entry to to struct cacheinfo to maintain a reference to the PPTT
>>>> node which can be used to match identical caches across cores. Also
>>>> stub out cache_setup_acpi() so that individual architectures can
>>>> enable ACPI topology parsing.
>>>>
>>>> Signed-off-by: Jeremy Linton <[email protected]>
>>>> ---
>>>> drivers/acpi/pptt.c | 1 +
>>>> drivers/base/cacheinfo.c | 20 ++++++++++++++------
>>>> include/linux/cacheinfo.h | 13 ++++++++++++-
>>>> 3 files changed, 27 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>>>> index 0f8a1631af33..a35e457cefb7 100644
>>>> --- a/drivers/acpi/pptt.c
>>>> +++ b/drivers/acpi/pptt.c
>>>> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo
>>>> *this_leaf,
>>>> {
>>>> int valid_flags = 0;
>>>> + this_leaf->firmware_node = cpu_node;
>>>> if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
>>>> this_leaf->size = found_cache->size;
>>>> valid_flags++;
>>>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>>>> index eb3af2739537..ba89f9310e6f 100644
>>>> --- a/drivers/base/cacheinfo.c
>>>> +++ b/drivers/base/cacheinfo.c
>>>> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
>>>> static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>>>> struct cacheinfo *sib_leaf)
>>>> {
>>>> - return sib_leaf->of_node == this_leaf->of_node;
>>>> + if (acpi_disabled)
>>>> + return sib_leaf->of_node == this_leaf->of_node;
>>>> + else
>>>> + return sib_leaf->firmware_node ==
>>>> this_leaf->firmware_node;
>>>> }
>>>> /* OF properties to query for a given cache type */
>>>> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct
>>>> cacheinfo *this_leaf,
>>>> }
>>>> #endif
>>>> +int __weak cache_setup_acpi(unsigned int cpu)
>>>> +{
>>>> + return -ENOTSUPP;
>>>> +}
>>>> +
>>>> static int cache_shared_cpu_map_setup(unsigned int cpu)
>>>> {
>>>> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>>> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int
>>>> cpu)
>>>> if (this_cpu_ci->cpu_map_populated)
>>>> return 0;
>>>> - if (of_have_populated_dt())
>>>> + if (!acpi_disabled)
>>>> + ret = cache_setup_acpi(cpu);
>>>> + else if (of_have_populated_dt())
>>>> ret = cache_setup_of_node(cpu);
>>>> - else if (!acpi_disabled)
>>>> - /* No cache property/hierarchy support yet in ACPI */
>>>> - ret = -ENOTSUPP;
>>>> +
>>>> if (ret)
>>>> return ret;
>>>> @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned
>>>> int cpu)
>>>> static void cache_override_properties(unsigned int cpu)
>>>> {
>>>> - if (of_have_populated_dt())
>>>> + if (acpi_disabled && of_have_populated_dt())
>>>> return cache_of_override_properties(cpu);
>>>> }
>>>> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
>>>> index 3d9805297cda..7ebff157ae6c 100644
>>>> --- a/include/linux/cacheinfo.h
>>>> +++ b/include/linux/cacheinfo.h
>>>> @@ -37,6 +37,8 @@ enum cache_type {
>>>> * @of_node: if devicetree is used, this represents either the cpu node
>>>> in
>>>> * case there's no explicit cache node or the cache node itself in
>>>> the
>>>> * device tree
>>>> + * @firmware_node: When not using DT, this may contain pointers to other
>>>> + * firmware based values. Particularly ACPI/PPTT unique values.
>>>> * @disable_sysfs: indicates whether this node is visible to the user
>>>> via
>>>> * sysfs or not
>>>> * @priv: pointer to any private data structure specific to particular
>>>> @@ -65,8 +67,8 @@ struct cacheinfo {
>>>> #define CACHE_ALLOCATE_POLICY_MASK \
>>>> (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
>>>> #define CACHE_ID BIT(4)
>>>> -
>>>> struct device_node *of_node;
>>>> + void *firmware_node;
>>>
>>>
>>> What about converting this to using struct fwnode instead of adding
>>> fields to it?
>>
>>
>> I didn't really want to add another field here, but I've also pointed out
>> how I thought converting it to a fwnode wasn't a good choice.
>>
>> https://lkml.org/lkml/2017/11/20/502
>>
>> Mostly because IMHO its even more misleading (lacking any fwnode_operations)
>> than misusing the of_node as a void *.
>
> I'm not sure what you mean.

Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is
straightforward. But IMHO it doesn't solve the readability problem of
either casting the ACPI/PPTT token directly to the resulting
fwnode_handle *, or alternatively an actual fwnode_handle with bogus
fwnode_operations to wrap that token.

>
> Anyway, the idea is to have one pointer in there instead of two that
> cannot be used at the same time and there's no reason why of_node
> should be special.

Avoid two pointers for size, or readability? Because the last version
had a union with of_node, which isn't strictly necessary as I can just
cast the pptt token to of_node. There is exactly one line of code after
that which uses the token and it doesn't care about type.

(in cache_leaves_are_shared())

>
> of_node should just be one of multiple choices.
>
>> Given that I'm in the minority thinking this, how far down the fwnode path
>> on the ACPI side do we want to go?
>
> I have no idea. :-)
>
>> Is simply treating it as a void pointer
>> sufficient for the ACPI side, considering all the PPTT code needs is a
>> unique token?
>
> I guess you can think about it as of_node under a different name, but
> whether or not this is sufficient depends on what you need it for.

>
> Thanks,
> Rafael
>

2017-12-12 23:03:02

[permalink] [raw]

Subject: Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

On Tue, Dec 12, 2017 at 11:55 PM, Jeremy Linton <[email protected]> wrote:
> Hi,
>
>
> On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote:
>>

[cut]

>>>>
>>>>
>>>>
>>>> What about converting this to using struct fwnode instead of adding
>>>> fields to it?
>>>
>>>
>>>
>>> I didn't really want to add another field here, but I've also pointed out
>>> how I thought converting it to a fwnode wasn't a good choice.
>>>
>>> https://lkml.org/lkml/2017/11/20/502
>>>
>>> Mostly because IMHO its even more misleading (lacking any
>>> fwnode_operations)
>>> than misusing the of_node as a void *.
>>
>>
>> I'm not sure what you mean.
>
>
> Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is
> straightforward. But IMHO it doesn't solve the readability problem of either
> casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or
> alternatively an actual fwnode_handle with bogus fwnode_operations to wrap
> that token.

I'm not talking about that at all.

>>
>> Anyway, the idea is to have one pointer in there instead of two that
>> cannot be used at the same time and there's no reason why of_node
>> should be special.
>
>
> Avoid two pointers for size, or readability? Because the last
> version had a union with of_node, which isn't strictly necessary as I can
> just cast the pptt token to of_node. There is exactly one line of code after
> that which uses the token and it doesn't care about type.

So call this field "token" or similar. Don't call it "of_node" and
don't introduce another "firmware_node" thing in addition to that.
That just is a mess, sorry.

Thanks,
Rafael

2017-12-12 23:37:59

[permalink] [raw]

Subject: Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2017 at 11:55 PM, Jeremy Linton <[email protected]> wrote:
>> Hi,
>>
>>
>> On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote:
>>>
>
> [cut]

(trimming list)

>
>>>>>
>>>>>
>>>>>
>>>>> What about converting this to using struct fwnode instead of adding
>>>>> fields to it?
>>>>
>>>>
>>>>
>>>> I didn't really want to add another field here, but I've also pointed out
>>>> how I thought converting it to a fwnode wasn't a good choice.
>>>>
>>>> https://lkml.org/lkml/2017/11/20/502
>>>>
>>>> Mostly because IMHO its even more misleading (lacking any
>>>> fwnode_operations)
>>>> than misusing the of_node as a void *.
>>>
>>>
>>> I'm not sure what you mean.
>>
>>
>> Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is
>> straightforward. But IMHO it doesn't solve the readability problem of either
>> casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or
>> alternatively an actual fwnode_handle with bogus fwnode_operations to wrap
>> that token.
>
> I'm not talking about that at all.
>
>>>
>>> Anyway, the idea is to have one pointer in there instead of two that
>>> cannot be used at the same time and there's no reason why of_node
>>> should be special.
>>
>>
>> Avoid two pointers for size, or readability? Because the last
>> version had a union with of_node, which isn't strictly necessary as I can
>> just cast the pptt token to of_node. There is exactly one line of code after
>> that which uses the token and it doesn't care about type.
>
> So call this field "token" or similar. Don't call it "of_node" and
> don't introduce another "firmware_node" thing in addition to that.
> That just is a mess, sorry.

I sort of agree, I think I can just change the whole of_node to a
generic 'void *firmware_unique' which works fine for the PPTT code, it
should also work for the DT code in cache_leaves_are_shared().

The slight gocha is there is a bit of DT code which initially runs
earlier that uses of_node as an indirect parameter to a couple functions
(by just passing the cacheinfo). Let me see if I can tweak that a bit.

Frankly, If I understood completely all the *priv cases I suspect it
might be possible to collapse *of_node into that as well. That is as
long as no one decides to flush out DT on x86, or PPTT on x86.

2017-12-12 23:41:13

[permalink] [raw]

Subject: Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

On Wed, Dec 13, 2017 at 12:37 AM, Jeremy Linton <[email protected]> wrote:
> On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote:
>>
>> On Tue, Dec 12, 2017 at 11:55 PM, Jeremy Linton <[email protected]>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote:
>>>>
>>>>
>>
>> [cut]
>
>
> (trimming list)
>
>
>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> What about converting this to using struct fwnode instead of adding
>>>>>> fields to it?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I didn't really want to add another field here, but I've also pointed
>>>>> out
>>>>> how I thought converting it to a fwnode wasn't a good choice.
>>>>>
>>>>> https://lkml.org/lkml/2017/11/20/502
>>>>>
>>>>> Mostly because IMHO its even more misleading (lacking any
>>>>> fwnode_operations)
>>>>> than misusing the of_node as a void *.
>>>>
>>>>
>>>>
>>>> I'm not sure what you mean.
>>>
>>>
>>>
>>> Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is
>>> straightforward. But IMHO it doesn't solve the readability problem of
>>> either
>>> casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or
>>> alternatively an actual fwnode_handle with bogus fwnode_operations to
>>> wrap
>>> that token.
>>
>>
>> I'm not talking about that at all.
>>
>>>>
>>>> Anyway, the idea is to have one pointer in there instead of two that
>>>> cannot be used at the same time and there's no reason why of_node
>>>> should be special.
>>>
>>>
>>>
>>> Avoid two pointers for size, or readability? Because the last
>>> version had a union with of_node, which isn't strictly necessary as I can
>>> just cast the pptt token to of_node. There is exactly one line of code
>>> after
>>> that which uses the token and it doesn't care about type.
>>
>>
>> So call this field "token" or similar. Don't call it "of_node" and
>> don't introduce another "firmware_node" thing in addition to that.
>> That just is a mess, sorry.
>
>
> I sort of agree, I think I can just change the whole of_node to a generic
> 'void *firmware_unique' which works fine for the PPTT code, it should also
> work for the DT code in cache_leaves_are_shared().
>
> The slight gocha is there is a bit of DT code which initially runs earlier
> that uses of_node as an indirect parameter to a couple functions (by just
> passing the cacheinfo). Let me see if I can tweak that a bit.
>
> Frankly, If I understood completely all the *priv cases I suspect it might
> be possible to collapse *of_node into that as well. That is as long as no
> one decides to flush out DT on x86, or PPTT on x86.

I'm not aware of any plans to go in that direction.

Anyway, that would be a worry of whoever wanted to do that. No need
to worry about it upfront.

2017-12-13 17:26:15

[permalink] [raw]

Subject: Re: [PATCH v5 3/9] ACPI: Enable PPTT support on ARM64

On Fri, Dec 01, 2017 at 04:23:24PM -0600, Jeremy Linton wrote:
> Now that we have a PPTT parser, in preparation for its use
> on arm64, lets build it.
>
> Signed-off-by: Jeremy Linton <[email protected]>
> ---
> arch/arm64/Kconfig | 1 +
> drivers/acpi/Kconfig | 3 +++
> drivers/acpi/Makefile | 1 +
> 3 files changed, 5 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a93339f5178f..e62fd1e08c1f 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -7,6 +7,7 @@ config ARM64
> select ACPI_REDUCED_HARDWARE_ONLY if ACPI
> select ACPI_MCFG if ACPI
> select ACPI_SPCR_TABLE if ACPI
> + select ACPI_PPTT if ACPI
> select ARCH_CLOCKSOURCE_DATA
> select ARCH_HAS_DEBUG_VIRTUAL
> select ARCH_HAS_DEVMEM_IS_ALLOWED
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 46505396869e..df7aebf0af0e 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -545,6 +545,9 @@ config ACPI_CONFIGFS
>
> if ARM64
> source "drivers/acpi/arm64/Kconfig"
> +
> +config ACPI_PPTT
> + bool

We need to make a choice here. Either PPTT is considered ARM64 only and
we move code and config to drivers/acpi/arm64/Kconfig or we leave it in
drivers/acpi/pptt.c and we add a Kconfig entry in drivers/acpi/Kconfig
(and we make pptt.c compile on !ARM64 - which is what it should be given
that there is nothing ARM64 specific in it).

Lorenzo

> endif
>
> config TPS68470_PMIC_OPREGION
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 41954a601989..b6056b566df4 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_ACPI_BGRT) += bgrt.o
> obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o
> obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o
> obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
> +obj-$(CONFIG_ACPI_PPTT) += pptt.o
>
> # processor has its own "processor." module_param namespace
> processor-y := processor_driver.o
> --
> 2.13.5
>

2017-12-13 17:38:19

[permalink] [raw]

Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
> Hi,
>
> First, thanks for taking a look at this.
>
> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
> >On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
> >>The PPTT can be used to determine the groupings of CPU's at
> >>given levels in the system. Lets add a few routines to the PPTT
> >>parsing code to return a unique id for each unique level in the
> >>processor hierarchy. This can then be matched to build
> >>thread/core/cluster/die/package/etc mappings for each processing
> >>element in the system.
> >>
> >>Signed-off-by: Jeremy Linton <[email protected]>
> >
> >Why can't this be folded into patch [2/9]?
>
> It can, and I will be happy squash it.
>
> It was requested that the topology portion of the parser be split
> out back in v3.
>
> https://www.spinics.net/lists/linux-acpi/msg78487.html

I asked to split cache/topology since I am not familiar with cache
code and Sudeep - who looks after the cache code - won't be able
to review this series in time for v4.16.

Lorenzo

2017-12-13 18:01:42

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[+Morten, Dietmar]

$SUBJECT should be:

arm64: topology: rename cluster_id

On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
> Lets match the name of the arm64 topology field
> to the kernel macro that uses it.
>
> Signed-off-by: Jeremy Linton <[email protected]>
> ---
> arch/arm64/include/asm/topology.h | 4 ++--
> arch/arm64/kernel/topology.c | 27 ++++++++++++++-------------
> 2 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index c4f2d50491eb..118136268f66 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -7,14 +7,14 @@
> struct cpu_topology {
> int thread_id;
> int core_id;
> - int cluster_id;
> + int physical_id;

package_id ?

It has been debated before, I know. Should we keep the cluster_id too
(even if it would be 1:1 mapped to package_id - for now) ?

There is also arch/arm to take into account, again, this patch is
just renaming (as it should have named since the beginning) a
topology level but we should consider everything from a legacy
perspective.

Lorenzo

> cpumask_t thread_sibling;
> cpumask_t core_sibling;
> };
>
> extern struct cpu_topology cpu_topology[NR_CPUS];
>
> -#define topology_physical_package_id(cpu) (cpu_topology[cpu].cluster_id)
> +#define topology_physical_package_id(cpu) (cpu_topology[cpu].physical_id)
> #define topology_core_id(cpu) (cpu_topology[cpu].core_id)
> #define topology_core_cpumask(cpu) (&cpu_topology[cpu].core_sibling)
> #define topology_sibling_cpumask(cpu) (&cpu_topology[cpu].thread_sibling)
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 8d48b233e6ce..74a8a5173a35 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -51,7 +51,7 @@ static int __init get_cpu_for_node(struct device_node *node)
> return -1;
> }
>
> -static int __init parse_core(struct device_node *core, int cluster_id,
> +static int __init parse_core(struct device_node *core, int physical_id,
> int core_id)
> {
> char name[10];
> @@ -67,7 +67,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
> leaf = false;
> cpu = get_cpu_for_node(t);
> if (cpu >= 0) {
> - cpu_topology[cpu].cluster_id = cluster_id;
> + cpu_topology[cpu].physical_id = physical_id;
> cpu_topology[cpu].core_id = core_id;
> cpu_topology[cpu].thread_id = i;
> } else {
> @@ -89,7 +89,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
> return -EINVAL;
> }
>
> - cpu_topology[cpu].cluster_id = cluster_id;
> + cpu_topology[cpu].physical_id = physical_id;
> cpu_topology[cpu].core_id = core_id;
> } else if (leaf) {
> pr_err("%pOF: Can't get CPU for leaf core\n", core);
> @@ -105,7 +105,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
> bool leaf = true;
> bool has_cores = false;
> struct device_node *c;
> - static int cluster_id __initdata;
> + static int physical_id __initdata;
> int core_id = 0;
> int i, ret;
>
> @@ -144,7 +144,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
> }
>
> if (leaf) {
> - ret = parse_core(c, cluster_id, core_id++);
> + ret = parse_core(c, physical_id, core_id++);
> } else {
> pr_err("%pOF: Non-leaf cluster with core %s\n",
> cluster, name);
> @@ -162,7 +162,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
> pr_warn("%pOF: empty cluster\n", cluster);
>
> if (leaf)
> - cluster_id++;
> + physical_id++;
>
> return 0;
> }
> @@ -198,7 +198,7 @@ static int __init parse_dt_topology(void)
> * only mark cores described in the DT as possible.
> */
> for_each_possible_cpu(cpu)
> - if (cpu_topology[cpu].cluster_id == -1)
> + if (cpu_topology[cpu].physical_id == -1)
> ret = -EINVAL;
>
> out_map:
> @@ -228,7 +228,7 @@ static void update_siblings_masks(unsigned int cpuid)
> for_each_possible_cpu(cpu) {
> cpu_topo = &cpu_topology[cpu];
>
> - if (cpuid_topo->cluster_id != cpu_topo->cluster_id)
> + if (cpuid_topo->physical_id != cpu_topo->physical_id)
> continue;
>
> cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
> @@ -249,7 +249,7 @@ void store_cpu_topology(unsigned int cpuid)
> struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> u64 mpidr;
>
> - if (cpuid_topo->cluster_id != -1)
> + if (cpuid_topo->physical_id != -1)
> goto topology_populated;
>
> mpidr = read_cpuid_mpidr();
> @@ -263,19 +263,19 @@ void store_cpu_topology(unsigned int cpuid)
> /* Multiprocessor system : Multi-threads per core */
> cpuid_topo->thread_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> - cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
> + cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
> MPIDR_AFFINITY_LEVEL(mpidr, 3) << 8;
> } else {
> /* Multiprocessor system : Single-thread per core */
> cpuid_topo->thread_id = -1;
> cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> - cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
> + cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
> MPIDR_AFFINITY_LEVEL(mpidr, 2) << 8 |
> MPIDR_AFFINITY_LEVEL(mpidr, 3) << 16;
> }
>
> pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n",
> - cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
> + cpuid, cpuid_topo->physical_id, cpuid_topo->core_id,
> cpuid_topo->thread_id, mpidr);
>
> topology_populated:
> @@ -291,7 +291,7 @@ static void __init reset_cpu_topology(void)
>
> cpu_topo->thread_id = -1;
> cpu_topo->core_id = 0;
> - cpu_topo->cluster_id = -1;
> + cpu_topo->physical_id = -1;
>
> cpumask_clear(&cpu_topo->core_sibling);
> cpumask_set_cpu(cpu, &cpu_topo->core_sibling);
> @@ -300,6 +300,7 @@ static void __init reset_cpu_topology(void)
> }
> }
>
> +
> void __init init_cpu_topology(void)
> {
> reset_cpu_topology();
> --
> 2.13.5
>

2017-12-13 18:21:43

[permalink] [raw]

Subject: Re: [PATCH v5 8/9] arm64: topology: Enable ACPI/PPTT based CPU topology.

Nit: remove the period in $SUBJECT and capitalize with a coherent
policy for the patches touching the same code.

On Fri, Dec 01, 2017 at 04:23:29PM -0600, Jeremy Linton wrote:
> Propagate the topology information from the PPTT tree to the
> cpu_topology array. We can get the thread id, core_id and
> cluster_id by assuming certain levels of the PPTT tree correspond
> to those concepts. The package_id is flagged in the tree and can be
> found by calling find_acpi_cpu_topology_package() which terminates
> its search when it finds an ACPI node flagged as the physical
> package. If the tree doesn't contain enough levels to represent
> all of the requested levels then the root node will be returned
> for all subsequent levels.
>
> Signed-off-by: Jeremy Linton <[email protected]>
> ---
> arch/arm64/kernel/topology.c | 47 +++++++++++++++++++++++++++++++++++++++++++-
> include/linux/topology.h | 2 ++
> 2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 74a8a5173a35..198714aca9e8 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -11,6 +11,7 @@
> * for more details.
> */
>
> +#include <linux/acpi.h>
> #include <linux/arch_topology.h>
> #include <linux/cpu.h>
> #include <linux/cpumask.h>
> @@ -22,6 +23,7 @@
> #include <linux/sched.h>
> #include <linux/sched/topology.h>
> #include <linux/slab.h>
> +#include <linux/smp.h>
> #include <linux/string.h>
>
> #include <asm/cpu.h>
> @@ -300,6 +302,47 @@ static void __init reset_cpu_topology(void)
> }
> }
>
> +#ifdef CONFIG_ACPI
> +/*
> + * Propagate the topology information of the processor_topology_node tree to the
> + * cpu_topology array.
> + */
> +static int __init parse_acpi_topology(void)
> +{
> + u64 is_threaded;

Nit: a bool would be preferable.

> + int cpu;
> + int topology_id;

int cpu, topology_id;

> + is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;

> + for_each_possible_cpu(cpu) {
> + topology_id = find_acpi_cpu_topology(cpu, 0);
> + if (topology_id < 0)
> + return topology_id;
> +
> + if (is_threaded) {
> + cpu_topology[cpu].thread_id = topology_id;
> + topology_id = find_acpi_cpu_topology(cpu, 1);
> + cpu_topology[cpu].core_id = topology_id;
> + topology_id = find_acpi_cpu_topology_package(cpu);
> + cpu_topology[cpu].physical_id = topology_id;
> + } else {
> + cpu_topology[cpu].thread_id = -1;
> + cpu_topology[cpu].core_id = topology_id;
> + topology_id = find_acpi_cpu_topology_package(cpu);
> + cpu_topology[cpu].physical_id = topology_id;
> + }
> + }

Add a space.

It is probably my fault so apologies if that's the case. The

find_acpi_cpu_topology()

API is a bit strange since it behaves differently according to the
level passed in.

I think it is better to define two calls (it might well have been like
that in one of the previous series versions):

- find_acpi_cpu_package_level() (returns: package level if success, <0 on
failure)
- acpi_cpu_topology_id()

It would even be better to lump the two calls together but you do not
know how many topology levels are there so it becomes a bit complicated
to handle.

> + return 0;
> +}
> +
> +#else
> +static int __init parse_acpi_topology(void)

static inline ?

> +{
> + /*ACPI kernels should be built with PPTT support*/

I think you can remove this comment.

> + return -EINVAL;
> +}
> +#endif
>
> void __init init_cpu_topology(void)
> {
> @@ -309,6 +352,8 @@ void __init init_cpu_topology(void)
> * Discard anything that was parsed if we hit an error so we
> * don't use partial information.
> */
> - if (of_have_populated_dt() && parse_dt_topology())
> + if ((!acpi_disabled) && parse_acpi_topology())
> + reset_cpu_topology();
> + else if (of_have_populated_dt() && parse_dt_topology())
> reset_cpu_topology();
> }
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index cb0775e1ee4b..170ce87edd88 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -43,6 +43,8 @@
> if (nr_cpus_node(node))
>
> int arch_update_cpu_topology(void);
> +int find_acpi_cpu_topology(unsigned int cpu, int level);
> +int find_acpi_cpu_topology_package(unsigned int cpu);

I do not think these two declarations:

a) belong in this patch
b) belong in include/linux/topology.h (should be acpi.h)

Lorenzo

2017-12-13 22:28:38

[permalink] [raw]

Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
<[email protected]> wrote:
> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>> Hi,
>>
>> First, thanks for taking a look at this.
>>
>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>> >On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>> >>The PPTT can be used to determine the groupings of CPU's at
>> >>given levels in the system. Lets add a few routines to the PPTT
>> >>parsing code to return a unique id for each unique level in the
>> >>processor hierarchy. This can then be matched to build
>> >>thread/core/cluster/die/package/etc mappings for each processing
>> >>element in the system.
>> >>
>> >>Signed-off-by: Jeremy Linton <[email protected]>
>> >
>> >Why can't this be folded into patch [2/9]?
>>
>> It can, and I will be happy squash it.
>>
>> It was requested that the topology portion of the parser be split
>> out back in v3.
>>
>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>
> I asked to split cache/topology since I am not familiar with cache
> code and Sudeep - who looks after the cache code - won't be able
> to review this series in time for v4.16.

OK, so why do we need it in 4.16?

2017-12-13 23:06:37

[permalink] [raw]

Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

Hi,

On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
> <[email protected]> wrote:
>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>>> Hi,
>>>
>>> First, thanks for taking a look at this.
>>>
>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>>>>> The PPTT can be used to determine the groupings of CPU's at
>>>>> given levels in the system. Lets add a few routines to the PPTT
>>>>> parsing code to return a unique id for each unique level in the
>>>>> processor hierarchy. This can then be matched to build
>>>>> thread/core/cluster/die/package/etc mappings for each processing
>>>>> element in the system.
>>>>>
>>>>> Signed-off-by: Jeremy Linton <[email protected]>
>>>>
>>>> Why can't this be folded into patch [2/9]?
>>>
>>> It can, and I will be happy squash it.
>>>
>>> It was requested that the topology portion of the parser be split
>>> out back in v3.
>>>
>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>>
>> I asked to split cache/topology since I am not familiar with cache
>> code and Sudeep - who looks after the cache code - won't be able
>> to review this series in time for v4.16.
>
> OK, so why do we need it in 4.16?

I think its more case of as soon as possible. That is because there are
machines where the topology is completely incorrect due to assumptions
the kernel makes based on registers that aren't defined for that purpose
(say describing which cores are in a physical socket, or LLC's attached
to interconnects or memory controllers).

This incorrect topology information is reported to things like the
kernel scheduler, which then makes poor scheduling decisions resulting
in sub-optimal system performance.

This patchset (and ACPI 6.2) clears up a lot of those problems.

Thanks,

2017-12-13 23:09:57

[permalink] [raw]

Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton <[email protected]> wrote:
> Hi,
>
>
> On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
>>
>> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
>> <[email protected]> wrote:
>>>
>>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>>>>
>>>> Hi,
>>>>
>>>> First, thanks for taking a look at this.
>>>>
>>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>>>>>
>>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>>>>>>
>>>>>> The PPTT can be used to determine the groupings of CPU's at
>>>>>> given levels in the system. Lets add a few routines to the PPTT
>>>>>> parsing code to return a unique id for each unique level in the
>>>>>> processor hierarchy. This can then be matched to build
>>>>>> thread/core/cluster/die/package/etc mappings for each processing
>>>>>> element in the system.
>>>>>>
>>>>>> Signed-off-by: Jeremy Linton <[email protected]>
>>>>>
>>>>>
>>>>> Why can't this be folded into patch [2/9]?
>>>>
>>>>
>>>> It can, and I will be happy squash it.
>>>>
>>>> It was requested that the topology portion of the parser be split
>>>> out back in v3.
>>>>
>>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>>>
>>>
>>> I asked to split cache/topology since I am not familiar with cache
>>> code and Sudeep - who looks after the cache code - won't be able
>>> to review this series in time for v4.16.
>>
>>
>> OK, so why do we need it in 4.16?
>
>
> I think its more case of as soon as possible. That is because there are
> machines where the topology is completely incorrect due to assumptions the
> kernel makes based on registers that aren't defined for that purpose (say
> describing which cores are in a physical socket, or LLC's attached to
> interconnects or memory controllers).
>
> This incorrect topology information is reported to things like the kernel
> scheduler, which then makes poor scheduling decisions resulting in
> sub-optimal system performance.
>
> This patchset (and ACPI 6.2) clears up a lot of those problems.

As long as the ACPI tables are as expected that is, I suppose?

Anyway, fair enough, but I don't want to rush it in.

Thanks,
Rafael

2017-12-15 16:36:39

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

Hi,

On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> [+Morten, Dietmar]
>
> $SUBJECT should be:
>
> arm64: topology: rename cluster_id

Sure..

>
> On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
>> Lets match the name of the arm64 topology field
>> to the kernel macro that uses it.
>>
>> Signed-off-by: Jeremy Linton <[email protected]>
>> ---
>> arch/arm64/include/asm/topology.h | 4 ++--
>> arch/arm64/kernel/topology.c | 27 ++++++++++++++-------------
>> 2 files changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
>> index c4f2d50491eb..118136268f66 100644
>> --- a/arch/arm64/include/asm/topology.h
>> +++ b/arch/arm64/include/asm/topology.h
>> @@ -7,14 +7,14 @@
>> struct cpu_topology {
>> int thread_id;
>> int core_id;
>> - int cluster_id;
>> + int physical_id;
>
> package_id ?

Given the macro is topology_physical_package_id, either makes sense to
me. <shrug> I will change it in the next set.

>
> It has been debated before, I know. Should we keep the cluster_id too
> (even if it would be 1:1 mapped to package_id - for now) ?

Well given that this patch replaces the patch that did that at your
request..

I was hoping someone else would comment here, but my take at this point
is that it doesn't really matter in a functional sense at the moment.
Like the chiplet discussion it can be the subject of a future patch
along with the patches which tweak the scheduler to understand the split.

BTW, given that i'm OoO next week, and the following that are the
holidays, I don't intend to repost this for a couple weeks. I don't
think there are any issues with this set.

>
> There is also arch/arm to take into account, again, this patch is
> just renaming (as it should have named since the beginning) a
> topology level but we should consider everything from a legacy
> perspective.
>
> Lorenzo
>
>> cpumask_t thread_sibling;
>> cpumask_t core_sibling;
>> };
>>
>> extern struct cpu_topology cpu_topology[NR_CPUS];
>>
>> -#define topology_physical_package_id(cpu) (cpu_topology[cpu].cluster_id)
>> +#define topology_physical_package_id(cpu) (cpu_topology[cpu].physical_id)
>> #define topology_core_id(cpu) (cpu_topology[cpu].core_id)
>> #define topology_core_cpumask(cpu) (&cpu_topology[cpu].core_sibling)
>> #define topology_sibling_cpumask(cpu) (&cpu_topology[cpu].thread_sibling)
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 8d48b233e6ce..74a8a5173a35 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -51,7 +51,7 @@ static int __init get_cpu_for_node(struct device_node *node)
>> return -1;
>> }
>>
>> -static int __init parse_core(struct device_node *core, int cluster_id,
>> +static int __init parse_core(struct device_node *core, int physical_id,
>> int core_id)
>> {
>> char name[10];
>> @@ -67,7 +67,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
>> leaf = false;
>> cpu = get_cpu_for_node(t);
>> if (cpu >= 0) {
>> - cpu_topology[cpu].cluster_id = cluster_id;
>> + cpu_topology[cpu].physical_id = physical_id;
>> cpu_topology[cpu].core_id = core_id;
>> cpu_topology[cpu].thread_id = i;
>> } else {
>> @@ -89,7 +89,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
>> return -EINVAL;
>> }
>>
>> - cpu_topology[cpu].cluster_id = cluster_id;
>> + cpu_topology[cpu].physical_id = physical_id;
>> cpu_topology[cpu].core_id = core_id;
>> } else if (leaf) {
>> pr_err("%pOF: Can't get CPU for leaf core\n", core);
>> @@ -105,7 +105,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
>> bool leaf = true;
>> bool has_cores = false;
>> struct device_node *c;
>> - static int cluster_id __initdata;
>> + static int physical_id __initdata;
>> int core_id = 0;
>> int i, ret;
>>
>> @@ -144,7 +144,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
>> }
>>
>> if (leaf) {
>> - ret = parse_core(c, cluster_id, core_id++);
>> + ret = parse_core(c, physical_id, core_id++);
>> } else {
>> pr_err("%pOF: Non-leaf cluster with core %s\n",
>> cluster, name);
>> @@ -162,7 +162,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
>> pr_warn("%pOF: empty cluster\n", cluster);
>>
>> if (leaf)
>> - cluster_id++;
>> + physical_id++;
>>
>> return 0;
>> }
>> @@ -198,7 +198,7 @@ static int __init parse_dt_topology(void)
>> * only mark cores described in the DT as possible.
>> */
>> for_each_possible_cpu(cpu)
>> - if (cpu_topology[cpu].cluster_id == -1)
>> + if (cpu_topology[cpu].physical_id == -1)
>> ret = -EINVAL;
>>
>> out_map:
>> @@ -228,7 +228,7 @@ static void update_siblings_masks(unsigned int cpuid)
>> for_each_possible_cpu(cpu) {
>> cpu_topo = &cpu_topology[cpu];
>>
>> - if (cpuid_topo->cluster_id != cpu_topo->cluster_id)
>> + if (cpuid_topo->physical_id != cpu_topo->physical_id)
>> continue;
>>
>> cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
>> @@ -249,7 +249,7 @@ void store_cpu_topology(unsigned int cpuid)
>> struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
>> u64 mpidr;
>>
>> - if (cpuid_topo->cluster_id != -1)
>> + if (cpuid_topo->physical_id != -1)
>> goto topology_populated;
>>
>> mpidr = read_cpuid_mpidr();
>> @@ -263,19 +263,19 @@ void store_cpu_topology(unsigned int cpuid)
>> /* Multiprocessor system : Multi-threads per core */
>> cpuid_topo->thread_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> - cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
>> + cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
>> MPIDR_AFFINITY_LEVEL(mpidr, 3) << 8;
>> } else {
>> /* Multiprocessor system : Single-thread per core */
>> cpuid_topo->thread_id = -1;
>> cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> - cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
>> + cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
>> MPIDR_AFFINITY_LEVEL(mpidr, 2) << 8 |
>> MPIDR_AFFINITY_LEVEL(mpidr, 3) << 16;
>> }
>>
>> pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n",
>> - cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
>> + cpuid, cpuid_topo->physical_id, cpuid_topo->core_id,
>> cpuid_topo->thread_id, mpidr);
>>
>> topology_populated:
>> @@ -291,7 +291,7 @@ static void __init reset_cpu_topology(void)
>>
>> cpu_topo->thread_id = -1;
>> cpu_topo->core_id = 0;
>> - cpu_topo->cluster_id = -1;
>> + cpu_topo->physical_id = -1;
>>
>> cpumask_clear(&cpu_topo->core_sibling);
>> cpumask_set_cpu(cpu, &cpu_topo->core_sibling);
>> @@ -300,6 +300,7 @@ static void __init reset_cpu_topology(void)
>> }
>> }
>>
>> +
>> void __init init_cpu_topology(void)
>> {
>> reset_cpu_topology();
>> --
>> 2.13.5
>>

2017-12-15 17:42:09

[permalink] [raw]

Subject: Re: [PATCH v5 8/9] arm64: topology: Enable ACPI/PPTT based CPU topology.

Hi,

On 12/13/2017 12:22 PM, Lorenzo Pieralisi wrote:
> Nit: remove the period in $SUBJECT and capitalize with a coherent
> policy for the patches touching the same code.

Ok, thanks.

>
> On Fri, Dec 01, 2017 at 04:23:29PM -0600, Jeremy Linton wrote:
>> Propagate the topology information from the PPTT tree to the
>> cpu_topology array. We can get the thread id, core_id and
>> cluster_id by assuming certain levels of the PPTT tree correspond
>> to those concepts. The package_id is flagged in the tree and can be
>> found by calling find_acpi_cpu_topology_package() which terminates
>> its search when it finds an ACPI node flagged as the physical
>> package. If the tree doesn't contain enough levels to represent
>> all of the requested levels then the root node will be returned
>> for all subsequent levels.
>>
>> Signed-off-by: Jeremy Linton <[email protected]>
>> ---
>> arch/arm64/kernel/topology.c | 47 +++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/topology.h | 2 ++
>> 2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 74a8a5173a35..198714aca9e8 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -11,6 +11,7 @@
>> * for more details.
>> */
>>
>> +#include <linux/acpi.h>
>> #include <linux/arch_topology.h>
>> #include <linux/cpu.h>
>> #include <linux/cpumask.h>
>> @@ -22,6 +23,7 @@
>> #include <linux/sched.h>
>> #include <linux/sched/topology.h>
>> #include <linux/slab.h>
>> +#include <linux/smp.h>
>> #include <linux/string.h>
>>
>> #include <asm/cpu.h>
>> @@ -300,6 +302,47 @@ static void __init reset_cpu_topology(void)
>> }
>> }
>>
>> +#ifdef CONFIG_ACPI
>> +/*
>> + * Propagate the topology information of the processor_topology_node tree to the
>> + * cpu_topology array.
>> + */
>> +static int __init parse_acpi_topology(void)
>> +{
>> + u64 is_threaded;
>
> Nit: a bool would be preferable.
>
>> + int cpu;
>> + int topology_id;
>
> int cpu, topology_id;
>
>> + is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
>
>> + for_each_possible_cpu(cpu) {
>> + topology_id = find_acpi_cpu_topology(cpu, 0);
>> + if (topology_id < 0)
>> + return topology_id;
>> +
>> + if (is_threaded) {
>> + cpu_topology[cpu].thread_id = topology_id;
>> + topology_id = find_acpi_cpu_topology(cpu, 1);
>> + cpu_topology[cpu].core_id = topology_id;
>> + topology_id = find_acpi_cpu_topology_package(cpu);
>> + cpu_topology[cpu].physical_id = topology_id;
>> + } else {
>> + cpu_topology[cpu].thread_id = -1;
>> + cpu_topology[cpu].core_id = topology_id;
>> + topology_id = find_acpi_cpu_topology_package(cpu);
>> + cpu_topology[cpu].physical_id = topology_id;
>> + }
>> + }
>
> Add a space.
>
> It is probably my fault so apologies if that's the case. The
>
> find_acpi_cpu_topology()
>
> API is a bit strange since it behaves differently according to the
> level passed in.

? In the sense that the id is more directly defined by the firmware for
level0? Not sure this really matters, particularly if at some future
point we come up with a way to standardize an id for the sockets/etc (or
we just renumber the nodes).

AKA, I don't see a problem as we aren't making any guarantees about what
the return id represents other than its unique for siblings at a given
level.

>
> I think it is better to define two calls (it might well have been like
> that in one of the previous series versions):
>
> - find_acpi_cpu_package_level() (returns: package level if success, <0 on
> failure)
> - acpi_cpu_topology_id()

So, knowing the absolute level a given flag is set at allows you to
query a node relative to that level. That is a good idea if you wanted
to identify say a numa in package level (say package-1). But its
complicated by that fact that package-1 may be meaningless in a lot of
cases (maybe package-1 is just a core). The alternative here for finding
a numa proximity domain is to try to find a PPTT node with a subset of
cores which happens to match an proximity domain. But given there is
currently nothing in the specification which requires a 1:1 mapping
between a given set of PPTT nodes and a proximity domain (given the PPTT
is focused on cache nodes) I tend to think we want further flags in the
PPTT and language that makes it clear when this happens. So the
interface should be more generic find_acpi_cpu_flag_level(cpu, flag).
That way we avoid the complexity of trying to guess from a relative
level if a particular topological feature is appropriate.

>
> It would even be better to lump the two calls together but you do not
> know how many topology levels are there so it becomes a bit complicated
> to handle.

Right, which is basically what the underlying
find_acpi_cpu_topology_tag() is doing at the moment. My tendency here is
just to export that call and let the PPTT parsing code wrap the decision
about what to do if the flag can't be found. Which means the whole
discussion above about letting the higher level code try to infer
relative levels is a last resort. I'm more for creating a couple
additional flags (FLAG_GIVE_ME_LEVEL_WHERE_NUMA_STARTS) and feeding it
to acpi_cpu_topology_tag() with some additional logic which says
something to the affect, return the NUMA level in the PPTT described by
some future standardized flag, otherwise return the socket level, and if
that doesn't exist return the root node level (or maybe if we get
desperate a node which seems to match a SRAT proximity domain). That
keeps the PPTT interface fairly simple, and keeps the level selection
out of the common topology code.

There are also some other possible future directions which don't fit any
of these models. So in that regard I think we want to keep the inteface
as simple as possible, and transform it in the future when we have a
direct need for it.

Thanks,

2017-12-18 12:42:39

by Morten Rasmussen

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> Hi,
>
> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> >[+Morten, Dietmar]
> >
> >$SUBJECT should be:
> >
> >arm64: topology: rename cluster_id
>
> Sure..
>
> >
> >On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
> >>Lets match the name of the arm64 topology field
> >>to the kernel macro that uses it.
> >>
> >>Signed-off-by: Jeremy Linton <[email protected]>
> >>---
> >> arch/arm64/include/asm/topology.h | 4 ++--
> >> arch/arm64/kernel/topology.c | 27 ++++++++++++++-------------
> >> 2 files changed, 16 insertions(+), 15 deletions(-)
> >>
> >>diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> >>index c4f2d50491eb..118136268f66 100644
> >>--- a/arch/arm64/include/asm/topology.h
> >>+++ b/arch/arm64/include/asm/topology.h
> >>@@ -7,14 +7,14 @@
> >> struct cpu_topology {
> >> int thread_id;
> >> int core_id;
> >>- int cluster_id;
> >>+ int physical_id;
> >
> >package_id ?
>
> Given the macro is topology_physical_package_id, either makes sense to me.
> <shrug> I will change it in the next set.

I would vote for package_id too. arch/arm has 'socket_id' though.

> >
> >It has been debated before, I know. Should we keep the cluster_id too
> >(even if it would be 1:1 mapped to package_id - for now) ?
>
> Well given that this patch replaces the patch that did that at your
> request..
>
> I was hoping someone else would comment here, but my take at this point is
> that it doesn't really matter in a functional sense at the moment.
> Like the chiplet discussion it can be the subject of a future patch along
> with the patches which tweak the scheduler to understand the split.
>
> BTW, given that i'm OoO next week, and the following that are the holidays,
> I don't intend to repost this for a couple weeks. I don't think there are
> any issues with this set.
>
> >
> >There is also arch/arm to take into account, again, this patch is
> >just renaming (as it should have named since the beginning) a
> >topology level but we should consider everything from a legacy
> >perspective.

arch/arm has gone for thread/core/socket for the three topology levels
it supports.

I'm not sure what short term value keeping cluster_id has? Isn't it just
about where we make the package = cluster assignment? Currently it is in
the definition of topology_physical_package_id. If we keep cluster_id
and add package_id, it gets moved into the MPIDR/DT parsing code.

Keeping cluster_id and introducing a topology_cluster_id function could
help cleaning up some of the users of topology_physical_package_id that
currently assumes package_id == cluster_id though.

Morten

2017-12-18 15:46:25

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

On Mon, Dec 18, 2017 at 12:42:29PM +0000, Morten Rasmussen wrote:
> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> > Hi,
> >
> > On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> > >[+Morten, Dietmar]
> > >
> > >$SUBJECT should be:
> > >
> > >arm64: topology: rename cluster_id
> >
> > Sure..
> >
> > >
> > >On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
> > >>Lets match the name of the arm64 topology field
> > >>to the kernel macro that uses it.
> > >>
> > >>Signed-off-by: Jeremy Linton <[email protected]>
> > >>---
> > >> arch/arm64/include/asm/topology.h | 4 ++--
> > >> arch/arm64/kernel/topology.c | 27 ++++++++++++++-------------
> > >> 2 files changed, 16 insertions(+), 15 deletions(-)
> > >>
> > >>diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> > >>index c4f2d50491eb..118136268f66 100644
> > >>--- a/arch/arm64/include/asm/topology.h
> > >>+++ b/arch/arm64/include/asm/topology.h
> > >>@@ -7,14 +7,14 @@
> > >> struct cpu_topology {
> > >> int thread_id;
> > >> int core_id;
> > >>- int cluster_id;
> > >>+ int physical_id;
> > >
> > >package_id ?
> >
> > Given the macro is topology_physical_package_id, either makes sense to me.
> > <shrug> I will change it in the next set.
>
> I would vote for package_id too. arch/arm has 'socket_id' though.
>
> > >
> > >It has been debated before, I know. Should we keep the cluster_id too
> > >(even if it would be 1:1 mapped to package_id - for now) ?
> >
> > Well given that this patch replaces the patch that did that at your
> > request..
> >
> > I was hoping someone else would comment here, but my take at this point is
> > that it doesn't really matter in a functional sense at the moment.
> > Like the chiplet discussion it can be the subject of a future patch along
> > with the patches which tweak the scheduler to understand the split.
> >
> > BTW, given that i'm OoO next week, and the following that are the holidays,
> > I don't intend to repost this for a couple weeks. I don't think there are
> > any issues with this set.
> >
> > >
> > >There is also arch/arm to take into account, again, this patch is
> > >just renaming (as it should have named since the beginning) a
> > >topology level but we should consider everything from a legacy
> > >perspective.
>
> arch/arm has gone for thread/core/socket for the three topology levels
> it supports.
>
> I'm not sure what short term value keeping cluster_id has? Isn't it just
> about where we make the package = cluster assignment? Currently it is in
> the definition of topology_physical_package_id. If we keep cluster_id
> and add package_id, it gets moved into the MPIDR/DT parsing code.
>
> Keeping cluster_id and introducing a topology_cluster_id function could
> help cleaning up some of the users of topology_physical_package_id that
> currently assumes package_id == cluster_id though.

I think we should settle for a name (eg package_id), remove cluster_id
and convert arch/arm socket_id to the same naming used on arm64 (for
consistency - it is just a variable rename).

This leaves us with the naming "cluster" only in DT topology bindings,
which should be fine, given that "cluster" in that context is just a
"processor-container" - yes we could have chosen a better naming in
the first place but that's what it is.

We should nuke the existing users of topology_physical_package_id()
to identify clusters, I would not add another function for that purpose,
let's nip it in the bud.

Lorenzo

2017-12-19 09:38:16

by Morten Rasmussen

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

On Mon, Dec 18, 2017 at 03:47:03PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Dec 18, 2017 at 12:42:29PM +0000, Morten Rasmussen wrote:
> > On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> > > Hi,
> > >
> > > On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> > > >[+Morten, Dietmar]
> > > >
> > > >$SUBJECT should be:
> > > >
> > > >arm64: topology: rename cluster_id
> > >
> > > Sure..
> > >
> > > >
> > > >On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
> > > >>Lets match the name of the arm64 topology field
> > > >>to the kernel macro that uses it.
> > > >>
> > > >>Signed-off-by: Jeremy Linton <[email protected]>
> > > >>---
> > > >> arch/arm64/include/asm/topology.h | 4 ++--
> > > >> arch/arm64/kernel/topology.c | 27 ++++++++++++++-------------
> > > >> 2 files changed, 16 insertions(+), 15 deletions(-)
> > > >>
> > > >>diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> > > >>index c4f2d50491eb..118136268f66 100644
> > > >>--- a/arch/arm64/include/asm/topology.h
> > > >>+++ b/arch/arm64/include/asm/topology.h
> > > >>@@ -7,14 +7,14 @@
> > > >> struct cpu_topology {
> > > >> int thread_id;
> > > >> int core_id;
> > > >>- int cluster_id;
> > > >>+ int physical_id;
> > > >
> > > >package_id ?
> > >
> > > Given the macro is topology_physical_package_id, either makes sense to me.
> > > <shrug> I will change it in the next set.
> >
> > I would vote for package_id too. arch/arm has 'socket_id' though.
> >
> > > >
> > > >It has been debated before, I know. Should we keep the cluster_id too
> > > >(even if it would be 1:1 mapped to package_id - for now) ?
> > >
> > > Well given that this patch replaces the patch that did that at your
> > > request..
> > >
> > > I was hoping someone else would comment here, but my take at this point is
> > > that it doesn't really matter in a functional sense at the moment.
> > > Like the chiplet discussion it can be the subject of a future patch along
> > > with the patches which tweak the scheduler to understand the split.
> > >
> > > BTW, given that i'm OoO next week, and the following that are the holidays,
> > > I don't intend to repost this for a couple weeks. I don't think there are
> > > any issues with this set.
> > >
> > > >
> > > >There is also arch/arm to take into account, again, this patch is
> > > >just renaming (as it should have named since the beginning) a
> > > >topology level but we should consider everything from a legacy
> > > >perspective.
> >
> > arch/arm has gone for thread/core/socket for the three topology levels
> > it supports.
> >
> > I'm not sure what short term value keeping cluster_id has? Isn't it just
> > about where we make the package = cluster assignment? Currently it is in
> > the definition of topology_physical_package_id. If we keep cluster_id
> > and add package_id, it gets moved into the MPIDR/DT parsing code.
> >
> > Keeping cluster_id and introducing a topology_cluster_id function could
> > help cleaning up some of the users of topology_physical_package_id that
> > currently assumes package_id == cluster_id though.
>
> I think we should settle for a name (eg package_id), remove cluster_id
> and convert arch/arm socket_id to the same naming used on arm64 (for
> consistency - it is just a variable rename).

Agreed.

> This leaves us with the naming "cluster" only in DT topology bindings,
> which should be fine, given that "cluster" in that context is just a
> "processor-container" - yes we could have chosen a better naming in
> the first place but that's what it is.

I think having "clusters" in DT is fine as it represent the actual
hardware topology and uses the actual "Arm" term to describe it. The
default topology in Linux just doesn't have an equivalent topology
level, but that can be fixed. DT is however missing a notion of package.

> We should nuke the existing users of topology_physical_package_id()
> to identify clusters, I would not add another function for that purpose,
> let's nip it in the bud.

Even better if that can be pulled of.

Morten

2018-01-02 02:29:54

by Xiongfeng Wang

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

Hi,

On 2017/12/18 20:42, Morten Rasmussen wrote:
> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>> Hi,
>>
>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>> [+Morten, Dietmar]
>>>
>>> $SUBJECT should be:
>>>
>>> arm64: topology: rename cluster_id
>>
[cut]
>>
>> I was hoping someone else would comment here, but my take at this point is
>> that it doesn't really matter in a functional sense at the moment.
>> Like the chiplet discussion it can be the subject of a future patch along
>> with the patches which tweak the scheduler to understand the split.
>>
>> BTW, given that i'm OoO next week, and the following that are the holidays,
>> I don't intend to repost this for a couple weeks. I don't think there are
>> any issues with this set.
>>
>>>
>>> There is also arch/arm to take into account, again, this patch is
>>> just renaming (as it should have named since the beginning) a
>>> topology level but we should consider everything from a legacy
>>> perspective.
>
> arch/arm has gone for thread/core/socket for the three topology levels
> it supports.
>
> I'm not sure what short term value keeping cluster_id has? Isn't it just
> about where we make the package = cluster assignment? Currently it is in
> the definition of topology_physical_package_id. If we keep cluster_id
> and add package_id, it gets moved into the MPIDR/DT parsing code.
>
> Keeping cluster_id and introducing a topology_cluster_id function could
> help cleaning up some of the users of topology_physical_package_id that
> currently assumes package_id == cluster_id though.

I think we still need the information describing which cores are in one cluster.
Many arm64 chips have the architecture core/cluster/socket. Cores in one cluster may
share a same L2 cache. That information can be used to build the sched_domain. If we put
cores in one cluster in one sched_domain, the performance will be better.(please see
kernel/sched/topology.c:1197, cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
sched_domain)
So I think we still need variable to record which cores are in one sched_domain for future use.

Thanks,
Xiongfeng

>
> Morten
>
> .
>

2018-01-02 11:30:29

by Morten Rasmussen

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

On Tue, Jan 02, 2018 at 10:29:35AM +0800, Xiongfeng Wang wrote:
> Hi,
>
> On 2017/12/18 20:42, Morten Rasmussen wrote:
> > On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> >> Hi,
> >>
> >> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> >>> [+Morten, Dietmar]
> >>>
> >>> $SUBJECT should be:
> >>>
> >>> arm64: topology: rename cluster_id
> >>
> [cut]
> >>
> >> I was hoping someone else would comment here, but my take at this point is
> >> that it doesn't really matter in a functional sense at the moment.
> >> Like the chiplet discussion it can be the subject of a future patch along
> >> with the patches which tweak the scheduler to understand the split.
> >>
> >> BTW, given that i'm OoO next week, and the following that are the holidays,
> >> I don't intend to repost this for a couple weeks. I don't think there are
> >> any issues with this set.
> >>
> >>>
> >>> There is also arch/arm to take into account, again, this patch is
> >>> just renaming (as it should have named since the beginning) a
> >>> topology level but we should consider everything from a legacy
> >>> perspective.
> >
> > arch/arm has gone for thread/core/socket for the three topology levels
> > it supports.
> >
> > I'm not sure what short term value keeping cluster_id has? Isn't it just
> > about where we make the package = cluster assignment? Currently it is in
> > the definition of topology_physical_package_id. If we keep cluster_id
> > and add package_id, it gets moved into the MPIDR/DT parsing code.
> >
> > Keeping cluster_id and introducing a topology_cluster_id function could
> > help cleaning up some of the users of topology_physical_package_id that
> > currently assumes package_id == cluster_id though.
>
> I think we still need the information describing which cores are in one cluster.
> Many arm64 chips have the architecture core/cluster/socket. Cores in one cluster may
> share a same L2 cache. That information can be used to build the sched_domain. If we put
> cores in one cluster in one sched_domain, the performance will be better.(please see
> kernel/sched/topology.c:1197, cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
> sched_domain)
> So I think we still need variable to record which cores are in one sched_domain for future use.

I agree that information about clusters is useful. The problem is that
we currently treat clusters as sockets (also known as dies or
physical_package_id depending on which bit of code you are looking at). We
don't handle the core/cluster/socket topology you mention as three
'levels' of grouping of cores. We currently create one sched_domain
levels for cores (in a cluster) and a parent sched_domain level of all
clusters in the system ignoring if those are in different sockets and
tell the scheduler that those clusters are all separate sockets. This
doesn't work very well for systems with many clusters and/or multiple
sockets.

How to solve that problem is a longer discussion which I'm happy to
have. This patch is just preparing to get rid of the assumption that
clusters are sockets as this is not in line with what other
architectures does and the assumptions made by the scheduler.

Morten

2018-01-03 08:49:54

by vkilari

[permalink] [raw]

Subject: RE: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

Hi Jeremy,

Sorry, I don't have your previous patch emails to reply on right patch
context.
So commenting on top of this patch.

AFAIU, the PPTT v5 patches still rely on CLIDR_EL1 register to know the type
of
Caches enabled/available on the platform. With PPTT, it should not rely on
architecture
registers. There can be platforms which can report cache availability in
PPTT but not in
architecture registers.

The following code snippet shows usage of CLIDR_EL1

In arch/arm64/kernel/cacheinfo.c

static inline enum cache_type get_cache_type(int level)
{
u64 clidr;

if (level > MAX_CACHE_LEVEL)
return CACHE_TYPE_NOCACHE;
clidr = read_sysreg(clidr_el1);
return CLIDR_CTYPE(clidr, level);
}

static int __populate_cache_leaves(unsigned int cpu)
{
unsigned int level, idx;
enum cache_type type;
struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
struct cacheinfo *this_leaf = this_cpu_ci->info_list;

for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
idx < this_cpu_ci->num_leaves; idx++, level++) {
type = get_cache_type(level);
if (type == CACHE_TYPE_SEPARATE) {
ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
} else {
ci_leaf_init(this_leaf++, type, level);
}
}
return 0;
}

In populate_cache_leaves() the cache type is read from CLIDR_EL1 register.
If CLIDR_EL1 reports CACHE_TYPE_NOCACHE for a particular level then sysfs
entry
/sys/devices/system/cpu/cpu0/index<n>/type is not created and hence
userspace tools
like lstopo will not report this cache level.

Regards
Vijay

> -----Original Message-----
> From: linux-arm-kernel
[mailto:[email protected]]
> On Behalf Of Rafael J. Wysocki
> Sent: Thursday, December 14, 2017 4:40 AM
> To: Jeremy Linton <[email protected]>
> Cc: Mark Rutland <[email protected]>; [email protected];
> [email protected]; Lorenzo Pieralisi
> <[email protected]>; Catalin Marinas <[email protected]>;
> Rafael J. Wysocki <[email protected]>; [email protected]; Will Deacon
> <[email protected]>; Linux PM <[email protected]>; Rafael J.
> Wysocki <[email protected]>; Greg Kroah-Hartman
> <[email protected]>; Linux Kernel Mailing List <linux-
> [email protected]>; ACPI Devel Maling List
<[email protected]>;
> Viresh Kumar <[email protected]>; Hanjun Guo
> <[email protected]>; Al Stone <[email protected]>; Sudeep Holla
> <[email protected]>; [email protected];
> [email protected]; [email protected]; Len
> Brown <[email protected]>
> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
>
> On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton <[email protected]>
> wrote:
> > Hi,
> >
> >
> > On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
> >>
> >> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
> >> <[email protected]> wrote:
> >>>
> >>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> First, thanks for taking a look at this.
> >>>>
> >>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
> >>>>>
> >>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
> >>>>>>
> >>>>>> The PPTT can be used to determine the groupings of CPU's at given
> >>>>>> levels in the system. Lets add a few routines to the PPTT parsing
> >>>>>> code to return a unique id for each unique level in the processor
> >>>>>> hierarchy. This can then be matched to build
> >>>>>> thread/core/cluster/die/package/etc mappings for each processing
> >>>>>> element in the system.
> >>>>>>
> >>>>>> Signed-off-by: Jeremy Linton <[email protected]>
> >>>>>
> >>>>>
> >>>>> Why can't this be folded into patch [2/9]?
> >>>>
> >>>>
> >>>> It can, and I will be happy squash it.
> >>>>
> >>>> It was requested that the topology portion of the parser be split
> >>>> out back in v3.
> >>>>
> >>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
> >>>
> >>>
> >>> I asked to split cache/topology since I am not familiar with cache
> >>> code and Sudeep - who looks after the cache code - won't be able to
> >>> review this series in time for v4.16.
> >>
> >>
> >> OK, so why do we need it in 4.16?
> >
> >
> > I think its more case of as soon as possible. That is because there
> > are machines where the topology is completely incorrect due to
> > assumptions the kernel makes based on registers that aren't defined
> > for that purpose (say describing which cores are in a physical socket,
> > or LLC's attached to interconnects or memory controllers).
> >
> > This incorrect topology information is reported to things like the
> > kernel scheduler, which then makes poor scheduling decisions resulting
> > in sub-optimal system performance.
> >
> > This patchset (and ACPI 6.2) clears up a lot of those problems.
>
> As long as the ACPI tables are as expected that is, I suppose?
>
> Anyway, fair enough, but I don't want to rush it in.
>
> Thanks,
> Rafael
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2018-01-03 14:21:38

[permalink] [raw]

Subject: Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

(Sorry for the delay, just returning from vacation)

On 12/12/17 23:37, Jeremy Linton wrote:
> On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote:

[...]

>>
>> So call this field "token" or similar. Don't call it "of_node" and
>> don't introduce another "firmware_node" thing in addition to that.
>> That just is a mess, sorry.

I completely agree. Both me and Lorenzo pointed that out in previous
revisions and fair enough you have a valid concern it's use with PPTT.

>
> I sort of agree, I think I can just change the whole of_node to a
> generic 'void *firmware_unique' which works fine for the PPTT code, it
> should also work for the DT code in cache_leaves_are_shared().
>

Should be fine.

> The slight gocha is there is a bit of DT code which initially runs
> earlier that uses of_node as an indirect parameter to a couple functions
> (by just passing the cacheinfo). Let me see if I can tweak that a bit.
>

May be use a simple inline wrapper functions to convert, might help if
we diverge too.

> Frankly, If I understood completely all the *priv cases I suspect it
> might be possible to collapse *of_node into that as well. That is as
> long as no one decides to flush out DT on x86, or PPTT on x86.
>

priv is used to save architecture/cache specific details that can't be
generalized. I doubt if this of_node or PPTT pointer/offset falls in
that category.

--
Regards,
Sudeep

2018-01-03 14:29:47

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

On 02/01/18 02:29, Xiongfeng Wang wrote:
> Hi,
>
> On 2017/12/18 20:42, Morten Rasmussen wrote:
>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>> Hi,
>>>
>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>> [+Morten, Dietmar]
>>>>
>>>> $SUBJECT should be:
>>>>
>>>> arm64: topology: rename cluster_id
>>>
> [cut]
>>>
> I think we still need the information describing which cores are in one
> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
> in one cluster may share a same L2 cache. That information can be used to
> build the sched_domain. If we put cores in one cluster in one sched_domain,
> the performance will be better.(please see kernel/sched/topology.c:1197,
> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
> sched_domain).

We get all the cache information from DT/ACPI PPTT(mainly topology) and now
even the geometry. So ideally, the sharing information must come from that.
Any other solution might end up in conflict if DT/PPTT and that mismatch.

> So I think we still need variable to record which cores are in one
> sched_domain for future use.

I tend to say no, at-least not as is.
--
Regards,
Sudeep

2018-01-03 16:57:55

[permalink] [raw]

Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

Hi,

On 01/03/2018 02:49 AM, [email protected] wrote:
> Hi Jeremy,
>
> Sorry, I don't have your previous patch emails to reply on right patch
> context.
> So commenting on top of this patch.
>
> AFAIU, the PPTT v5 patches still rely on CLIDR_EL1 register to know the type
> of
> Caches enabled/available on the platform. With PPTT, it should not rely on
> architecture
> registers. There can be platforms which can report cache availability in
> PPTT but not in
> architecture registers.
>
> The following code snippet shows usage of CLIDR_EL1
>
> In arch/arm64/kernel/cacheinfo.c
>
> static inline enum cache_type get_cache_type(int level)
> {
> u64 clidr;
>
> if (level > MAX_CACHE_LEVEL)
> return CACHE_TYPE_NOCACHE;
> clidr = read_sysreg(clidr_el1);
> return CLIDR_CTYPE(clidr, level);
> }
>
> static int __populate_cache_leaves(unsigned int cpu)
> {
> unsigned int level, idx;
> enum cache_type type;
> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> struct cacheinfo *this_leaf = this_cpu_ci->info_list;
>
> for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
> idx < this_cpu_ci->num_leaves; idx++, level++) {
> type = get_cache_type(level);
> if (type == CACHE_TYPE_SEPARATE) {
> ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
> ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
> } else {
> ci_leaf_init(this_leaf++, type, level);
> }
> }
> return 0;
> }
>
> In populate_cache_leaves() the cache type is read from CLIDR_EL1 register.
> If CLIDR_EL1 reports CACHE_TYPE_NOCACHE for a particular level then sysfs
> entry
> /sys/devices/system/cpu/cpu0/index<n>/type is not created and hence
> userspace tools
> like lstopo will not report this cache level.

This sounds suspiciously like one of things tweaked between v4->v5. If
you look at update_cache_properties() in patch 2/9, you will see that we
only update/find NOCACHE nodes and convert them to UNIFIED when all the
attributes in the node are supplied.

This means that if the node has an incomplete set of attributes we won't
update it. Can you verify that you have all those attributes set for
nodes which aren't being described by the hardware?

Thanks,

>
> Regards
> Vijay
>
>> -----Original Message-----
>> From: linux-arm-kernel
> [mailto:[email protected]]
>> On Behalf Of Rafael J. Wysocki
>> Sent: Thursday, December 14, 2017 4:40 AM
>> To: Jeremy Linton <[email protected]>
>> Cc: Mark Rutland <[email protected]>; [email protected];
>> [email protected]; Lorenzo Pieralisi
>> <[email protected]>; Catalin Marinas <[email protected]>;
>> Rafael J. Wysocki <[email protected]>; [email protected]; Will Deacon
>> <[email protected]>; Linux PM <[email protected]>; Rafael J.
>> Wysocki <[email protected]>; Greg Kroah-Hartman
>> <[email protected]>; Linux Kernel Mailing List <linux-
>> [email protected]>; ACPI Devel Maling List
> <[email protected]>;
>> Viresh Kumar <[email protected]>; Hanjun Guo
>> <[email protected]>; Al Stone <[email protected]>; Sudeep Holla
>> <[email protected]>; [email protected];
>> [email protected]; [email protected]; Len
>> Brown <[email protected]>
>> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
>>
>> On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton <[email protected]>
>> wrote:
>>> Hi,
>>>
>>>
>>> On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
>>>>
>>>> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
>>>> <[email protected]> wrote:
>>>>>
>>>>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> First, thanks for taking a look at this.
>>>>>>
>>>>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>>>>>>>
>>>>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>>>>>>>>
>>>>>>>> The PPTT can be used to determine the groupings of CPU's at given
>>>>>>>> levels in the system. Lets add a few routines to the PPTT parsing
>>>>>>>> code to return a unique id for each unique level in the processor
>>>>>>>> hierarchy. This can then be matched to build
>>>>>>>> thread/core/cluster/die/package/etc mappings for each processing
>>>>>>>> element in the system.
>>>>>>>>
>>>>>>>> Signed-off-by: Jeremy Linton <[email protected]>
>>>>>>>
>>>>>>>
>>>>>>> Why can't this be folded into patch [2/9]?
>>>>>>
>>>>>>
>>>>>> It can, and I will be happy squash it.
>>>>>>
>>>>>> It was requested that the topology portion of the parser be split
>>>>>> out back in v3.
>>>>>>
>>>>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>>>>>
>>>>>
>>>>> I asked to split cache/topology since I am not familiar with cache
>>>>> code and Sudeep - who looks after the cache code - won't be able to
>>>>> review this series in time for v4.16.
>>>>
>>>>
>>>> OK, so why do we need it in 4.16?
>>>
>>>
>>> I think its more case of as soon as possible. That is because there
>>> are machines where the topology is completely incorrect due to
>>> assumptions the kernel makes based on registers that aren't defined
>>> for that purpose (say describing which cores are in a physical socket,
>>> or LLC's attached to interconnects or memory controllers).
>>>
>>> This incorrect topology information is reported to things like the
>>> kernel scheduler, which then makes poor scheduling decisions resulting
>>> in sub-optimal system performance.
>>>
>>> This patchset (and ACPI 6.2) clears up a lot of those problems.
>>
>> As long as the ACPI tables are as expected that is, I suppose?
>>
>> Anyway, fair enough, but I don't want to rush it in.
>>
>> Thanks,
>> Rafael
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2018-01-03 17:32:05

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

Hi,

On 01/03/2018 08:29 AM, Sudeep Holla wrote:
>
> On 02/01/18 02:29, Xiongfeng Wang wrote:
>> Hi,
>>
>> On 2017/12/18 20:42, Morten Rasmussen wrote:
>>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>>> Hi,
>>>>
>>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>>> [+Morten, Dietmar]
>>>>>
>>>>> $SUBJECT should be:
>>>>>
>>>>> arm64: topology: rename cluster_id
>>>>
>> [cut]
>>>>
>> I think we still need the information describing which cores are in one
>> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
>> in one cluster may share a same L2 cache. That information can be used to
>> build the sched_domain. If we put cores in one cluster in one sched_domain,
>> the performance will be better.(please see kernel/sched/topology.c:1197,
>> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
>> sched_domain).
>
> We get all the cache information from DT/ACPI PPTT(mainly topology) and now
> even the geometry. So ideally, the sharing information must come from that.
> Any other solution might end up in conflict if DT/PPTT and that mismatch.
>
>> So I think we still need variable to record which cores are in one
>> sched_domain for future use.
>
> I tend to say no, at-least not as is.
>

Well, either way, with DynamiQ (and a55/a75) the cores have private
L2's, which means that the cluster sharing is happening at what is then
the L3 level. So, the code I had in earlier versions would have needed
tweaks to deal with that anyway.

IMHO, if we want to detect this kind of sharing for future scheduling
domains, it should probably be done independent of PPTT/DT/MIPDR by
picking out shared cache levels from struct cacheinfo *. Which makes
that change unrelated to the basic population of cacheinfo and
cpu_topology in this patchset.

2018-01-03 17:43:38

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

On Wed, Jan 03, 2018 at 11:32:00AM -0600, Jeremy Linton wrote:
> Hi,
>
> On 01/03/2018 08:29 AM, Sudeep Holla wrote:
> >
> >On 02/01/18 02:29, Xiongfeng Wang wrote:
> >>Hi,
> >>
> >>On 2017/12/18 20:42, Morten Rasmussen wrote:
> >>>On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> >>>>Hi,
> >>>>
> >>>>On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> >>>>>[+Morten, Dietmar]
> >>>>>
> >>>>>$SUBJECT should be:
> >>>>>
> >>>>>arm64: topology: rename cluster_id
> >>>>
> >>[cut]
> >>>>
> >>I think we still need the information describing which cores are in one
> >>cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
> >>in one cluster may share a same L2 cache. That information can be used to
> >>build the sched_domain. If we put cores in one cluster in one sched_domain,
> >>the performance will be better.(please see kernel/sched/topology.c:1197,
> >>cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
> >>sched_domain).
> >
> >We get all the cache information from DT/ACPI PPTT(mainly topology) and now
> >even the geometry. So ideally, the sharing information must come from that.
> >Any other solution might end up in conflict if DT/PPTT and that mismatch.
> >
> >>So I think we still need variable to record which cores are in one
> >>sched_domain for future use.
> >
> >I tend to say no, at-least not as is.
> >
>
> Well, either way, with DynamiQ (and a55/a75) the cores have private L2's,
> which means that the cluster sharing is happening at what is then the L3
> level. So, the code I had in earlier versions would have needed tweaks to
> deal with that anyway.
>

Indeed.

> IMHO, if we want to detect this kind of sharing for future scheduling
> domains, it should probably be done independent of PPTT/DT/MIPDR by picking
> out shared cache levels from struct cacheinfo *. Which makes that change
> unrelated to the basic population of cacheinfo and cpu_topology in this
> patchset.
>

Sure, that's what I meant above. i.e. we need to depend on firmware(DT/ACPI)
rather than architected way(which doesn't exist anyways). Since cacheinfo
abstracts DT/ACPI, it sounds right to use that to fetch any information
on cache topology.

--
Regards,
Sudeep

2018-01-04 03:59:53

by Xiongfeng Wang

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

On 2018/1/4 1:32, Jeremy Linton wrote:
> Hi,
>
> On 01/03/2018 08:29 AM, Sudeep Holla wrote:
>>
>> On 02/01/18 02:29, Xiongfeng Wang wrote:
>>> Hi,
>>>
>>> On 2017/12/18 20:42, Morten Rasmussen wrote:
>>>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>>>> Hi,
>>>>>
>>>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>>>> [+Morten, Dietmar]
>>>>>>
>>>>>> $SUBJECT should be:
>>>>>>
>>>>>> arm64: topology: rename cluster_id
>>>>>
>>> [cut]
>>>>>
>>> I think we still need the information describing which cores are in one
>>> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
>>> in one cluster may share a same L2 cache. That information can be used to
>>> build the sched_domain. If we put cores in one cluster in one sched_domain,
>>> the performance will be better.(please see kernel/sched/topology.c:1197,
>>> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
>>> sched_domain).
>>
>> We get all the cache information from DT/ACPI PPTT(mainly topology) and now
>> even the geometry. So ideally, the sharing information must come from that.
>> Any other solution might end up in conflict if DT/PPTT and that mismatch.
>>
>>> So I think we still need variable to record which cores are in one
>>> sched_domain for future use.
>>
>> I tend to say no, at-least not as is.
>>
>
> Well, either way, with DynamiQ (and a55/a75) the cores have private L2's, which means that the cluster sharing is happening at what is then the L3 level. So, the code I had in earlier versions would have needed tweaks to deal with that anyway.
>
> IMHO, if we want to detect this kind of sharing for future scheduling domains, it should probably be done independent of PPTT/DT/MIPDR by picking out shared cache levels from struct cacheinfo *. Which makes that change unrelated to the basic population of cacheinfo and cpu_topology in this patchset.
>
I think we need to build scheduling domains not only on the cache-sharing information,
but also some other information, such as which cores use the same cache coherent interconnect
(I don't know the detail, I just guess)

I think PPTT is used to report the cores topology, which cores are more related to each other.
They may share the same cache, or use the same CCI, or are physically near to each other.
I think we should use this information to build MC(multi-cores) scheduling domains.

Or maybe we can just discard the MC scheduling domain and handle this scheduling-domain-building
task to the NUMA subsystem entirely, I don't know if it is proper.

Thanks,
Xiongfeng

>
> .
>

2018-01-04 04:15:59

by Xiongfeng Wang

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

On 2018/1/4 1:32, Jeremy Linton wrote:
> Hi,
>
> On 01/03/2018 08:29 AM, Sudeep Holla wrote:
>>
>> On 02/01/18 02:29, Xiongfeng Wang wrote:
>>> Hi,
>>>
>>> On 2017/12/18 20:42, Morten Rasmussen wrote:
>>>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>>>> Hi,
>>>>>
>>>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>>>> [+Morten, Dietmar]
>>>>>>
>>>>>> $SUBJECT should be:
>>>>>>
>>>>>> arm64: topology: rename cluster_id
>>>>>
>>> [cut]
>>>>>
>>> I think we still need the information describing which cores are in one
>>> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
>>> in one cluster may share a same L2 cache. That information can be used to
>>> build the sched_domain. If we put cores in one cluster in one sched_domain,
>>> the performance will be better.(please see kernel/sched/topology.c:1197,
>>> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
>>> sched_domain).
>>
>> We get all the cache information from DT/ACPI PPTT(mainly topology) and now
>> even the geometry. So ideally, the sharing information must come from that.
>> Any other solution might end up in conflict if DT/PPTT and that mismatch.

Sorry, I didn't express myself clearly. There may be some misunderstanding above.
I mean that PPTT report the cores topology, such as a level of the topology tree maybe cores in one cluster,
another level maybe cores in one package.
We not only need variable in 'struct topology' to record which cores are in one package,
but also need variable to record which cores are in one cluster.
>>
>>> So I think we still need variable to record which cores are in one
>>> sched_domain for future use.
>>
>> I tend to say no, at-least not as is.
>>
>
> Well, either way, with DynamiQ (and a55/a75) the cores have private L2's, which means that the cluster sharing is happening at what is then the L3 level. So, the code I had in earlier versions would have needed tweaks to deal with that anyway.
>
> IMHO, if we want to detect this kind of sharing for future scheduling domains, it should probably be done independent of PPTT/DT/MIPDR by picking out shared cache levels from struct cacheinfo *. Which makes that change unrelated to the basic population of cacheinfo and cpu_topology in this patchset.
>
>
> .
>

2018-01-04 06:48:59

by vkilari

[permalink] [raw]

Subject: RE: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

Hi Jeremy

> -----Original Message-----
> From: linux-arm-kernel
[mailto:[email protected]]
> On Behalf Of Jeremy Linton
> Sent: Wednesday, January 3, 2018 10:28 PM
> To: [email protected]
> Cc: 'Mark Rutland' <[email protected]>; [email protected];
> [email protected]; 'Lorenzo Pieralisi'
> <[email protected]>; [email protected]; 'Linux PM' <linux-
> [email protected]>; [email protected]; 'Catalin Marinas'
> <[email protected]>; 'Sudeep Holla' <[email protected]>; 'Will
> Deacon' <[email protected]>; 'Linux Kernel Mailing List' <linux-
> [email protected]>; [email protected]; 'ACPI Devel Maling
> List' <[email protected]>; 'Viresh Kumar'
<[email protected]>;
> 'Rafael J. Wysocki' <[email protected]>; 'Hanjun Guo'
> <[email protected]>; 'Greg Kroah-Hartman'
> <[email protected]>; 'Rafael J. Wysocki' <[email protected]>; 'Al
> Stone' <[email protected]>; [email protected]; 'Len
Brown'
> <[email protected]>
> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
>
> Hi,
>
> On 01/03/2018 02:49 AM, [email protected] wrote:
> > Hi Jeremy,
> >
> > Sorry, I don't have your previous patch emails to reply on right
> > patch context.
> > So commenting on top of this patch.
> >
> > AFAIU, the PPTT v5 patches still rely on CLIDR_EL1 register to know
> > the type of Caches enabled/available on the platform. With PPTT, it
> > should not rely on architecture registers. There can be platforms
> > which can report cache availability in PPTT but not in architecture
> > registers.
> >
> > The following code snippet shows usage of CLIDR_EL1
> >
> > In arch/arm64/kernel/cacheinfo.c
> >
> > static inline enum cache_type get_cache_type(int level) {
> > u64 clidr;
> >
> > if (level > MAX_CACHE_LEVEL)
> > return CACHE_TYPE_NOCACHE;
> > clidr = read_sysreg(clidr_el1);
> > return CLIDR_CTYPE(clidr, level); }
> >
> > static int __populate_cache_leaves(unsigned int cpu) {
> > unsigned int level, idx;
> > enum cache_type type;
> > struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> > struct cacheinfo *this_leaf = this_cpu_ci->info_list;
> >
> > for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
> > idx < this_cpu_ci->num_leaves; idx++, level++) {
> > type = get_cache_type(level);
> > if (type == CACHE_TYPE_SEPARATE) {
> > ci_leaf_init(this_leaf++, CACHE_TYPE_DATA,
level);
> > ci_leaf_init(this_leaf++, CACHE_TYPE_INST,
level);
> > } else {
> > ci_leaf_init(this_leaf++, type, level);
> > }
> > }
> > return 0;
> > }
> >
> > In populate_cache_leaves() the cache type is read from CLIDR_EL1
register.
> > If CLIDR_EL1 reports CACHE_TYPE_NOCACHE for a particular level then
> > sysfs entry /sys/devices/system/cpu/cpu0/index<n>/type is not created
> > and hence userspace tools like lstopo will not report this cache
> > level.
>
>
> This sounds suspiciously like one of things tweaked between v4->v5. If you
look
> at update_cache_properties() in patch 2/9, you will see that we only
> update/find NOCACHE nodes and convert them to UNIFIED when all the
> attributes in the node are supplied.
>
> This means that if the node has an incomplete set of attributes we won't
update
> it. Can you verify that you have all those attributes set for nodes which
aren't
> being described by the hardware?

Thanks for pointing out.
Why do we need to check for set of attributes and decide it as UNIFIED
cache.?
We can get cache type from attributes bits[3:2] if cache type valid flag is
set
irrespective of other attributes. If cache type valid flag is not set then
we can assume
it as NOCACHE type as neither architecture register nor in PPTT has valid
cache type.

>
> Thanks,
>
>
> >
> > Regards
> > Vijay
> >
> >> -----Original Message-----
> >> From: linux-arm-kernel
> > [mailto:[email protected]]
> >> On Behalf Of Rafael J. Wysocki
> >> Sent: Thursday, December 14, 2017 4:40 AM
> >> To: Jeremy Linton <[email protected]>
> >> Cc: Mark Rutland <[email protected]>; [email protected];
> >> [email protected]; Lorenzo Pieralisi
> >> <[email protected]>; Catalin Marinas
> >> <[email protected]>; Rafael J. Wysocki <[email protected]>;
> >> [email protected]; Will Deacon <[email protected]>; Linux PM
> <[email protected]>; Rafael J.
> >> Wysocki <[email protected]>; Greg Kroah-Hartman
> >> <[email protected]>; Linux Kernel Mailing List <linux-
> >> [email protected]>; ACPI Devel Maling List
> > <[email protected]>;
> >> Viresh Kumar <[email protected]>; Hanjun Guo
> >> <[email protected]>; Al Stone <[email protected]>; Sudeep Holla
> >> <[email protected]>; [email protected];
> >> [email protected]; [email protected]; Len
> >> Brown <[email protected]>
> >> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
> >>
> >> On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton
> >> <[email protected]>
> >> wrote:
> >>> Hi,
> >>>
> >>>
> >>> On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
> >>>>
> >>>> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
> >>>> <[email protected]> wrote:
> >>>>>
> >>>>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> First, thanks for taking a look at this.
> >>>>>>
> >>>>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
> >>>>>>>
> >>>>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
> >>>>>>>>
> >>>>>>>> The PPTT can be used to determine the groupings of CPU's at
> >>>>>>>> given levels in the system. Lets add a few routines to the PPTT
> >>>>>>>> parsing code to return a unique id for each unique level in the
> >>>>>>>> processor hierarchy. This can then be matched to build
> >>>>>>>> thread/core/cluster/die/package/etc mappings for each
> >>>>>>>> processing element in the system.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jeremy Linton <[email protected]>
> >>>>>>>
> >>>>>>>
> >>>>>>> Why can't this be folded into patch [2/9]?
> >>>>>>
> >>>>>>
> >>>>>> It can, and I will be happy squash it.
> >>>>>>
> >>>>>> It was requested that the topology portion of the parser be split
> >>>>>> out back in v3.
> >>>>>>
> >>>>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
> >>>>>
> >>>>>
> >>>>> I asked to split cache/topology since I am not familiar with cache
> >>>>> code and Sudeep - who looks after the cache code - won't be able
> >>>>> to review this series in time for v4.16.
> >>>>
> >>>>
> >>>> OK, so why do we need it in 4.16?
> >>>
> >>>
> >>> I think its more case of as soon as possible. That is because there
> >>> are machines where the topology is completely incorrect due to
> >>> assumptions the kernel makes based on registers that aren't defined
> >>> for that purpose (say describing which cores are in a physical
> >>> socket, or LLC's attached to interconnects or memory controllers).
> >>>
> >>> This incorrect topology information is reported to things like the
> >>> kernel scheduler, which then makes poor scheduling decisions
> >>> resulting in sub-optimal system performance.
> >>>
> >>> This patchset (and ACPI 6.2) clears up a lot of those problems.
> >>
> >> As long as the ACPI tables are as expected that is, I suppose?
> >>
> >> Anyway, fair enough, but I don't want to rush it in.
> >>
> >> Thanks,
> >> Rafael
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> [email protected]
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2018-01-04 11:46:58

[permalink] [raw]

Subject: Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

On 03/01/18 14:21, Sudeep Holla wrote:
> On 12/12/17 23:37, Jeremy Linton wrote:
>> On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote:
>
> [...]
>
>>>
>>> So call this field "token" or similar. Don't call it "of_node" and
>>> don't introduce another "firmware_node" thing in addition to that.
>>> That just is a mess, sorry.
>
> I completely agree. Both me and Lorenzo pointed that out in previous
> revisions and fair enough you have a valid concern it's use with PPTT.
>
>>
>> I sort of agree, I think I can just change the whole of_node to a
>> generic 'void *firmware_unique' which works fine for the PPTT code, it
>> should also work for the DT code in cache_leaves_are_shared().
>>
>
> Should be fine.
>

Just to let you know, I don't have much to add. I will wait for the
patches that replace of_node with firmware cookie or something similar.
--
Regards,
Sudeep

2018-01-04 17:50:17

[permalink] [raw]

Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

Hi,

On 01/04/2018 12:48 AM, [email protected] wrote:
> Hi Jeremy
>
>> -----Original Message-----
>> From: linux-arm-kernel
> [mailto:[email protected]]
>> On Behalf Of Jeremy Linton
>> Sent: Wednesday, January 3, 2018 10:28 PM
>> To: [email protected]
>> Cc: 'Mark Rutland' <[email protected]>; [email protected];
>> [email protected]; 'Lorenzo Pieralisi'
>> <[email protected]>; [email protected]; 'Linux PM' <linux-
>> [email protected]>; [email protected]; 'Catalin Marinas'
>> <[email protected]>; 'Sudeep Holla' <[email protected]>; 'Will
>> Deacon' <[email protected]>; 'Linux Kernel Mailing List' <linux-
>> [email protected]>; [email protected]; 'ACPI Devel Maling
>> List' <[email protected]>; 'Viresh Kumar'
> <[email protected]>;
>> 'Rafael J. Wysocki' <[email protected]>; 'Hanjun Guo'
>> <[email protected]>; 'Greg Kroah-Hartman'
>> <[email protected]>; 'Rafael J. Wysocki' <[email protected]>; 'Al
>> Stone' <[email protected]>; [email protected]; 'Len
> Brown'
>> <[email protected]>
>> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
>>
>> Hi,
>>
>> On 01/03/2018 02:49 AM, [email protected] wrote:
>>> Hi Jeremy,
>>>
>>> Sorry, I don't have your previous patch emails to reply on right
>>> patch context.
>>> So commenting on top of this patch.
>>>
>>> AFAIU, the PPTT v5 patches still rely on CLIDR_EL1 register to know
>>> the type of Caches enabled/available on the platform. With PPTT, it
>>> should not rely on architecture registers. There can be platforms
>>> which can report cache availability in PPTT but not in architecture
>>> registers.
>>>
>>> The following code snippet shows usage of CLIDR_EL1
>>>
>>> In arch/arm64/kernel/cacheinfo.c
>>>
>>> static inline enum cache_type get_cache_type(int level) {
>>> u64 clidr;
>>>
>>> if (level > MAX_CACHE_LEVEL)
>>> return CACHE_TYPE_NOCACHE;
>>> clidr = read_sysreg(clidr_el1);
>>> return CLIDR_CTYPE(clidr, level); }
>>>
>>> static int __populate_cache_leaves(unsigned int cpu) {
>>> unsigned int level, idx;
>>> enum cache_type type;
>>> struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>> struct cacheinfo *this_leaf = this_cpu_ci->info_list;
>>>
>>> for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
>>> idx < this_cpu_ci->num_leaves; idx++, level++) {
>>> type = get_cache_type(level);
>>> if (type == CACHE_TYPE_SEPARATE) {
>>> ci_leaf_init(this_leaf++, CACHE_TYPE_DATA,
> level);
>>> ci_leaf_init(this_leaf++, CACHE_TYPE_INST,
> level);
>>> } else {
>>> ci_leaf_init(this_leaf++, type, level);
>>> }
>>> }
>>> return 0;
>>> }
>>>
>>> In populate_cache_leaves() the cache type is read from CLIDR_EL1
> register.
>>> If CLIDR_EL1 reports CACHE_TYPE_NOCACHE for a particular level then
>>> sysfs entry /sys/devices/system/cpu/cpu0/index<n>/type is not created
>>> and hence userspace tools like lstopo will not report this cache
>>> level.
>>
>>
>> This sounds suspiciously like one of things tweaked between v4->v5. If you
> look
>> at update_cache_properties() in patch 2/9, you will see that we only
>> update/find NOCACHE nodes and convert them to UNIFIED when all the
>> attributes in the node are supplied.
>>
>> This means that if the node has an incomplete set of attributes we won't
> update
>> it. Can you verify that you have all those attributes set for nodes which
> aren't
>> being described by the hardware?
>
> Thanks for pointing out.
> Why do we need to check for set of attributes and decide it as UNIFIED
> cache.?
> We can get cache type from attributes bits[3:2] if cache type valid flag is
> set
> irrespective of other attributes. If cache type valid flag is not set then
> we can assume
> it as NOCACHE type as neither architecture register nor in PPTT has valid
> cache type.

To answer the first question, in a strict sense we don't need to check
any of the attributes in order to override the cache type. That said,
initially I was going to trigger the override only when important
attributes were set to assure that we weren't exporting meaningless
nodes into sysfs. Then while picking which attributes I considered
important, I came to the conclusion that it was simply better to assure
that they were all set for nodes entirely generated by the PPTT. AKA, I
don't want to see L3 cache nodes with their size or associativity unset,
its better in that case that they remain hidden.

Per, the cache type valid bit. The code is written with the assumption
that it is overriding probed values (despite that not being true at the
moment for arm64) in the spirit of the standard. This informs/restricts
how the code works because we aren't simply generating the entire
cacheinfo directly from PPTT walks. Instead we are merging the PPTT
information with anything previously probed, meaning we need a way to
match existing cacheinfo structures with PPTT nodes.

So, the logic finding/matching an existing probed cache node requires
that the cache type is valid because the cache level, and type is used
as the match key. If the PPTT cache node doesn't have the cache type
valid set, then the match logic won't find the node, and the PPTT code
won't make any updates. That may also be what your seeing.. Basically
what is happening is that cacheinfo NOCACHE nodes that happen to match
valid PPTT UNIFIED nodes, can have their cache types overridden, but
only if we determine the remainder of the PPTT node has sufficient
information that we aren't exporting cacheinfo structures without useful
information. Currently, the only time this can happen is for nodes which
are entirely PPTT generated, so I think its fair the PPTT contain enough
information to make those nodes useful.

Thanks,

>
>>
>> Thanks,
>>
>>
>>>
>>> Regards
>>> Vijay
>>>
>>>> -----Original Message-----
>>>> From: linux-arm-kernel
>>> [mailto:[email protected]]
>>>> On Behalf Of Rafael J. Wysocki
>>>> Sent: Thursday, December 14, 2017 4:40 AM
>>>> To: Jeremy Linton <[email protected]>
>>>> Cc: Mark Rutland <[email protected]>; [email protected];
>>>> [email protected]; Lorenzo Pieralisi
>>>> <[email protected]>; Catalin Marinas
>>>> <[email protected]>; Rafael J. Wysocki <[email protected]>;
>>>> [email protected]; Will Deacon <[email protected]>; Linux PM
>> <[email protected]>; Rafael J.
>>>> Wysocki <[email protected]>; Greg Kroah-Hartman
>>>> <[email protected]>; Linux Kernel Mailing List <linux-
>>>> [email protected]>; ACPI Devel Maling List
>>> <[email protected]>;
>>>> Viresh Kumar <[email protected]>; Hanjun Guo
>>>> <[email protected]>; Al Stone <[email protected]>; Sudeep Holla
>>>> <[email protected]>; [email protected];
>>>> [email protected]; [email protected]; Len
>>>> Brown <[email protected]>
>>>> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
>>>>
>>>> On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton
>>>> <[email protected]>
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
>>>>>>
>>>>>> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> First, thanks for taking a look at this.
>>>>>>>>
>>>>>>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>>>>>>>>>
>>>>>>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>>>>>>>>>>
>>>>>>>>>> The PPTT can be used to determine the groupings of CPU's at
>>>>>>>>>> given levels in the system. Lets add a few routines to the PPTT
>>>>>>>>>> parsing code to return a unique id for each unique level in the
>>>>>>>>>> processor hierarchy. This can then be matched to build
>>>>>>>>>> thread/core/cluster/die/package/etc mappings for each
>>>>>>>>>> processing element in the system.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jeremy Linton <[email protected]>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Why can't this be folded into patch [2/9]?
>>>>>>>>
>>>>>>>>
>>>>>>>> It can, and I will be happy squash it.
>>>>>>>>
>>>>>>>> It was requested that the topology portion of the parser be split
>>>>>>>> out back in v3.
>>>>>>>>
>>>>>>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>>>>>>>
>>>>>>>
>>>>>>> I asked to split cache/topology since I am not familiar with cache
>>>>>>> code and Sudeep - who looks after the cache code - won't be able
>>>>>>> to review this series in time for v4.16.
>>>>>>
>>>>>>
>>>>>> OK, so why do we need it in 4.16?
>>>>>
>>>>>
>>>>> I think its more case of as soon as possible. That is because there
>>>>> are machines where the topology is completely incorrect due to
>>>>> assumptions the kernel makes based on registers that aren't defined
>>>>> for that purpose (say describing which cores are in a physical
>>>>> socket, or LLC's attached to interconnects or memory controllers).
>>>>>
>>>>> This incorrect topology information is reported to things like the
>>>>> kernel scheduler, which then makes poor scheduling decisions
>>>>> resulting in sub-optimal system performance.
>>>>>
>>>>> This patchset (and ACPI 6.2) clears up a lot of those problems.
>>>>
>>>> As long as the ACPI tables are as expected that is, I suppose?
>>>>
>>>> Anyway, fair enough, but I don't want to rush it in.
>>>>
>>>> Thanks,
>>>> Rafael
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> [email protected]
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

2018-01-04 18:00:39

[permalink] [raw]

Subject: Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

Hi,

On 01/03/2018 09:59 PM, Xiongfeng Wang wrote:
>
>
> On 2018/1/4 1:32, Jeremy Linton wrote:
>> Hi,
>>
>> On 01/03/2018 08:29 AM, Sudeep Holla wrote:
>>>
>>> On 02/01/18 02:29, Xiongfeng Wang wrote:
>>>> Hi,
>>>>
>>>> On 2017/12/18 20:42, Morten Rasmussen wrote:
>>>>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>>>>> [+Morten, Dietmar]
>>>>>>>
>>>>>>> $SUBJECT should be:
>>>>>>>
>>>>>>> arm64: topology: rename cluster_id
>>>>>>
>>>> [cut]
>>>>>>
>>>> I think we still need the information describing which cores are in one
>>>> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
>>>> in one cluster may share a same L2 cache. That information can be used to
>>>> build the sched_domain. If we put cores in one cluster in one sched_domain,
>>>> the performance will be better.(please see kernel/sched/topology.c:1197,
>>>> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
>>>> sched_domain).
>>>
>>> We get all the cache information from DT/ACPI PPTT(mainly topology) and now
>>> even the geometry. So ideally, the sharing information must come from that.
>>> Any other solution might end up in conflict if DT/PPTT and that mismatch.
>>>
>>>> So I think we still need variable to record which cores are in one
>>>> sched_domain for future use.
>>>
>>> I tend to say no, at-least not as is.
>>>
>>
>> Well, either way, with DynamiQ (and a55/a75) the cores have private L2's, which means that the cluster sharing is happening at what is then the L3 level. So, the code I had in earlier versions would have needed tweaks to deal with that anyway.
>>
>> IMHO, if we want to detect this kind of sharing for future scheduling domains, it should probably be done independent of PPTT/DT/MIPDR by picking out shared cache levels from struct cacheinfo *. Which makes that change unrelated to the basic population of cacheinfo and cpu_topology in this patchset.
>>
> I think we need to build scheduling domains not only on the cache-sharing information,
> but also some other information, such as which cores use the same cache coherent interconnect
> (I don't know the detail, I just guess)
>
> I think PPTT is used to report the cores topology, which cores are more related to each other.
> They may share the same cache, or use the same CCI, or are physically near to each other.
> I think we should use this information to build MC(multi-cores) scheduling domains.
>
> Or maybe we can just discard the MC scheduling domain and handle this scheduling-domain-building
> task to the NUMA subsystem entirely, I don't know if it is proper.

For the immediate future what I would like is a way to identify where in
the PPTT topology the NUMA domains begin (rather than assuming socket,
which is the current plan). That allows the manufactures of systems
(with say say MCM based topologies) to dictate at which level in the
cpu/cache topology they want to start describing the topology with the
SLIT/SRAT tables. I think that moves us in the direction you are
indicating while still leaving the door open for something like a
cluster level scheduling domain (based on cores sharing caches) or a
split LLC domain (also based on cores sharing caches) that happens to be
on die...

2018-01-05 21:58:30

[permalink] [raw]

Subject: Re: [PATCH v5 3/9] ACPI: Enable PPTT support on ARM64

Hi,

On 12/13/2017 11:26 AM, Lorenzo Pieralisi wrote:
> On Fri, Dec 01, 2017 at 04:23:24PM -0600, Jeremy Linton wrote:
>> Now that we have a PPTT parser, in preparation for its use
>> on arm64, lets build it.
>>
>> Signed-off-by: Jeremy Linton <[email protected]>
>> ---
>> arch/arm64/Kconfig | 1 +
>> drivers/acpi/Kconfig | 3 +++
>> drivers/acpi/Makefile | 1 +
>> 3 files changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index a93339f5178f..e62fd1e08c1f 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -7,6 +7,7 @@ config ARM64
>> select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>> select ACPI_MCFG if ACPI
>> select ACPI_SPCR_TABLE if ACPI
>> + select ACPI_PPTT if ACPI
>> select ARCH_CLOCKSOURCE_DATA
>> select ARCH_HAS_DEBUG_VIRTUAL
>> select ARCH_HAS_DEVMEM_IS_ALLOWED
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 46505396869e..df7aebf0af0e 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -545,6 +545,9 @@ config ACPI_CONFIGFS
>>
>> if ARM64
>> source "drivers/acpi/arm64/Kconfig"
>> +
>> +config ACPI_PPTT
>> + bool
>
> We need to make a choice here. Either PPTT is considered ARM64 only and
> we move code and config to drivers/acpi/arm64/Kconfig or we leave it in
> drivers/acpi/pptt.c and we add a Kconfig entry in drivers/acpi/Kconfig
> (and we make pptt.c compile on !ARM64 - which is what it should be given
> that there is nothing ARM64 specific in it).

No one else has expressed and opinion about this here..

So i'm not sure what to think.

OTOH a lot of people didn't like it when I had it in the arm64
directory, which was my original opinion. It seems other people thought
that at some point in the future other ACPI platforms would want to use
it so putting it in the arm64 directory was a mistake.

So, I'm leaning towards leaving it like it is, under the assumption that
when someone puts in the effort to verify it on another ACPI platform
they can move the config option up 6 lines. In the meantime I don't
think it should be enabled on platforms where it hasn't been tested or
is basically blocked (cpu_cacheinfo->cpu_map_populated) from executing
or there isn't currently any benefit.

Hence I'm planning on leaving it as is until I get further clarity.

>
> Lorenzo
>
>> endif
>>
>> config TPS68470_PMIC_OPREGION
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 41954a601989..b6056b566df4 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -87,6 +87,7 @@ obj-$(CONFIG_ACPI_BGRT) += bgrt.o
>> obj-$(CONFIG_ACPI_CPPC_LIB) += cppc_acpi.o
>> obj-$(CONFIG_ACPI_SPCR_TABLE) += spcr.o
>> obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
>> +obj-$(CONFIG_ACPI_PPTT) += pptt.o
>>
>> # processor has its own "processor." module_param namespace
>> processor-y := processor_driver.o
>> --
>> 2.13.5
>>

2018-01-05 22:07:35