Re: [PATCH v6 1/2] powerpc: Detect the presence of big-cores via "ibm,thread-groups"
On Mon, 2018-08-13 at 17:06 +0530, Gautham R Shenoy wrote:
> Hi Srikar,
>
> Thanks for reviewing the patch.
>
> On Thu, Aug 09, 2018 at 06:27:43AM -0700, Srikar Dronamraju wrote:
> > * Gautham R. Shenoy [2018-08-09 11:02:07]:
> >
> > >
> > > int threads_per_core, threads_per_subcore, threads_shift;
> > > +bool has_big_cores;
> > > 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);
> >
> > Why do we need EXPORT_SYMBOL_GPL?
>
> As Christoph pointed out, I was blindly following the suit.
>
> You are right, we don't need to export it at the moment. The remaining
> EXPORT_SYMBOL_GPL are required by KVM. However, as of now, there is no
> need for "has_big_cores" in the KVM.
There is actually. KVM needs to refuse to start on big cores, at least
HV KVM. And when KVM grows support for big cores (may or may not
happen), it will need to know. So keep the GPL export.
> Will remove this in the next version.
> >
> > > EXPORT_SYMBOL_GPL(threads_core_mask);
> > >
> > > + *
> > > + * 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)
> > > + goto out_err;
> > > +
> > > + 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;
>
>
> > > +
> >
> > Shouldnt we check for property and nr_groups
> > If the property is not 1 and nr_groups < 1, we should error out
> > No point in calling a of_property read if property is not right.
>
> Yes, the nr_groups < 1 check can be moved into this function.
>
> However, this function merely parses the the thread group structure
> exposed by the device tree. So it should error out only if there is a
> failure in parsing, or as you said the parsed values are incorrect
> (nr_groups < 1, threads_per_group < 1, etc). Whether the thread group
> is relevant or not (in this case we are interested in thread groups
> that share L1 cache, translation etc) is something for the caller to
> decide.
>
> However, I see what you mean. We can avoid parsing the remainder of
> the array if the property in the device-tree isn't the property that
> the caller is interested in.
>
> This can be solved by passing the interested property value as a
> parameter and so that the code errors out if this property doesn't
> match the property in the device-tree. Will add this in the next
> version.
>
> >
> >
> > Nit:
> > Cant we directly assign to tg->property, and hence avoid local
> > variables, property, nr_groups and threads_per_group?
>
> Will clean this up. This was from an older version where I added the
> local variables so that the statements referencing them don't need to
> be split across multiple lines. However, the code has been optimized
> since then. So, the local variables are not needed.
>
> >
> > > + ret = of_property_read_u32_array(dn, "ibm,thread-groups",
> > > + thread_group_array,
> > > + 3 + total_threads);
> > > +
> > > +static inline bool dt_has_big_core(struct device_node *dn,
> > > +struct thread_groups *tg)
> > > +{
> > > + if (parse_thread_groups(dn, tg))
> > > + return false;
> > > +
> > > + if (tg->property != 1)
> > > + return false;
> > > +
> > > + if (tg->nr_groups < 1)
> > > + return false;
> >
> > Can avoid these check if we can check in parse_thread_groups.
> >
> > > /**
> > > * setup_cpu_maps - initialize the following cpu maps:
> > > * cpu_possible_mask
> > > @@ -457,6 +605,7 @@ void __init smp_setup_cpu_maps(void)
> > > int cpu = 0;
> > > int nthreads = 1;
> > >
> > > 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
> > > #include
> > > #include
> > > +#include
> > >
> > > #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
Re: [PATCH v6 1/2] powerpc: Detect the presence of big-cores via "ibm,thread-groups"
Hi Srikar,
Thanks for reviewing the patch.
On Thu, Aug 09, 2018 at 06:27:43AM -0700, Srikar Dronamraju wrote:
> * Gautham R. Shenoy [2018-08-09 11:02:07]:
>
> >
> > int threads_per_core, threads_per_subcore, threads_shift;
> > +bool has_big_cores;
> > 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);
>
> Why do we need EXPORT_SYMBOL_GPL?
As Christoph pointed out, I was blindly following the suit.
You are right, we don't need to export it at the moment. The remaining
EXPORT_SYMBOL_GPL are required by KVM. However, as of now, there is no
need for "has_big_cores" in the KVM.
Will remove this in the next version.
>
> > EXPORT_SYMBOL_GPL(threads_core_mask);
> >
> > + *
> > + * 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)
> > + goto out_err;
> > +
> > + 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;
> > +
>
> Shouldnt we check for property and nr_groups
> If the property is not 1 and nr_groups < 1, we should error out
> No point in calling a of_property read if property is not right.
Yes, the nr_groups < 1 check can be moved into this function.
However, this function merely parses the the thread group structure
exposed by the device tree. So it should error out only if there is a
failure in parsing, or as you said the parsed values are incorrect
(nr_groups < 1, threads_per_group < 1, etc). Whether the thread group
is relevant or not (in this case we are interested in thread groups
that share L1 cache, translation etc) is something for the caller to
decide.
However, I see what you mean. We can avoid parsing the remainder of
the array if the property in the device-tree isn't the property that
the caller is interested in.
This can be solved by passing the interested property value as a
parameter and so that the code errors out if this property doesn't
match the property in the device-tree. Will add this in the next
version.
>
>
> Nit:
> Cant we directly assign to tg->property, and hence avoid local
> variables, property, nr_groups and threads_per_group?
Will clean this up. This was from an older version where I added the
local variables so that the statements referencing them don't need to
be split across multiple lines. However, the code has been optimized
since then. So, the local variables are not needed.
>
> > + ret = of_property_read_u32_array(dn, "ibm,thread-groups",
> > +thread_group_array,
> > +3 + total_threads);
> > +
> > +static inline bool dt_has_big_core(struct device_node *dn,
> > + struct thread_groups *tg)
> > +{
> > + if (parse_thread_groups(dn, tg))
> > + return false;
> > +
> > + if (tg->property != 1)
> > + return false;
> > +
> > + if (tg->nr_groups < 1)
> > + return false;
>
> Can avoid these check if we can check in parse_thread_groups.
>
> > /**
> > * setup_cpu_maps - initialize the following cpu maps:
> > * cpu_possible_mask
> > @@ -457,6 +605,7 @@ void __init smp_setup_cpu_maps(void)
> > int cpu = 0;
> > int nthreads = 1;
> >
> > 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
> > #include
> > #include
> > +#include
> >
> > #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;
> > +
>
> Here we need to check for validity of dn and error out accordingly.
Will add this check.
>
>
Re: [PATCH v6 1/2] powerpc: Detect the presence of big-cores via "ibm,thread-groups"
On Thu, Aug 09, 2018 at 06:27:43AM -0700, Srikar Dronamraju wrote: > * Gautham R. Shenoy [2018-08-09 11:02:07]: > > > > > int threads_per_core, threads_per_subcore, threads_shift; > > +bool has_big_cores; > > 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); > > Why do we need EXPORT_SYMBOL_GPL? Because it is deeply internal, and in that matches the other related exports.
Re: [PATCH v6 1/2] powerpc: Detect the presence of big-cores via "ibm,thread-groups"
* Gautham R. Shenoy [2018-08-09 11:02:07]:
>
> int threads_per_core, threads_per_subcore, threads_shift;
> +bool has_big_cores;
> 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);
Why do we need EXPORT_SYMBOL_GPL?
> EXPORT_SYMBOL_GPL(threads_core_mask);
>
> + *
> + * 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)
> + goto out_err;
> +
> + 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;
> +
Shouldnt we check for property and nr_groups
If the property is not 1 and nr_groups < 1, we should error out
No point in calling a of_property read if property is not right.
Nit:
Cant we directly assign to tg->property, and hence avoid local
variables, property, nr_groups and threads_per_group?
> + ret = of_property_read_u32_array(dn, "ibm,thread-groups",
> + thread_group_array,
> + 3 + total_threads);
> +
> +static inline bool dt_has_big_core(struct device_node *dn,
> +struct thread_groups *tg)
> +{
> + if (parse_thread_groups(dn, tg))
> + return false;
> +
> + if (tg->property != 1)
> + return false;
> +
> + if (tg->nr_groups < 1)
> + return false;
Can avoid these check if we can check in parse_thread_groups.
> /**
> * setup_cpu_maps - initialize the following cpu maps:
> * cpu_possible_mask
> @@ -457,6 +605,7 @@ void __init smp_setup_cpu_maps(void)
> int cpu = 0;
> int nthreads = 1;
>
> 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
> #include
> #include
> +#include
>
> #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;
> +
Here we need to check for validity of dn and error out accordingly.
> + if (parse_thread_groups(dn, ))
> + return -ENODATA;
Did we miss a of_node_put(dn)?
> +
> + i = get_cpu_thread_group_start(cpu->dev.id, );
> +
> + 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]);
Here, we are making the assumption that group_start will always be the
first thread in the thread_group. However we didnt make the same
assumption in get_cpu_thread_group_start.
[PATCH v6 1/2] powerpc: Detect the presence of big-cores via "ibm, thread-groups"
From: "Gautham R. Shenoy"
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
---
Documentation/ABI/testing/sysfs-devices-system-cpu | 8 ++
arch/powerpc/include/asm/cputhreads.h | 22 +++
arch/powerpc/kernel/setup-common.c | 154 +
arch/powerpc/kernel/sysfs.c| 35 +
4 files changed, 219 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu
b/Documentation/ABI/testing/sysfs-devices-system-cpu
index 9c5e7732..52c9b50 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_siblings
+Date: 06-Aug-2018
+KernelVersion: v4.19.0
+Contact: Linux for PowerPC mailing list
+Description: List of Physical ids of CPUs which share 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_subcore1
#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..989edc1 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;
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,152 @@ 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.
+ *
+ * @dn: The device node of the CPU device.
+ * @tg: Pointer to a thread group structure into which the parsed
+ * output of "ibm,thread-groups" is stored.
+ *
+ * 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
