rt-tests: cyclictest: Add support to report standard deviation

From: Punit Agrawal <[email protected]>

KVARS, KVARNAMELEN and KVALUELEN defines are not used in cyclictest. Drop them.

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 4 ----
 1 file changed, 4 deletions(-)

src/cyclictest/cyclictest.c +0 -4

index 2187d98de725..0c1e6617e0e1 100644
@@ -99,10 +99,6 @@ extern int clock_nanosleep(clockid_t __clock_id, int __flags,
 /* Must be power of 2 ! */
 #define VALBUF_SIZE		16384
 
-#define KVARS			32
-#define KVARNAMELEN		32
-#define KVALUELEN		32
-
 #if (defined(__i386__) || defined(__x86_64__))
 #define ARCH_HAS_SMI_COUNTER
 #endif

Signature · 1 line

2.32.0

From: Punit Agrawal <[email protected]>

KVARS, KVARNAMELEN and KVALUELEN defines are not used in cyclictest. Drop them.

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 4 ----
 1 file changed, 4 deletions(-)

src/cyclictest/cyclictest.c +0 -4

index 2187d98de725..0c1e6617e0e1 100644
@@ -99,10 +99,6 @@ extern int clock_nanosleep(clockid_t __clock_id, int __flags,
 /* Must be power of 2 ! */
 #define VALBUF_SIZE		16384
 
-#define KVARS			32
-#define KVARNAMELEN		32
-#define KVALUELEN		32
-
 #if (defined(__i386__) || defined(__x86_64__))
 #define ARCH_HAS_SMI_COUNTER
 #endif

Signature · 1 line

2.32.0

From: Punit Agrawal <[email protected]>

The timespec structure "stop" is initialised whether it is used or not as the compiler is not smart enough to figure out that it's use is always guarded by the "duration" variable. As a result, "stop" needs to be initialised whether it's used or not to avoid a compiler warning.

Replace the duplicate memset statements by initializing "stop" using structure initialiser.

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

src/cyclictest/cyclictest.c +1 -4

index 0c1e6617e0e1..d06ed01c58f4 100644
@@ -507,7 +507,7 @@ static void *timerthread(void *param)
 	struct sigevent sigev;
 	sigset_t sigset;
 	timer_t timer;
-	struct timespec now, next, interval, stop;
+	struct timespec now, next, interval, stop = { 0 };
 	struct itimerval itimer;
 	struct itimerspec tspec;
 	struct thread_stat *stat = par->stats;

@@ -516,8 +516,6 @@ static void *timerthread(void *param)
 	pthread_t thread;
 	unsigned long smi_now, smi_old = 0;
 
-	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);

@@ -598,7 +596,6 @@ static void *timerthread(void *param)
 	tsnorm(&next);
 
 	if (duration) {
-		memset(&stop, 0, sizeof(stop)); /* grrr */
 		stop = now;
 		stop.tv_sec += duration;
 	}

Signature · 1 line

2.32.0

From: Punit Agrawal <[email protected]>

The timespec structure "stop" is initialised whether it is used or not as the compiler is not smart enough to figure out that it's use is always guarded by the "duration" variable. As a result, "stop" needs to be initialised whether it's used or not to avoid a compiler warning.

Replace the duplicate memset statements by initializing "stop" using structure initialiser.

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

src/cyclictest/cyclictest.c +1 -4

index 0c1e6617e0e1..d06ed01c58f4 100644
@@ -507,7 +507,7 @@ static void *timerthread(void *param)
 	struct sigevent sigev;
 	sigset_t sigset;
 	timer_t timer;
-	struct timespec now, next, interval, stop;
+	struct timespec now, next, interval, stop = { 0 };
 	struct itimerval itimer;
 	struct itimerspec tspec;
 	struct thread_stat *stat = par->stats;

@@ -516,8 +516,6 @@ static void *timerthread(void *param)
 	pthread_t thread;
 	unsigned long smi_now, smi_old = 0;
 
-	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);

@@ -598,7 +596,6 @@ static void *timerthread(void *param)
 	tsnorm(&next);
 
 	if (duration) {
-		memset(&stop, 0, sizeof(stop)); /* grrr */
 		stop = now;
 		stop.tv_sec += duration;
 	}

Signature · 1 line

2.32.0

From: Punit Agrawal <[email protected]>

Two copies of "bufsize", initialised with the same value are declared in enclosed blocks. Remove the redundant declaration.

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 1 -
 1 file changed, 1 deletion(-)

src/cyclictest/cyclictest.c +0 -1

index 721d242a1da0..9c67a3ce3034 100644
@@ -2054,7 +2054,6 @@ int main(int argc, char **argv)
 			memset(stat->values, 0, bufsize);
 			par->bufmsk = VALBUF_SIZE - 1;
 			if (smi) {
-				int bufsize = VALBUF_SIZE * sizeof(long);
 				stat->smis = threadalloc(bufsize, node);
 				if (!stat->smis)
 					goto outall;

Signature · 1 line

2.32.0

Quoted 19 lines · Punit Agrawal

From: Punit Agrawal <[email protected]>

Two copies of "bufsize", initialised with the same value are declared in enclosed blocks. Remove the redundant declaration.

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 1 -
 1 file changed, 1 deletion(-)

src/cyclictest/cyclictest.c +0 -1

index 721d242a1da0..9c67a3ce3034 100644
@@ -2054,7 +2054,6 @@ int main(int argc, char **argv)
 			memset(stat->values, 0, bufsize);
 			par->bufmsk = VALBUF_SIZE - 1;
 			if (smi) {
-				int bufsize = VALBUF_SIZE * sizeof(long);

Quoted 7 lines

src/cyclictest/cyclictest.c +0 -0

 				stat->smis = threadalloc(bufsize, node);
 				if (!stat->smis)
 					goto outall;

Signature · 1 line

2.32.0

NACK: two different scopes, two different variables.

Quoted 21 lines · John Kacur

Quoted 19 lines · Punit Agrawal

From: Punit Agrawal <[email protected]>

Two copies of "bufsize", initialised with the same value are declared in enclosed blocks. Remove the redundant declaration.

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 1 -
 1 file changed, 1 deletion(-)

src/cyclictest/cyclictest.c +0 -1

index 721d242a1da0..9c67a3ce3034 100644
@@ -2054,7 +2054,6 @@ int main(int argc, char **argv)
 			memset(stat->values, 0, bufsize);
 			par->bufmsk = VALBUF_SIZE - 1;
 			if (smi) {
-				int bufsize = VALBUF_SIZE * sizeof(long);

Quoted 8 lines

Quoted 7 lines

src/cyclictest/cyclictest.c +0 -0

 				stat->smis = threadalloc(bufsize, node);
 				if (!stat->smis)
 					goto outall;

Signature · 1 line

2.32.0

NACK: two different scopes, two different variables.

If they had different values or if the inner scope was not dependent (nested) I would agree.

But no strong opinion - I'll drop this one.

From: Punit Agrawal <[email protected]>

Two copies of "bufsize", initialised with the same value are declared in enclosed blocks. Remove the redundant declaration.

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 1 -
 1 file changed, 1 deletion(-)

src/cyclictest/cyclictest.c +0 -1

index 721d242a1da0..9c67a3ce3034 100644
@@ -2054,7 +2054,6 @@ int main(int argc, char **argv)
 			memset(stat->values, 0, bufsize);
 			par->bufmsk = VALBUF_SIZE - 1;
 			if (smi) {
-				int bufsize = VALBUF_SIZE * sizeof(long);
 				stat->smis = threadalloc(bufsize, node);
 				if (!stat->smis)
 					goto outall;

Signature · 1 line

2.32.0

From: Punit Agrawal <[email protected]>

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 78 ++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 41 deletions(-)

src/cyclictest/cyclictest.c +37 -41

index 9c67a3ce3034..48a782fe167d 100644
@@ -220,10 +220,6 @@ static char jsonfile[MAX_PATH];
 static struct thread_param **parameters;
 static struct thread_stat **statistics;
 
-static void print_stat(FILE *fp, struct thread_param *par, int index, int verbose, int quiet);
-static void rstat_print_stat(struct thread_param *par, int index, int verbose, int quiet);
-static void rstat_setup(void);
-
 static int latency_target_fd = -1;
 static int32_t latency_target_value = 0;
 

@@ -1311,43 +1307,6 @@ static int check_timer(void)
 	return (ts.tv_sec != 0 || ts.tv_nsec != 1);
 }
 
-static void sighand(int sig)
-{
-	if (sig == SIGUSR1) {
-		int i;
-		int oldquiet = quiet;
-
-		quiet = 0;
-		fprintf(stderr, "#---------------------------\n");
-		fprintf(stderr, "# cyclictest current status:\n");
-		for (i = 0; i < num_threads; i++)
-			print_stat(stderr, parameters[i], i, 0, 0);
-		fprintf(stderr, "#---------------------------\n");
-		quiet = oldquiet;
-		return;
-	} else if (sig == SIGUSR2) {
-		int i;
-		int oldquiet = quiet;
-
-		if (rstat_fd == -1) {
-			fprintf(stderr, "ERROR: rstat_fd not valid\n");
-			return;
-		}
-		rstat_ftruncate(rstat_fd, 0);
-		quiet = 0;
-		dprintf(rstat_fd, "#---------------------------\n");
-		dprintf(rstat_fd, "# cyclictest current status:\n");
-		for (i = 0; i < num_threads; i++)
-			rstat_print_stat(parameters[i], i, 0, 0);
-		dprintf(rstat_fd, "#---------------------------\n");
-		quiet = oldquiet;
-		return;
-	}
-	shutdown = 1;
-	if (refresh_on_max)
-		pthread_cond_signal(&refresh_on_max_cond);
-}
-
 static void print_tids(struct thread_param *par[], int nthreads)
 {
 	int i;

@@ -1566,6 +1525,43 @@ static void rstat_print_stat(struct thread_param *par, int index, int verbose, i
 }
 
 
+static void sighand(int sig)
+{
+	if (sig == SIGUSR1) {
+		int i;
+		int oldquiet = quiet;
+
+		quiet = 0;
+		fprintf(stderr, "#---------------------------\n");
+		fprintf(stderr, "# cyclictest current status:\n");
+		for (i = 0; i < num_threads; i++)
+			print_stat(stderr, parameters[i], i, 0, 0);
+		fprintf(stderr, "#---------------------------\n");
+		quiet = oldquiet;
+		return;
+	} else if (sig == SIGUSR2) {
+		int i;
+		int oldquiet = quiet;
+
+		if (rstat_fd == -1) {
+			fprintf(stderr, "ERROR: rstat_fd not valid\n");
+			return;
+		}
+		rstat_ftruncate(rstat_fd, 0);
+		quiet = 0;
+		dprintf(rstat_fd, "#---------------------------\n");
+		dprintf(rstat_fd, "# cyclictest current status:\n");
+		for (i = 0; i < num_threads; i++)
+			rstat_print_stat(parameters[i], i, 0, 0);
+		dprintf(rstat_fd, "#---------------------------\n");
+		quiet = oldquiet;
+		return;
+	}
+	shutdown = 1;
+	if (refresh_on_max)
+		pthread_cond_signal(&refresh_on_max_cond);
+}
+
 /*
  * thread that creates a named fifo and hands out run stats when someone
  * reads from the fifo.

Signature · 1 line

2.32.0

Quoted 109 lines · Punit Agrawal

From: Punit Agrawal <[email protected]>

Signed-off-by Punit Agrawal <[email protected]>

---
 src/cyclictest/cyclictest.c | 78 ++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 41 deletions(-)

src/cyclictest/cyclictest.c +37 -41

index 9c67a3ce3034..48a782fe167d 100644
@@ -220,10 +220,6 @@ static char jsonfile[MAX_PATH];
 static struct thread_param **parameters;
 static struct thread_stat **statistics;
 
-static void print_stat(FILE *fp, struct thread_param *par, int index, int verbose, int quiet);
-static void rstat_print_stat(struct thread_param *par, int index, int verbose, int quiet);
-static void rstat_setup(void);
-
 static int latency_target_fd = -1;
 static int32_t latency_target_value = 0;
 

@@ -1311,43 +1307,6 @@ static int check_timer(void)
 	return (ts.tv_sec != 0 || ts.tv_nsec != 1);
 }
 
-static void sighand(int sig)
-{
-	if (sig == SIGUSR1) {
-		int i;
-		int oldquiet = quiet;
-
-		quiet = 0;
-		fprintf(stderr, "#---------------------------\n");
-		fprintf(stderr, "# cyclictest current status:\n");
-		for (i = 0; i < num_threads; i++)
-			print_stat(stderr, parameters[i], i, 0, 0);
-		fprintf(stderr, "#---------------------------\n");
-		quiet = oldquiet;
-		return;
-	} else if (sig == SIGUSR2) {
-		int i;
-		int oldquiet = quiet;
-
-		if (rstat_fd == -1) {
-			fprintf(stderr, "ERROR: rstat_fd not valid\n");
-			return;
-		}
-		rstat_ftruncate(rstat_fd, 0);
-		quiet = 0;
-		dprintf(rstat_fd, "#---------------------------\n");
-		dprintf(rstat_fd, "# cyclictest current status:\n");
-		for (i = 0; i < num_threads; i++)
-			rstat_print_stat(parameters[i], i, 0, 0);
-		dprintf(rstat_fd, "#---------------------------\n");
-		quiet = oldquiet;
-		return;
-	}
-	shutdown = 1;
-	if (refresh_on_max)
-		pthread_cond_signal(&refresh_on_max_cond);
-}
-
 static void print_tids(struct thread_param *par[], int nthreads)
 {
 	int i;

@@ -1566,6 +1525,43 @@ static void rstat_print_stat(struct thread_param *par, int index, int verbose, i
 }
 
 
+static void sighand(int sig)
+{
+	if (sig == SIGUSR1) {
+		int i;
+		int oldquiet = quiet;
+
+		quiet = 0;
+		fprintf(stderr, "#---------------------------\n");
+		fprintf(stderr, "# cyclictest current status:\n");
+		for (i = 0; i < num_threads; i++)
+			print_stat(stderr, parameters[i], i, 0, 0);
+		fprintf(stderr, "#---------------------------\n");
+		quiet = oldquiet;
+		return;
+	} else if (sig == SIGUSR2) {
+		int i;
+		int oldquiet = quiet;
+
+		if (rstat_fd == -1) {
+			fprintf(stderr, "ERROR: rstat_fd not valid\n");
+			return;
+		}
+		rstat_ftruncate(rstat_fd, 0);
+		quiet = 0;
+		dprintf(rstat_fd, "#---------------------------\n");
+		dprintf(rstat_fd, "# cyclictest current status:\n");
+		for (i = 0; i < num_threads; i++)
+			rstat_print_stat(parameters[i], i, 0, 0);
+		dprintf(rstat_fd, "#---------------------------\n");
+		quiet = oldquiet;
+		return;
+	}
+	shutdown = 1;
+	if (refresh_on_max)
+		pthread_cond_signal(&refresh_on_max_cond);
+}
+
 /*
  * thread that creates a named fifo and hands out run stats when someone

Quoted 5 lines

src/cyclictest/cyclictest.c +0 -0

  * reads from the fifo.

Signature · 1 line

2.32.0

NACK: What's wrong with function declarations? They allow the freedeom to put your function where you wish. This patch is unnecessary churn, sorry.

Hi Punit,

src/cyclictest/cyclictest.c static void write_stats(FILE *f, void *data) · line 1816

Quoted 8 lines · Punit Agrawal

src/cyclictest/cyclictest.c +1 -0

@@ -1781,6 +1810,7 @@ static void write_stats(FILE *f, void *data)
 		fprintf(f, "      \"min\": %ld,\n", s->min);
 		fprintf(f, "      \"max\": %ld,\n", s->max);
 		fprintf(f, "      \"avg\": %.2f,\n", s->avg);
+		fprintf(f, "      \"stddev\": %.2f,\n", calc_stddev(s));
 		fprintf(f, "      \"cpu\": %d,\n", par[i]->cpu);
 		fprintf(f, "      \"node\": %d\n", par[i]->node);
 		fprintf(f, "    }%s\n", i == num_threads - 1 ? "" : ",");

I think in this case you should also increase the version number of the JSON file.

And while at it, I'd love to see the same stats fixes/extension for the other tools, not just cyclictest.

Sure, I can bump up the version to 1.1 - minor update as it's adding a new field rather than changing existing ones. Or "2" if that is preferred.

I can look at adding support for standard deviation to the other tools if there's interest once the cyclictest changes have been merged.

Thanks for taking a look.

Hi,

Standard deviation is an important measure to evaluate the latency response of a real time system. It is calculated by downstream tools such as rteval but the value cannot be correctly calculated when the latency value exceeds the range of histogram.

This series adds support to report the streaming standard deviation of latencies to cyclictest. This approach avoids having to track all latency values and adds minimal overhead (two additional doubles and compute steps) to each iteration.

In terms of patch organization, the first 5 patches are clean-ups that were noticed in the course of developing the support for standard deviation. It should be possible to apply them indepndently.

Patch 6 converts the existing average calculation to a streaming version. This running average value is needed for the standard deviation.

Patch 7 finally adds support for calculating standard deviation.

The changes were verified by capturing the latency samples and verifying the average and standard deviation with manual calculation.

All feedback welcome.

Thanks,
Punit

Punit Agrawal (7):
rt-tests: cyclictest: Drop unused defines
rt-tests: cyclictest: Simplify duplicate initialization of "stop"
rt-tests: cyclictest: Drop unnecessary variable "stopped"
rt-tests: cyclictest: Drop unnecessary variable "bufsize"
rt-tests: cyclictest: Move signal handler to avoid function
declaration
rt-tests: cyclictest: Use streaming algorithm to calculate averages
rt-tests: cyclictest: Add support to report standard deviation

 Makefile                    |   2 +-
 src/cyclictest/cyclictest.c | 149 +++++++++++++++++++++---------------
 2 files changed, 87 insertions(+), 64 deletions(-)

--
2.32.0

Quoted 41 lines · Punit Agrawal

Hi,

Standard deviation is an important measure to evaluate the latency response of a real time system. It is calculated by downstream tools such as rteval but the value cannot be correctly calculated when the latency value exceeds the range of histogram.

This series adds support to report the streaming standard deviation of latencies to cyclictest. This approach avoids having to track all latency values and adds minimal overhead (two additional doubles and compute steps) to each iteration.

In terms of patch organization, the first 5 patches are clean-ups that were noticed in the course of developing the support for standard deviation. It should be possible to apply them indepndently.

Patch 6 converts the existing average calculation to a streaming version. This running average value is needed for the standard deviation.

Patch 7 finally adds support for calculating standard deviation.

The changes were verified by capturing the latency samples and verifying the average and standard deviation with manual calculation.

All feedback welcome.

Thanks,
Punit

Punit Agrawal (7):
rt-tests: cyclictest: Drop unused defines
rt-tests: cyclictest: Simplify duplicate initialization of "stop"
rt-tests: cyclictest: Drop unnecessary variable "stopped"
rt-tests: cyclictest: Drop unnecessary variable "bufsize"
rt-tests: cyclictest: Move signal handler to avoid function
declaration
rt-tests: cyclictest: Use streaming algorithm to calculate averages
rt-tests: cyclictest: Add support to report standard deviation

Quoted 7 lines

 Makefile                    |   2 +-
 src/cyclictest/cyclictest.c | 149 +++++++++++++++++++++---------------
 2 files changed, 87 insertions(+), 64 deletions(-)

--
2.32.0

Hi Punit,

Thanks for your contributions!

I noticed there were no commit messages for patches 1 and 5.