diff mbox series

[rt-tests,v2,v2,01/20] cyclictest: Always use libnuma

Message ID 20201218161843.1764-2-dwagner@suse.de
State Superseded
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
libnuma is hard dependency for cyclictest. Thus we can always call
numa_initialize(). This allows us to remove the global 'numa' variable
to track if libnuma has been initialized or not.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/cyclictest/cyclictest.c | 63 +++++++++++++++++--------------------
 src/cyclictest/rt_numa.h    |  2 --
 2 files changed, 29 insertions(+), 36 deletions(-)

Comments

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

> libnuma is hard dependency for cyclictest. Thus we can always call

> numa_initialize(). This allows us to remove the global 'numa' variable

> to track if libnuma has been initialized or not.

> 

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

> ---

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

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

>  2 files changed, 29 insertions(+), 36 deletions(-)

> 

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

> index f38c453f1975..514ed7b20fdb 100644

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

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

> @@ -1018,9 +1018,6 @@ static void process_options(int argc, char *argv[], int max_cpus)

>  			/* smp sets AFFINITY_USEALL in OPT_SMP */

>  			if (smp)

>  				break;

> -			if (numa_initialize())

> -				fatal("Couldn't initialize libnuma");

> -			numa = 1;

>  			if (optarg) {

>  				parse_cpumask(optarg, max_cpus, &affinity_mask);

>  				setaffinity = AFFINITY_SPECIFIED;

> @@ -1126,8 +1123,6 @@ static void process_options(int argc, char *argv[], int max_cpus)

>  			use_system = MODE_SYS_OFFSET; break;

>  		case 'S':

>  		case OPT_SMP: /* SMP testing */

> -			if (numa)

> -				fatal("numa and smp options are mutually exclusive\n");

>  			smp = 1;

>  			num_threads = -1; /* update after parsing */

>  			setaffinity = AFFINITY_USEALL;

> @@ -1201,16 +1196,17 @@ static void process_options(int argc, char *argv[], int max_cpus)

>  

>  	/* if smp wasn't requested, test for numa automatically */

>  	if (!smp) {

> -		if (numa_initialize())

> -			fatal("Couldn't initialize libnuma");

> -		numa = 1;

>  		if (setaffinity == AFFINITY_UNSPECIFIED)

>  			setaffinity = AFFINITY_USEALL;

>  	}

>  

> -	if (option_affinity) {

> -		if (smp)

> -			warn("-a ignored due to smp mode\n");

> +	if (option_affinity && smp) {

> +		warn("-a ignored due to smp mode\n");

> +		if (affinity_mask) {

> +			numa_bitmask_free(affinity_mask);

> +			affinity_mask = NULL;

> +		}

> +		setaffinity = AFFINITY_USEALL;

>  	}

>  

>  	if (smi) {

> @@ -1744,6 +1740,12 @@ int main(int argc, char **argv)

>  	int max_cpus = sysconf(_SC_NPROCESSORS_ONLN);

>  	int i, ret = -1;

>  	int status;

> +	void *stack;

> +	void *currstk;

> +	size_t stksize;

> +

> +	if (numa_initialize())

> +		fatal("Couldn't initialize libnuma");

>  

>  	process_options(argc, argv, max_cpus);

>  

> @@ -1926,34 +1928,27 @@ int main(int argc, char **argv)

>  		default: cpu = -1;

>  		}

>  

> -		node = -1;

> -		if (numa) {

> -			void *stack;

> -			void *currstk;

> -			size_t stksize;

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

> +		node = rt_numa_numa_node_of_cpu(cpu);

>  

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

> -			node = rt_numa_numa_node_of_cpu(cpu);

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

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

> +			fatal("failed to get stack size for thread %d\n", i);

>  

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

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

> -				fatal("failed to get stack size for thread %d\n", i);

> +		/* if the stack size is zero, set a default */

> +		if (stksize == 0)

> +			stksize = PTHREAD_STACK_MIN * 2;

>  

> -			/* if the stack size is zero, set a default */

> -			if (stksize == 0)

> -				stksize = PTHREAD_STACK_MIN * 2;

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

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

>  

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

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

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

> +		memset(stack, 0, stksize);

>  

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

> -			memset(stack, 0, stksize);

> -

> -			/* set the thread's stack */

> -			if (pthread_attr_setstack(&attr, stack, stksize))

> -				fatal("failed to set stack addr for thread %d to 0x%x\n",

> -				      i, stack+stksize);

> -		}

> +		/* set the thread's stack */

> +		if (pthread_attr_setstack(&attr, stack, stksize))

> +			fatal("failed to set stack addr for thread %d to 0x%x\n",

> +				i, stack+stksize);

>  

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

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

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

> index 46690941e0a6..8d02f419ed6d 100644

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

> +++ b/src/cyclictest/rt_numa.h

> @@ -13,8 +13,6 @@

>  #include "rt-utils.h"

>  #include "error.h"

>  

> -static int numa = 0;

> -

>  #include <numa.h>

>  

>  static void *

> -- 

> 2.29.2

> 

> 

Signed-off-by: John Kacur <jkacur@redhat.com>
Kurt Kanzenbach Feb. 19, 2021, 1:44 p.m. UTC | #2
Hi,

On Fri Dec 18 2020, Daniel Wagner wrote:
> libnuma is hard dependency for cyclictest. Thus we can always call

> numa_initialize(). This allows us to remove the global 'numa' variable

> to track if libnuma has been initialized or not.

>

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


It seems like with this particular commit, it's not possible to run
cyclictest on arm32 systems anymore. I guess due to missing NUMA
support?

Just tested on a dual core Cyclone V:

root@tsn:~/rt-tests# ./cyclictest -S -m -p 99 --secaligned
FATAL: Couldn't initialize libnuma
root@tsn:~/rt-tests# 

I've used the current unstable/devel/latest branch. Any suggestions?

Thanks,
Kurt
Daniel Wagner Feb. 19, 2021, 2:12 p.m. UTC | #3
Hi Kurt,

On Fri, Feb 19, 2021 at 02:44:36PM +0100, Kurt Kanzenbach wrote:
> It seems like with this particular commit, it's not possible to run

> cyclictest on arm32 systems anymore. I guess due to missing NUMA

> support?


Yes, your distro needs to provide libnuma. cyclictest runs fine on arm32
with the library.

> Just tested on a dual core Cyclone V:

> 

> root@tsn:~/rt-tests# ./cyclictest -S -m -p 99 --secaligned

> FATAL: Couldn't initialize libnuma


I think you would see the same error when trying to use the '-a' option
without the patch. The dependency is not new.

> I've used the current unstable/devel/latest branch. Any suggestions?


The simplest thing is obviously to get libnuma on your system. I assume
this is not so simple in your case. In this case you could build
cyclictest a static binary.

First, build numactl as static libary:

  ./configure --enable-static && make

and then rt-tests with

  CFLAGS="-static -L../numactl/.libs/" make

Thanks,
Daniel
Kurt Kanzenbach Feb. 19, 2021, 2:39 p.m. UTC | #4
Hi Daniel,

On Fri Feb 19 2021, Daniel Wagner wrote:
> Hi Kurt,

>

> On Fri, Feb 19, 2021 at 02:44:36PM +0100, Kurt Kanzenbach wrote:

>> It seems like with this particular commit, it's not possible to run

>> cyclictest on arm32 systems anymore. I guess due to missing NUMA

>> support?

>

> Yes, your distro needs to provide libnuma. cyclictest runs fine on arm32

> with the library.


I'm using Debian and it provides libnuma.

>

>> Just tested on a dual core Cyclone V:

>> 

>> root@tsn:~/rt-tests# ./cyclictest -S -m -p 99 --secaligned

>> FATAL: Couldn't initialize libnuma

>

> I think you would see the same error when trying to use the '-a' option

> without the patch. The dependency is not new.

>

>> I've used the current unstable/devel/latest branch. Any suggestions?

>

> The simplest thing is obviously to get libnuma on your system. I assume

> this is not so simple in your case. In this case you could build

> cyclictest a static binary.

>

> First, build numactl as static libary:

>

>   ./configure --enable-static && make

>

> and then rt-tests with

>

>   CFLAGS="-static -L../numactl/.libs/" make


Static building with the newest numactl library also doesn't work:

|root@tsn:~/rt-tests# file cyclictest 
|cyclictest: ELF 32-bit LSB executable, ARM, EABI5 version 1 (GNU/Linux), statically linked, for GNU/Linux 3.2.0, BuildID[sha1]=248eaf8847544423dc51c6ceea18bbffc487991e, with debug_info, not stripped
|root@tsn:~/rt-tests# ./cyclictest 
|FATAL: Couldn't initialize libnuma
|root@tsn:~/rt-tests#

Hmm.

Thanks,
Kurt
Daniel Wagner Feb. 19, 2021, 2:54 p.m. UTC | #5
On Fri, Feb 19, 2021 at 03:39:45PM +0100, Kurt Kanzenbach wrote:
> I'm using Debian and it provides libnuma.


Hmm, that is strange. Indeed. I have to update my BBB first to a more
recent Debian release to try this out.

> Static building with the newest numactl library also doesn't work:

> 

> |root@tsn:~/rt-tests# file cyclictest 

> |cyclictest: ELF 32-bit LSB executable, ARM, EABI5 version 1 (GNU/Linux), statically linked, for GNU/Linux 3.2.0, BuildID[sha1]=248eaf8847544423dc51c6ceea18bbffc487991e, with debug_info, not stripped

> |root@tsn:~/rt-tests# ./cyclictest 

> |FATAL: Couldn't initialize libnuma


I try to reproduce this here. Building libnuma from source right now.
Sebastian Andrzej Siewior Feb. 19, 2021, 3:17 p.m. UTC | #6
On 2021-02-19 15:54:22 [+0100], Daniel Wagner wrote:
> I try to reproduce this here. Building libnuma from source right now.


|~# uname -m
|x86_64
|~# ./cyclictest 
|FATAL: Couldn't initialize libnuma~# 

Just do
 # CONFIG_NUMA is not set

in your .config and it is done.

Sebastian
Christian Eggers Feb. 19, 2021, 3:21 p.m. UTC | #7
CONFIG_NUMA is not available on ARM32:

https://cateee.net/lkddb/web-lkddb/NUMA.html

regards,
Christian

On Friday, 19 February 2021, 16:17:17 CET, Sebastian Andrzej Siewior wrote:
> On 2021-02-19 15:54:22 [+0100], Daniel Wagner wrote:

> > I try to reproduce this here. Building libnuma from source right now.

> 

> |~# uname -m

> |x86_64

> |~# ./cyclictest 

> |FATAL: Couldn't initialize libnuma~# 

> 

> Just do

>  # CONFIG_NUMA is not set

> 

> in your .config and it is done.

> 

> Sebastian

>
Daniel Wagner Feb. 19, 2021, 4:16 p.m. UTC | #8
On Fri, Feb 19, 2021 at 04:21:46PM +0100, Christian Eggers wrote:
> CONFIG_NUMA is not available on ARM32:


libnuma is not happy if the kernel doesn't have the config
option. Well, in this case we just revert the patch I suppose.
John Kacur Feb. 19, 2021, 4:21 p.m. UTC | #9
On Fri, 19 Feb 2021, Daniel Wagner wrote:

> On Fri, Feb 19, 2021 at 04:21:46PM +0100, Christian Eggers wrote:

> > CONFIG_NUMA is not available on ARM32:

> 

> libnuma is not happy if the kernel doesn't have the config

> option. Well, in this case we just revert the patch I suppose.

> 


Yeah, I'm not happy with this.
To be sure Daniel did a lot of good work cleaning-up some of the numa 
calls and cleaning up the somewhat artificial smp / numa divide.

That work needs to continue, but I want the ability to build cyclictest
without NUMA, and that also means we need the small helpers.

John
Christian Eggers Feb. 19, 2021, 4:27 p.m. UTC | #10
On Friday, 19 February 2021, 17:21:21 CET, John Kacur wrote:
> 

> On Fri, 19 Feb 2021, Daniel Wagner wrote:

> 

> > On Fri, Feb 19, 2021 at 04:21:46PM +0100, Christian Eggers wrote:

> > > CONFIG_NUMA is not available on ARM32:

> > 

> > libnuma is not happy if the kernel doesn't have the config

> > option. Well, in this case we just revert the patch I suppose.

> > 

> 

> Yeah, I'm not happy with this.

> To be sure Daniel did a lot of good work cleaning-up some of the numa 

> calls and cleaning up the somewhat artificial smp / numa divide.

> 

> That work needs to continue, but I want the ability to build cyclictest

> without NUMA, and that also means we need the small helpers.


Just for clarification: Does this mean that cyclictest shall link against
libnuma (libnuma can be built on ARM without problems), but not call it
functions at runtime?

From the numa.h:

/* NUMA support available. If this returns a negative value all other function
   in this library are undefined. */
int numa_available(void);

regards,
Christian
Daniel Wagner Feb. 19, 2021, 4:35 p.m. UTC | #11
On 19.02.21 17:27, Christian Eggers wrote:
> Just for clarification: Does this mean that cyclictest shall link against

> libnuma (libnuma can be built on ARM without problems), but not call it

> functions at runtime?


Yes, I think that is the idea of libnuma.
John Kacur Feb. 19, 2021, 4:39 p.m. UTC | #12
On Fri, 19 Feb 2021, Christian Eggers wrote:

> On Friday, 19 February 2021, 17:21:21 CET, John Kacur wrote:

> > 

> > On Fri, 19 Feb 2021, Daniel Wagner wrote:

> > 

> > > On Fri, Feb 19, 2021 at 04:21:46PM +0100, Christian Eggers wrote:

> > > > CONFIG_NUMA is not available on ARM32:

> > > 

> > > libnuma is not happy if the kernel doesn't have the config

> > > option. Well, in this case we just revert the patch I suppose.

> > > 

> > 

> > Yeah, I'm not happy with this.

> > To be sure Daniel did a lot of good work cleaning-up some of the numa 

> > calls and cleaning up the somewhat artificial smp / numa divide.

> > 

> > That work needs to continue, but I want the ability to build cyclictest

> > without NUMA, and that also means we need the small helpers.

> 

> Just for clarification: Does this mean that cyclictest shall link against

> libnuma (libnuma can be built on ARM without problems), but not call it

> functions at runtime?

> 

> From the numa.h:

> 

> /* NUMA support available. If this returns a negative value all other function

>    in this library are undefined. */

> int numa_available(void);

> 

> regards,

> Christian


We used to be able to build cyclictest without libnuma.
The small helpers that wrapped the calls to numa contained versions
without numa calls. Most people did have libnuma, so we were requiring 
libnuma say on x86_64.

I think at some point before Daniel starting removing the wrappers this
support was broken.

So, I mean the ability to build cyclitest without libnuma at least
on some architectures.

John
John Kacur Feb. 19, 2021, 4:45 p.m. UTC | #13
On Fri, 19 Feb 2021, Kurt Kanzenbach wrote:

> Hi,

> 

> On Fri Dec 18 2020, Daniel Wagner wrote:

> > libnuma is hard dependency for cyclictest. Thus we can always call

> > numa_initialize(). This allows us to remove the global 'numa' variable

> > to track if libnuma has been initialized or not.


Just a small note, the global 'numa' variable was NOT used to track 
whether libnuma had been intialized or not.

The user used to specify smp or numa, then we decided that we would remove 
the numa option.

What happened was that if numa was available and smp was not specified
the program would automatically use numa else it would use smp.

If smp was specified then smp was used. The numa variable would track 
that.

the smp option used to just collect a common set of options
-a -t and priorities all the same.

It made sense way back when, but it's less useful now-a-days

> >

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

> 

> It seems like with this particular commit, it's not possible to run

> cyclictest on arm32 systems anymore. I guess due to missing NUMA

> support?

> 

> Just tested on a dual core Cyclone V:

> 

> root@tsn:~/rt-tests# ./cyclictest -S -m -p 99 --secaligned

> FATAL: Couldn't initialize libnuma

> root@tsn:~/rt-tests# 

> 

> I've used the current unstable/devel/latest branch. Any suggestions?

> 

> Thanks,

> Kurt

>
John Kacur Feb. 19, 2021, 5:07 p.m. UTC | #14
On Fri, 19 Feb 2021, Christian Eggers wrote:

> On Friday, 19 February 2021, 17:21:21 CET, John Kacur wrote:

> > 

> > On Fri, 19 Feb 2021, Daniel Wagner wrote:

> > 

> > > On Fri, Feb 19, 2021 at 04:21:46PM +0100, Christian Eggers wrote:

> > > > CONFIG_NUMA is not available on ARM32:

> > > 

> > > libnuma is not happy if the kernel doesn't have the config

> > > option. Well, in this case we just revert the patch I suppose.

> > > 

> > 

> > Yeah, I'm not happy with this.

> > To be sure Daniel did a lot of good work cleaning-up some of the numa 

> > calls and cleaning up the somewhat artificial smp / numa divide.

> > 

> > That work needs to continue, but I want the ability to build cyclictest

> > without NUMA, and that also means we need the small helpers.

> 

> Just for clarification: Does this mean that cyclictest shall link against

> libnuma (libnuma can be built on ARM without problems), but not call it

> functions at runtime?

> 

> From the numa.h:

> 

> /* NUMA support available. If this returns a negative value all other function

>    in this library are undefined. */

> int numa_available(void);


On the other hand, this is what was working for you before the changes 
right? We required libnuma for building but not for runtime?

That would be easier to maintain.

> 

> regards,

> Christian

> 

> 

> 

>
diff mbox series

Patch

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index f38c453f1975..514ed7b20fdb 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -1018,9 +1018,6 @@  static void process_options(int argc, char *argv[], int max_cpus)
 			/* smp sets AFFINITY_USEALL in OPT_SMP */
 			if (smp)
 				break;
-			if (numa_initialize())
-				fatal("Couldn't initialize libnuma");
-			numa = 1;
 			if (optarg) {
 				parse_cpumask(optarg, max_cpus, &affinity_mask);
 				setaffinity = AFFINITY_SPECIFIED;
@@ -1126,8 +1123,6 @@  static void process_options(int argc, char *argv[], int max_cpus)
 			use_system = MODE_SYS_OFFSET; break;
 		case 'S':
 		case OPT_SMP: /* SMP testing */
-			if (numa)
-				fatal("numa and smp options are mutually exclusive\n");
 			smp = 1;
 			num_threads = -1; /* update after parsing */
 			setaffinity = AFFINITY_USEALL;
@@ -1201,16 +1196,17 @@  static void process_options(int argc, char *argv[], int max_cpus)
 
 	/* if smp wasn't requested, test for numa automatically */
 	if (!smp) {
-		if (numa_initialize())
-			fatal("Couldn't initialize libnuma");
-		numa = 1;
 		if (setaffinity == AFFINITY_UNSPECIFIED)
 			setaffinity = AFFINITY_USEALL;
 	}
 
-	if (option_affinity) {
-		if (smp)
-			warn("-a ignored due to smp mode\n");
+	if (option_affinity && smp) {
+		warn("-a ignored due to smp mode\n");
+		if (affinity_mask) {
+			numa_bitmask_free(affinity_mask);
+			affinity_mask = NULL;
+		}
+		setaffinity = AFFINITY_USEALL;
 	}
 
 	if (smi) {
@@ -1744,6 +1740,12 @@  int main(int argc, char **argv)
 	int max_cpus = sysconf(_SC_NPROCESSORS_ONLN);
 	int i, ret = -1;
 	int status;
+	void *stack;
+	void *currstk;
+	size_t stksize;
+
+	if (numa_initialize())
+		fatal("Couldn't initialize libnuma");
 
 	process_options(argc, argv, max_cpus);
 
@@ -1926,34 +1928,27 @@  int main(int argc, char **argv)
 		default: cpu = -1;
 		}
 
-		node = -1;
-		if (numa) {
-			void *stack;
-			void *currstk;
-			size_t stksize;
+		/* find the memory node associated with the cpu i */
+		node = rt_numa_numa_node_of_cpu(cpu);
 
-			/* find the memory node associated with the cpu i */
-			node = rt_numa_numa_node_of_cpu(cpu);
+		/* get the stack size set for this thread */
+		if (pthread_attr_getstack(&attr, &currstk, &stksize))
+			fatal("failed to get stack size for thread %d\n", i);
 
-			/* get the stack size set for this thread */
-			if (pthread_attr_getstack(&attr, &currstk, &stksize))
-				fatal("failed to get stack size for thread %d\n", i);
+		/* if the stack size is zero, set a default */
+		if (stksize == 0)
+			stksize = PTHREAD_STACK_MIN * 2;
 
-			/* if the stack size is zero, set a default */
-			if (stksize == 0)
-				stksize = PTHREAD_STACK_MIN * 2;
+		/*  allocate memory for a stack on appropriate node */
+		stack = rt_numa_numa_alloc_onnode(stksize, node, cpu);
 
-			/*  allocate memory for a stack on appropriate node */
-			stack = rt_numa_numa_alloc_onnode(stksize, node, cpu);
+		/* touch the stack pages to pre-fault them in */
+		memset(stack, 0, stksize);
 
-			/* touch the stack pages to pre-fault them in */
-			memset(stack, 0, stksize);
-
-			/* set the thread's stack */
-			if (pthread_attr_setstack(&attr, stack, stksize))
-				fatal("failed to set stack addr for thread %d to 0x%x\n",
-				      i, stack+stksize);
-		}
+		/* set the thread's stack */
+		if (pthread_attr_setstack(&attr, stack, stksize))
+			fatal("failed to set stack addr for thread %d to 0x%x\n",
+				i, stack+stksize);
 
 		/* allocate the thread's parameter block  */
 		parameters[i] = par = threadalloc(sizeof(struct thread_param), node);
diff --git a/src/cyclictest/rt_numa.h b/src/cyclictest/rt_numa.h
index 46690941e0a6..8d02f419ed6d 100644
--- a/src/cyclictest/rt_numa.h
+++ b/src/cyclictest/rt_numa.h
@@ -13,8 +13,6 @@ 
 #include "rt-utils.h"
 #include "error.h"
 
-static int numa = 0;
-
 #include <numa.h>
 
 static void *