From: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
To: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Michael Neuling <mikey@neuling.org>,
Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>,
Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>,
Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>,
"Oliver O'Halloran" <oohall@gmail.com>,
Nicholas Piggin <npiggin@gmail.com>,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [v2 PATCH 1/2] powerpc: Detect the presence of big-cores via "ibm, thread-groups"
Date: Tue, 3 Jul 2018 14:16:55 -0300 [thread overview]
Message-ID: <20180703171655.GA6474@kermit-br-ibm-com.br.ibm.com> (raw)
In-Reply-To: <b922aa0ea79a83ad733508c861900a08c4fee25a.1530609795.git.ego@linux.vnet.ibm.com>
On Tue, Jul 03, 2018 at 04:33:50PM +0530, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>
> On IBM POWER9, the device tree exposes a property array identifed by
> "ibm,thread-groups" which will indicate which groups of threads share a
> particular set of resources.
>
> As of today we only have one form of grouping identifying the group of
> threads in the core that share the L1 cache, translation cache and
> instruction data flow.
>
> This patch defines the helper function to parse the contents of
> "ibm,thread-groups" and a new structure to contain the parsed output.
>
> The patch also creates the sysfs file named "small_core_siblings" that
> returns the physical ids of the threads in the core that share the L1
> cache, translation cache and instruction data flow.
>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> Documentation/ABI/testing/sysfs-devices-system-cpu | 8 ++
> arch/powerpc/include/asm/cputhreads.h | 22 +++++
> arch/powerpc/kernel/setup-common.c | 110 +++++++++++++++++++++
> arch/powerpc/kernel/sysfs.c | 35 +++++++
> 4 files changed, 175 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index 9c5e7732..53a823a 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -487,3 +487,11 @@ Description: Information about CPU vulnerabilities
> "Not affected" CPU is not affected by the vulnerability
> "Vulnerable" CPU is affected and no mitigation in effect
> "Mitigation: $M" CPU is affected and mitigation $M is in effect
> +
> +What: /sys/devices/system/cpu/cpu[0-9]+/small_core_sibings
s/small_core_sibings/small_core_siblings
By the way, big_core_siblings was mentioned in the introductory email.
> +Date: 03-Jul-2018
> +KernelVersion: v4.18.0
> +Contact: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> +Description: List of Physical ids of CPUs which share the the L1 cache,
> + translation cache and instruction data-flow with this CPU.
> +Values: Comma separated list of decimal integers.
> diff --git a/arch/powerpc/include/asm/cputhreads.h b/arch/powerpc/include/asm/cputhreads.h
> index d71a909..33226d7 100644
> --- a/arch/powerpc/include/asm/cputhreads.h
> +++ b/arch/powerpc/include/asm/cputhreads.h
> @@ -23,11 +23,13 @@
> extern int threads_per_core;
> extern int threads_per_subcore;
> extern int threads_shift;
> +extern bool has_big_cores;
> extern cpumask_t threads_core_mask;
> #else
> #define threads_per_core 1
> #define threads_per_subcore 1
> #define threads_shift 0
> +#define has_big_cores 0
> #define threads_core_mask (*get_cpu_mask(0))
> #endif
>
> @@ -69,12 +71,32 @@ static inline cpumask_t cpu_online_cores_map(void)
> return cpu_thread_mask_to_cores(cpu_online_mask);
> }
>
> +#define MAX_THREAD_LIST_SIZE 8
> +struct thread_groups {
> + unsigned int property;
> + unsigned int nr_groups;
> + unsigned int threads_per_group;
> + unsigned int thread_list[MAX_THREAD_LIST_SIZE];
> +};
> +
> #ifdef CONFIG_SMP
> int cpu_core_index_of_thread(int cpu);
> int cpu_first_thread_of_core(int core);
> +int parse_thread_groups(struct device_node *dn, struct thread_groups *tg);
> +int get_cpu_thread_group_start(int cpu, struct thread_groups *tg);
> #else
> static inline int cpu_core_index_of_thread(int cpu) { return cpu; }
> static inline int cpu_first_thread_of_core(int core) { return core; }
> +static inline int parse_thread_groups(struct device_node *dn,
> + struct thread_groups *tg)
> +{
> + return -ENODATA;
> +}
> +
> +static inline int get_cpu_thread_group_start(int cpu, struct thread_groups *tg)
> +{
> + return -1;
> +}
> #endif
>
> static inline int cpu_thread_in_core(int cpu)
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 40b44bb..a78ec66 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -402,10 +402,12 @@ void __init check_for_initrd(void)
> #ifdef CONFIG_SMP
>
> int threads_per_core, threads_per_subcore, threads_shift;
> +bool has_big_cores = true;
> cpumask_t threads_core_mask;
> EXPORT_SYMBOL_GPL(threads_per_core);
> EXPORT_SYMBOL_GPL(threads_per_subcore);
> EXPORT_SYMBOL_GPL(threads_shift);
> +EXPORT_SYMBOL_GPL(has_big_cores);
> EXPORT_SYMBOL_GPL(threads_core_mask);
>
> static void __init cpu_init_thread_core_maps(int tpc)
> @@ -433,6 +435,108 @@ static void __init cpu_init_thread_core_maps(int tpc)
>
> u32 *cpu_to_phys_id = NULL;
>
> +/*
> + * parse_thread_groups: Parses the "ibm,thread-groups" device tree
> + * property for the CPU device node dn and stores
> + * the parsed output in the thread_groups
> + * structure tg.
Perhaps document the arguments of this function, as done in the second
patch?
> + *
> + * ibm,thread-groups[0..N-1] array defines which group of threads in
> + * the CPU-device node can be grouped together based on the property.
> + *
> + * ibm,thread-groups[0] tells us the property based on which the
> + * threads are being grouped together. If this value is 1, it implies
> + * that the threads in the same group share L1, translation cache.
> + *
> + * ibm,thread-groups[1] tells us how many such thread groups exist.
> + *
> + * ibm,thread-groups[2] tells us the number of threads in each such
> + * group.
> + *
> + * ibm,thread-groups[3..N-1] is the list of threads identified by
> + * "ibm,ppc-interrupt-server#s" arranged as per their membership in
> + * the grouping.
> + *
> + * Example: If ibm,thread-groups = [1,2,4,5,6,7,8,9,10,11,12] it
> + * implies that there are 2 groups of 4 threads each, where each group
> + * of threads share L1, translation cache.
> + *
> + * The "ibm,ppc-interrupt-server#s" of the first group is {5,6,7,8}
> + * and the "ibm,ppc-interrupt-server#s" of the second group is {9, 10,
> + * 11, 12} structure
> + *
> + * Returns 0 on success, -EINVAL if the property does not exist,
> + * -ENODATA if property does not have a value, and -EOVERFLOW if the
> + * property data isn't large enough.
> + */
> +int parse_thread_groups(struct device_node *dn,
> + struct thread_groups *tg)
> +{
> + unsigned int nr_groups, threads_per_group, property;
> + int i;
> + u32 thread_group_array[3 + MAX_THREAD_LIST_SIZE];
> + u32 *thread_list;
> + size_t total_threads;
> + int ret;
> +
> + ret = of_property_read_u32_array(dn, "ibm,thread-groups",
> + thread_group_array, 3);
> +
> + if (ret)
> + return ret;
> +
> + property = thread_group_array[0];
> + nr_groups = thread_group_array[1];
> + threads_per_group = thread_group_array[2];
> + total_threads = nr_groups * threads_per_group;
> +
> + ret = of_property_read_u32_array(dn, "ibm,thread-groups",
> + thread_group_array,
> + 3 + total_threads);
> + if (ret)
> + return ret;
> +
> + thread_list = &thread_group_array[3];
> +
> + for (i = 0 ; i < total_threads; i++)
> + tg->thread_list[i] = thread_list[i];
> +
> + tg->property = property;
> + tg->nr_groups = nr_groups;
> + tg->threads_per_group = threads_per_group;
> +
> + return 0;
> +}
> +
> +/*
> + * get_cpu_thread_group_start : Searches the thread group in tg->thread_list
> + * that @cpu belongs to.
Same here.
> + *
> + * Returns the index to tg->thread_list that points to the the start
> + * of the thread_group that @cpu belongs to.
> + *
> + * Returns -1 if cpu doesn't belong to any of the groups pointed
> + * to by tg->thread_list.
> + */
> +int get_cpu_thread_group_start(int cpu, struct thread_groups *tg)
> +{
> + int hw_cpu_id = get_hard_smp_processor_id(cpu);
> + int i, j;
> +
> + for (i = 0; i < tg->nr_groups; i++) {
> + int group_start = i * tg->threads_per_group;
> +
> + for (j = 0; j < tg->threads_per_group; j++) {
> + int idx = group_start + j;
> +
> + if (tg->thread_list[idx] == hw_cpu_id)
> + return group_start;
> + }
> + }
> +
> + return -1;
> +}
> +
> /**
> * setup_cpu_maps - initialize the following cpu maps:
> * cpu_possible_mask
> @@ -467,6 +571,7 @@ void __init smp_setup_cpu_maps(void)
> const __be32 *intserv;
> __be32 cpu_be;
> int j, len;
> + struct thread_groups tg = {.nr_groups = 0};
We assume has_big_cores = true but here we initialize .nr_groups
otherwise. It's kind of contradictory.
What if has_big_cores is assumed false and members of tg are initialized
with zeroes?
>
> DBG(" * %pOF...\n", dn);
>
> @@ -505,6 +610,11 @@ void __init smp_setup_cpu_maps(void)
> cpu++;
> }
>
> + if (parse_thread_groups(dn, &tg) ||
> + tg.nr_groups < 1 || tg.property != 1) {
> + has_big_cores = false;
> + }
> +
parse_thread_groups() returns before setting tg.property if property
doesn't exist. Are we confident that tg.property won't contain any
garbage that could lead to a false positive here? Shouldn't we also
initialize .property when declaring tg?
What if this logic is encapsulated in a function? For example:
has_big_cores = dt_has_big_cores(dn, &tg);
> if (cpu >= nr_cpu_ids) {
> of_node_put(dn);
> break;
> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index 755dc98..f5717de 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -18,6 +18,7 @@
> #include <asm/smp.h>
> #include <asm/pmc.h>
> #include <asm/firmware.h>
> +#include <asm/cputhreads.h>
>
> #include "cacheinfo.h"
> #include "setup.h"
> @@ -1025,6 +1026,33 @@ static ssize_t show_physical_id(struct device *dev,
> }
> static DEVICE_ATTR(physical_id, 0444, show_physical_id, NULL);
>
> +static ssize_t show_small_core_siblings(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct cpu *cpu = container_of(dev, struct cpu, dev);
> + struct device_node *dn = of_get_cpu_node(cpu->dev.id, NULL);
> + struct thread_groups tg;
> + int i, j;
> + ssize_t ret = 0;
> +
> + if (parse_thread_groups(dn, &tg))
> + return -ENODATA;
> +
> + i = get_cpu_thread_group_start(cpu->dev.id, &tg);
> +
> + if (i == -1)
> + return -ENODATA;
> +
> + for (j = 0; j < tg.threads_per_group - 1; j++)
> + ret += sprintf(buf + ret, "%d,", tg.thread_list[i + j]);
> +
> + ret += sprintf(buf + ret, "%d\n", tg.thread_list[i + j]);
> +
> + return ret;
> +}
> +static DEVICE_ATTR(small_core_siblings, 0444, show_small_core_siblings, NULL);
> +
> static int __init topology_init(void)
> {
> int cpu, r;
> @@ -1048,6 +1076,13 @@ static int __init topology_init(void)
> register_cpu(c, cpu);
>
> device_create_file(&c->dev, &dev_attr_physical_id);
> +
> + if (has_big_cores) {
> + const struct device_attribute *attr =
> + &dev_attr_small_core_siblings;
> +
> + device_create_file(&c->dev, attr);
> + }
> }
> }
> r = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "powerpc/topology:online",
> --
> 1.9.4
>
Cheers
Murilo
next prev parent reply other threads:[~2018-07-03 17:17 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-03 11:03 [v2 PATCH 0/2] powerpc: Detection and scheduler optimization for POWER9 bigcore Gautham R. Shenoy
2018-07-03 11:03 ` [v2 PATCH 1/2] powerpc: Detect the presence of big-cores via "ibm,thread-groups" Gautham R. Shenoy
2018-07-03 17:16 ` Murilo Opsfelder Araujo [this message]
2018-07-04 8:09 ` [v2 PATCH 1/2] powerpc: Detect the presence of big-cores via "ibm, thread-groups" Gautham R Shenoy
2018-07-03 11:03 ` [v2 PATCH 2/2] powerpc: Enable CPU_FTR_ASYM_SMT for interleaved big-cores Gautham R. Shenoy
2018-07-03 17:53 ` Murilo Opsfelder Araujo
2018-07-04 8:15 ` Gautham R Shenoy
2018-07-04 17:36 ` Murilo Opsfelder Araujo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180703171655.GA6474@kermit-br-ibm-com.br.ibm.com \
--to=muriloo@linux.ibm.com \
--cc=akshay.adiga@linux.vnet.ibm.com \
--cc=benh@kernel.crashing.org \
--cc=ego@linux.vnet.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mikey@neuling.org \
--cc=mpe@ellerman.id.au \
--cc=npiggin@gmail.com \
--cc=oohall@gmail.com \
--cc=shilpa.bhat@linux.vnet.ibm.com \
--cc=svaidy@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).