[rt-tests v1 0/6] libnuma cleanups for cyclictest

[rt-tests v1 0/6] libnuma cleanups for cyclictest

[Daniel Wagner]

As we have a hard dependency on libnuma we can simplify the code in
cyclictest. This allows remove all the small helpers in rt_numa.h. And
with this we can remove the header and reduce the confusion with
rt-numa.h

While at it, I simplified the --smp vs --affinity vs --threads
logic. There is no need for additional variables to keep state. With
this we also make --affinity to behave as with the rest of
rt-tests. That is a plan -a will be the same as with -S. There is no
need for -S anymore but I think we should leave it in place for
backwards compatibility. I suspect, there must be a lot of muscle
memory out there :)

Daniel Wagner (6):
  cyclictest: Always use libnuma
  cyclictest: Use numa API directly
  cyclictest: Use affinity_mask for stearing thread placement
  cyclictest: Mimik --smp behavior with --affinity
  cyclictest: Simplify --smp vs --affinity vs --threads argument logic
  cyclictest: Move verbose message into main

 src/cyclictest/cyclictest.c | 154 ++++++++++++++----------------------
 src/cyclictest/rt_numa.h    |  98 -----------------------
 2 files changed, 58 insertions(+), 194 deletions(-)
 delete mode 100644 src/cyclictest/rt_numa.h

-- 
2.29.2

[rt-tests v1 1/6] cyclictest: Always use libnuma

[Daniel Wagner]

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

[rt-tests v1 2/6] cyclictest: Use numa API directly

[Daniel Wagner]

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);
 
 		/* 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

[rt-tests v1 3/6] cyclictest: Use affinity_mask for stearing thread placement

[Daniel Wagner]

We don't need an extra variable to track the state if a bitmask is
available or not. Check directly if the mask is usable.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/cyclictest/cyclictest.c | 28 ++++------------------------
 1 file changed, 4 insertions(+), 24 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index a5ca764da254..0e6519125c2f 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -893,7 +893,6 @@ static int interval = DEFAULT_INTERVAL;
 static int distance = -1;
 static struct bitmask *affinity_mask = NULL;
 static int smp = 0;
-static int setaffinity = AFFINITY_UNSPECIFIED;
 
 static int clocksources[] = {
 	CLOCK_MONOTONIC,
@@ -1020,19 +1019,13 @@ static void process_options(int argc, char *argv[], int max_cpus)
 				break;
 			if (optarg) {
 				parse_cpumask(optarg, max_cpus, &affinity_mask);
-				setaffinity = AFFINITY_SPECIFIED;
 			} else if (optind < argc &&
 				   (atoi(argv[optind]) ||
 				    argv[optind][0] == '0' ||
 				    argv[optind][0] == '!')) {
 				parse_cpumask(argv[optind], max_cpus, &affinity_mask);
-				setaffinity = AFFINITY_SPECIFIED;
-			} else {
-				setaffinity = AFFINITY_USEALL;
 			}
 
-			if (setaffinity == AFFINITY_SPECIFIED && !affinity_mask)
-				display_help(1);
 			if (verbose)
 				printf("Using %u cpus.\n",
 					numa_bitmask_weight(affinity_mask));
@@ -1125,7 +1118,6 @@ static void process_options(int argc, char *argv[], int max_cpus)
 		case OPT_SMP: /* SMP testing */
 			smp = 1;
 			num_threads = -1; /* update after parsing */
-			setaffinity = AFFINITY_USEALL;
 			break;
 		case 't':
 		case OPT_THREADS:
@@ -1194,23 +1186,16 @@ static void process_options(int argc, char *argv[], int max_cpus)
 		use_nanosleep = MODE_CLOCK_NANOSLEEP;
 	}
 
-	/* if smp wasn't requested, test for numa automatically */
-	if (!smp) {
-		if (setaffinity == AFFINITY_UNSPECIFIED)
-			setaffinity = AFFINITY_USEALL;
-	}
-
 	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) {
-		if (setaffinity == AFFINITY_UNSPECIFIED)
+		if (affinity_mask)
 			fatal("SMI counter relies on thread affinity\n");
 
 		if (!has_smi_counter())
@@ -1756,7 +1741,7 @@ int main(int argc, char **argv)
 		printf("Max CPUs = %d\n", max_cpus);
 
 	/* Restrict the main pid to the affinity specified by the user */
-	if (affinity_mask != NULL) {
+	if (affinity_mask) {
 		int res;
 
 		errno = 0;
@@ -1915,17 +1900,12 @@ int main(int argc, char **argv)
 		if (status != 0)
 			fatal("error from pthread_attr_init for thread %d: %s\n", i, strerror(status));
 
-		switch (setaffinity) {
-		case AFFINITY_UNSPECIFIED: cpu = -1; break;
-		case AFFINITY_SPECIFIED:
+		if (affinity_mask) {
 			cpu = cpu_for_thread_sp(i, max_cpus, affinity_mask);
 			if (verbose)
 				printf("Thread %d using cpu %d.\n", i, cpu);
-			break;
-		case AFFINITY_USEALL:
+		} else {
 			cpu = cpu_for_thread_ua(i, max_cpus);
-			break;
-		default: cpu = -1;
 		}
 
 		/* find the memory node associated with the cpu i */
-- 
2.29.2

[rt-tests v1 4/6] cyclictest: Mimik --smp behavior with --affinity

[Daniel Wagner]

Let --affinity without an argument behave in the same way as
--smp. Run a thread on each CPU. This makes cyclictest behave as the
rest of the rt-tests when --affinity is used.

Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
 src/cyclictest/cyclictest.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 0e6519125c2f..b9b0eb3575e3 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -1024,6 +1024,8 @@ static void process_options(int argc, char *argv[], int max_cpus)
 				    argv[optind][0] == '0' ||
 				    argv[optind][0] == '!')) {
 				parse_cpumask(argv[optind], max_cpus, &affinity_mask);
+			} else {
+				num_threads = -1;
 			}
 
 			if (verbose)
-- 
2.29.2

[rt-tests v1 5/6] cyclictest: Simplify --smp vs --affinity vs --threads argument logic

[Daniel Wagner]

Allow each command line only to update one variable and do the final
decission at the end of the parsing step. With this the order
of the command line is not important.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/cyclictest/cyclictest.c | 34 ++++++++--------------------------
 1 file changed, 8 insertions(+), 26 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index b9b0eb3575e3..a364b4ad203a 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -885,14 +885,13 @@ static int timermode = TIMER_ABSTIME;
 static int use_system;
 static int priority;
 static int policy = SCHED_OTHER;	/* default policy if not specified */
-static int num_threads = 1;
+static int num_threads = -1;
 static int max_cycles;
 static int clocksel = 0;
 static int quiet;
 static int interval = DEFAULT_INTERVAL;
 static int distance = -1;
 static struct bitmask *affinity_mask = NULL;
-static int smp = 0;
 
 static int clocksources[] = {
 	CLOCK_MONOTONIC,
@@ -958,7 +957,7 @@ enum option_values {
 static void process_options(int argc, char *argv[], int max_cpus)
 {
 	int error = 0;
-	int option_affinity = 0;
+	int smp = 0;
 
 	for (;;) {
 		int option_index = 0;
@@ -1013,10 +1012,6 @@ static void process_options(int argc, char *argv[], int max_cpus)
 		switch (c) {
 		case 'a':
 		case OPT_AFFINITY:
-			option_affinity = 1;
-			/* smp sets AFFINITY_USEALL in OPT_SMP */
-			if (smp)
-				break;
 			if (optarg) {
 				parse_cpumask(optarg, max_cpus, &affinity_mask);
 			} else if (optind < argc &&
@@ -1024,8 +1019,6 @@ static void process_options(int argc, char *argv[], int max_cpus)
 				    argv[optind][0] == '0' ||
 				    argv[optind][0] == '!')) {
 				parse_cpumask(argv[optind], max_cpus, &affinity_mask);
-			} else {
-				num_threads = -1;
 			}
 
 			if (verbose)
@@ -1117,22 +1110,14 @@ static void process_options(int argc, char *argv[], int max_cpus)
 		case OPT_SYSTEM:
 			use_system = MODE_SYS_OFFSET; break;
 		case 'S':
-		case OPT_SMP: /* SMP testing */
-			smp = 1;
-			num_threads = -1; /* update after parsing */
-			break;
+		case OPT_SMP:
+			smp = 1; break;
 		case 't':
 		case OPT_THREADS:
-			if (smp) {
-				warn("-t ignored due to smp mode\n");
-				break;
-			}
 			if (optarg != NULL)
 				num_threads = atoi(optarg);
 			else if (optind < argc && atoi(argv[optind]))
 				num_threads = atoi(argv[optind]);
-			else
-				num_threads = -1; /* update after parsing */
 			break;
 		case OPT_TRIGGER:
 			trigger = atoi(optarg);
@@ -1188,13 +1173,10 @@ static void process_options(int argc, char *argv[], int max_cpus)
 		use_nanosleep = MODE_CLOCK_NANOSLEEP;
 	}
 
-	if (option_affinity && smp) {
-		warn("-a ignored due to smp mode\n");
-		if (affinity_mask) {
-			numa_bitmask_free(affinity_mask);
-			affinity_mask = NULL;
-		}
-	}
+	if (smp && num_threads != -1)
+		warn("--threads overwrites smp mode\n");
+	if (smp && affinity_mask)
+		warn("--affinity overwrites smp mode\n");
 
 	if (smi) {
 		if (affinity_mask)
-- 
2.29.2

[rt-tests v1 6/6] cyclictest: Move verbose message into main

[Daniel Wagner]

By moving the verbose message down we print the message with the final
affinity_mask. This also handles the case where the bitmask is not set.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/cyclictest/cyclictest.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index a364b4ad203a..5abb69d0b28f 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -1020,10 +1020,6 @@ static void process_options(int argc, char *argv[], int max_cpus)
 				    argv[optind][0] == '!')) {
 				parse_cpumask(argv[optind], max_cpus, &affinity_mask);
 			}
-
-			if (verbose)
-				printf("Using %u cpus.\n",
-					numa_bitmask_weight(affinity_mask));
 			break;
 		case 'A':
 		case OPT_ALIGNED:
@@ -1732,6 +1728,10 @@ int main(int argc, char **argv)
 		res = numa_sched_setaffinity(getpid(), affinity_mask);
 		if (res != 0)
 			warn("Couldn't setaffinity in main thread: %s\n", strerror(errno));
+
+		if (verbose)
+			printf("Using %u cpus.\n",
+				numa_bitmask_weight(affinity_mask));
 	}
 
 	if (trigger) {
-- 
2.29.2

Re: [rt-tests v1 0/6] libnuma cleanups for cyclictest

[Daniel Wagner]

On 18.12.20 15:19, Daniel Wagner wrote:
> As we have a hard dependency on libnuma we can simplify the code in
> cyclictest. This allows remove all the small helpers in rt_numa.h. And
> with this we can remove the header and reduce the confusion with
> rt-numa.h
> 
> While at it, I simplified the --smp vs --affinity vs --threads
> logic. There is no need for additional variables to keep state. With
> this we also make --affinity to behave as with the rest of
> rt-tests. That is a plan -a will be the same as with -S. There is no
> need for -S anymore but I think we should leave it in place for
> backwards compatibility. I suspect, there must be a lot of muscle
> memory out there :)

I just realized there are some programs which need the same treatment. 
Wait for v2...

Re: [rt-tests v1 0/6] libnuma cleanups for cyclictest

[John Kacur]

On Fri, 18 Dec 2020, Daniel Wagner wrote:

> As we have a hard dependency on libnuma we can simplify the code in
> cyclictest. This allows remove all the small helpers in rt_numa.h. And
> with this we can remove the header and reduce the confusion with
> rt-numa.h

This is why I asked you to keep numa functions separate from any common
library. The reason the file exists, is in the past I had two versions
of each numa function, one for architectures that supported libnuma, and
empty wrapper versions for ones that didn't. This implementation detail
was pushed up into that header file, to make the code cleaner. Then all the 
architectures that we cared about seemed to support to libnuma so I 
removed the versions of the functions that were empty, so we were at the
point that the header file could just be removed, but I never got around
to that and things just worked fine. So thank you for doing this, but, 
there is always a but right?

There are two buts. The first one, is that I see the other programs in
the test suite that mimic cyclictest behaviour are getting more attention,
so I could imagine as we try to strengthen their behaviour that we could
add numa support to these programs as well. The other but, is that I'm 
seeing the need recently to add support for a lot of architectures that
we didn't care so much about in the past, and not all of them have libnuma
so we might want to do the same trick as in the past, add wrapper versions
in a header file.

So I propose, we take your patches to get rid of the file, because it will
clean things up a lot, but then we may have to go back to creating 
something like the rt_numa.h file anew. Removing rt_numa.h first is good 
though, because if we don't have to create it anew the code has been 
cleaned-up, and if we do recreate it from scratch, it will be better for 
having cleaned-up how numa works in cyclictest

> 
> While at it, I simplified the --smp vs --affinity vs --threads
> logic. There is no need for additional variables to keep state. With
> this we also make --affinity to behave as with the rest of
> rt-tests. That is a plan -a will be the same as with -S. There is no
> need for -S anymore but I think we should leave it in place for
> backwards compatibility. I suspect, there must be a lot of muscle
> memory out there :)

This is also an example of something that was evoling in exactly this
direction. When Thomas wrote cyclictest, it made sense to have smp, but if
I were to rewrite everything from scratch I wouldn't have it there.

Some more background, we used to have to specify numa, but we changed it
so that numa was detected automatically, but you could still specify
smp if you wanted to test that behaviour. This also helped
simplify the alphabet soup of options by removing --numa.

Note also that smp isn't just a kind of hardware for cyclictest, you 
can still see this is the help - it gathered up the -a and -t options
and put all threads at the same prio.

So, thanks again for doing this. A word of warning I spent a long
time fiddling with affinity to make it finally work correctly, so
I'm going to test the sh*t out of your changes to make sure they don't 
break anything before I accept them.

A final comment, the idea of an "unstable" devel branch,
the "unstable" refers not to the code being iffy, but to the fact
that I wanted developers to feel free to break the old API, and not be
constrained with backwards compatability, so as you clean this stuff-up
if you want to remove -S, it's fine with me. Hopefully if we take away
anything that users want they'll yell at us, but I think we're pretty
careful about necessary functionality. Maybe after things are cleaned-up
we will have to add a new flag to specify the same prio for all threads?

I also maintain the rteval program which uses cyclictest for measuring
performance while running loads, changes to the cyclictest API sometimes 
require me to tweak how rteval works. Nobody has ever told me about any 
other programs that use cyclictest. Anyway, changes to the cyclictest api
have to be synced with rteval. rteval has develped in the background to 
this community and not received much attention here before, but, well, 
thinking about how that might change.

I think I'll spin off a new version of rt-tests before accepting this
set of patches.

John


> 
> Daniel Wagner (6):
>   cyclictest: Always use libnuma
>   cyclictest: Use numa API directly
>   cyclictest: Use affinity_mask for stearing thread placement
>   cyclictest: Mimik --smp behavior with --affinity
>   cyclictest: Simplify --smp vs --affinity vs --threads argument logic
>   cyclictest: Move verbose message into main
> 
>  src/cyclictest/cyclictest.c | 154 ++++++++++++++----------------------
>  src/cyclictest/rt_numa.h    |  98 -----------------------
>  2 files changed, 58 insertions(+), 194 deletions(-)
>  delete mode 100644 src/cyclictest/rt_numa.h
> 
> -- 
> 2.29.2
> 
> 

Re: [rt-tests v1 0/6] libnuma cleanups for cyclictest

[John Kacur]

On Fri, 18 Dec 2020, Daniel Wagner wrote:

> On 18.12.20 15:19, Daniel Wagner wrote:
> > As we have a hard dependency on libnuma we can simplify the code in
> > cyclictest. This allows remove all the small helpers in rt_numa.h. And
> > with this we can remove the header and reduce the confusion with
> > rt-numa.h
> > 
> > While at it, I simplified the --smp vs --affinity vs --threads
> > logic. There is no need for additional variables to keep state. With
> > this we also make --affinity to behave as with the rest of
> > rt-tests. That is a plan -a will be the same as with -S. There is no
> > need for -S anymore but I think we should leave it in place for
> > backwards compatibility. I suspect, there must be a lot of muscle
> > memory out there :)
> 
> I just realized there are some programs which need the same treatment. Wait
> for v2...
> 
> 

For the future, I'm okay with things evolving in steps, as long as what 
you sent me works correctly, you can then add support to do the same kind of things
in other programs in the suite later, as long as it gets done and not 
neglected.

Since you told me to wait this time though, I will wait for v2

John

Re: [rt-tests v1 0/6] libnuma cleanups for cyclictest

[Daniel Wagner]

Hi John,

On Fri, Dec 18, 2020 at 10:57:22AM -0500, John Kacur wrote:
> On Fri, 18 Dec 2020, Daniel Wagner wrote:
> 
> > As we have a hard dependency on libnuma we can simplify the code in
> > cyclictest. This allows remove all the small helpers in rt_numa.h. And
> > with this we can remove the header and reduce the confusion with
> > rt-numa.h
> 
> This is why I asked you to keep numa functions separate from any common
> library. The reason the file exists, is in the past I had two versions
> of each numa function, one for architectures that supported libnuma, and
> empty wrapper versions for ones that didn't.

This is what I figured as well. So with the decission to have the
dependency on libnuma a lot of things can be simplified.

> There are two buts. The first one, is that I see the other programs in
> the test suite that mimic cyclictest behaviour are getting more attention,
> so I could imagine as we try to strengthen their behaviour that we could
> add numa support to these programs as well.

Indeed, this is what I hand in mind too. Parsing the --affinity string
should be done with the libnuma. One thing we could try to achieve is to
common parser options so that we only have one function for it. Not sure
if this would be possible though.

> The other but, is that I'm
> seeing the need recently to add support for a lot of architectures that
> we didn't care so much about in the past, and not all of them have libnuma
> so we might want to do the same trick as in the past, add wrapper versions
> in a header file.

I am sure with rt getting closer to mainline there will be more interest
in the tests. I'd say we should rather get libnuma supported on those
architectures instead of trying to work around here. Debian says
libnuma is available on

 - alpha
 - amd64
 - arm64
 - armel
 - armhf
 - hppa
 - i386
 - m68k
 - mips64el
 - mipsel
 - ppc64
 - ppc64el
 - riscv64
 - s390x
 - sh4
 - sparc64
 - x32

Which architectures is missing?

> So I propose, we take your patches to get rid of the file, because it will
> clean things up a lot, but then we may have to go back to creating 
> something like the rt_numa.h file anew. Removing rt_numa.h first is good 
> though, because if we don't have to create it anew the code has been 
> cleaned-up, and if we do recreate it from scratch, it will be better for 
> having cleaned-up how numa works in cyclictest

Yes, that was my whole indent of this series. First to cleanup the
current rt-numa.h code as much as possible before we adding new stuff.

> This is also an example of something that was evoling in exactly this
> direction. When Thomas wrote cyclictest, it made sense to have smp, but if
> I were to rewrite everything from scratch I wouldn't have it there.

That's why I opted to make '-a' a bit more powerful :)

> Note also that smp isn't just a kind of hardware for cyclictest, you
> can still see this is the help - it gathered up the -a and -t options
> and put all threads at the same prio.

This hasn't changed. So you can place N threads evenly placed on the
provided cpumask. Not sure why I want to have more threads on the CPU,
but well, it's supported.

> So, thanks again for doing this. A word of warning I spent a long
> time fiddling with affinity to make it finally work correctly, so
> I'm going to test the sh*t out of your changes to make sure they don't 
> break anything before I accept them.

Please test it carefully. I tried not to breaks it but I wouldn't be
surprised you find something.

> A final comment, the idea of an "unstable" devel branch,
> the "unstable" refers not to the code being iffy, but to the fact
> that I wanted developers to feel free to break the old API, and not be
> constrained with backwards compatability, so as you clean this stuff-up
> if you want to remove -S, it's fine with me. Hopefully if we take away
> anything that users want they'll yell at us, but I think we're pretty
> careful about necessary functionality. Maybe after things are cleaned-up
> we will have to add a new flag to specify the same prio for all
> threads?

IIRC, Debian was not particular happy with the option getting dropped
within a major version. So it's not just rtval using it. Also there is
LAVA's test-definitions (which I use) but this shouldn't be a real
problem as it's easy to update.

Though as said, distros are not happy if we change the existing
options. Given that it's not a real burden to keep -S around I would
just cary around until we have a real need for it to go.

> I think I'll spin off a new version of rt-tests before accepting this
> set of patches.

Good idea!

Thanks,
Daniel

Re: [rt-tests v1 0/6] libnuma cleanups for cyclictest

[Daniel Wagner]

On Fri, Dec 18, 2020 at 11:02:45AM -0500, John Kacur wrote:
> For the future, I'm okay with things evolving in steps, as long as what 
> you sent me works correctly, you can then add support to do the same kind of things
> in other programs in the suite later, as long as it gets done and not 
> neglected.

I am on the same side here. We should really try to unify the code base
match as possible. This series is just one small step into this
direction :)

> Since you told me to wait this time though, I will wait for v2

See your inbox. Happy testing.

Re: [rt-tests v1 0/6] libnuma cleanups for cyclictest

[Alison Chaiken]

On Fri, Dec 18, 2020 at 7:59 AM John Kacur <jkacur@redhat.com> wrote:
> I also maintain the rteval program which uses cyclictest for measuring
> performance while running loads, changes to the cyclictest API sometimes
> require me to tweak how rteval works.

What is the authoritative source of rteval?  I don't see it in
rt-tests, in kernel source or in Debian packages.

Thanks,
Alison Chaiken
Aurora Innovation

Re: [rt-tests v1 0/6] libnuma cleanups for cyclictest

[John Kacur]

On Tue, 22 Dec 2020, Alison Chaiken wrote:

> On Fri, Dec 18, 2020 at 7:59 AM John Kacur <jkacur@redhat.com> wrote:
> > I also maintain the rteval program which uses cyclictest for measuring
> > performance while running loads, changes to the cyclictest API sometimes
> > require me to tweak how rteval works.
> 
> What is the authoritative source of rteval?  I don't see it in
> rt-tests, in kernel source or in Debian packages.
> 
> Thanks,
> Alison Chaiken
> Aurora Innovation
>


Clone
git://git.kernel.org/pub/scm/utils/rteval/rteval.git
https://git.kernel.org/pub/scm/utils/rteval/rteval.git
https://kernel.googlesource.com/pub/scm/utils/rteval/rteval.git

Re: [rt-tests v1 3/6] cyclictest: Use affinity_mask for stearing thread placement

[John Kacur]

On Fri, 18 Dec 2020, Daniel Wagner wrote:

> We don't need an extra variable to track the state if a bitmask is
> available or not. Check directly if the mask is usable.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  src/cyclictest/cyclictest.c | 28 ++++------------------------
>  1 file changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index a5ca764da254..0e6519125c2f 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -893,7 +893,6 @@ static int interval = DEFAULT_INTERVAL;
>  static int distance = -1;
>  static struct bitmask *affinity_mask = NULL;
>  static int smp = 0;
> -static int setaffinity = AFFINITY_UNSPECIFIED;
>  
>  static int clocksources[] = {
>  	CLOCK_MONOTONIC,
> @@ -1020,19 +1019,13 @@ static void process_options(int argc, char *argv[], int max_cpus)
>  				break;
>  			if (optarg) {
>  				parse_cpumask(optarg, max_cpus, &affinity_mask);
> -				setaffinity = AFFINITY_SPECIFIED;
>  			} else if (optind < argc &&
>  				   (atoi(argv[optind]) ||
>  				    argv[optind][0] == '0' ||
>  				    argv[optind][0] == '!')) {
>  				parse_cpumask(argv[optind], max_cpus, &affinity_mask);
> -				setaffinity = AFFINITY_SPECIFIED;
> -			} else {
> -				setaffinity = AFFINITY_USEALL;
>  			}
>  
> -			if (setaffinity == AFFINITY_SPECIFIED && !affinity_mask)
> -				display_help(1);
>  			if (verbose)
>  				printf("Using %u cpus.\n",
>  					numa_bitmask_weight(affinity_mask));
> @@ -1125,7 +1118,6 @@ static void process_options(int argc, char *argv[], int max_cpus)
>  		case OPT_SMP: /* SMP testing */
>  			smp = 1;
>  			num_threads = -1; /* update after parsing */
> -			setaffinity = AFFINITY_USEALL;
>  			break;
>  		case 't':
>  		case OPT_THREADS:
> @@ -1194,23 +1186,16 @@ static void process_options(int argc, char *argv[], int max_cpus)
>  		use_nanosleep = MODE_CLOCK_NANOSLEEP;
>  	}
>  
> -	/* if smp wasn't requested, test for numa automatically */
> -	if (!smp) {
> -		if (setaffinity == AFFINITY_UNSPECIFIED)
> -			setaffinity = AFFINITY_USEALL;
> -	}
> -
>  	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) {
> -		if (setaffinity == AFFINITY_UNSPECIFIED)
> +		if (affinity_mask)
>  			fatal("SMI counter relies on thread affinity\n");
>  
>  		if (!has_smi_counter())
> @@ -1756,7 +1741,7 @@ int main(int argc, char **argv)
>  		printf("Max CPUs = %d\n", max_cpus);
>  
>  	/* Restrict the main pid to the affinity specified by the user */
> -	if (affinity_mask != NULL) {
> +	if (affinity_mask) {
>  		int res;
>  
>  		errno = 0;
> @@ -1915,17 +1900,12 @@ int main(int argc, char **argv)
>  		if (status != 0)
>  			fatal("error from pthread_attr_init for thread %d: %s\n", i, strerror(status));
>  
> -		switch (setaffinity) {
> -		case AFFINITY_UNSPECIFIED: cpu = -1; break;
> -		case AFFINITY_SPECIFIED:
> +		if (affinity_mask) {
>  			cpu = cpu_for_thread_sp(i, max_cpus, affinity_mask);
>  			if (verbose)
>  				printf("Thread %d using cpu %d.\n", i, cpu);
> -			break;
> -		case AFFINITY_USEALL:
> +		} else {
>  			cpu = cpu_for_thread_ua(i, max_cpus);
> -			break;
> -		default: cpu = -1;
>  		}
>  
>  		/* find the memory node associated with the cpu i */
> -- 
> 2.29.2
> 
> 
-Fixed spelling of "steering"
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [rt-tests v1 4/6] cyclictest: Mimik --smp behavior with --affinity

[John Kacur]

On Fri, 18 Dec 2020, Daniel Wagner wrote:

> Let --affinity without an argument behave in the same way as
> --smp. Run a thread on each CPU. This makes cyclictest behave as the
> rest of the rt-tests when --affinity is used.
> 
> Reviewed-by: Daniel Wagner <dwagner@suse.de>
> ---
>  src/cyclictest/cyclictest.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 0e6519125c2f..b9b0eb3575e3 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -1024,6 +1024,8 @@ static void process_options(int argc, char *argv[], int max_cpus)
>  				    argv[optind][0] == '0' ||
>  				    argv[optind][0] == '!')) {
>  				parse_cpumask(argv[optind], max_cpus, &affinity_mask);
> +			} else {
> +				num_threads = -1;
>  			}
>  
>  			if (verbose)
> -- 
> 2.29.2
> 
> 
Well, --smp historically combined -a -t and threads at the same priority
You could argue that it is reasonable for -a to automatically imply -t
but I have had debates with people about this and we settled on -a just 
specifies the affinity, and the default number of threads is one unless
you use -t.

I'm not sure what you mean by this makes cyclictest behave the way the 
rest of rt-tests does, the rest of rt-tests should match what cyclitest 
does. That said, I did some quick runs of signaltest and -a seems broken,
sigh.

Re: [rt-tests v1 4/6] cyclictest: Mimik --smp behavior with --affinity

[Daniel Wagner]

On Tue, Jan 26, 2021 at 12:55:33AM -0500, John Kacur wrote:
> Well, --smp historically combined -a -t and threads at the same priority
> You could argue that it is reasonable for -a to automatically imply -t
> but I have had debates with people about this and we settled on -a just 
> specifies the affinity, and the default number of threads is one unless
> you use -t.

My thinking is if you set an affinity mask, you want also to assign a
thread to the CPUs. If not what's the point to set an affinity mask
without a thread on it. Though the user is able to overwrite this
by providing the -t options along side the -a option.

> I'm not sure what you mean by this makes cyclictest behave the way the 
> rest of rt-tests does, the rest of rt-tests should match what cyclitest 
> does.

As I wrote, make cyclictest behave the same way as the to other tools
when -a/-t is used.

> That said, I did some quick runs of signaltest and -a seems broken,
> sigh.

Is it broken without my patches or with my patches? What is broken?

Re: [rt-tests v1 3/6] cyclictest: Use affinity_mask for stearing thread placement

[Daniel Wagner]

On Tue, Jan 26, 2021 at 12:39:02AM -0500, John Kacur wrote:
> Signed-off-by: John Kacur <jkacur@redhat.com>

Is there any reason why you picked this version of the patch? It was
also part of v2?

Re: [rt-tests v1 4/6] cyclictest: Mimik --smp behavior with --affinity

[John Kacur]

On Tue, 26 Jan 2021, Daniel Wagner wrote:

> On Tue, Jan 26, 2021 at 12:55:33AM -0500, John Kacur wrote:
> > Well, --smp historically combined -a -t and threads at the same priority
> > You could argue that it is reasonable for -a to automatically imply -t
> > but I have had debates with people about this and we settled on -a just 
> > specifies the affinity, and the default number of threads is one unless
> > you use -t.
> 
> My thinking is if you set an affinity mask, you want also to assign a
> thread to the CPUs. If not what's the point to set an affinity mask
> without a thread on it. Though the user is able to overwrite this
> by providing the -t options along side the -a option.

Not necessarily, but you want to limit where the threads, no matter
how few or how many run. As I said, it's not an unreasonable default,
but it is not the current default of 1 unless you specify -t
I suggest we stick with the current default in this clean-up effort.
We can discuss if there are some better defaults after this effort is 
complete.

> 
> > I'm not sure what you mean by this makes cyclictest behave the way the 
> > rest of rt-tests does, the rest of rt-tests should match what cyclitest 
> > does.
> 
> As I wrote, make cyclictest behave the same way as the to other tools
> when -a/-t is used.
> 
> > That said, I did some quick runs of signaltest and -a seems broken,
> > sigh.
> 
> Is it broken without my patches or with my patches? What is broken?
> 
> 
I haven't done a git bisect or something of that sort to determine if your 
patches broke it, but here's what I see. (running on non-rt laptop now, so 
ignore numbers), but it looks like at least the -t option requiring an 
argument existed before your changes, but your changes to affinity 
probably could use some more testing.

The following is okay

./signaltest  -a1-4
2.00 1.52 1.14 2/1079 6541          

T: 0 ( 6537) P: 0 C:  14896 Min:      3 Act:    4 Avg:   26 Max:     366


The following is broken, the default without an argument should be 2
./signaltest  -t
./signaltest: option requires an argument -- 't'

In the following note that threads other than thread 0 didn't appear
on the screen until after the ctrl-c

./signaltest -t9 -a1-4
1.11 1.14 1.14 2/1075 6864          

T: 0 ( 6856) P: 0 C:   1649 Min:     38 Act:  511 Avg:  457 Max:    4911
^CT: 0 ( 6856) P: 0 C:   1660 Min:     38 Act:  554 Avg:  457 Max:    4911
T: 1 ( 6857) P: 0 C:   1660 Min:     38 Act:  490 Avg: 1090 Max:   15024
T: 2 ( 6858) P: 0 C:   1660 Min:     38 Act:  449 Avg: 1091 Max:   15053
T: 3 ( 6859) P: 0 C:   1660 Min:     38 Act:  412 Avg: 1091 Max:   15075
T: 4 ( 6860) P: 0 C:   1660 Min:     38 Act:  378 Avg: 1090 Max:   15081
T: 5 ( 6861) P: 0 C:   1659 Min:     38 Act:  517 Avg: 1091 Max:   15072
T: 6 ( 6862) P: 0 C:   1659 Min:     38 Act:  485 Avg: 1091 Max:   15067
T: 7 ( 6863) P: 0 C:   1659 Min:     38 Act:  522 Avg: 1091 Max:   12832
T: 8 ( 6864) P: 0 C:   1659 Min:     38 Act:  549 Avg: 1091 Max:   12839


etc, it's broken in many ways.

Re: [rt-tests v1 3/6] cyclictest: Use affinity_mask for stearing thread placement

[John Kacur]

On Tue, 26 Jan 2021, Daniel Wagner wrote:

> On Tue, Jan 26, 2021 at 12:39:02AM -0500, John Kacur wrote:
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> 
> Is there any reason why you picked this version of the patch? It was
> also part of v2?
> 

I reviewed the correct version of your patch, which I pulled from git,
but when I went to reply to it, I replied to the wrong email, sorry for
the confusion.

John

Re: [rt-tests v1 4/6] cyclictest: Mimik --smp behavior with --affinity

[Daniel Wagner]

On Tue, Jan 26, 2021 at 11:32:25AM -0500, John Kacur wrote:
> Not necessarily, but you want to limit where the threads, no matter
> how few or how many run. As I said, it's not an unreasonable default,
> but it is not the current default of 1 unless you specify -t
> I suggest we stick with the current default in this clean-up effort.
> We can discuss if there are some better defaults after this effort is
> complete.

Understood. BTW, you can do '-a 1 -t' which will spawn N threads on CPU
1. So it's still possible to get the same setup.

> I haven't done a git bisect or something of that sort to determine if your
> patches broke it, but here's what I see. (running on non-rt laptop now, so
> ignore numbers), but it looks like at least the -t option requiring an
> argument existed before your changes, but your changes to affinity
> probably could use some more testing.
>
> The following is okay
>
> ./signaltest  -a1-4
> 2.00 1.52 1.14 2/1079 6541
>
> T: 0 ( 6537) P: 0 C:  14896 Min:      3 Act:    4 Avg:   26 Max:     366
>
>
> The following is broken, the default without an argument should be 2
> ./signaltest  -t
> ./signaltest: option requires an argument -- 't'

Right, this shouldn't be too hard to fix. I'll send an update.

> In the following note that threads other than thread 0 didn't appear
> on the screen until after the ctrl-c
>
> ./signaltest -t9 -a1-4
> 1.11 1.14 1.14 2/1075 6864
>
> T: 0 ( 6856) P: 0 C:   1649 Min:     38 Act:  511 Avg:  457 Max:    4911
> ^CT: 0 ( 6856) P: 0 C:   1660 Min:     38 Act:  554 Avg:  457 Max:    4911
> T: 1 ( 6857) P: 0 C:   1660 Min:     38 Act:  490 Avg: 1090 Max:   15024
> T: 2 ( 6858) P: 0 C:   1660 Min:     38 Act:  449 Avg: 1091 Max:   15053
> T: 3 ( 6859) P: 0 C:   1660 Min:     38 Act:  412 Avg: 1091 Max:   15075
> T: 4 ( 6860) P: 0 C:   1660 Min:     38 Act:  378 Avg: 1090 Max:   15081
> T: 5 ( 6861) P: 0 C:   1659 Min:     38 Act:  517 Avg: 1091 Max:   15072
> T: 6 ( 6862) P: 0 C:   1659 Min:     38 Act:  485 Avg: 1091 Max:   15067
> T: 7 ( 6863) P: 0 C:   1659 Min:     38 Act:  522 Avg: 1091 Max:   12832
> T: 8 ( 6864) P: 0 C:   1659 Min:     38 Act:  549 Avg: 1091 Max:   12839
>
>
> etc, it's broken in many ways.

I don't think I broke this. This is since I can remember the output
behavior. You only see the live update from T0, never the from the
helper threads.

Re: [rt-tests v1 3/6] cyclictest: Use affinity_mask for stearing thread placement

[Daniel Wagner]

On Tue, Jan 26, 2021 at 11:33:24AM -0500, John Kacur wrote:
> I reviewed the correct version of your patch, which I pulled from git,
> but when I went to reply to it, I replied to the wrong email, sorry for
> the confusion.

Ah, okay. I was very confused :)