perf: Optimise topology iteration
Subsystems (2)
PATCH 5/7 perf: Optimise topology iteration
Currently we iterate the full machine looking for a matching core_id/nb for the percore and the amd northbridge stuff , using a smaller topology mask makes sense.
--- arch/x86/kernel/cpu/perf_event_amd.c | 2 +- arch/x86/kernel/cpu/perf_event_intel.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/perf_event_amd.c +1 -1
@@ -323,7 +323,7 @@ static void amd_pmu_cpu_starting(int cpu) nb_id =amd_get_nb_id (cpu);WARN_ON_ONCE (nb_id ==BAD_APICID ); -for_each_online_cpu (i) { +for_each_cpu (i,topology_core_cpumask (cpu)) { nb =per_cpu (cpu_hw_events, i).amd_nb;if (WARN_ON_ONCE (!nb))continue ;
arch/x86/kernel/cpu/perf_event_intel.c +1 -1
@@ -1111,7 +1111,7 @@ static void intel_pmu_cpu_starting(int cpu)if (!ht_enabled (cpu))return ; -for_each_online_cpu (i) { +for_each_cpu (i,topology_thread_cpumask (cpu)) {struct intel_percore *pc =per_cpu (cpu_hw_events, i).per_core;if (pc && pc->core_id == core_id) {
Signature · 1 line
1.7.3
Quoted 3 lines · Lin Ming
Currently we iterate the full machine looking for a matching core_id/nb for the percore and the amd northbridge stuff , using a smaller topology mask makes sense.
Does topology_thread_cpumask() include offline cpus? I tried looking at it, but I cannot find any code clearing bits in that mask on offline.
Quoted 32 lines
Signed-off-by Lin Ming <[email protected]>--- arch/x86/kernel/cpu/perf_event_amd.c | 2 +- arch/x86/kernel/cpu/perf_event_intel.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)arch/x86/kernel/cpu/perf_event_amd.c +1
-1@@ -323,7 +323,7 @@ static void amd_pmu_cpu_starting(int cpu) nb_id =amd_get_nb_id (cpu);WARN_ON_ONCE (nb_id ==BAD_APICID ); -for_each_online_cpu (i) { +for_each_cpu (i,topology_core_cpumask (cpu)) { nb =per_cpu (cpu_hw_events, i).amd_nb;if (WARN_ON_ONCE (!nb))continue ;arch/x86/kernel/cpu/perf_event_intel.c +1
-1@@ -1111,7 +1111,7 @@ static void intel_pmu_cpu_starting(int cpu)if (!ht_enabled (cpu))return ; -for_each_online_cpu (i) { +for_each_cpu (i,topology_thread_cpumask (cpu)) {struct intel_percore *pc =per_cpu (cpu_hw_events, i).per_core;if (pc && pc->core_id == core_id) {
Quoted 7 lines · Peter Zijlstra
Quoted 3 lines · Lin Ming
Currently we iterate the full machine looking for a matching core_id/nb for the percore and the amd northbridge stuff , using a smaller topology mask makes sense.
Does
topology_thread_cpumask()include offline cpus? I tried looking at it, but I cannot find any code clearing bits in that mask on offline.
The problem is not only at offline, but also at online between CPUs going online. I don't think the patch is a good idea and it doesn't even have any advantages either since this is a initialization only slow path.
-Andi
Quoted 7 lines · Peter Zijlstra
Quoted 3 lines · Lin Ming
Currently we iterate the full machine looking for a matching core_id/nb for the percore and the amd northbridge stuff , using a smaller topology mask makes sense.
Does
topology_thread_cpumask()include offline cpus? I tried looking at it, but I cannot find any code clearing bits in that mask on offline.
No, it does not include offline cpus.
For x86 code, remove_siblinginfo() clears the bits.
take_cpu_down ->
__cpu_disable ->
native_cpu_disable ->
cpu_disable_common ->
remove_siblinginfostatic void remove_siblinginfo(int cpu)
{
int sibling;
struct cpuinfo_x86 *c = &cpu_data(cpu);
for_each_cpu(sibling, cpu_core_mask(cpu)) {
cpumask_clear_cpu(cpu, cpu_core_mask(sibling));
/*/
* last thread sibling in this cpu core going down
*/
if (cpumask_weight(cpu_sibling_mask(cpu)) == 1)
cpu_data(sibling).booted_cores--;
}
for_each_cpu(sibling, cpu_sibling_mask(cpu))
cpumask_clear_cpu(cpu, cpu_sibling_mask(sibling));
cpumask_clear(cpu_sibling_mask(cpu));
cpumask_clear(cpu_core_mask(cpu));
c->phys_proc_id = 0;
c->cpu_core_id = 0;
cpumask_clear_cpu(cpu, cpu_sibling_setup_mask);
}Lin Ming
Quoted 35 lines
Quoted 33 lines
Signed-off-by Lin Ming <[email protected]>--- arch/x86/kernel/cpu/perf_event_amd.c | 2 +- arch/x86/kernel/cpu/perf_event_intel.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)arch/x86/kernel/cpu/perf_event_amd.c +1
-1@@ -323,7 +323,7 @@ static void amd_pmu_cpu_starting(int cpu) nb_id =amd_get_nb_id (cpu);WARN_ON_ONCE (nb_id ==BAD_APICID ); -for_each_online_cpu (i) { +for_each_cpu (i,topology_core_cpumask (cpu)) { nb =per_cpu (cpu_hw_events, i).amd_nb;if (WARN_ON_ONCE (!nb))continue ;arch/x86/kernel/cpu/perf_event_intel.c +1
-1@@ -1111,7 +1111,7 @@ static void intel_pmu_cpu_starting(int cpu)if (!ht_enabled (cpu))return ; -for_each_online_cpu (i) { +for_each_cpu (i,topology_thread_cpumask (cpu)) {struct intel_percore *pc =per_cpu (cpu_hw_events, i).per_core;if (pc && pc->core_id == core_id) {
Quoted 11 lines · Andi Kleen
Quoted 7 lines · Peter Zijlstra
Quoted 3 lines · Lin Ming
Currently we iterate the full machine looking for a matching core_id/nb for the percore and the amd northbridge stuff , using a smaller topology mask makes sense.
Does
topology_thread_cpumask()include offline cpus? I tried looking at it, but I cannot find any code clearing bits in that mask on offline.The problem is not only at offline, but also at online between CPUs going online. I don't think the patch is a good idea and it doesn't
I didn't see the problem.
Assume logical cpu 3, 7 are 2 threads in a core, and they are plug/unpluged as below sequence,
CPU 3 offline, CPU7 offline, CPU3 online, CPU7 online
1. After cpu3 offline
topology_thread_cpumask(3) returns emptytopology_thread_cpumask(7) returns 7
2. After CPU7 offline
topology_thread_cpumask(3) returns emptytopology_thread_cpumask(7) returns empty
3. When CPU3 online, calling intel_pmu_cpu_starting
topology_thread_cpumask(3) returns 3topology_thread_cpumask(7) returns empty
for_each_cpu(i, topology_thread_cpumask(cpu)) {
struct intel_percore *pc = per_cpu(cpu_hw_events, i).per_core;
if (pc && pc->core_id == core_id) {
kfree(cpuc->per_core);
cpuc->per_core = pc;
break;
}
}Above "if" statement will not be executed, because pc->core_id was initialized to -1 in intel_pmu_cpu_prepare.
4. When CPU7 online, calling intel_pmu_cpu_starting
topology_thread_cpumask(3) returns 3, 7topology_thread_cpumask(7) returns 3, 7
for_each_cpu(i, topology_thread_cpumask(cpu)) {
struct intel_percore *pc = per_cpu(cpu_hw_events, i).per_core;
if (pc && pc->core_id == core_id) {
kfree(cpuc->per_core);
cpuc->per_core = pc;
break;
}
}
cpuc->per_core->core_id = core_id;
cpuc->per_core->refcnt++;Above "if" statement will be executed and the per_core data allocated for cpu7 will be freed.
All above is right, or could you explain more about the problem at CPUs offline and online?
Thanks,
Lin Ming
Quoted 4 lines
even have any advantages either since this is a initialization only slow path.
-Andi
Quoted 15 lines · Lin Ming
Currently we iterate the full machine looking for a matching core_id/nb for the percore and the amd northbridge stuff , using a smaller topology mask makes sense.
Signed-off-by Lin Ming <[email protected]>--- arch/x86/kernel/cpu/perf_event_amd.c | 2 +- arch/x86/kernel/cpu/perf_event_intel.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)arch/x86/kernel/cpu/perf_event_amd.c +0
-0
arch/x86/kernel/cpu/perf_event_amd.c static void amd_pmu_cpu_starting(int cpu) · line 329
Quoted 8 lines
arch/x86/kernel/cpu/perf_event_amd.c +1
-1@@ -323,7 +323,7 @@ static void amd_pmu_cpu_starting(int cpu) nb_id =amd_get_nb_id (cpu);WARN_ON_ONCE (nb_id ==BAD_APICID ); -for_each_online_cpu (i) { +for_each_cpu (i,topology_core_cpumask (cpu)) { nb =per_cpu (cpu_hw_events, i).amd_nb;if (WARN_ON_ONCE (!nb))continue ;
Borislav, is topology_core_cpumask() the right mask for northbridge_id span? I could imagine Magny-Cours would have all 12 cores in the core_cpumask() and have the node_mask() be half that.
Adding Andreas since this is his code.
Quoted 19 lines · Peter Zijlstra
Quoted 18 lines · Lin Ming
Currently we iterate the full machine looking for a matching core_id/nb for the percore and the amd northbridge stuff , using a smaller topology mask makes sense.
Signed-off-by Lin Ming <[email protected]>--- arch/x86/kernel/cpu/perf_event_amd.c | 2 +- arch/x86/kernel/cpu/perf_event_intel.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)arch/x86/kernel/cpu/perf_event_amd.c +0
-0@@ -323,7 +323,7 @@ static void amd_pmu_cpu_starting(int cpu) nb_id =amd_get_nb_id (cpu);WARN_ON_ONCE (nb_id ==BAD_APICID );
Quoted 9 lines
arch/x86/kernel/cpu/perf_event_amd.c static void amd_pmu_cpu_starting(int cpu) · line 329
Quoted 5 lines
arch/x86/kernel/cpu/perf_event_amd.c +1
-1-for_each_online_cpu (i) { +for_each_cpu (i,topology_core_cpumask (cpu)) { nb =per_cpu (cpu_hw_events, i).amd_nb;if (WARN_ON_ONCE (!nb))continue ;Borislav, is
topology_core_cpumask()the right mask fornorthbridge_idspan? I could imagine Magny-Cours would have all 12 cores in thecore_cpumask()and have thenode_mask()be half that.
So, topology_core_cpumask() or cpu_core_mask() both are cpu_core_map which represents the socket mask. I.e., on a multisocket cpu you'll have in it all the cores on one socket. A 12-cores Magny-Cours contains two internal northbridges and this mask will have 12 bits set.
AFAICT, you want to iterate over the cores on a single node here (an internal node in the Magny-Cours case) so for this we have the llc_shared_map. See near the top of cache_shared_cpu_map_setup() in <arch/x86/kernel/cpu/intel_cacheinfo.c> for an example.
node_mask() is roughly the same but contains correct info only with CONFIG_NUMA on and correct SRAT table on the system.
HTH.
Signature · 8 lines
Regards/Gruss, Boris. Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach General Managers: Alberto Bozzo, Andrew Bowd Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
Quoted 37 lines · Borislav Petkov
Adding Andreas since this is his code.
Quoted 19 lines · Peter Zijlstra
Quoted 18 lines · Lin Ming
Currently we iterate the full machine looking for a matching core_id/nb for the percore and the amd northbridge stuff , using a smaller topology mask makes sense.
Signed-off-by Lin Ming <[email protected]>--- arch/x86/kernel/cpu/perf_event_amd.c | 2 +- arch/x86/kernel/cpu/perf_event_intel.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)arch/x86/kernel/cpu/perf_event_amd.c +0
-0@@ -323,7 +323,7 @@ static void amd_pmu_cpu_starting(int cpu) nb_id =amd_get_nb_id (cpu);WARN_ON_ONCE (nb_id ==BAD_APICID );Quoted 9 lines
arch/x86/kernel/cpu/perf_event_amd.c static void amd_pmu_cpu_starting(int cpu) · line 329
Quoted 5 lines
arch/x86/kernel/cpu/perf_event_amd.c +1
-1-for_each_online_cpu (i) { +for_each_cpu (i,topology_core_cpumask (cpu)) { nb =per_cpu (cpu_hw_events, i).amd_nb;if (WARN_ON_ONCE (!nb))continue ;Borislav, is
topology_core_cpumask()the right mask fornorthbridge_idspan? I could imagine Magny-Cours would have all 12 cores in thecore_cpumask()and have thenode_mask()be half that.So,
topology_core_cpumask()orcpu_core_mask()both arecpu_core_mapwhich represents the socket mask. I.e., on a multisocket cpu you'll have in it all the cores on one socket. A 12-cores Magny-Cours contains two internal northbridges and this mask will have 12 bits set.
Quoted 4 lines
AFAICT, you want to iterate over the cores on a single node here (an internal node in the Magny-Cours case) so for this we have the
llc_shared_map. See near the top ofcache_shared_cpu_map_setup()in <arch/x86/kernel/cpu/intel_cacheinfo.c> for an example.
cpu_coregroup_mask() seems the right mask for northbridge_id span.
arch/x86/kernel/smpboot.c:
/* maps the cpu to the sched domain representing multi-core */ const struct cpumask *cpu_coregroup_mask(int cpu)
{
struct cpuinfo_x86 *c = &cpu_data(cpu);
/*
* For perf, we return last level cache shared map.
* And for power savings, we return cpu_core_map
*/
if ((sched_mc_power_savings || sched_smt_power_savings) &&
!(cpu_has(c, X86_FEATURE_AMD_DCM)))
return cpu_core_mask(cpu);
else
return c->llc_shared_map;
}Quoted 6 lines
node_mask()is roughly the same but contains correct info only withCONFIG_NUMAon and correct SRAT table on the system.HTH.
Quoted 25 lines · Lin Ming
Quoted 14 lines
Quoted 8 lines
Quoted 4 lines
+ for_each_cpu(i, topology_core_cpumask(cpu)) { nb = per_cpu(cpu_hw_events, i).amd_nb; if (WARN_ON_ONCE(!nb)) continue;Borislav, is
topology_core_cpumask()the right mask fornorthbridge_idspan? I could imagine Magny-Cours would have all 12 cores in thecore_cpumask()and have thenode_mask()be half that.So,
topology_core_cpumask()orcpu_core_mask()both arecpu_core_mapwhich represents the socket mask. I.e., on a multisocket cpu you'll have in it all the cores on one socket. A 12-cores Magny-Cours contains two internal northbridges and this mask will have 12 bits set.Quoted 4 lines
AFAICT, you want to iterate over the cores on a single node here (an internal node in the Magny-Cours case) so for this we have the
llc_shared_map. See near the top ofcache_shared_cpu_map_setup()in <arch/x86/kernel/cpu/intel_cacheinfo.c> for an example.
cpu_coregroup_mask()seems the right mask fornorthbridge_idspan.
arch/x86/kernel/smpboot.c:/* maps the cpu to the sched domain representing multi-core */ const struct cpumask *cpu_coregroup_mask(int cpu)
Quoted 12 lines
{
struct cpuinfo_x86 *c = &cpu_data(cpu);
/*
* For perf, we return last level cache shared map.
* And for power savings, we return cpu_core_map
*/
if ((sched_mc_power_savings || sched_smt_power_savings) &&
!(cpu_has(c, X86_FEATURE_AMD_DCM)))
return cpu_core_mask(cpu);
else
return c->llc_shared_map;
} Argh, that function really must die, its the most horrible brain damage around. Andreas promised he'd clean that up after making it worse for Magny-Cours.
But yes, assuming all Magny-Cours have this AMD_DCM thing set, it seems to return the right map.