[PATCH 1/2] perf/bench/numa: Add functions to detect sparse numa nodes

[PATCH 1/2] perf/bench/numa: Add functions to detect sparse numa nodes

[sathnaga]

From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>

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.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
---
 tools/perf/bench/numa.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/tools/perf/bench/numa.c b/tools/perf/bench/numa.c
index 469d65b..efd7595 100644
--- a/tools/perf/bench/numa.c
+++ b/tools/perf/bench/numa.c
@@ -215,6 +215,41 @@ static const char * const numa_usage[] = {
 	NULL
 };
 
+static int nr_numa_nodes(void)
+{
+	int node = 0, i;
+
+        for (i = 0; i < g->p.nr_nodes; i++) {
+		if (numa_bitmask_isbitset(numa_nodes_ptr, i))
+			node++;
+	}
+	return node;
+}
+
+static bool is_node_present(int node)
+{
+	if (numa_bitmask_isbitset(numa_nodes_ptr, node))
+		return true;
+	else
+		return false;
+}
+
+static bool is_node_hascpu(int node)
+{
+	struct bitmask *cpu;
+	unsigned int i;
+
+	cpu = numa_allocate_cpumask();
+	if (numa_node_to_cpus(node, cpu) == 0) {
+		for (i = 0; i < cpu->size; i++) {
+			if (numa_bitmask_isbitset(cpu, i))
+				return true;
+			}
+	} else
+		return false; // lets fall back to nocpus safely
+	return false;
+}
+
 static cpu_set_t bind_to_cpu(int target_cpu)
 {
 	cpu_set_t orig_mask, mask;
-- 
2.7.4

Re: [PATCH 1/2] perf/bench/numa: Add functions to detect sparse numa nodes

[Arnaldo Carvalho de Melo]

Em Thu, Aug 10, 2017 at 12:58:49PM +0530, sathnaga@linux.vnet.ibm.com escreveu:
> From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> 
> 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.
> 
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> ---
>  tools/perf/bench/numa.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/tools/perf/bench/numa.c b/tools/perf/bench/numa.c
> index 469d65b..efd7595 100644
> --- a/tools/perf/bench/numa.c
> +++ b/tools/perf/bench/numa.c
> @@ -215,6 +215,41 @@ static const char * const numa_usage[] = {
>  	NULL
>  };
>  
> +static int nr_numa_nodes(void)
> +{
> +	int node = 0, i;
> +
> +        for (i = 0; i < g->p.nr_nodes; i++) {
> +		if (numa_bitmask_isbitset(numa_nodes_ptr, i))
> +			node++;
> +	}
> +	return node;

Humm, can you rename 'node' to 'nr_nodes'?

> +}
> +
> +static bool is_node_present(int node)
> +{
> +	if (numa_bitmask_isbitset(numa_nodes_ptr, node))
> +		return true;
> +	else
> +		return false;
> +}

Why four lines instead of just one? Isn't this equivalent:

static bool is_node_present(int node)
{
	return numa_bitmask_isbitset(numa_nodes_ptr, node);
}

?

> +
> +static bool is_node_hascpu(int node)

Can you rename this function, the name is confusing :-\

Based on the documentation for this function, that you left only in the
changelog (please put it just before the function, as a comment, I think
it should be named node_has_cpus()?

> +{
> +	struct bitmask *cpu;
> +	unsigned int i;
> +
> +	cpu = numa_allocate_cpumask();

Please put the line with the initialization together with the
declaration, making it:

struct bitmask *cpu = numa_allocate_cpumask();

Also, this is a "alloc" function, I bet it can fail? If so, check it and
return something useful if it fails, which probably will be difficult
since this function returns bool?


> +	if (numa_node_to_cpus(node, cpu) == 0) {
> +		for (i = 0; i < cpu->size; i++) {
> +			if (numa_bitmask_isbitset(cpu, i))
> +				return true;
> +			}
> +	} else
> +		return false; // lets fall back to nocpus safely
> +	return false;
> +}
> +
>  static cpu_set_t bind_to_cpu(int target_cpu)
>  {
>  	cpu_set_t orig_mask, mask;
> -- 
> 2.7.4

Re: [PATCH 1/2] perf/bench/numa: Add functions to detect sparse numa nodes

[Satheesh Rajendran]

Thanks Arnaldo for the detailed review :-)
Will address them and send across v2. On Thu, 2017-08-10 at 16:22 -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Aug 10, 2017 at 12:58:49PM +0530, sathnaga@linux.vnet.ibm.com
>  escreveu:
> > 
> > From: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > 
> > 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.
> > 
> > Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > Signed-off-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> > ---
> >  tools/perf/bench/numa.c | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> > 
> > diff --git a/tools/perf/bench/numa.c b/tools/perf/bench/numa.c
> > index 469d65b..efd7595 100644
> > --- a/tools/perf/bench/numa.c
> > +++ b/tools/perf/bench/numa.c
> > @@ -215,6 +215,41 @@ static const char * const numa_usage[] = {
> >  	NULL
> >  };
> >  
> > +static int nr_numa_nodes(void)
> > +{
> > +	int node = 0, i;
> > +
> > +        for (i = 0; i < g->p.nr_nodes; i++) {
> > +		if (numa_bitmask_isbitset(numa_nodes_ptr, i))
> > +			node++;
> > +	}
> > +	return node;
> Humm, can you rename 'node' to 'nr_nodes'?
> 
Sure, will Change it.
> > 
> > +}
> > +
> > +static bool is_node_present(int node)
> > +{
> > +	if (numa_bitmask_isbitset(numa_nodes_ptr, node))
> > +		return true;
> > +	else
> > +		return false;
> > +}
> Why four lines instead of just one? Isn't this equivalent:
> 
Sure.
> static bool is_node_present(int node)
> {
> 	return numa_bitmask_isbitset(numa_nodes_ptr, node);
> }
> 
> ?
> 
> > +
> > +static bool is_node_hascpu(int node)
> Can you rename this function, the name is confusing :-\
> 
> Based on the documentation for this function, that you left only in
> the
> changelog (please put it just before the function, as a comment, I
> think
> it should be named node_has_cpus()?
> 
make sense, will change it.
> > 
> > +{
> > +	struct bitmask *cpu;
> > +	unsigned int i;
> > +
> > +	cpu = numa_allocate_cpumask();
> Please put the line with the initialization together with the
> declaration, making it:
> 
> struct bitmask *cpu = numa_allocate_cpumask();
> 
> Also, this is a "alloc" function, I bet it can fail? If so, check it
> and
> return something useful if it fails, which probably will be difficult
> since this function returns bool?
> 
Sure, will check return false as failsafe.
> 
> > 
> > +	if (numa_node_to_cpus(node, cpu) == 0) {
> > +		for (i = 0; i < cpu->size; i++) {
> > +			if (numa_bitmask_isbitset(cpu, i))
> > +				return true;
> > +			}
> > +	} else
> > +		return false; // lets fall back to nocpus safely
> > +	return false;
> > +}
> > +
> >  static cpu_set_t bind_to_cpu(int target_cpu)
> >  {
> >  	cpu_set_t orig_mask, mask;