Fixup for discontiguous/sparse numa nodes
v3 cover Fixup for discontiguous/sparse numa nodes
From: Satheesh Rajendran <[email protected]>
Certain systems would have sparse/discontinguous numa nodes.
perf bench numa doesnt work well on such nodes.
1. It shows wrong values.
2. It can hang.
3. It can show redundant information for non-existant nodes.
#numactl -H
available: 2 nodes (0,8)
node 0 cpus: 0 1 2 3 4 5 6 7
node 0 size: 61352 MB
node 0 free: 57168 MB
node 8 cpus: 8 9 10 11 12 13 14 15
node 8 size: 65416 MB
node 8 free: 36593 MB
node distances:
node 0 8
0: 10 40 8: 40 10
Scenario 1:
Before Fix:
# perf bench numa mem --no-data_rand_walk -p 2 -t 20 -G 0 -P 3072 -T 0 -l 50 -c -s 1000 ... ... # 40 tasks will execute (on 9 nodes, 16 CPUs): ----> Wrong number of nodes ... # 2.0% [0.2 mins] 1/1 0/0 0/0 0/0 0/0 0/0 0/0 0/0 4/1 [ 4/2 ] l: 0-0 ( 0) ----> Shows info on non-existant nodes.
After Fix:
# ./perf bench numa mem --no-data_rand_walk -p 2 -t 20 -G 0 -P 3072 -T 0 -l 50 -c -s 1000
...
...
# 40 tasks will execute (on 2 nodes, 16 CPUs):
...
# 2.0% [0.2 mins] 9/1 0/0 [ 9/1 ] l: 0-0 ( 0)
# 4.0% [0.4 mins] 21/2 19/1 [ 2/3 ] l: 0-1 ( 1) {1-2}Scenario 2:
Before Fix:
# perf bench numa all # Running numa/mem benchmark... .... ... # Running RAM-bw-remote, "perf bench numa mem -p 1 -t 1 -P 1024 -C 0 -M 1 -s 20 -zZq --thp 1 --no-data_rand_walk" perf: bench/numa.c:306: bind_to_memnode: Assertion `!(ret)' failed. ------------> Got hung
After Fix:
# ./perf bench numa all # Running numa/mem benchmark... .... ... # Running RAM-bw-remote, "perf bench numa mem -p 1 -t 1 -P 1024 -C 0 -M 1 -s 20 -zZq --thp 1 --no-data_rand_walk"
# NOTE: ignoring bind NODEs starting at NODE#1 # NOTE: 0 tasks mem-bound, 1 tasks unbound
20.017 secs slowest (max) thread-runtime 20.000 secs fastest (min) thread-runtime 20.006 secs average thread-runtime
0.043 % difference between max/avg runtime
413.794 GB data processed, per thread 413.794 GB data processed, total
0.048 nsecs/byte/thread runtime
20.672 GB/sec/thread speed
20.672 GB/sec total speed
Changes in v2:
Fixed review comments for function names and alloc failure handle
Changes in v3:
Coding Style fixes.
Satheesh Rajendran (2):
perf/bench/numa: Add functions to detect sparse numa nodes perf/bench/numa: Handle discontiguous/sparse numa nodes
tools/perf/bench/numa.c | 61 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 54 insertions(+), 7 deletions(-)
Signature · 1 line
2.7.4
PATCH v3 1/2 perf/bench/numa: Add functions to detect sparse numa nodes
From: Satheesh Rajendran <[email protected]>
Added functions 1) to get a count of all nodes that are exposed to userspace. These nodes could be memoryless cpu nodes or cpuless memory nodes, 2) to check given node is present and 3) to check given node has cpus
This information can be used to handle sparse/discontiguous nodes.
--- tools/perf/bench/numa.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)
tools/perf/bench/numa.c +44 -0
@@ -215,6 +215,50 @@ static const char * const numa_usage[] = { NULL }; +/* +* To get number of numa nodes present. +*/ +static int nr_numa_nodes (void ) +{ +int i, nr_nodes =0 ; + +for (i =0 ; i < g->p.nr_nodes; i++) { +if (numa_bitmask_isbitset (numa_nodes_ptr, i)) + nr_nodes++; + } + +return nr_nodes; +} + +/* +* To check if given numa node is present. +*/ +static int is_node_present (int node) +{ +return numa_bitmask_isbitset (numa_nodes_ptr, node); +} + +/* +* To check given numa node has cpus. +*/ +static bool node_has_cpus (int node) +{ +struct bitmask *cpu =numa_allocate_cpumask (); +unsigned int i; + +if (cpu ==NULL ) +return false;/* lets fall back to nocpus safely */ + +if (numa_node_to_cpus (node, cpu) ==0 ) { +for (i =0 ; i < cpu->size; i++) { +if (numa_bitmask_isbitset (cpu, i)) +return true; + } + } + +return false; +} +static cpu_set_t bind_to_cpu (int target_cpu) {cpu_set_t orig_mask, mask;
Signature · 1 line
2.7.4
Re: [PATCH v3 1/2] perf/bench/numa: Add functions to detect sparse numa nodes
Hi Satheesh,
Quoted 31 lines · On 2017/08/21 10:15AM, [email protected] wrote:
From: Satheesh Rajendran <[email protected]>
Added functions 1) to get a count of all nodes that are exposed to userspace. These nodes could be memoryless cpu nodes or cpuless memory nodes, 2) to check given node is present and 3) to check given node has cpus
This information can be used to handle sparse/discontiguous nodes.
Cc Arnaldo Carvalho de Melo <[email protected]>Reviewed-by Srikar Dronamraju <[email protected]>Signed-off-by Satheesh Rajendran <[email protected]>Signed-off-by Balamuruhan S <[email protected]>--- tools/perf/bench/numa.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)tools/perf/bench/numa.c +6
-0@@ -215,6 +215,50 @@ static const char * const numa_usage[] = { NULL }; +/* +* To get number of numa nodes present. +*/ +static int nr_numa_nodes (void ) +{ +int i, nr_nodes =0 ;
tools/perf/bench/numa.c static const char * const numa_usage[] = { · line 233
Quoted 10 lines
tools/perf/bench/numa.c +10
-0+ +for (i =0 ; i < g->p.nr_nodes; i++) { +if (numa_bitmask_isbitset (numa_nodes_ptr, i)) + nr_nodes++; + } + +return nr_nodes; +} + +/*
Please run patches through scripts/checkpatch.pl. There is a trailing whitespace above...
Quoted 9 lines
tools/perf/bench/numa.c +9
-0+* To check if given numa node is present. +*/ +static int is_node_present (int node) +{ +return numa_bitmask_isbitset (numa_nodes_ptr, node); +} + +/* +* To check given numa node has cpus.
tools/perf/bench/numa.c static const char * const numa_usage[] = { · line 252
Quoted 10 lines
tools/perf/bench/numa.c +10
-0+*/ +static bool node_has_cpus (int node) +{ +struct bitmask *cpu =numa_allocate_cpumask (); +unsigned int i; + +if (cpu ==NULL ) +return false;/* lets fall back to nocpus safely */ + +if (numa_node_to_cpus (node, cpu) ==0 ) {
This can be simplified to:
if (cpu && !numa_node_to_cpus(node, cpu)) {tools/perf/bench/numa.c static const char * const numa_usage[] = { · line 257
Quoted 5 lines
tools/perf/bench/numa.c +5
-0+for (i =0 ; i < cpu->size; i++) { +if (numa_bitmask_isbitset (cpu, i)) +return true; + } + }
The indentation on those brackets look to be wrong.
tools/perf/bench/numa.c static const char * const numa_usage[] = { · line 261
Quoted 4 lines
tools/perf/bench/numa.c +4
-0+ +return false; +} +
More importantly, you've introduced few functions in this patch, but none of those are being used. This is not a useful way to split your patches. In fact, this hurts bisect since trying to build perf with just this patch applied throws errors.
You seem to be addressing a few different issues related to perf bench numa. You might want to split your patch based on the specific issue(s) each change fixes.
- Naveen
Quoted 11 lines
tools/perf/bench/numa.c +0
-0static cpu_set_t bind_to_cpu (int target_cpu) {cpu_set_t orig_mask, mask;Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] perf/bench/numa: Add functions to detect sparse numa nodes
Hi Naveen,Thanks for detailed review, my comments inline.
Quoted 42 lines · Naveen N. Rao
Hi Satheesh,
Quoted 39 lines · On 2017/08/21 10:15AM, [email protected] wrote:
From: Satheesh Rajendran <[email protected]>
Added functions 1) to get a count of all nodes that are exposed to userspace. These nodes could be memoryless cpu nodes or cpuless memory
nodes, 2) to check given node is present and 3) to check given node has cpusThis information can be used to handle sparse/discontiguous nodes.
Cc Arnaldo Carvalho de Melo <[email protected]>Reviewed-by Srikar Dronamraju <[email protected]>Signed-off-by Satheesh Rajendran <[email protected]>Signed-off-by Balamuruhan S <[email protected]>---
tools/perf/bench/numa.c | 44
++++++++++++++++++++++++++++++++++++++++++++1 file changed, 44 insertions(+)tools/perf/bench/numa.c +0
-0@@ -215,6 +215,50 @@ static const char * const numa_usage[] = {NULL
};tools/perf/bench/numa.c +11
-0+/* +* To get number of numa nodes present. +*/ +static int nr_numa_nodes (void ) +{ +int i, nr_nodes =0 ; + +for (i =0 ; i < g->p.nr_nodes; i++) { +if (numa_bitmask_isbitset (numa_nodes_ptr, i)) + nr_nodes++; + }
Quoted 9 lines
tools/perf/bench/numa.c static const char * const numa_usage[] = { · line 233
Quoted 5 lines
tools/perf/bench/numa.c +5
-0+ +return nr_nodes; +} + +/* Please run patches through
scripts/checkpatch.pl. There is a trailing
whitespace above...
Sure
Quoted 25 lines
Quoted 19 lines
+ * To check if given numa node is present.
+ */tools/perf/bench/numa.c +15
-0+static int is_node_present (int node) +{ +return numa_bitmask_isbitset (numa_nodes_ptr, node); +} + +/* +* To check given numa node has cpus. +*/ +static bool node_has_cpus (int node) +{ +struct bitmask *cpu =numa_allocate_cpumask (); +unsigned int i; + +if (cpu ==NULL ) +return false;/* lets fall back to nocpus safely */
tools/perf/bench/numa.c
Quoted 2 lines
tools/perf/bench/numa.c +2
-0+ +if (numa_node_to_cpus (node, cpu) ==0 ) {This can be simplified to:
if (cpu && !numa_node_to_cpus(node, cpu)) {Quoted 1 line
Quoted 7 lines
tools/perf/bench/numa.c static const char * const numa_usage[] = { · line 257
Quoted 5 lines
tools/perf/bench/numa.c +5
-0+for (i =0 ; i < cpu->size; i++) { +if (numa_bitmask_isbitset (cpu, i)) +return true; + } + }The indentation on those brackets look to be wrong.
Sure
Quoted 11 lines
tools/perf/bench/numa.c static const char * const numa_usage[] = { · line 261
Quoted 5 lines
tools/perf/bench/numa.c +4
-0+ +return false; +} +More importantly, you've introduced few functions in this patch, but none of those are being used. This is not a useful way to split your patches. In fact, this hurts bisect since trying to build perf with just
this patch applied throws errors.
Sure, This can be merged to single patch, will do it in next version.
Quoted 10 lines
You seem to be addressing a few different issues related to perf bench
numa. You might want to split your patch based on the specific issue(s)
each change fixes.- Naveen
Regards,
-Satheesh.
Quoted 4 lines
Quoted 4 lines
static
cpu_set_tbind_to_cpu(int target_cpu)
{
cpu_set_torig_mask, mask;
PATCH v3 2/2 perf/bench/numa: Handle discontiguous/sparse numa nodes
In reply to [email protected]From: Satheesh Rajendran <[email protected]>
Certain systems are designed to have sparse/discontiguous nodes. On such systems, perf bench numa hangs, shows wrong number of nodes and shows values for non-existent nodes. Handle this by only taking nodes that are exposed by kernel to userspace.
--- tools/perf/bench/numa.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
tools/perf/bench/numa.c +10 -7
@@ -287,12 +287,12 @@ static cpu_set_t bind_to_cpu(int target_cpu)static cpu_set_t bind_to_node (int target_node) { -int cpus_per_node = g->p.nr_cpus/g->p.nr_nodes; +int cpus_per_node = g->p.nr_cpus/nr_numa_nodes ();cpu_set_t orig_mask, mask;int cpu;int ret; -BUG_ON (cpus_per_node*g->p.nr_nodes != g->p.nr_cpus); +BUG_ON (cpus_per_node*nr_numa_nodes () != g->p.nr_cpus);BUG_ON (!cpus_per_node); ret =sched_getaffinity (0 ,sizeof (orig_mask), &orig_mask);
@@ -692,7 +692,7 @@ static int parse_setup_node_list(void)int i;for (i =0 ; i < mul; i++) { -if (t >= g->p.nr_tasks) { +if (t >= g->p.nr_tasks || !node_has_cpus (bind_node)) {printf ("\n# NOTE: ignoring bind NODEs starting at NODE#%d\n" , bind_node); goto out; }
@@ -973,6 +973,7 @@ static void calc_convergence(double runtime_ns_max, double *convergence)int node;int cpu;int t; +int processes;if (!g->p.show_convergence && !g->p.measure_convergence)return ;
@@ -1007,13 +1008,14 @@ static void calc_convergence(double runtime_ns_max, double *convergence) sum =0 ;for (node =0 ; node < g->p.nr_nodes; node++) { +if (!is_node_present (node)) +continue ; nr = nodes[node]; nr_min =min (nr, nr_min); nr_max =max (nr, nr_max); sum += nr; }BUG_ON (nr_min > nr_max); -BUG_ON (sum > g->p.nr_tasks);if (0 && (sum < g->p.nr_tasks))
@@ -1027,8 +1029,9 @@ static void calc_convergence(double runtime_ns_max, double *convergence) process_groups =0 ;for (node =0 ; node < g->p.nr_nodes; node++) { -int processes =count_node_processes (node); - +if (!is_node_present (node)) +continue ; + processes =count_node_processes (node); nr = nodes[node];tprintf (" %2d/%-2d" , nr, processes);
@@ -1334,7 +1337,7 @@ static void print_summary(void)printf ("\n ###\n" );printf (" # %d %s will execute (on %d nodes, %d CPUs):\n" , - g->p.nr_tasks, g->p.nr_tasks ==1 ?"task" :"tasks" , g->p.nr_nodes, g->p.nr_cpus); + g->p.nr_tasks, g->p.nr_tasks ==1 ?"task" :"tasks" ,nr_numa_nodes (), g->p.nr_cpus);printf (" # %5dx %5ldMB global shared mem operations\n" , g->p.nr_loops, g->p.bytes_global/1024 /1024 ); printf(" # %5dx %5ldMB process shared mem operations\n",
Signature · 1 line
2.7.4
Re: [PATCH v3 2/2] perf/bench/numa: Handle discontiguous/sparse numa nodes
Quoted 54 lines · On 2017/08/21 10:17AM, [email protected] wrote:
From: Satheesh Rajendran <[email protected]>
Certain systems are designed to have sparse/discontiguous nodes. On such systems, perf bench numa hangs, shows wrong number of nodes and shows values for non-existent nodes. Handle this by only taking nodes that are exposed by kernel to userspace.
Cc Arnaldo Carvalho de Melo <[email protected]>Reviewed-by Srikar Dronamraju <[email protected]>Signed-off-by Satheesh Rajendran <[email protected]>Signed-off-by Balamuruhan S <[email protected]>--- tools/perf/bench/numa.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)tools/perf/bench/numa.c +4
-3@@ -287,12 +287,12 @@ static cpu_set_t bind_to_cpu(int target_cpu)static cpu_set_t bind_to_node (int target_node) { -int cpus_per_node = g->p.nr_cpus/g->p.nr_nodes; +int cpus_per_node = g->p.nr_cpus/nr_numa_nodes ();cpu_set_t orig_mask, mask;int cpu;int ret; -BUG_ON (cpus_per_node*g->p.nr_nodes != g->p.nr_cpus); +BUG_ON (cpus_per_node*nr_numa_nodes () != g->p.nr_cpus);BUG_ON (!cpus_per_node); ret =sched_getaffinity (0 ,sizeof (orig_mask), &orig_mask);@@ -692,7 +692,7 @@ static int parse_setup_node_list(void)int i;for (i =0 ; i < mul; i++) { -if (t >= g->p.nr_tasks) { +if (t >= g->p.nr_tasks || !node_has_cpus (bind_node)) {printf ("\n# NOTE: ignoring bind NODEs starting at NODE#%d\n" , bind_node); goto out; }@@ -973,6 +973,7 @@ static void calc_convergence(double runtime_ns_max, double *convergence)int node;int cpu;int t; +int processes;if (!g->p.show_convergence && !g->p.measure_convergence)return ;@@ -1007,13 +1008,14 @@ static void calc_convergence(double runtime_ns_max, double *convergence) sum =0 ;
tools/perf/bench/numa.c static void calc_convergence(double runtime_ns_max, double *convergence) · line 1016
Quoted 10 lines
tools/perf/bench/numa.c +2
-1for (node =0 ; node < g->p.nr_nodes; node++) { +if (!is_node_present (node)) +continue ; nr = nodes[node]; nr_min =min (nr, nr_min); nr_max =max (nr, nr_max); sum += nr; }BUG_ON (nr_min > nr_max); -
Looks like an un-necessary change there.
- Naveen
Quoted 32 lines
tools/perf/bench/numa.c +4
-3BUG_ON (sum > g->p.nr_tasks);if (0 && (sum < g->p.nr_tasks))@@ -1027,8 +1029,9 @@ static void calc_convergence(double runtime_ns_max, double *convergence) process_groups =0 ;for (node =0 ; node < g->p.nr_nodes; node++) { -int processes =count_node_processes (node); - +if (!is_node_present (node)) +continue ; + processes =count_node_processes (node); nr = nodes[node];tprintf (" %2d/%-2d" , nr, processes);@@ -1334,7 +1337,7 @@ static void print_summary(void)printf ("\n ###\n" );printf (" # %d %s will execute (on %d nodes, %d CPUs):\n" , - g->p.nr_tasks, g->p.nr_tasks ==1 ?"task" :"tasks" , g->p.nr_nodes, g->p.nr_cpus); + g->p.nr_tasks, g->p.nr_tasks ==1 ?"task" :"tasks" ,nr_numa_nodes (), g->p.nr_cpus);printf (" # %5dx %5ldMB global shared mem operations\n" , g->p.nr_loops, g->p.bytes_global/1024 /1024 ); printf(" # %5dx %5ldMB process shared mem operations\n",Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Arnaldo Carvalho de Melo
maintainerRe: [PATCH v3 2/2] perf/bench/numa: Handle discontiguous/sparse numa nodes
Em Tue, Oct 31, 2017 at 08:46:58PM +0530, Naveen N. Rao escreveu:
Quoted 55 lines · On 2017/08/21 10:17AM, [email protected] wrote:
Quoted 54 lines
From: Satheesh Rajendran <[email protected]>
Certain systems are designed to have sparse/discontiguous nodes. On such systems, perf bench numa hangs, shows wrong number of nodes and shows values for non-existent nodes. Handle this by only taking nodes that are exposed by kernel to userspace.
Cc Arnaldo Carvalho de Melo <[email protected]>Reviewed-by Srikar Dronamraju <[email protected]>Signed-off-by Satheesh Rajendran <[email protected]>Signed-off-by Balamuruhan S <[email protected]>--- tools/perf/bench/numa.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)tools/perf/bench/numa.c +4
-3@@ -287,12 +287,12 @@ static cpu_set_t bind_to_cpu(int target_cpu)static cpu_set_t bind_to_node (int target_node) { -int cpus_per_node = g->p.nr_cpus/g->p.nr_nodes; +int cpus_per_node = g->p.nr_cpus/nr_numa_nodes ();cpu_set_t orig_mask, mask;int cpu;int ret; -BUG_ON (cpus_per_node*g->p.nr_nodes != g->p.nr_cpus); +BUG_ON (cpus_per_node*nr_numa_nodes () != g->p.nr_cpus);BUG_ON (!cpus_per_node); ret =sched_getaffinity (0 ,sizeof (orig_mask), &orig_mask);@@ -692,7 +692,7 @@ static int parse_setup_node_list(void)int i;for (i =0 ; i < mul; i++) { -if (t >= g->p.nr_tasks) { +if (t >= g->p.nr_tasks || !node_has_cpus (bind_node)) {printf ("\n# NOTE: ignoring bind NODEs starting at NODE#%d\n" , bind_node); goto out; }@@ -973,6 +973,7 @@ static void calc_convergence(double runtime_ns_max, double *convergence)int node;int cpu;int t; +int processes;if (!g->p.show_convergence && !g->p.measure_convergence)return ;@@ -1007,13 +1008,14 @@ static void calc_convergence(double runtime_ns_max, double *convergence) sum =0 ;
Quoted 12 lines
tools/perf/bench/numa.c static void calc_convergence(double runtime_ns_max, double *convergence) · line 1016
Quoted 10 lines
tools/perf/bench/numa.c +2
-1for (node =0 ; node < g->p.nr_nodes; node++) { +if (!is_node_present (node)) +continue ; nr = nodes[node]; nr_min =min (nr, nr_min); nr_max =max (nr, nr_max); sum += nr; }BUG_ON (nr_min > nr_max); -Looks like an un-necessary change there.
Right, and I would leave the 'int processes' declaration where it is, as it is not used outside that loop.
The move of that declaration to the top of the calc_convergence() function made me spend some cycles trying to figure out why that was done, only to realize that it was an unnecessary change :-\
Quoted 26 lines
- Naveen
Quoted 24 lines
tools/perf/bench/numa.c +4
-3BUG_ON (sum > g->p.nr_tasks);if (0 && (sum < g->p.nr_tasks))@@ -1027,8 +1029,9 @@ static void calc_convergence(double runtime_ns_max, double *convergence) process_groups =0 ;for (node =0 ; node < g->p.nr_nodes; node++) { -int processes =count_node_processes (node); - +if (!is_node_present (node)) +continue ; + processes =count_node_processes (node); nr = nodes[node];tprintf (" %2d/%-2d" , nr, processes);@@ -1334,7 +1337,7 @@ static void print_summary(void)printf ("\n ###\n" );printf (" # %d %s will execute (on %d nodes, %d CPUs):\n" , - g->p.nr_tasks, g->p.nr_tasks ==1 ?"task" :"tasks" , g->p.nr_nodes, g->p.nr_cpus); + g->p.nr_tasks, g->p.nr_tasks ==1 ?"task" :"tasks" ,nr_numa_nodes (), g->p.nr_cpus);printf (" # %5dx %5ldMB global shared mem operations\n" , g->p.nr_loops, g->p.bytes_global/1024 /1024 ); printf(" # %5dx %5ldMB process shared mem operations\n",
Re: [PATCH v3 2/2] perf/bench/numa: Handle discontiguous/sparse numa nodes
Hi Arnaldo,Please find my reply inline.
Quoted 76 lines · Arnaldo Carvalho de Melo
Em Tue, Oct 31, 2017 at 08:46:58PM +0530, Naveen N. Rao escreveu:
Quoted 64 lines · On 2017/08/21 10:17AM, [email protected] wrote:
Quoted 62 lines
From: Satheesh Rajendran <[email protected]>
Certain systems are designed to have sparse/discontiguous nodes. On such systems, perf bench numa hangs, shows wrong number of nodes
and shows values for non-existent nodes. Handle this by only taking nodes that are exposed by kernel to userspace.Cc Arnaldo Carvalho de Melo <[email protected]>Reviewed-by Srikar Dronamraju <[email protected]>Signed-off-by Satheesh Rajendran <[email protected]>Signed-off-by Balamuruhan S <[email protected]>---
tools/perf/bench/numa.c | 17 ++++++++++-------1 file changed, 10 insertions(+), 7 deletions(-)tools/perf/bench/numa.c +2
-2@@ -287,12 +287,12 @@ static cpu_set_t bind_to_cpu(int target_cpu) static cpu_set_t bind_to_node(int target_node) { -int cpus_per_node = g->p.nr_cpus/g->p.nr_nodes; +int cpus_per_node = g->p.nr_cpus/nr_numa_nodes (); cpu_set_t orig_mask, mask; int cpu; int ret; -BUG_ON (cpus_per_node*g->p.nr_nodes != g->p.nr_cpus); +BUG_ON (cpus_per_node*nr_numa_nodes () != g->p.nr_cpus); BUG_ON(!cpus_per_node);ret =
sched_getaffinity(0, sizeof(orig_mask), &orig_mask);tools/perf/bench/numa.c +1
-1@@ -692,7 +692,7 @@ static int parse_setup_node_list(void) int i; for (i = 0; i < mul; i++) { -if (t >= g->p.nr_tasks) { +if (t >= g->p.nr_tasks || !node_has_cpus (bind_node)) {printf("\n# NOTE:
ignoring bind NODEs starting at NODE#%d\n",bind_node); goto out; }tools/perf/bench/numa.c +1
-0@@ -973,6 +973,7 @@ static void calc_convergence(double runtime_ns_max, double *convergence) int node; int cpu; int t; +int processes; if (!g->p.show_convergence && !g->p.measure_convergence)return;
tools/perf/bench/numa.c +0
-0@@ -1007,13 +1008,14 @@ static void calc_convergence(double runtime_ns_max, double *convergence) sum = 0;Quoted 11 lines
tools/perf/bench/numa.c static void calc_convergence(double runtime_ns_max, double *convergence) · line 1016
Quoted 10 lines
tools/perf/bench/numa.c +2
-1for (node = 0; node < g->p.nr_nodes; node++) { +if (!is_node_present (node)) +continue ; nr = nodes[node]; nr_min = min(nr, nr_min); nr_max = max(nr, nr_max); sum += nr; } BUG_ON(nr_min > nr_max); -Looks like an un-necessary change there.
Quoted 4 lines
Right, and I would leave the 'int processes' declaration where it is, as
it is not used outside that loop.
I had hit with this compilation error, so had to move the initialization above.
CC bench/numa.obench/numa.c: In function ‘calc_convergence’:bench/numa.c:1035:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
int processes = count_node_processes(node);
^
cc1: all warnings being treated as errors
mv: cannot stat ‘bench/.numa.o.tmp’: No such file or directory make[4]: *** [bench/numa.o] Error 1
make[3]: *** [bench] Error 2
make[2]: *** [perf-in.o] Error 2
make[1]: *** [sub-make] Error 2
make: *** [all] Error 2
Quoted 4 lines
The move of that declaration to the top of the
calc_convergence()function made me spend some cycles trying to figure out why that was done, only to realize that it was an unnecessary change :-\
Agree, I would have kept it in the same scope, will keep as below,
@@ -984,8 +1026,11 @@ static void calc_convergence(double runtime_ns_max, double *convergence) process_groups = 0;
for (node = 0; node < g->p.nr_nodes; node++) {
- int processes = count_node_processes(node);
+ int processes;
+ if (!is_node_present(node))
+ continue;
+ processes = count_node_processes(node);
nr = nodes[node];
tprintf(" %2d/%-2d", nr, processes);
Please advice. Thanks.
Regards,
-Satheesh.
Quoted 35 lines
Quoted 35 lines
- Naveen
Quoted 32 lines
tools/perf/bench/numa.c +0
-0BUG_ON(sum > g->p.nr_tasks);if (0 && (sum <
g->p.nr_tasks))tools/perf/bench/numa.c +3
-2@@ -1027,8 +1029,9 @@ static void calc_convergence(double runtime_ns_max, double *convergence) process_groups = 0; for (node = 0; node < g->p.nr_nodes; node++) { -int processes =count_node_processes (node); - +if (!is_node_present (node)) +continue ; + processes =count_node_processes (node); nr = nodes[node]; tprintf(" %2d/%-2d", nr, processes);@@ -1334,7 +1337,7 @@ static void print_summary(void) printf("\n ###\n");printf(" # %d %s will execute (on %d nodes, %d CPUs):\n",
tools/perf/bench/numa.c +0
-1- g->p.nr_tasks, g->p.nr_tasks == 1 ? "task" :"tasks",
g->p.nr_nodes,g->p.nr_cpus);tools/perf/bench/numa.c +1
-0+ g->p.nr_tasks, g->p.nr_tasks == 1 ? "task" :"tasks",
nr_numa_nodes(),g->p.nr_cpus); printf(" # %5dx %5ldMB global shared mem operations\n",
g->p.nr_loops, g-Quoted 1 line
p.bytes_global/1024/1024);
printf(" # %5dx %5ldMB process shared mem operations\n",
Re: [PATCH v3 2/2] perf/bench/numa: Handle discontiguous/sparse numa nodes
In reply to Naveen N. RaoHi Naveen,
Quoted 8 lines · Naveen N. Rao
Quoted 3 lines
}
BUG_ON(nr_min > nr_max);
-Looks like an un-necessary change there.
- Naveen
I had hit with this compilation error, so had to move the initialization above.
Please advice. Thanks.
CC bench/numa.obench/numa.c: In function ‘calc_convergence’:bench/numa.c:1035:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
int processes = count_node_processes(node);
^
cc1: all warnings being treated as errors
mv: cannot stat ‘bench/.numa.o.tmp’: No such file or directory make[4]: *** [bench/numa.o] Error 1
make[3]: *** [bench] Error 2
make[2]: *** [perf-in.o] Error 2
make[1]: *** [sub-make] Error 2
make: *** [all] Error 2
Regards,
-Satheesh.
Quoted 4 lines
Quoted 4 lines
BUG_ON(sum > g->p.nr_tasks);if (0 && (sum <
g->p.nr_tasks))
@@ -1027,8 +1029,9 @@ static void calc_convergence(double
Re: [PATCH v3 2/2] perf/bench/numa: Handle discontiguous/sparse numa nodes
Hi Satheesh,
Quoted 8 lines · Satheesh Rajendran
Hi Naveen,
Quoted 4 lines · Naveen N. Rao
Quoted 3 lines
}
BUG_ON(nr_min > nr_max);
-
Quoted 6 lines
Quoted 4 lines
Looks like an un-necessary change there.
- Naveen
I had hit with this compilation error, so had to move the initialization above.
I suppose you intended to reply to Arnaldo's comment about the declaration being moved, since my comment above was about a blank line being deleted in your patch. It's preferable to not do any un-related changes/cleanups in a patch.
Quoted 8 lines
Please advice. Thanks.
CC bench/numa.o
bench/numa.c: In function ‘calc_convergence’:bench/numa.c:1035:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
int processes =count_node_processes(node);
^
Not sure what changes you made, but from the error message, I'm guessing
you placed the above statement _after_ the check for is_node_present(). What Arnaldo recommended is to retain the processes declaration within the scope where it's used - in this case, the for() loop - rather than moving it out to the start of the function.
- Naveen
Quoted 16 lines
cc1: all warnings being treated as errors
mv: cannot stat ‘bench/.numa.o.tmp’: No such file or directory make[4]: *** [bench/numa.o] Error 1
make[3]: *** [bench] Error 2
make[2]: *** [perf-in.o] Error 2
make[1]: *** [sub-make] Error 2
make: *** [all] Error 2Regards,
-Satheesh.Quoted 4 lines
Quoted 4 lines
BUG_ON(sum > g->p.nr_tasks);if (0 && (sum <
g->p.nr_tasks))(quoted diff) +0
-0@@ -1027,8 +1029,9 @@ static void calc_convergence(double
Hi Arnaldo,
Please let me know if any further comments.
Thanks in advance :-)
Regards,
Quoted 93 lines · -Satheesh.On Mon, 2017-08-21 at 15:45 +0530, [email protected] wrote:
From: Satheesh Rajendran <[email protected]>
Certain systems would have sparse/discontinguous numa nodes.
perf bench numa doesnt work well on such nodes.
1. It shows wrong values.
2. It can hang.
3. It can show redundant information for non-existant nodes.#numactl -H
available: 2 nodes (0,8)
node 0 cpus: 0 1 2 3 4 5 6 7
node 0 size: 61352 MB
node 0 free: 57168 MB
node 8 cpus: 8 9 10 11 12 13 14 15
node 8 size: 65416 MB
node 8 free: 36593 MB
node distances:
node 0 8
0: 10 40
8: 40 10Scenario 1:
Before Fix:
# perf bench numa mem --no-data_rand_walk -p 2 -t 20 -G 0 -P 3072 -T 0 -l 50 -c -s 1000 ... ... # 40 tasks will execute (on 9 nodes, 16 CPUs): ----> Wrong number of nodes ... # 2.0% [0.2 mins] 1/1 0/0 0/0 0/0 0/0 0/0 0/0 0/0 4/1 [ 4/2 ] l: 0-0 ( 0) ----> Shows info on non-existant nodes.After Fix:
# ./perf bench numa mem --no-data_rand_walk -p 2 -t 20 -G 0 -P 3072 -T 0 -l 50 -c -s 1000 ... ... # 40 tasks will execute (on 2 nodes, 16 CPUs): ... # 2.0% [0.2 mins] 9/1 0/0 [ 9/1 ] l: 0-0 ( 0) # 4.0% [0.4 mins] 21/2 19/1 [ 2/3 ] l: 0-1 ( 1) {1-2}Scenario 2:
Before Fix:
# perf bench numa all # Running numa/mem benchmark... .... ... # Running RAM-bw-remote, "perf bench numa mem -p 1 -t 1 -P 1024 -C 0 -M 1 -s 20 -zZq --thp 1 --no-data_rand_walk" perf: bench/numa.c:306: bind_to_memnode: Assertion `!(ret)' failed. ------------> Got hungAfter Fix:
# ./perf bench numa all # Running numa/mem benchmark... .... ... # Running RAM-bw-remote, "perf bench numa mem -p 1 -t 1 -P 1024 -C 0 -M 1 -s 20 -zZq --thp 1 --no-data_rand_walk"# NOTE: ignoring bind NODEs starting at NODE#1 # NOTE: 0 tasks mem-bound, 1 tasks unbound 20.017 secs slowest (max) thread-runtime 20.000 secs fastest (min) thread-runtime 20.006 secs average thread-runtime 0.043 % difference between max/avg runtime 413.794 GB data processed, per thread 413.794 GB data processed, total 0.048 nsecs/byte/thread runtime 20.672 GB/sec/thread speed
20.672 GB/sec total speedChanges in v2:
Fixed review comments for function names and alloc failure handleChanges in v3:
Coding Style fixes.Satheesh Rajendran (2):
perf/bench/numa: Add functions to detect sparse numa nodes perf/bench/numa: Handle discontiguous/sparse numa nodestools/perf/bench/numa.c | 61
+++++++++++++++++++++++++++++++++++++++++++------1 file changed, 54 insertions(+), 7 deletions(-)