From: Gautham R. Shenoy <hidden> Date: 2018-08-09 05:32:34
From: "Gautham R. Shenoy" <redacted>
Hi,
This is the fifth iteration of the patchset to add support for
big-core on POWER9. This patch also optimizes the task placement on
such big-core systems.
The previous versions can be found here:
v5: https://lkml.org/lkml/2018/8/6/587
v4: https://lkml.org/lkml/2018/7/24/79
v3: https://lkml.org/lkml/2018/7/6/255
v2: https://lkml.org/lkml/2018/7/3/401
v1: https://lkml.org/lkml/2018/5/11/245
Changes :
v5 --> v6:
- Fixed the code to build without warnings for !CONFIG_SCHED_SMT.
- While checking for shared caches on big-core system, use the
smallcore_sibling_mask to compare with compare with
l2_cache_mask, which will ensure that the CACHE level
sched-domain is created.
- Added benchmark results with hackbench to demonstrate the
benefits of having the CACHE level sched-domain.
v4 --> v5:
- Patch 2 is entirely different: Instead of using CPU_FTR_ASYM_SMT
feature, use the small core siblings at the SMT level
sched-domain. This was suggested by Nicholas Piggin and Michael
Ellerman.
- A more detailed description follows below.
v3 --> v4:
- Build fix for powerpc-g5 : Enable CPU_FTR_ASYM_SMT only on
CONFIG_PPC_POWERNV and CONFIG_PPC_PSERIES.
- Fixed a minor error in the ABI description.
v2 --> v3
- Set sane values in the tg->property, tg->nr_groups inside
parse_thread_groups before returning due to an error.
- Define a helper function to determine whether a CPU device node
is a big-core or not.
- Updated the comments around the functions to describe the
arguments passed to them.
v1 --> v2
- Added comments explaining the "ibm,thread-groups" device tree property.
- Uses cleaner device-tree parsing functions to parse the u32 arrays.
- Adds a sysfs file listing the small-core siblings for every CPU.
- Enables the scheduler optimization by setting the CPU_FTR_ASYM_SMT bit
in the cur_cpu_spec->cpu_features on detecting the presence
of interleaved big-core.
- Handles the corner case where there is only a single thread-group
or when there is a single thread in a thread-group.
Description:
~~~~~~~~~~~~~~~~~~~~
A pair of IBM POWER9 SMT4 cores can be fused together to form a
big-core with 8 SMT threads. This can be discovered via the
"ibm,thread-groups" CPU property in the device tree which will
indicate which group of threads that share the L1 cache, translation
cache and instruction data flow. If there are multiple such group of
threads, then the core is a big-core. Furthermore, on POWER9 the thread-ids of
such a big-core is obtained by interleaving the thread-ids of the
component SMT4 cores.
Eg: Threads in the pair of component SMT4 cores of an interleaved
big-core are numbered {0,2,4,6} and {1,3,5,7} respectively.
-------------------------
| L1 Cache |
----------------------------------
|L2| | | | |
| | 0 | 2 | 4 | 6 |Small Core0
|C | | | | |
Big |a --------------------------
Core |c | | | | |
|h | 1 | 3 | 5 | 7 | Small Core1
|e | | | | |
-----------------------------
| L1 Cache |
--------------------------
On such a big-core system, when multiple tasks are scheduled to run on
the big-core, we get the best performance when the tasks are spread
across the pair of SMT4 cores.
Eg: Suppose there 4 tasks {p1, p2, p3, p4} are run on a big core, then
An Example of Optimal Task placement:
--------------------------
| | | | |
| 0 | 2 | 4 | 6 | Small Core0
| (p1)| (p2)| | |
Big Core --------------------------
| | | | |
| 1 | 3 | 5 | 7 | Small Core1
| | (p3)| | (p4) |
--------------------------
An example of Suboptimal Task placement:
--------------------------
| | | | |
| 0 | 2 | 4 | 6 | Small Core0
| (p1)| (p2)| | (p4)|
Big Core --------------------------
| | | | |
| 1 | 3 | 5 | 7 | Small Core1
| | (p3)| | |
--------------------------
In order to achieve optimal task placement, on big-core systems, we
define the SMT level sched-domain to consist of the threads belonging
to the small cores. The CACHE level sched domain will consist of all
the threads belonging to the big-core. With this, the Linux Kernel
load-balancer will ensure that the tasks are spread across all the
component small cores in the system, thereby yielding optimum
performance.
Furthermore, this solution works correctly across all SMT modes
(8,4,2), as the interleaved thread-ids ensures that when we go to
lower SMT modes (4,2) the threads are offlined in a descending order,
thereby leaving equal number of threads from the component small cores
online as illustrated below.
With Patches: (ppc64_cpu --smt=on) : SMT domain
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CPU0 attaching sched-domain(s):
domain-0: span=0,2,4,6 level=SMT
groups: 0:{ span=0 cap=294 }, 2:{ span=2 cap=294 },
4:{ span=4 cap=294 }, 6:{ span=6 cap=294 }
CPU1 attaching sched-domain(s):
domain-0: span=1,3,5,7 level=SMT
groups: 1:{ span=1 cap=294 }, 3:{ span=3 cap=294 },
5:{ span=5 cap=294 }, 7:{ span=7 cap=294 }
Optimal Task placement (SMT 8)
--------------------------
| | | | |
| 0 | 2 | 4 | 6 | Small Core0
| (p1)| (p2)| | |
Big Core --------------------------
| | | | |
| 1 | 3 | 5 | 7 | Small Core1
| | (p3)| | (p4) |
--------------------------
With Patches : (ppc64_cpu --smt=4) : SMT domain
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CPU0 attaching sched-domain(s):
domain-0: span=0,2 level=SMT
groups: 0:{ span=0 cap=589 }, 2:{ span=2 cap=589 }
CPU1 attaching sched-domain(s):
domain-0: span=1,3 level=SMT
groups: 1:{ span=1 cap=589 }, 3:{ span=3 cap=589 }
Optimal Task placement (SMT 4)
--------------------------
| | | | |
| 0 | 2 | 4 | 6 | Small Core0
| (p1)| (p2)| Off | Off |
Big Core --------------------------
| | | | |
| 1 | 3 | 5 | 7 | Small Core1
| (p4)| (p3)| Off | Off |
--------------------------
With Patches : (ppc64_cpu --smt=2) : SMT domain ceases to exist.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Optimal Task placement (SMT 2)
--------------------------
| (p2)| | | |
| 0 | 2 | 4 | 6 | Small Core0
| (p1)| Off | Off | Off |
Big Core --------------------------
| (p3)| | | |
| 1 | 3 | 5 | 7 | Small Core1
| (p4)| Off | Off | Off |
--------------------------
Thus, as an added advantage in SMT=2 mode, we will only have 3 levels
in the sched-domain topology (CACHE, DIE and NUMA).
The SMT levels, without the patches are as follows.
Without Patches: (ppc64_cpu --smt=on) : SMT domain
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CPU0 attaching sched-domain(s):
domain-0: span=0-7 level=SMT
groups: 0:{ span=0 cap=147 }, 1:{ span=1 cap=147 },
2:{ span=2 cap=147 }, 3:{ span=3 cap=147 },
4:{ span=4 cap=147 }, 5:{ span=5 cap=147 },
6:{ span=6 cap=147 }, 7:{ span=7 cap=147 }
CPU1 attaching sched-domain(s):
domain-0: span=0-7 level=SMT
groups: 1:{ span=1 cap=147 }, 2:{ span=2 cap=147 },
3:{ span=3 cap=147 }, 4:{ span=4 cap=147 },
5:{ span=5 cap=147 }, 6:{ span=6 cap=147 },
7:{ span=7 cap=147 }, 0:{ span=0 cap=147 }
Without Patches: (ppc64_cpu --smt=4) : SMT domain
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CPU0 attaching sched-domain(s):
domain-0: span=0-3 level=SMT
groups: 0:{ span=0 cap=294 }, 1:{ span=1 cap=294 },
2:{ span=2 cap=294 }, 3:{ span=3 cap=294 },
CPU1 attaching sched-domain(s):
domain-0: span=0-3 level=SMT
groups: 1:{ span=1 cap=294 }, 2:{ span=2 cap=294 },
3:{ span=3 cap=294 }, 0:{ span=0 cap=294 }
Without Patches: (ppc64_cpu --smt=2) : SMT domain
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
CPU0 attaching sched-domain(s):
domain-0: span=0-1 level=SMT
groups: 0:{ span=0 cap=589 }, 1:{ span=1 cap=589 },
CPU1 attaching sched-domain(s):
domain-0: span=0-1 level=SMT
groups: 1:{ span=1 cap=589 }, 0:{ span=0 cap=589 },
This patchset contains two patches which on detecting the presence of
big-cores, defines the SMT level sched domain to correspond to the
threads of the small cores.
Patch 1: adds support to detect the presence of
big-cores and reports the small-core siblings of each CPU X
via the sysfs file "/sys/devices/system/cpu/cpuX/small_core_siblings".
Patch 2: Defines the SMT level sched domain to correspond to the
threads of the small cores.
Results:
~~~~~~~~~~~~~~~~~
1) 2 thread ebizzy
~~~~~~~~~~~~~~~~~~~~~~
Experimental results for ebizzy with 2 threads, bound to a single big-core
show a marked improvement with this patchset over the 4.18-rc5 vanilla
kernel.
The result of 100 such runs for 4.18-rc7 kernel and the 4.18-rc7 +
big-core-smt-patches are as follows
4.18.0-rc7 vanilla
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
records/s : # samples : Histogram
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[ 0 - 1000000] : 0 : #
[1000000 - 2000000] : 3 : #
[2000000 - 3000000] : 7 : ##
[3000000 - 4000000] : 26 : ######
[4000000 - 5000000] : 4 : #
[5000000 - 6000000] : 60 : #############
4.18.0-rc7 + big-core-smt-patches
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
records/s : # samples : Histogram
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[ 0 - 1000000] : 0 : #
[1000000 - 2000000] : 0 : #
[2000000 - 3000000] : 11 : ###
[3000000 - 4000000] : 0 : #
[4000000 - 5000000] : 0 : #
[5000000 - 6000000] : 89 : ##################
2) Hackbench (perf bench sched pipe)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
100 iterations of the hackbench run both on 4.18-rc7 vanilla kernel
and v.18-rc7 + big-core-smt-patches. All the values are time in
seconds (Lower the better)
4.18.0-rc7 vanilla
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
N Min Max Median Avg Stddev
x 100 4.225 9.754 6.174 6.00402 0.88311027
4.18.0-rc7 + big-core-smt-patches (v6 : the present version)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
N Min Max Median Avg Stddev
x 100 4.069 6.745 6.08 5.72414 0.73853727
The presence of the CACHE level sched-domain in v6, which was absent
in v5 of the patches seems to be making a difference, as the median
and the average times taken by hackbench both drop.
4.18.0-rc7 + big-core-smt-patches (v5 : the previous version)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
N Min Max Median Avg Stddev
x 100 4.972 10.123 6.177 6.323 0.68728617
Gautham R. Shenoy (2):
powerpc: Detect the presence of big-cores via "ibm,thread-groups"
powerpc: Use cpu_smallcore_sibling_mask at SMT level on bigcores
Documentation/ABI/testing/sysfs-devices-system-cpu | 8 ++
arch/powerpc/include/asm/cputhreads.h | 22 +++
arch/powerpc/include/asm/smp.h | 6 +
arch/powerpc/kernel/setup-common.c | 154 +++++++++++++++++++++
arch/powerpc/kernel/smp.c | 62 ++++++++-
arch/powerpc/kernel/sysfs.c | 35 +++++
6 files changed, 282 insertions(+), 5 deletions(-)
--
1.9.4
From: Gautham R. Shenoy <hidden> Date: 2018-08-09 05:32:37
From: "Gautham R. Shenoy" <redacted>
Each of the SMT4 cores forming a big-core are more or less independent
units. Thus when multiple tasks are scheduled to run on the fused
core, we get the best performance when the tasks are spread across the
pair of SMT4 cores.
This patch achieves this by setting the SMT level mask to correspond
to the smallcore sibling mask on big-core systems. This patch also
ensures that while checked for shared-caches on big-core system, we
use the smallcore_sibling_mask to compare with the l2_cache_mask.
This ensure that the CACHE level sched-domain is created, whose groups
correspond to the threads of the big-core.
With this patch, the SMT sched-domain with SMT=8,4,2 on big-core
systems are as follows:
1) ppc64_cpu --smt=8
CPU0 attaching sched-domain(s):
domain-0: span=0,2,4,6 level=SMT
groups: 0:{ span=0 cap=294 }, 2:{ span=2 cap=294 },
4:{ span=4 cap=294 }, 6:{ span=6 cap=294 }
CPU1 attaching sched-domain(s):
domain-0: span=1,3,5,7 level=SMT
groups: 1:{ span=1 cap=294 }, 3:{ span=3 cap=294 },
5:{ span=5 cap=294 }, 7:{ span=7 cap=294 }
2) ppc64_cpu --smt=4
CPU0 attaching sched-domain(s):
domain-0: span=0,2 level=SMT
groups: 0:{ span=0 cap=589 }, 2:{ span=2 cap=589 }
CPU1 attaching sched-domain(s):
domain-0: span=1,3 level=SMT
groups: 1:{ span=1 cap=589 }, 3:{ span=3 cap=589 }
3) ppc64_cpu --smt=2
SMT domain ceases to exist as each domain consists of just one
group.
Signed-off-by: Gautham R. Shenoy <redacted>
---
arch/powerpc/include/asm/smp.h | 6 ++++
arch/powerpc/kernel/smp.c | 62 ++++++++++++++++++++++++++++++++++++++----
2 files changed, 63 insertions(+), 5 deletions(-)
@@ -1158,6 +1203,13 @@ void __init smp_cpus_done(unsigned int max_cpus)dump_numa_cpu_topology();+#ifdef CONFIG_SCHED_SMT+if(has_big_cores){+pr_info("Using small cores at SMT level\n");+power9_topology[0].mask=smallcore_smt_mask;+powerpc_topology[0].mask=smallcore_smt_mask;+}+#endif/**IfanyCPUdetectsthatit'ssharingacachewithanotherCPUthen*usethedeepertopologythatisawareofthissharing.
From: Gautham R. Shenoy <hidden> Date: 2018-08-09 05:32:42
From: "Gautham R. Shenoy" <redacted>
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 <redacted>
---
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(+)
@@ -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 <linuxppc-dev@ozlabs.org>+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.
* Gautham R. Shenoy [off-list ref] [2018-08-09 11:02:08]:
3) ppc64_cpu --smt=2
SMT domain ceases to exist as each domain consists of just one
group.
When seen in isolation, the above looks as if ppc64_cpu --smt=2 o/p says
" SMT domain ceases to exist...."
quoted hunk
@@ -999,7 +1012,17 @@ static void add_cpu_to_masks(int cpu) { int first_thread = cpu_first_thread_sibling(cpu); int chipid = cpu_to_chip_id(cpu);- int i;++ struct thread_groups tg;+ int i, cpu_group_start = -1;++ if (has_big_cores) {+ struct device_node *dn = of_get_cpu_node(cpu, NULL);+
Not checking for validity of dn and no of_node_puts?
+ parse_thread_groups(dn, &tg);
+ cpu_group_start = get_cpu_thread_group_start(cpu, &tg);
+ cpumask_set_cpu(cpu, cpu_smallcore_sibling_mask(cpu));
+ }
/*
* This CPU will not be in the online mask yet so we need to manually
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?
Here we need to check for validity of dn and error out accordingly.
+ if (parse_thread_groups(dn, &tg))
+ return -ENODATA;
Did we miss a of_node_put(dn)?
+
+ 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]);
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.
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.
quoted
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;
quoted
+
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.
Here we need to check for validity of dn and error out accordingly.
Will add this check.
quoted
+ if (parse_thread_groups(dn, &tg))
+ return -ENODATA;
Did we miss a of_node_put(dn)?
Yes we did. Will fix this.
quoted
+
+ 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]);
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.
Above the get_cpu_thread_group_start function , we have the following
comment which indicates that group_start will point to the start of
the thread group. Is this what you are referring to?
/*
* 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.
*/
From: Gautham R Shenoy <hidden> Date: 2018-08-13 12:07:46
On Thu, Aug 09, 2018 at 06:26:57AM -0700, Srikar Dronamraju wrote:
* Gautham R. Shenoy [off-list ref] [2018-08-09 11:02:08]:
quoted
3) ppc64_cpu --smt=2
SMT domain ceases to exist as each domain consists of just one
group.
When seen in isolation, the above looks as if ppc64_cpu --smt=2 o/p says
" SMT domain ceases to exist...."
Ok. The intent was to say that one of the sched-domain level
collapses, thereby leaving only CACHE, DIE and NUMA. Will word it
better.
quoted
@@ -999,7 +1012,17 @@ static void add_cpu_to_masks(int cpu) { int first_thread = cpu_first_thread_sibling(cpu); int chipid = cpu_to_chip_id(cpu);- int i;++ struct thread_groups tg;+ int i, cpu_group_start = -1;++ if (has_big_cores) {+ struct device_node *dn = of_get_cpu_node(cpu, NULL);+
Not checking for validity of dn and no of_node_puts?
Will fix this. Thanks for catching this.
quoted
+ parse_thread_groups(dn, &tg);
+ cpu_group_start = get_cpu_thread_group_start(cpu, &tg);
+ cpumask_set_cpu(cpu, cpu_smallcore_sibling_mask(cpu));
+ }
/*
* This CPU will not be in the online mask yet so we need to manually
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.
quoted
quoted
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;
quoted
quoted
+
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.
quoted
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.
Here we need to check for validity of dn and error out accordingly.
Will add this check.
quoted
quoted
+ if (parse_thread_groups(dn, &tg))
+ return -ENODATA;
Did we miss a of_node_put(dn)?
Yes we did. Will fix this.
quoted
quoted
+
+ 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]);
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.
Above the get_cpu_thread_group_start function , we have the following
comment which indicates that group_start will point to the start of
the thread group. Is this what you are referring to?
/*
* 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.
*/