diff mbox series

[rt-tests,v2,v2,02/20] cyclictest: Use numa API directly

Message ID 20201218161843.1764-3-dwagner@suse.de
State New
Headers show
Series [rt-tests,v2,v2,01/20] cyclictest: Always use libnuma | expand

Commit Message

Daniel Wagner Dec. 18, 2020, 4:18 p.m. UTC 
There is no need for small libnuma wrappers functions as we always
use libnuma. Remove them and get rid of rt_numa.h, so there is
no confusion with rt-numa.h anymore.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/cyclictest/cyclictest.c | 41 ++++++++--------
 src/cyclictest/rt_numa.h    | 96 -------------------------------------
 2 files changed, 22 insertions(+), 115 deletions(-)
 delete mode 100644 src/cyclictest/rt_numa.h

Comments

John Kacur Jan. 26, 2021, 5:31 a.m. UTC | #1 
On Fri, 18 Dec 2020, Daniel Wagner wrote:

> There is no need for small libnuma wrappers functions as we always

> use libnuma. Remove them and get rid of rt_numa.h, so there is

> no confusion with rt-numa.h anymore.

> 

> Signed-off-by: Daniel Wagner <dwagner@suse.de>

> ---

>  src/cyclictest/cyclictest.c | 41 ++++++++--------

>  src/cyclictest/rt_numa.h    | 96 -------------------------------------

>  2 files changed, 22 insertions(+), 115 deletions(-)

>  delete mode 100644 src/cyclictest/rt_numa.h

> 

> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c

> index 514ed7b20fdb..a5ca764da254 100644

> --- a/src/cyclictest/cyclictest.c

> +++ b/src/cyclictest/cyclictest.c

> @@ -33,10 +33,10 @@

>  #include <sys/utsname.h>

>  #include <sys/mman.h>

>  #include <sys/syscall.h>

> -#include "rt_numa.h"

>  

>  #include "rt-utils.h"

>  #include "rt-numa.h"

> +#include "error.h"

>  

>  #include <bionic.h>

>  

> @@ -514,9 +514,9 @@ static void *timerthread(void *param)

>  

>  	memset(&stop, 0, sizeof(stop));

>  

> -	/* if we're running in numa mode, set our memory node */

> -	if (par->node != -1)

> -		rt_numa_set_numa_run_on_node(par->node, par->cpu);

> +	if (numa_run_on_node(par->node))

> +		warn("Could not set NUMA node %d for thread %d: %s\n",

> +				par->node, par->cpu, strerror(errno));

>  

>  	if (par->cpu != -1) {

>  		CPU_ZERO(&mask);

> @@ -1275,7 +1275,7 @@ static void process_options(int argc, char *argv[], int max_cpus)

>  	}

>  	if (error) {

>  		if (affinity_mask)

> -			rt_bitmask_free(affinity_mask);

> +			numa_bitmask_free(affinity_mask);

>  		display_help(1);

>  	}

>  }

> @@ -1929,7 +1929,7 @@ int main(int argc, char **argv)

>  		}

>  

>  		/* find the memory node associated with the cpu i */

> -		node = rt_numa_numa_node_of_cpu(cpu);

> +		node = numa_node_of_cpu(cpu);


You've eliminated the error handling that was in the wrapper here.

>  

>  		/* get the stack size set for this thread */

>  		if (pthread_attr_getstack(&attr, &currstk, &stksize))

> @@ -1940,7 +1940,10 @@ int main(int argc, char **argv)

>  			stksize = PTHREAD_STACK_MIN * 2;

>  

>  		/*  allocate memory for a stack on appropriate node */

> -		stack = rt_numa_numa_alloc_onnode(stksize, node, cpu);

> +		stack = numa_alloc_onnode(stksize, node);

> +		if (!stack)

> +			fatal("failed to allocate %d bytes on node %d for cpu %d\n",

> +				stksize, node, cpu);

>  

>  		/* touch the stack pages to pre-fault them in */

>  		memset(stack, 0, stksize);

> @@ -1951,13 +1954,13 @@ int main(int argc, char **argv)

>  				i, stack+stksize);

>  

>  		/* allocate the thread's parameter block  */

> -		parameters[i] = par = threadalloc(sizeof(struct thread_param), node);

> +		parameters[i] = par = numa_alloc_onnode(sizeof(struct thread_param), node);

>  		if (par == NULL)

>  			fatal("error allocating thread_param struct for thread %d\n", i);

>  		memset(par, 0, sizeof(struct thread_param));

>  

>  		/* allocate the thread's statistics block */

> -		statistics[i] = stat = threadalloc(sizeof(struct thread_stat), node);

> +		statistics[i] = stat = numa_alloc_onnode(sizeof(struct thread_stat), node);

>  		if (stat == NULL)

>  			fatal("error allocating thread status struct for thread %d\n", i);

>  		memset(stat, 0, sizeof(struct thread_stat));

> @@ -1966,8 +1969,8 @@ int main(int argc, char **argv)

>  		if (histogram) {

>  			int bufsize = histogram * sizeof(long);

>  

> -			stat->hist_array = threadalloc(bufsize, node);

> -			stat->outliers = threadalloc(bufsize, node);

> +			stat->hist_array = numa_alloc_onnode(bufsize, node);

> +			stat->outliers = numa_alloc_onnode(bufsize, node);

>  			if (stat->hist_array == NULL || stat->outliers == NULL)

>  				fatal("failed to allocate histogram of size %d on node %d\n",

>  				      histogram, i);

> @@ -1977,14 +1980,14 @@ int main(int argc, char **argv)

>  

>  		if (verbose) {

>  			int bufsize = VALBUF_SIZE * sizeof(long);

> -			stat->values = threadalloc(bufsize, node);

> +			stat->values = numa_alloc_onnode(bufsize, node);

>  			if (!stat->values)

>  				goto outall;

>  			memset(stat->values, 0, bufsize);

>  			par->bufmsk = VALBUF_SIZE - 1;

>  			if (smi) {

>  				int bufsize = VALBUF_SIZE * sizeof(long);

> -				stat->smis = threadalloc(bufsize, node);

> +				stat->smis = numa_alloc_onnode(bufsize, node);

>  				if (!stat->smis)

>  					goto outall;

>  				memset(stat->smis, 0, bufsize);

> @@ -2099,7 +2102,7 @@ int main(int argc, char **argv)

>  				print_stat(stdout, parameters[i], i, 0, 0);

>  		}

>  		if (statistics[i]->values)

> -			threadfree(statistics[i]->values, VALBUF_SIZE*sizeof(long), parameters[i]->node);

> +			numa_free(statistics[i]->values, VALBUF_SIZE*sizeof(long));

>  	}

>  

>  	if (trigger)

> @@ -2108,8 +2111,8 @@ int main(int argc, char **argv)

>  	if (histogram) {

>  		print_hist(parameters, num_threads);

>  		for (i = 0; i < num_threads; i++) {

> -			threadfree(statistics[i]->hist_array, histogram*sizeof(long), parameters[i]->node);

> -			threadfree(statistics[i]->outliers, histogram*sizeof(long), parameters[i]->node);

> +			numa_free(statistics[i]->hist_array, histogram*sizeof(long));

> +			numa_free(statistics[i]->outliers, histogram*sizeof(long));

>  		}

>  	}

>  

> @@ -2125,14 +2128,14 @@ int main(int argc, char **argv)

>  	for (i=0; i < num_threads; i++) {

>  		if (!statistics[i])

>  			continue;

> -		threadfree(statistics[i], sizeof(struct thread_stat), parameters[i]->node);

> +		numa_free(statistics[i], sizeof(struct thread_stat));

>  	}

>  

>   outpar:

>  	for (i = 0; i < num_threads; i++) {

>  		if (!parameters[i])

>  			continue;

> -		threadfree(parameters[i], sizeof(struct thread_param), parameters[i]->node);

> +		numa_free(parameters[i], sizeof(struct thread_param));

>  	}

>   out:

>  	/* close any tracer file descriptors */

> @@ -2147,7 +2150,7 @@ int main(int argc, char **argv)

>  		close(latency_target_fd);

>  

>  	if (affinity_mask)

> -		rt_bitmask_free(affinity_mask);

> +		numa_bitmask_free(affinity_mask);

>  

>  	/* Remove running status shared memory file if it exists */

>  	if (rstat_fd >= 0)

> diff --git a/src/cyclictest/rt_numa.h b/src/cyclictest/rt_numa.h

> deleted file mode 100644

> index 8d02f419ed6d..000000000000

> --- a/src/cyclictest/rt_numa.h

> +++ /dev/null

> @@ -1,96 +0,0 @@

> -// SPDX-License-Identifier: GPL-2.0-or-later

> -/*

> - * A numa library for cyclictest.

> - *

> - * (C) 2010 John Kacur <jkacur@redhat.com>

> - * (C) 2010 Clark Williams <williams@redhat.com>

> - *

> - */

> -

> -#ifndef _RT_NUMA_H

> -#define _RT_NUMA_H

> -

> -#include "rt-utils.h"

> -#include "error.h"

> -

> -#include <numa.h>

> -

> -static void *

> -threadalloc(size_t size, int node)

> -{

> -	if (node == -1)

> -		return malloc(size);

> -	return numa_alloc_onnode(size, node);

> -}

> -

> -static void

> -threadfree(void *ptr, size_t size, int node)

> -{

> -	if (node == -1)

> -		free(ptr);

> -	else

> -		numa_free(ptr, size);

> -}

> -

> -static void rt_numa_set_numa_run_on_node(int node, int cpu)

> -{

> -	int res;

> -	res = numa_run_on_node(node);

> -	if (res)

> -		warn("Could not set NUMA node %d for thread %d: %s\n",

> -				node, cpu, strerror(errno));

> -	return;

> -}

> -

> -static void *rt_numa_numa_alloc_onnode(size_t size, int node, int cpu)

> -{

> -	void *stack;

> -	stack = numa_alloc_onnode(size, node);

> -	if (stack == NULL)

> -		fatal("failed to allocate %d bytes on node %d for cpu %d\n",

> -				size, node, cpu);

> -	return stack;

> -}

> -

> -/*

> - * Use new bit mask CPU affinity behavior

> - */

> -static int rt_numa_numa_node_of_cpu(int cpu)

> -{

> -	int node;

> -	node = numa_node_of_cpu(cpu);

> -	if (node == -1)

> -		fatal("invalid cpu passed to numa_node_of_cpu(%d)\n", cpu);

> -	return node;

> -}

> -

> -static inline unsigned int rt_numa_bitmask_isbitset( const struct bitmask *mask,

> -	unsigned long i)

> -{

> -	return numa_bitmask_isbitset(mask,i);

> -}

> -

> -static inline struct bitmask* rt_numa_parse_cpustring(const char* s,

> -	int max_cpus)

> -{

> -	return numa_parse_cpustring_all(s);

> -}

> -

> -static inline void rt_bitmask_free(struct bitmask *mask)

> -{

> -	numa_bitmask_free(mask);

> -}

> -

> -/** Returns number of bits set in mask. */

> -static inline unsigned int rt_numa_bitmask_count(const struct bitmask *mask)

> -{

> -	unsigned int num_bits = 0, i;

> -	for (i = 0; i < mask->size; i++) {

> -		if (rt_numa_bitmask_isbitset(mask, i))

> -			num_bits++;

> -	}

> -	/* Could stash this instead of recomputing every time. */

> -	return num_bits;

> -}

> -

> -#endif	/* _RT_NUMA_H */

> -- 

> 2.29.2

> 

>
Daniel Wagner Jan. 26, 2021, 8:11 a.m. UTC | #2 
On Tue, Jan 26, 2021 at 12:31:37AM -0500, John Kacur wrote:
> > @@ -1929,7 +1929,7 @@ int main(int argc, char **argv)

> >  		}

> >  

> >  		/* find the memory node associated with the cpu i */

> > -		node = rt_numa_numa_node_of_cpu(cpu);

> > +		node = numa_node_of_cpu(cpu);

> 

> You've eliminated the error handling that was in the wrapper here.


I was under the impression that cpu will always be a valid. You are
right, we can't be sure of that, because we only warn if the
affinity_mask is not correct (should this be a error?) and things could
change until we reach this point. I'll add the error handling back.
diff mbox series

Patch

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 514ed7b20fdb..a5ca764da254 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -33,10 +33,10 @@ 
 #include <sys/utsname.h>
 #include <sys/mman.h>
 #include <sys/syscall.h>
-#include "rt_numa.h"
 
 #include "rt-utils.h"
 #include "rt-numa.h"
+#include "error.h"
 
 #include <bionic.h>
 
@@ -514,9 +514,9 @@  static void *timerthread(void *param)
 
 	memset(&stop, 0, sizeof(stop));
 
-	/* if we're running in numa mode, set our memory node */
-	if (par->node != -1)
-		rt_numa_set_numa_run_on_node(par->node, par->cpu);
+	if (numa_run_on_node(par->node))
+		warn("Could not set NUMA node %d for thread %d: %s\n",
+				par->node, par->cpu, strerror(errno));
 
 	if (par->cpu != -1) {
 		CPU_ZERO(&mask);
@@ -1275,7 +1275,7 @@  static void process_options(int argc, char *argv[], int max_cpus)
 	}
 	if (error) {
 		if (affinity_mask)
-			rt_bitmask_free(affinity_mask);
+			numa_bitmask_free(affinity_mask);
 		display_help(1);
 	}
 }
@@ -1929,7 +1929,7 @@  int main(int argc, char **argv)
 		}
 
 		/* find the memory node associated with the cpu i */
-		node = rt_numa_numa_node_of_cpu(cpu);
+		node = numa_node_of_cpu(cpu);
 
 		/* get the stack size set for this thread */
 		if (pthread_attr_getstack(&attr, &currstk, &stksize))
@@ -1940,7 +1940,10 @@  int main(int argc, char **argv)
 			stksize = PTHREAD_STACK_MIN * 2;
 
 		/*  allocate memory for a stack on appropriate node */
-		stack = rt_numa_numa_alloc_onnode(stksize, node, cpu);
+		stack = numa_alloc_onnode(stksize, node);
+		if (!stack)
+			fatal("failed to allocate %d bytes on node %d for cpu %d\n",
+				stksize, node, cpu);
 
 		/* touch the stack pages to pre-fault them in */
 		memset(stack, 0, stksize);
@@ -1951,13 +1954,13 @@  int main(int argc, char **argv)
 				i, stack+stksize);
 
 		/* allocate the thread's parameter block  */
-		parameters[i] = par = threadalloc(sizeof(struct thread_param), node);
+		parameters[i] = par = numa_alloc_onnode(sizeof(struct thread_param), node);
 		if (par == NULL)
 			fatal("error allocating thread_param struct for thread %d\n", i);
 		memset(par, 0, sizeof(struct thread_param));
 
 		/* allocate the thread's statistics block */
-		statistics[i] = stat = threadalloc(sizeof(struct thread_stat), node);
+		statistics[i] = stat = numa_alloc_onnode(sizeof(struct thread_stat), node);
 		if (stat == NULL)
 			fatal("error allocating thread status struct for thread %d\n", i);
 		memset(stat, 0, sizeof(struct thread_stat));
@@ -1966,8 +1969,8 @@  int main(int argc, char **argv)
 		if (histogram) {
 			int bufsize = histogram * sizeof(long);
 
-			stat->hist_array = threadalloc(bufsize, node);
-			stat->outliers = threadalloc(bufsize, node);
+			stat->hist_array = numa_alloc_onnode(bufsize, node);
+			stat->outliers = numa_alloc_onnode(bufsize, node);
 			if (stat->hist_array == NULL || stat->outliers == NULL)
 				fatal("failed to allocate histogram of size %d on node %d\n",
 				      histogram, i);
@@ -1977,14 +1980,14 @@  int main(int argc, char **argv)
 
 		if (verbose) {
 			int bufsize = VALBUF_SIZE * sizeof(long);
-			stat->values = threadalloc(bufsize, node);
+			stat->values = numa_alloc_onnode(bufsize, node);
 			if (!stat->values)
 				goto outall;
 			memset(stat->values, 0, bufsize);
 			par->bufmsk = VALBUF_SIZE - 1;
 			if (smi) {
 				int bufsize = VALBUF_SIZE * sizeof(long);
-				stat->smis = threadalloc(bufsize, node);
+				stat->smis = numa_alloc_onnode(bufsize, node);
 				if (!stat->smis)
 					goto outall;
 				memset(stat->smis, 0, bufsize);
@@ -2099,7 +2102,7 @@  int main(int argc, char **argv)
 				print_stat(stdout, parameters[i], i, 0, 0);
 		}
 		if (statistics[i]->values)
-			threadfree(statistics[i]->values, VALBUF_SIZE*sizeof(long), parameters[i]->node);
+			numa_free(statistics[i]->values, VALBUF_SIZE*sizeof(long));
 	}
 
 	if (trigger)
@@ -2108,8 +2111,8 @@  int main(int argc, char **argv)
 	if (histogram) {
 		print_hist(parameters, num_threads);
 		for (i = 0; i < num_threads; i++) {
-			threadfree(statistics[i]->hist_array, histogram*sizeof(long), parameters[i]->node);
-			threadfree(statistics[i]->outliers, histogram*sizeof(long), parameters[i]->node);
+			numa_free(statistics[i]->hist_array, histogram*sizeof(long));
+			numa_free(statistics[i]->outliers, histogram*sizeof(long));
 		}
 	}
 
@@ -2125,14 +2128,14 @@  int main(int argc, char **argv)
 	for (i=0; i < num_threads; i++) {
 		if (!statistics[i])
 			continue;
-		threadfree(statistics[i], sizeof(struct thread_stat), parameters[i]->node);
+		numa_free(statistics[i], sizeof(struct thread_stat));
 	}
 
  outpar:
 	for (i = 0; i < num_threads; i++) {
 		if (!parameters[i])
 			continue;
-		threadfree(parameters[i], sizeof(struct thread_param), parameters[i]->node);
+		numa_free(parameters[i], sizeof(struct thread_param));
 	}
  out:
 	/* close any tracer file descriptors */
@@ -2147,7 +2150,7 @@  int main(int argc, char **argv)
 		close(latency_target_fd);
 
 	if (affinity_mask)
-		rt_bitmask_free(affinity_mask);
+		numa_bitmask_free(affinity_mask);
 
 	/* Remove running status shared memory file if it exists */
 	if (rstat_fd >= 0)
diff --git a/src/cyclictest/rt_numa.h b/src/cyclictest/rt_numa.h
deleted file mode 100644
index 8d02f419ed6d..000000000000
--- a/src/cyclictest/rt_numa.h
+++ /dev/null
@@ -1,96 +0,0 @@ 
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * A numa library for cyclictest.
- *
- * (C) 2010 John Kacur <jkacur@redhat.com>
- * (C) 2010 Clark Williams <williams@redhat.com>
- *
- */
-
-#ifndef _RT_NUMA_H
-#define _RT_NUMA_H
-
-#include "rt-utils.h"
-#include "error.h"
-
-#include <numa.h>
-
-static void *
-threadalloc(size_t size, int node)
-{
-	if (node == -1)
-		return malloc(size);
-	return numa_alloc_onnode(size, node);
-}
-
-static void
-threadfree(void *ptr, size_t size, int node)
-{
-	if (node == -1)
-		free(ptr);
-	else
-		numa_free(ptr, size);
-}
-
-static void rt_numa_set_numa_run_on_node(int node, int cpu)
-{
-	int res;
-	res = numa_run_on_node(node);
-	if (res)
-		warn("Could not set NUMA node %d for thread %d: %s\n",
-				node, cpu, strerror(errno));
-	return;
-}
-
-static void *rt_numa_numa_alloc_onnode(size_t size, int node, int cpu)
-{
-	void *stack;
-	stack = numa_alloc_onnode(size, node);
-	if (stack == NULL)
-		fatal("failed to allocate %d bytes on node %d for cpu %d\n",
-				size, node, cpu);
-	return stack;
-}
-
-/*
- * Use new bit mask CPU affinity behavior
- */
-static int rt_numa_numa_node_of_cpu(int cpu)
-{
-	int node;
-	node = numa_node_of_cpu(cpu);
-	if (node == -1)
-		fatal("invalid cpu passed to numa_node_of_cpu(%d)\n", cpu);
-	return node;
-}
-
-static inline unsigned int rt_numa_bitmask_isbitset( const struct bitmask *mask,
-	unsigned long i)
-{
-	return numa_bitmask_isbitset(mask,i);
-}
-
-static inline struct bitmask* rt_numa_parse_cpustring(const char* s,
-	int max_cpus)
-{
-	return numa_parse_cpustring_all(s);
-}
-
-static inline void rt_bitmask_free(struct bitmask *mask)
-{
-	numa_bitmask_free(mask);
-}
-
-/** Returns number of bits set in mask. */
-static inline unsigned int rt_numa_bitmask_count(const struct bitmask *mask)
-{
-	unsigned int num_bits = 0, i;
-	for (i = 0; i < mask->size; i++) {
-		if (rt_numa_bitmask_isbitset(mask, i))
-			num_bits++;
-	}
-	/* Could stash this instead of recomputing every time. */
-	return num_bits;
-}
-
-#endif	/* _RT_NUMA_H */