[PATCH 0/4] rt-tests: cyclicdeadline: Add histogram support

[PATCH 0/4] rt-tests: cyclicdeadline: Add histogram support

[Crystal Wood]

Add histogram support to cyclicdeadline, with the goal of supporting
cyclicdeadline in rteval.  New output-compatible (except for the change
in patch 2/4) histogram code is used, to ease sharing.  The new code is
intended to also be usable by other measurers such as oslat.

Crystal Wood (4):
  rt-tests: Fix warnings
  rt-tests: cyclictest: Remove histogram totals
  rt-tests: cyclictest: Replace histogram code with library
  rt-tests: cyclicdeadline: Add histogram support

 Makefile                            |   5 +-
 src/cyclictest/cyclictest.c         |  94 ++++-----------
 src/hackbench/hackbench.c           |   2 +-
 src/include/histogram.h             |  42 +++++++
 src/lib/histogram.c                 | 174 ++++++++++++++++++++++++++++
 src/sched_deadline/cyclicdeadline.c | 119 ++++++++++++++++---
 src/sched_deadline/deadline_test.c  |  10 +-
 src/sigwaittest/sigwaittest.c       |   6 +-
 8 files changed, 357 insertions(+), 95 deletions(-)
 create mode 100644 src/include/histogram.h
 create mode 100644 src/lib/histogram.c

-- 
2.43.0

[PATCH 1/4] rt-tests: Fix warnings

[Crystal Wood]

Numerous places threw sign comparison warnings; we could fix them but
it's kind of an obnoxious warning that requires casts to deal with things
such as ARRAY_SIZE() while still being able to check for the user
entering a negative number.

-Wunused-parameter is another obnoxious warning as it flags perfectly
reasonable code that takes unneeded parameters in order to comply with
a function pointer interface or similar; however, all of the instances
that were flagged here were actual dead parameters, so just fix them.

Add volatile to timer_started in hackbench so that it doesn't get
clobbered by longjmp().

Signed-off-by: Crystal Wood <crwood@redhat.com>
--
Let me know if you'd rather I fix the sign warnings.
---
 Makefile                            |  2 +-
 src/hackbench/hackbench.c           |  2 +-
 src/sched_deadline/cyclicdeadline.c |  6 +++---
 src/sched_deadline/deadline_test.c  | 10 +++++-----
 src/sigwaittest/sigwaittest.c       |  6 +++---
 5 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index 2808c212058a..ad481a73cf93 100644
--- a/Makefile
+++ b/Makefile
@@ -31,7 +31,7 @@ prefix  ?= /usr/local
 bindir  ?= $(prefix)/bin
 mandir	?= $(prefix)/share/man
 
-CFLAGS ?= -Wall -Wno-nonnull -Wextra
+CFLAGS ?= -Wall -Wno-nonnull -Wextra -Wno-sign-compare
 CPPFLAGS += -D_GNU_SOURCE -Isrc/include
 LDFLAGS ?=
 
diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
index 69dd5f087fb6..4430db0e4ed6 100644
--- a/src/hackbench/hackbench.c
+++ b/src/hackbench/hackbench.c
@@ -494,7 +494,7 @@ int main(int argc, char *argv[])
 	struct timeval start, stop, diff;
 	int readyfds[2], wakefds[2];
 	char dummy;
-	int timer_started = 0;
+	volatile int timer_started = 0;
 	struct sched_param sp;
 
 	process_options (argc, argv);
diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
index 9bdc4b5deaf1..097e2e5d4580 100644
--- a/src/sched_deadline/cyclicdeadline.c
+++ b/src/sched_deadline/cyclicdeadline.c
@@ -750,7 +750,7 @@ static void print_stat(FILE *fp, struct sched_data *sd, int index, int verbose,
 	}
 }
 
-static u64 do_runtime(long tid, struct sched_data *sd, u64 period)
+static u64 do_runtime(struct sched_data *sd, u64 period)
 {
 	struct thread_stat *stat = &sd->stat;
 	u64 next_period = period + sd->deadline_us;
@@ -833,7 +833,7 @@ void *run_deadline(void *data)
 	period = get_time_us();
 
 	while (!shutdown) {
-		period = do_runtime(tid, sd, period);
+		period = do_runtime(sd, period);
 		if (tracelimit && (stat->max > tracelimit)) {
 			shutdown++;
 			pthread_mutex_lock(&break_thread_id_lock);
@@ -1266,7 +1266,7 @@ int main(int argc, char **argv)
 
 		/* Make sure that we can make our deadlines */
 		start_period = get_time_us();
-		do_runtime(gettid(), sd, start_period);
+		do_runtime(sd, start_period);
 		end_period = get_time_us();
 		if (end_period - start_period > sd->runtime_us)
 			fatal("Failed to perform task within runtime: Missed by %lld us\n",
diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
index cd8ef01f7d68..ca2da476ec95 100644
--- a/src/sched_deadline/deadline_test.c
+++ b/src/sched_deadline/deadline_test.c
@@ -1181,7 +1181,7 @@ static int read_ctx_switches(int *vol, int *nonvol, int *migrate)
  *  @data->total_time - Total time it took to complete all loops
  *  @data->nr_periods - Number of periods that were executed.
  */
-static u64 do_runtime(long tid, struct sched_data *data, u64 period)
+static u64 do_runtime(struct sched_data *data, u64 period)
 {
 	u64 next_period = period + data->deadline_us;
 	u64 now = get_time_us();
@@ -1354,7 +1354,7 @@ void *run_deadline(void *data)
 	period = get_time_us();
 
 	while (!done) {
-		period = do_runtime(tid, sched_data, period);
+		period = do_runtime(sched_data, period);
 		sched_yield();
 	}
 	ret = sched_getattr(0, &attr, sizeof(attr), 0);
@@ -1714,7 +1714,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
 	do_sleep(1000);
 
 	start = get_time_us();
-	do_runtime(0, &sd, start + sd.deadline_us);
+	do_runtime(&sd, start + sd.deadline_us);
 	end = get_time_us();
 
 	diff = end - start;
@@ -1743,7 +1743,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
 	do_sleep(1000);
 
 	start = get_time_us();
-	do_runtime(0, &sd, start + sd.deadline_us);
+	do_runtime(&sd, start + sd.deadline_us);
 	end = get_time_us();
 
 	odiff = end - start;
@@ -1962,7 +1962,7 @@ int main(int argc, char **argv)
 
 		/* Make sure that we can make our deadlines */
 		start_period = get_time_us();
-		do_runtime(gettid(), sd, start_period);
+		do_runtime(sd, start_period);
 		end_period = get_time_us();
 		if (end_period - start_period > sd->runtime_us) {
 			printf("Failed to perform task within runtime: Missed by %lld us\n",
diff --git a/src/sigwaittest/sigwaittest.c b/src/sigwaittest/sigwaittest.c
index 55855769c63b..8c1c16fb3081 100644
--- a/src/sigwaittest/sigwaittest.c
+++ b/src/sigwaittest/sigwaittest.c
@@ -375,7 +375,7 @@ static void sighand(int sig __attribute__ ((unused)))
 	mustshutdown = 1;
 }
 
-static void print_stat(FILE *fp, struct params *receiver, struct params *sender,
+static void print_stat(struct params *receiver, struct params *sender,
 		       int verbose __attribute__ ((unused)), int quiet)
 {
 	int i;
@@ -644,7 +644,7 @@ int main(int argc, char *argv[])
 			    sender[i].shutdown;
 
 		if (receiver[0].samples > oldsamples || mustshutdown) {
-			print_stat(stdout, receiver, sender, 0, quiet);
+			print_stat(receiver, sender, 0, quiet);
 			if (!quiet)
 				printf("\033[%dA", num_threads*2);
 		}
@@ -664,7 +664,7 @@ int main(int argc, char *argv[])
 	if (!quiet)
 		printf("\033[%dB", num_threads*2 + 2);
 	else
-		print_stat(stdout, receiver, sender, 0, 0);
+		print_stat(receiver, sender, 0, 0);
 
 	for (i = 0; i < num_threads; i++) {
 		receiver[i].shutdown = 1;
-- 
2.43.0

[PATCH 2/4] rt-tests: cyclictest: Remove histogram totals

[Crystal Wood]

The Total: line does not seem to contribute much value, as it should just
be the number of buckets minus the number of overflows.  Unless someone
complains, remove it to simplify moving to common histogram code.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
 src/cyclictest/cyclictest.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index a8039b49feb6..93ce201e9fca 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -1407,12 +1407,9 @@ static void print_tids(struct thread_param *par[], int nthreads)
 static void print_hist(struct thread_param *par[], int nthreads)
 {
 	int i, j;
-	unsigned long long int log_entries[nthreads+1];
 	unsigned long maxmax, alloverflows;
 	FILE *fd;
 
-	bzero(log_entries, sizeof(log_entries));
-
 	if (use_histfile) {
 		fd = fopen(histfile, "w");
 		if (!fd) {
@@ -1434,21 +1431,12 @@ static void print_hist(struct thread_param *par[], int nthreads)
 			fprintf(fd, "%06lu", curr_latency);
 			if (j < nthreads - 1)
 				fprintf(fd, "\t");
-			log_entries[j] += curr_latency;
 			allthreads += curr_latency;
 		}
-		if (histofall && nthreads > 1) {
+		if (histofall && nthreads > 1)
 			fprintf(fd, "\t%06llu", allthreads);
-			log_entries[nthreads] += allthreads;
-		}
 		fprintf(fd, "\n");
 	}
-	fprintf(fd, "# Total:");
-	for (j = 0; j < nthreads; j++)
-		fprintf(fd, " %09llu", log_entries[j]);
-	if (histofall && nthreads > 1)
-		fprintf(fd, " %09llu", log_entries[nthreads]);
-	fprintf(fd, "\n");
 	fprintf(fd, "# Min Latencies:");
 	for (j = 0; j < nthreads; j++)
 		fprintf(fd, " %05lu", par[j]->stats->min);
-- 
2.43.0

[PATCH 3/4] rt-tests: cyclictest: Replace histogram code with library

[Crystal Wood]

The new code is also intended to be used by cyclicdeadline, and possibly
oslat and other tests.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
 Makefile                    |   3 +-
 src/cyclictest/cyclictest.c |  82 +++++------------
 src/include/histogram.h     |  42 +++++++++
 src/lib/histogram.c         | 174 ++++++++++++++++++++++++++++++++++++
 4 files changed, 241 insertions(+), 60 deletions(-)
 create mode 100644 src/include/histogram.h
 create mode 100644 src/lib/histogram.c

diff --git a/Makefile b/Makefile
index ad481a73cf93..9502adea49cf 100644
--- a/Makefile
+++ b/Makefile
@@ -178,7 +178,8 @@ oslat: $(OBJDIR)/oslat.o $(OBJDIR)/librttest.a $(OBJDIR)/librttestnuma.a
 %.8.bz2: %.8
 	bzip2 -c $< > $@
 
-LIBOBJS =$(addprefix $(OBJDIR)/,rt-error.o rt-get_cpu.o rt-sched.o rt-utils.o)
+LIBOBJS =$(addprefix $(OBJDIR)/,rt-error.o rt-get_cpu.o rt-sched.o rt-utils.o \
+	histogram.o)
 $(OBJDIR)/librttest.a: $(LIBOBJS)
 	$(AR) rcs $@ $^
 
diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 93ce201e9fca..6169170fc66d 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -35,6 +35,7 @@
 #include "rt-utils.h"
 #include "rt-numa.h"
 #include "rt-error.h"
+#include "histogram.h"
 
 #include <bionic.h>
 
@@ -133,16 +134,13 @@ struct thread_stat {
 	double avg;
 	long *values;
 	long *smis;
-	long *hist_array;
-	long *outliers;
+	struct histogram *hist;
 	pthread_t thread;
 	int threadstarted;
 	int tid;
 	long reduce;
 	long redmax;
 	long cycleofmax;
-	long hist_overflow;
-	long num_outliers;
 	unsigned long smi_count;
 };
 
@@ -216,6 +214,7 @@ static char jsonfile[MAX_PATH];
 
 static struct thread_param **parameters;
 static struct thread_stat **statistics;
+static struct histoset hset;
 
 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);
@@ -777,15 +776,8 @@ static void *timerthread(void *param)
 		}
 
 		/* Update the histogram */
-		if (histogram) {
-			if (diff >= histogram) {
-				stat->hist_overflow++;
-				if (stat->num_outliers < histogram)
-					stat->outliers[stat->num_outliers++] = stat->cycles;
-			} else {
-				stat->hist_array[diff]++;
-			}
-		}
+		if (histogram)
+			hist_sample(stat->hist, diff);
 
 		stat->cycles++;
 
@@ -1422,19 +1414,13 @@ static void print_hist(struct thread_param *par[], int nthreads)
 
 	fprintf(fd, "# Histogram\n");
 	for (i = 0; i < histogram; i++) {
-		unsigned long long int allthreads = 0;
+		unsigned long flags = 0;
 
 		fprintf(fd, "%06d ", i);
 
-		for (j = 0; j < nthreads; j++) {
-			unsigned long curr_latency=par[j]->stats->hist_array[i];
-			fprintf(fd, "%06lu", curr_latency);
-			if (j < nthreads - 1)
-				fprintf(fd, "\t");
-			allthreads += curr_latency;
-		}
-		if (histofall && nthreads > 1)
-			fprintf(fd, "\t%06llu", allthreads);
+		if (histofall)
+			flags |= HSET_PRINT_SUM;
+		hset_print_bucket(&hset, fd, i, flags);
 		fprintf(fd, "\n");
 	}
 	fprintf(fd, "# Min Latencies:");
@@ -1459,8 +1445,8 @@ static void print_hist(struct thread_param *par[], int nthreads)
 	fprintf(fd, "# Histogram Overflows:");
 	alloverflows = 0;
 	for (j = 0; j < nthreads; j++) {
-		fprintf(fd, " %05lu", par[j]->stats->hist_overflow);
-		alloverflows += par[j]->stats->hist_overflow;
+		fprintf(fd, " %05lu", par[j]->stats->hist->oflow_count);
+		alloverflows += par[j]->stats->hist->oflow_count;
 	}
 	if (histofall && nthreads > 1)
 		fprintf(fd, " %05lu", alloverflows);
@@ -1468,11 +1454,8 @@ static void print_hist(struct thread_param *par[], int nthreads)
 
 	fprintf(fd, "# Histogram Overflow at cycle number:\n");
 	for (i = 0; i < nthreads; i++) {
-		fprintf(fd, "# Thread %d:", i);
-		for (j = 0; j < par[i]->stats->num_outliers; j++)
-			fprintf(fd, " %05lu", par[i]->stats->outliers[j]);
-		if (par[i]->stats->num_outliers < par[i]->stats->hist_overflow)
-			fprintf(fd, " # %05lu others", par[i]->stats->hist_overflow - par[i]->stats->num_outliers);
+		fprintf(fd, "# Thread %d: ", i);
+		hist_print_oflows(par[i]->stats->hist, fd);
 		fprintf(fd, "\n");
 	}
 	if (smi) {
@@ -1788,8 +1771,7 @@ rstat_err:
 static void write_stats(FILE *f, void *data __attribute__ ((unused)))
 {
 	struct thread_param **par = parameters;
-	int i, j;
-	unsigned comma;
+	int i;
 	struct thread_stat *s;
 
 	fprintf(f, "  \"num_threads\": %d,\n", num_threads);
@@ -1800,15 +1782,7 @@ static void write_stats(FILE *f, void *data __attribute__ ((unused)))
 
 		fprintf(f, "      \"histogram\": {");
 		s = par[i]->stats;
-		for (j = 0, comma = 0; j < histogram; j++) {
-			if (s->hist_array[j] == 0)
-				continue;
-			fprintf(f, "%s", comma ? ",\n" : "\n");
-			fprintf(f, "        \"%u\": %ld", j, s->hist_array[j]);
-			comma = 1;
-		}
-		if (comma)
-			fprintf(f, "\n");
+		hist_print_json(par[i]->stats->hist, f);
 		fprintf(f, "      },\n");
 		fprintf(f, "      \"cycles\": %ld,\n", s->cycles);
 		fprintf(f, "      \"min\": %ld,\n", s->min);
@@ -1991,6 +1965,10 @@ int main(int argc, char **argv)
 	/* Set-up shm */
 	rstat_setup();
 
+	if (histogram && hset_init(&hset, num_threads, 1, histogram, histogram))
+		fatal("failed to allocate histogram of size %d for %d threads\n",
+		      histogram, num_threads);
+
 	parameters = calloc(num_threads, sizeof(struct thread_param *));
 	if (!parameters)
 		goto out;
@@ -2066,18 +2044,8 @@ int main(int argc, char **argv)
 			fatal("error allocating thread status struct for thread %d\n", i);
 		memset(stat, 0, sizeof(struct thread_stat));
 
-		/* allocate the histogram if requested */
-		if (histogram) {
-			int bufsize = histogram * sizeof(long);
-
-			stat->hist_array = threadalloc(bufsize, node);
-			stat->outliers = threadalloc(bufsize, node);
-			if (stat->hist_array == NULL || stat->outliers == NULL)
-				fatal("failed to allocate histogram of size %d on node %d\n",
-				      histogram, i);
-			memset(stat->hist_array, 0, bufsize);
-			memset(stat->outliers, 0, bufsize);
-		}
+		if (histogram)
+			stat->hist = &hset.histos[i];
 
 		if (verbose) {
 			int bufsize = VALBUF_SIZE * sizeof(long);
@@ -2215,13 +2183,8 @@ int main(int argc, char **argv)
 	if (trigger)
 		trigger_print();
 
-	if (histogram) {
+	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);
-		}
-	}
 
 	if (tracelimit) {
 		print_tids(parameters, num_threads);
@@ -2263,5 +2226,6 @@ int main(int argc, char **argv)
 	if (rstat_fd >= 0)
 		shm_unlink(shm_name);
 
+	hset_destroy(&hset);
 	exit(ret);
 }
diff --git a/src/include/histogram.h b/src/include/histogram.h
new file mode 100644
index 000000000000..c7aba68ffb99
--- /dev/null
+++ b/src/include/histogram.h
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <stdint.h>
+#include <stdio.h>
+
+struct histogram {
+	unsigned long *buckets;
+	unsigned long width;		// interval covered by one bucket
+	unsigned long num;		// number of buckets
+	unsigned long events;		// number of events logged
+
+	unsigned long *oflows;		// events when overflow happened
+	unsigned long oflow_bufsize;	// number of overflows that can be logged
+	unsigned long oflow_count;	// number of events that overflowed
+	uint64_t oflow_magnitude;	// sum of how many buckets overflowed by
+};
+
+struct histoset {
+	struct histogram *histos;	// Group of related histograms (e.g. per cpu)
+	struct histogram *sum;		// Accumulates events from all histos
+	unsigned long num_histos;	// Not including sum
+	unsigned long num_buckets;
+};
+
+#define HIST_OVERFLOW		1
+#define HIST_OVERFLOW_MAG	2
+#define HIST_OVERFLOW_LOG	4
+
+int hist_init(struct histogram *h, unsigned long width, unsigned long num);
+int hist_init_oflow(struct histogram *h, unsigned long num);
+void hist_destroy(struct histogram *h);
+int hist_sample(struct histogram *h, uint64_t sample);
+
+#define HSET_PRINT_SUM		1
+#define HSET_PRINT_JSON		2
+
+int hset_init(struct histoset *hs, unsigned long histos, unsigned long bucket_width,
+	      unsigned long num_buckets, unsigned long overflow);
+void hset_destroy(struct histoset *hs);
+void hset_print_bucket(struct histoset *hs, FILE *f, unsigned long bucket,
+		       unsigned long flags);
+void hist_print_json(struct histogram *h, FILE *f);
+void hist_print_oflows(struct histogram *h, FILE *f);
diff --git a/src/lib/histogram.c b/src/lib/histogram.c
new file mode 100644
index 000000000000..ebc7bbaa02aa
--- /dev/null
+++ b/src/lib/histogram.c
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Latency histograms
+ *
+ * Copyright 2023 Red Hat Inc.
+ */
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
+#include "histogram.h"
+
+int hist_init(struct histogram *h, unsigned long width, unsigned long num)
+{
+	memset(h, 0, sizeof(*h));
+	h->width = width;
+	h->num = num;
+
+	h->buckets = calloc(num, sizeof(unsigned long));
+	if (!h->buckets)
+		return -ENOMEM;
+
+	return 0;
+}
+
+int hist_init_oflow(struct histogram *h, unsigned long num)
+{
+	h->oflow_bufsize = num;
+	h->oflows = calloc(num, sizeof(unsigned long));
+	if (!h->oflows)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void hist_destroy(struct histogram *h)
+{
+	free(h->oflows);
+	h->oflows = NULL;
+	free(h->buckets);
+	h->buckets = NULL;
+}
+
+int hist_sample(struct histogram *h, uint64_t sample)
+{
+	unsigned long bucket = sample / h->width;
+	unsigned long extra;
+	unsigned long event = h->events++;
+	int ret;
+
+	if (bucket < h->num) {
+		h->buckets[bucket]++;
+		return 0;
+	}
+
+	ret = HIST_OVERFLOW;
+	extra = bucket - h->num;
+	if (h->oflow_magnitude + extra > h->oflow_magnitude) {
+		h->oflow_magnitude += extra;
+	} else {
+		ret |= HIST_OVERFLOW_MAG;
+	}
+		
+	if (h->oflows) {
+		if (h->oflow_count < h->oflow_bufsize) {
+			h->oflows[h->oflow_count] = event;
+		} else {
+			ret |= HIST_OVERFLOW_LOG;
+		}
+	}
+
+	h->oflow_count++;
+	return ret;
+}
+
+int hset_init(struct histoset *hs, unsigned long num_histos,
+	      unsigned long bucket_width, unsigned long num_buckets,
+	      unsigned long overflow)
+{
+	unsigned long i;
+
+	if (num_histos == 0)
+		return -EINVAL;
+
+	hs->num_histos = num_histos;
+	hs->num_buckets = num_buckets;
+	hs->histos = calloc(num_histos, sizeof(struct histogram));
+	if (!hs->histos)
+		return -ENOMEM;
+
+	for (i = 0; i < num_histos; i++) {
+		if (hist_init(&hs->histos[i], bucket_width, num_buckets))
+			goto fail;
+		if (overflow && hist_init_oflow(&hs->histos[i], overflow))
+			goto fail;
+	}
+
+	return 0;
+
+fail:
+	hset_destroy(hs);
+	return -ENOMEM;
+}
+
+void hset_destroy(struct histoset *hs)
+{
+	unsigned long i;
+
+	if (hs->histos) {
+		for (i = 0; i < hs->num_histos; i++)
+			hist_destroy(&hs->histos[i]);
+	}
+
+	free(hs->histos);
+	hs->histos = NULL;
+}
+
+void hset_print_bucket(struct histoset *hs, FILE *f, unsigned long bucket,
+		       unsigned long flags)
+{
+	unsigned long long sum = 0;
+	unsigned long i;
+
+	if (bucket >= hs->num_buckets)
+		return;
+
+	for (i = 0; i < hs->num_histos; i++) {
+		unsigned long val = hs->histos[i].buckets[bucket];
+
+		sum += val;
+		if (i != 0)
+			fprintf(f, "\t");
+		fprintf(f, "%06lu", val);
+	}
+
+	if (flags & HSET_PRINT_SUM)
+		fprintf(f, "\t%06llu", sum);
+}
+
+void hist_print_json(struct histogram *h, FILE *f)
+{
+	unsigned long i;
+	bool comma = false;
+
+	for (i = 0; i < h->num; i++) {
+		unsigned long val = h->buckets[i];
+
+		if (val != 0) {
+			if (comma)
+				fprintf(f, ",");
+			fprintf(f, "\n        \"%lu\": %lu", i, val);
+			comma = true;
+		}
+	}
+
+	fprintf(f, "\n");
+}
+
+void hist_print_oflows(struct histogram *h, FILE *f)
+{
+	unsigned long i;
+
+	for (i = 0; i < h->oflow_count; i++) {
+		if (i >= h->oflow_bufsize)
+			break;
+		if (i != 0)
+			fprintf(f, " ");
+		fprintf(f, "%05lu", h->oflows[i]);
+	}
+
+	if (i >= h->oflow_bufsize)
+		fprintf(f, " # %05lu others", h->oflow_count - h->oflow_bufsize);
+}
-- 
2.43.0

[PATCH 4/4] rt-tests: cyclicdeadline: Add histogram support

[Crystal Wood]

Add support for the --histogram and --histfile options as in cyclictest.
The short -h option is not supported, as cyclicdeadline already uses that
for help.  -H/--histofall is not supported but could be easily added.

Signed-off-by: Crystal Wood <crwood@redhat.com>
---
Given the recent request for oslat, do we want to be able to do ns-granularity
histograms?  Or just stick with what cyclictest supports, as that's what rteval
wants?

Is histofall support needed?  rteval doesn't use it.
---
 src/sched_deadline/cyclicdeadline.c | 113 +++++++++++++++++++++++++---
 1 file changed, 103 insertions(+), 10 deletions(-)

diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
index 097e2e5d4580..f45ec3d6394b 100644
--- a/src/sched_deadline/cyclicdeadline.c
+++ b/src/sched_deadline/cyclicdeadline.c
@@ -33,6 +33,7 @@
 #include "rt-utils.h"
 #include "rt-sched.h"
 #include "rt-error.h"
+#include "histogram.h"
 
 #define _STR(x) #x
 #define STR(x) _STR(x)
@@ -40,6 +41,8 @@
 #define MAXPATH 1024
 #endif
 
+#define HIST_MAX	1000000
+
 #define CPUSET_ALL	"my_cpuset_all"
 #define CPUSET_LOCAL	"my_cpuset"
 
@@ -56,16 +59,13 @@ struct thread_stat {
 	long act;
 	double avg;
 	long *values;
-	long *hist_array;
-	long *outliers;
+	struct histogram *hist;
 	pthread_t thread;
 	int threadstarted;
 	int tid;
 	long reduce;
 	long redmax;
 	long cycleofmax;
-	long hist_overflow;
-	long num_outliers;
 };
 
 struct sched_data {
@@ -84,6 +84,8 @@ static int info_enable;
 static int debug_enable;
 static int tracelimit;
 static int trace_marker;
+static int histogram;
+static FILE *histfile;
 static pthread_mutex_t break_thread_id_lock = PTHREAD_MUTEX_INITIALIZER;
 static pid_t break_thread_id;
 static uint64_t break_thread_value;
@@ -97,6 +99,8 @@ static int mark_fd;
 static int quiet;
 static char jsonfile[MAX_PATH];
 
+static struct histoset hset;
+
 static int find_mount(const char *mount, char *debugfs)
 {
 	char type[100];
@@ -691,6 +695,10 @@ static void usage(int error)
 	       "                           Append 'm', 'h', or 'd' to specify minutes, hours or\n"
 	       "                           days\n"
 	       "-h       --help            Show this help menu.\n"
+	       "         --histogram=US    dump a latency histogram to stdout after the run\n"
+	       "                           US is the max latency time to be tracked in microseconds\n"
+	       "			   This option runs all threads at the same priority.\n"
+	       "	 --histfile=<path> dump the latency histogram to <path> instead of stdout\n"
 	       "-i INTV  --interval        The shortest deadline for the tasks in us\n"
 	       "                           (default 1000us).\n"
 	       "         --json=FILENAME   write final results into FILENAME, JSON formatted\n"
@@ -718,6 +726,55 @@ static u64 get_time_us(void)
 	return time;
 }
 
+static void print_hist(FILE *fp, struct sched_data *sd, int nthreads)
+{
+	int i;
+	unsigned long maxmax, alloverflows;
+
+	fprintf(fp, "# Histogram\n");
+	for (i = 0; i < histogram; i++) {
+		unsigned long flags = 0;
+
+		fprintf(fp, "%06d ", i);
+
+		hset_print_bucket(&hset, fp, i, flags);
+		fprintf(fp, "\n");
+	}
+	fprintf(fp, "# Min Latencies:");
+	for (i = 0; i < nthreads; i++)
+		fprintf(fp, " %05lu", sd[i].stat.min);
+	fprintf(fp, "\n");
+	fprintf(fp, "# Avg Latencies:");
+	for (i = 0; i < nthreads; i++)
+		fprintf(fp, " %05lu", sd[i].stat.cycles ?
+		       (long)(sd[i].stat.avg/sd[i].stat.cycles) : 0);
+	fprintf(fp, "\n");
+	fprintf(fp, "# Max Latencies:");
+	maxmax = 0;
+	for (i = 0; i < nthreads; i++) {
+		fprintf(fp, " %05lu", sd[i].stat.max);
+		if (sd[i].stat.max > maxmax)
+			maxmax = sd[i].stat.max;
+	}
+	fprintf(fp, "\n");
+	fprintf(fp, "# Histogram Overflows:");
+	alloverflows = 0;
+	for (i = 0; i < nthreads; i++) {
+		fprintf(fp, " %05lu", sd[i].stat.hist->oflow_count);
+		alloverflows += sd[i].stat.hist->oflow_count;
+	}
+	fprintf(fp, "\n");
+
+	fprintf(fp, "# Histogram Overflow at cycle number:\n");
+	for (i = 0; i < nthreads; i++) {
+		fprintf(fp, "# Thread %d: ", i);
+		hist_print_oflows(sd[i].stat.hist, fp);
+		fprintf(fp, "\n");
+	}
+
+	fprintf(fp, "\n");
+}
+
 static void print_stat(FILE *fp, struct sched_data *sd, int index, int verbose, int quiet)
 {
 	struct thread_stat *stat = &sd->stat;
@@ -784,6 +841,9 @@ static u64 do_runtime(struct sched_data *sd, u64 period)
 	stat->act = diff;
 	stat->avg += (double) diff;
 
+	if (histogram)
+		hist_sample(stat->hist, diff);
+
 	stat->cycles++;
 
 	return next_period;
@@ -1058,8 +1118,13 @@ static void loop(struct sched_data *sched_data, int nr_threads)
 	if (!quiet) {
 		printf("\033[%dB", nr_threads + 2);
 	} else {
-		for (i = 0; i < nr_threads; ++i)
-			print_stat(stdout, &sched_data[i], i, 0, 0);
+		if (histogram) {
+			FILE *out = histfile ? histfile : stdout;
+			print_hist(out, sched_data, nr_threads);
+		} else {
+			for (i = 0; i < nr_threads; ++i)
+				print_stat(stdout, &sched_data[i], i, 0, 0);
+		}
 	}
 }
 
@@ -1075,10 +1140,14 @@ static void write_stats(FILE *f, void *data)
 	for (i = 0; i < nr_threads; i++) {
 		s = &sd[i].stat;
 		fprintf(f, "    \"%u\": {\n", i);
-		fprintf(f, "	 \"cycles\": %ld,\n", s->cycles);
-		fprintf(f, "	 \"min\": %ld,\n", s->min);
-		fprintf(f, "	 \"max\": %ld,\n", s->max);
-		fprintf(f, "	 \"avg\": %.2f\n", s->avg/s->cycles);
+
+		fprintf(f, "      \"histogram\": {");
+		hist_print_json(s->hist, f);
+		fprintf(f, "      },\n");
+		fprintf(f, "      \"cycles\": %ld,\n", s->cycles);
+		fprintf(f, "      \"min\": %ld,\n", s->min);
+		fprintf(f, "      \"max\": %ld,\n", s->max);
+		fprintf(f, "      \"avg\": %.2f\n", s->avg/s->cycles);
 		fprintf(f, "    }%s\n", i == nr_threads - 1 ? "" : ",");
 	}
 	fprintf(f, "  }\n");
@@ -1088,6 +1157,7 @@ enum options_values {
 	OPT_AFFINITY=1, OPT_DURATION, OPT_HELP, OPT_INTERVAL,
 	OPT_JSON, OPT_STEP, OPT_THREADS, OPT_QUIET,
 	OPT_BREAKTRACE, OPT_TRACEMARK, OPT_INFO, OPT_DEBUG,
+	OPT_HISTOGRAM, OPT_HISTFILE
 };
 
 int main(int argc, char **argv)
@@ -1130,6 +1200,8 @@ int main(int argc, char **argv)
 			{ "tracemark",	     no_argument,	NULL, OPT_TRACEMARK },
 			{ "verbose",	no_argument,	NULL,	OPT_INFO},
 			{ "debug",	no_argument, 	NULL, 	OPT_DEBUG},
+			{ "histogram",	required_argument, NULL, OPT_HISTOGRAM },
+			{ "histfile",	required_argument, NULL, OPT_HISTFILE },
 			{ NULL,		0,			NULL,	0   },
 		};
 		c = getopt_long(argc, argv, "a::c:D:hi:s:t:b:q", options, NULL);
@@ -1188,6 +1260,17 @@ int main(int argc, char **argv)
 		case OPT_DEBUG:
 			debug_enable = 1;
 			break;
+		case OPT_HISTOGRAM:
+			histogram = atoi(optarg);
+			if (histogram <= 0 || histogram > HIST_MAX)
+				usage(1);
+			break;
+		case OPT_HISTFILE:
+			histfile = fopen(optarg, "w");
+			if (!histfile)
+				fatal("Couldn\'t open histfile %s: %s\n",
+				      optarg, strerror(errno));
+			break;
 		default:
 			usage(1);
 		}
@@ -1233,6 +1316,10 @@ int main(int argc, char **argv)
 	if (!thread || !sched_data)
 		fatal("allocating threads");
 
+	if (histogram && hset_init(&hset, nr_threads, 1, histogram, histogram))
+		fatal("failed to allocate histogram of size %d for %d threads\n",
+		      histogram, nr_threads);
+
 	if (nr_threads > nr_cpus) {
 		/*
 		 * More threads than CPUs, then have the total be
@@ -1262,6 +1349,9 @@ int main(int argc, char **argv)
 		sd->runtime_us = runtime;
 		sd->deadline_us = interval;
 
+		if (histogram)
+			sd->stat.hist = &hset.histos[i];
+
 		info(info_enable, "interval: %lld:%lld\n", sd->runtime_us, sd->deadline_us);
 
 		/* Make sure that we can make our deadlines */
@@ -1356,6 +1446,9 @@ int main(int argc, char **argv)
 		free(setcpu_buf);
 	free(thread);
 	free(sched_data);
+	if (histfile)
+		fclose(histfile);
+	hset_destroy(&hset);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH 1/4] rt-tests: Fix warnings

[John Kacur]

On Wed, 6 Dec 2023, Crystal Wood wrote:

> Numerous places threw sign comparison warnings; we could fix them but
> it's kind of an obnoxious warning that requires casts to deal with things
> such as ARRAY_SIZE() while still being able to check for the user
> entering a negative number.
> 
> -Wunused-parameter is another obnoxious warning as it flags perfectly
> reasonable code that takes unneeded parameters in order to comply with
> a function pointer interface or similar; however, all of the instances
> that were flagged here were actual dead parameters, so just fix them.
> 
> Add volatile to timer_started in hackbench so that it doesn't get
> clobbered by longjmp().
> 
> Signed-off-by: Crystal Wood <crwood@redhat.com>
> --
> Let me know if you'd rather I fix the sign warnings.
> ---
>  Makefile                            |  2 +-
>  src/hackbench/hackbench.c           |  2 +-
>  src/sched_deadline/cyclicdeadline.c |  6 +++---
>  src/sched_deadline/deadline_test.c  | 10 +++++-----
>  src/sigwaittest/sigwaittest.c       |  6 +++---
>  5 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 2808c212058a..ad481a73cf93 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -31,7 +31,7 @@ prefix  ?= /usr/local
>  bindir  ?= $(prefix)/bin
>  mandir	?= $(prefix)/share/man
>  
> -CFLAGS ?= -Wall -Wno-nonnull -Wextra
> +CFLAGS ?= -Wall -Wno-nonnull -Wextra -Wno-sign-compare
>  CPPFLAGS += -D_GNU_SOURCE -Isrc/include
>  LDFLAGS ?=
>  
> diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
> index 69dd5f087fb6..4430db0e4ed6 100644
> --- a/src/hackbench/hackbench.c
> +++ b/src/hackbench/hackbench.c
> @@ -494,7 +494,7 @@ int main(int argc, char *argv[])
>  	struct timeval start, stop, diff;
>  	int readyfds[2], wakefds[2];
>  	char dummy;
> -	int timer_started = 0;
> +	volatile int timer_started = 0;
>  	struct sched_param sp;
>  
>  	process_options (argc, argv);
> diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
> index 9bdc4b5deaf1..097e2e5d4580 100644
> --- a/src/sched_deadline/cyclicdeadline.c
> +++ b/src/sched_deadline/cyclicdeadline.c
> @@ -750,7 +750,7 @@ static void print_stat(FILE *fp, struct sched_data *sd, int index, int verbose,
>  	}
>  }
>  
> -static u64 do_runtime(long tid, struct sched_data *sd, u64 period)
> +static u64 do_runtime(struct sched_data *sd, u64 period)
>  {
>  	struct thread_stat *stat = &sd->stat;
>  	u64 next_period = period + sd->deadline_us;
> @@ -833,7 +833,7 @@ void *run_deadline(void *data)
>  	period = get_time_us();
>  
>  	while (!shutdown) {
> -		period = do_runtime(tid, sd, period);
> +		period = do_runtime(sd, period);
>  		if (tracelimit && (stat->max > tracelimit)) {
>  			shutdown++;
>  			pthread_mutex_lock(&break_thread_id_lock);
> @@ -1266,7 +1266,7 @@ int main(int argc, char **argv)
>  
>  		/* Make sure that we can make our deadlines */
>  		start_period = get_time_us();
> -		do_runtime(gettid(), sd, start_period);
> +		do_runtime(sd, start_period);
>  		end_period = get_time_us();
>  		if (end_period - start_period > sd->runtime_us)
>  			fatal("Failed to perform task within runtime: Missed by %lld us\n",
> diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
> index cd8ef01f7d68..ca2da476ec95 100644
> --- a/src/sched_deadline/deadline_test.c
> +++ b/src/sched_deadline/deadline_test.c
> @@ -1181,7 +1181,7 @@ static int read_ctx_switches(int *vol, int *nonvol, int *migrate)
>   *  @data->total_time - Total time it took to complete all loops
>   *  @data->nr_periods - Number of periods that were executed.
>   */
> -static u64 do_runtime(long tid, struct sched_data *data, u64 period)
> +static u64 do_runtime(struct sched_data *data, u64 period)
>  {
>  	u64 next_period = period + data->deadline_us;
>  	u64 now = get_time_us();
> @@ -1354,7 +1354,7 @@ void *run_deadline(void *data)
>  	period = get_time_us();
>  
>  	while (!done) {
> -		period = do_runtime(tid, sched_data, period);
> +		period = do_runtime(sched_data, period);
>  		sched_yield();
>  	}
>  	ret = sched_getattr(0, &attr, sizeof(attr), 0);
> @@ -1714,7 +1714,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
>  	do_sleep(1000);
>  
>  	start = get_time_us();
> -	do_runtime(0, &sd, start + sd.deadline_us);
> +	do_runtime(&sd, start + sd.deadline_us);
>  	end = get_time_us();
>  
>  	diff = end - start;
> @@ -1743,7 +1743,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
>  	do_sleep(1000);
>  
>  	start = get_time_us();
> -	do_runtime(0, &sd, start + sd.deadline_us);
> +	do_runtime(&sd, start + sd.deadline_us);
>  	end = get_time_us();
>  
>  	odiff = end - start;
> @@ -1962,7 +1962,7 @@ int main(int argc, char **argv)
>  
>  		/* Make sure that we can make our deadlines */
>  		start_period = get_time_us();
> -		do_runtime(gettid(), sd, start_period);
> +		do_runtime(sd, start_period);
>  		end_period = get_time_us();
>  		if (end_period - start_period > sd->runtime_us) {
>  			printf("Failed to perform task within runtime: Missed by %lld us\n",
> diff --git a/src/sigwaittest/sigwaittest.c b/src/sigwaittest/sigwaittest.c
> index 55855769c63b..8c1c16fb3081 100644
> --- a/src/sigwaittest/sigwaittest.c
> +++ b/src/sigwaittest/sigwaittest.c
> @@ -375,7 +375,7 @@ static void sighand(int sig __attribute__ ((unused)))
>  	mustshutdown = 1;
>  }
>  
> -static void print_stat(FILE *fp, struct params *receiver, struct params *sender,
> +static void print_stat(struct params *receiver, struct params *sender,
>  		       int verbose __attribute__ ((unused)), int quiet)
>  {
>  	int i;
> @@ -644,7 +644,7 @@ int main(int argc, char *argv[])
>  			    sender[i].shutdown;
>  
>  		if (receiver[0].samples > oldsamples || mustshutdown) {
> -			print_stat(stdout, receiver, sender, 0, quiet);
> +			print_stat(receiver, sender, 0, quiet);
>  			if (!quiet)
>  				printf("\033[%dA", num_threads*2);
>  		}
> @@ -664,7 +664,7 @@ int main(int argc, char *argv[])
>  	if (!quiet)
>  		printf("\033[%dB", num_threads*2 + 2);
>  	else
> -		print_stat(stdout, receiver, sender, 0, 0);
> +		print_stat(receiver, sender, 0, 0);
>  
>  	for (i = 0; i < num_threads; i++) {
>  		receiver[i].shutdown = 1;
> -- 
> 2.43.0
> 
> 
> 
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH 2/4] rt-tests: cyclictest: Remove histogram totals

[John Kacur]

On Wed, 6 Dec 2023, Crystal Wood wrote:

> The Total: line does not seem to contribute much value, as it should just
> be the number of buckets minus the number of overflows.  Unless someone
> complains, remove it to simplify moving to common histogram code.
> 
> Signed-off-by: Crystal Wood <crwood@redhat.com>
> ---
>  src/cyclictest/cyclictest.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index a8039b49feb6..93ce201e9fca 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -1407,12 +1407,9 @@ static void print_tids(struct thread_param *par[], int nthreads)
>  static void print_hist(struct thread_param *par[], int nthreads)
>  {
>  	int i, j;
> -	unsigned long long int log_entries[nthreads+1];
>  	unsigned long maxmax, alloverflows;
>  	FILE *fd;
>  
> -	bzero(log_entries, sizeof(log_entries));
> -
>  	if (use_histfile) {
>  		fd = fopen(histfile, "w");
>  		if (!fd) {
> @@ -1434,21 +1431,12 @@ static void print_hist(struct thread_param *par[], int nthreads)
>  			fprintf(fd, "%06lu", curr_latency);
>  			if (j < nthreads - 1)
>  				fprintf(fd, "\t");
> -			log_entries[j] += curr_latency;
>  			allthreads += curr_latency;
>  		}
> -		if (histofall && nthreads > 1) {
> +		if (histofall && nthreads > 1)
>  			fprintf(fd, "\t%06llu", allthreads);
> -			log_entries[nthreads] += allthreads;
> -		}
>  		fprintf(fd, "\n");
>  	}
> -	fprintf(fd, "# Total:");
> -	for (j = 0; j < nthreads; j++)
> -		fprintf(fd, " %09llu", log_entries[j]);
> -	if (histofall && nthreads > 1)
> -		fprintf(fd, " %09llu", log_entries[nthreads]);
> -	fprintf(fd, "\n");
>  	fprintf(fd, "# Min Latencies:");
>  	for (j = 0; j < nthreads; j++)
>  		fprintf(fd, " %05lu", par[j]->stats->min);
> -- 
> 2.43.0
> 
> 

-Tested with rteval
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH 2/4] rt-tests: cyclictest: Remove histogram totals

[Crystal Wood]

On Wed, 2023-12-06 at 14:55 -0600, Crystal Wood wrote:
> The Total: line does not seem to contribute much value, as it should just
> be the number of buckets minus the number of overflows.  Unless someone
> complains, remove it to simplify moving to common histogram code.

Eh, that should be "number of cycles", not "number of buckets". :-P

-Crystal

Re: [PATCH 2/4] rt-tests: cyclictest: Remove histogram totals

[John Kacur]

On Wed, 6 Dec 2023, Crystal Wood wrote:

> The Total: line does not seem to contribute much value, as it should just
> be the number of buckets minus the number of overflows.  Unless someone
> complains, remove it to simplify moving to common histogram code.
> 
> Signed-off-by: Crystal Wood <crwood@redhat.com>
> ---
>  src/cyclictest/cyclictest.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index a8039b49feb6..93ce201e9fca 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -1407,12 +1407,9 @@ static void print_tids(struct thread_param *par[], int nthreads)
>  static void print_hist(struct thread_param *par[], int nthreads)
>  {
>  	int i, j;
> -	unsigned long long int log_entries[nthreads+1];
>  	unsigned long maxmax, alloverflows;
>  	FILE *fd;
>  
> -	bzero(log_entries, sizeof(log_entries));
> -
>  	if (use_histfile) {
>  		fd = fopen(histfile, "w");
>  		if (!fd) {
> @@ -1434,21 +1431,12 @@ static void print_hist(struct thread_param *par[], int nthreads)
>  			fprintf(fd, "%06lu", curr_latency);
>  			if (j < nthreads - 1)
>  				fprintf(fd, "\t");
> -			log_entries[j] += curr_latency;
>  			allthreads += curr_latency;
>  		}
> -		if (histofall && nthreads > 1) {
> +		if (histofall && nthreads > 1)
>  			fprintf(fd, "\t%06llu", allthreads);
> -			log_entries[nthreads] += allthreads;
> -		}
>  		fprintf(fd, "\n");
>  	}
> -	fprintf(fd, "# Total:");
> -	for (j = 0; j < nthreads; j++)
> -		fprintf(fd, " %09llu", log_entries[j]);
> -	if (histofall && nthreads > 1)
> -		fprintf(fd, " %09llu", log_entries[nthreads]);
> -	fprintf(fd, "\n");
>  	fprintf(fd, "# Min Latencies:");
>  	for (j = 0; j < nthreads; j++)
>  		fprintf(fd, " %05lu", par[j]->stats->min);
> -- 
> 2.43.0
> 
> 
> 
- Tested in rteval
- Edited commit message to say "cycles" instead of buckets
Signed-off-by: John Kacur <jkacur@redhat.com>

Re: [PATCH 1/4] rt-tests: Fix warnings

[John Kacur]

On Wed, 6 Dec 2023, Crystal Wood wrote:

> Numerous places threw sign comparison warnings; we could fix them but
> it's kind of an obnoxious warning that requires casts to deal with things
> such as ARRAY_SIZE() while still being able to check for the user
> entering a negative number.
> 
> -Wunused-parameter is another obnoxious warning as it flags perfectly
> reasonable code that takes unneeded parameters in order to comply with
> a function pointer interface or similar; however, all of the instances
> that were flagged here were actual dead parameters, so just fix them.
> 
> Add volatile to timer_started in hackbench so that it doesn't get
> clobbered by longjmp().
> 
> Signed-off-by: Crystal Wood <crwood@redhat.com>
> --
> Let me know if you'd rather I fix the sign warnings.
> ---
>  Makefile                            |  2 +-
>  src/hackbench/hackbench.c           |  2 +-
>  src/sched_deadline/cyclicdeadline.c |  6 +++---
>  src/sched_deadline/deadline_test.c  | 10 +++++-----
>  src/sigwaittest/sigwaittest.c       |  6 +++---
>  5 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 2808c212058a..ad481a73cf93 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -31,7 +31,7 @@ prefix  ?= /usr/local
>  bindir  ?= $(prefix)/bin
>  mandir	?= $(prefix)/share/man
>  
> -CFLAGS ?= -Wall -Wno-nonnull -Wextra
> +CFLAGS ?= -Wall -Wno-nonnull -Wextra -Wno-sign-compare
>  CPPFLAGS += -D_GNU_SOURCE -Isrc/include
>  LDFLAGS ?=
>  
> diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
> index 69dd5f087fb6..4430db0e4ed6 100644
> --- a/src/hackbench/hackbench.c
> +++ b/src/hackbench/hackbench.c
> @@ -494,7 +494,7 @@ int main(int argc, char *argv[])
>  	struct timeval start, stop, diff;
>  	int readyfds[2], wakefds[2];
>  	char dummy;
> -	int timer_started = 0;
> +	volatile int timer_started = 0;
>  	struct sched_param sp;
>  
>  	process_options (argc, argv);
> diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
> index 9bdc4b5deaf1..097e2e5d4580 100644
> --- a/src/sched_deadline/cyclicdeadline.c
> +++ b/src/sched_deadline/cyclicdeadline.c
> @@ -750,7 +750,7 @@ static void print_stat(FILE *fp, struct sched_data *sd, int index, int verbose,
>  	}
>  }
>  
> -static u64 do_runtime(long tid, struct sched_data *sd, u64 period)
> +static u64 do_runtime(struct sched_data *sd, u64 period)
>  {
>  	struct thread_stat *stat = &sd->stat;
>  	u64 next_period = period + sd->deadline_us;
> @@ -833,7 +833,7 @@ void *run_deadline(void *data)
>  	period = get_time_us();
>  
>  	while (!shutdown) {
> -		period = do_runtime(tid, sd, period);
> +		period = do_runtime(sd, period);
>  		if (tracelimit && (stat->max > tracelimit)) {
>  			shutdown++;
>  			pthread_mutex_lock(&break_thread_id_lock);
> @@ -1266,7 +1266,7 @@ int main(int argc, char **argv)
>  
>  		/* Make sure that we can make our deadlines */
>  		start_period = get_time_us();
> -		do_runtime(gettid(), sd, start_period);
> +		do_runtime(sd, start_period);
>  		end_period = get_time_us();
>  		if (end_period - start_period > sd->runtime_us)
>  			fatal("Failed to perform task within runtime: Missed by %lld us\n",
> diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
> index cd8ef01f7d68..ca2da476ec95 100644
> --- a/src/sched_deadline/deadline_test.c
> +++ b/src/sched_deadline/deadline_test.c
> @@ -1181,7 +1181,7 @@ static int read_ctx_switches(int *vol, int *nonvol, int *migrate)
>   *  @data->total_time - Total time it took to complete all loops
>   *  @data->nr_periods - Number of periods that were executed.
>   */
> -static u64 do_runtime(long tid, struct sched_data *data, u64 period)
> +static u64 do_runtime(struct sched_data *data, u64 period)
>  {
>  	u64 next_period = period + data->deadline_us;
>  	u64 now = get_time_us();
> @@ -1354,7 +1354,7 @@ void *run_deadline(void *data)
>  	period = get_time_us();
>  
>  	while (!done) {
> -		period = do_runtime(tid, sched_data, period);
> +		period = do_runtime(sched_data, period);
>  		sched_yield();
>  	}
>  	ret = sched_getattr(0, &attr, sizeof(attr), 0);
> @@ -1714,7 +1714,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
>  	do_sleep(1000);
>  
>  	start = get_time_us();
> -	do_runtime(0, &sd, start + sd.deadline_us);
> +	do_runtime(&sd, start + sd.deadline_us);
>  	end = get_time_us();
>  
>  	diff = end - start;
> @@ -1743,7 +1743,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
>  	do_sleep(1000);
>  
>  	start = get_time_us();
> -	do_runtime(0, &sd, start + sd.deadline_us);
> +	do_runtime(&sd, start + sd.deadline_us);
>  	end = get_time_us();
>  
>  	odiff = end - start;
> @@ -1962,7 +1962,7 @@ int main(int argc, char **argv)
>  
>  		/* Make sure that we can make our deadlines */
>  		start_period = get_time_us();
> -		do_runtime(gettid(), sd, start_period);
> +		do_runtime(sd, start_period);
>  		end_period = get_time_us();
>  		if (end_period - start_period > sd->runtime_us) {
>  			printf("Failed to perform task within runtime: Missed by %lld us\n",
> diff --git a/src/sigwaittest/sigwaittest.c b/src/sigwaittest/sigwaittest.c
> index 55855769c63b..8c1c16fb3081 100644
> --- a/src/sigwaittest/sigwaittest.c
> +++ b/src/sigwaittest/sigwaittest.c
> @@ -375,7 +375,7 @@ static void sighand(int sig __attribute__ ((unused)))
>  	mustshutdown = 1;
>  }
>  
> -static void print_stat(FILE *fp, struct params *receiver, struct params *sender,
> +static void print_stat(struct params *receiver, struct params *sender,
>  		       int verbose __attribute__ ((unused)), int quiet)
>  {
>  	int i;
> @@ -644,7 +644,7 @@ int main(int argc, char *argv[])
>  			    sender[i].shutdown;
>  
>  		if (receiver[0].samples > oldsamples || mustshutdown) {
> -			print_stat(stdout, receiver, sender, 0, quiet);
> +			print_stat(receiver, sender, 0, quiet);
>  			if (!quiet)
>  				printf("\033[%dA", num_threads*2);
>  		}
> @@ -664,7 +664,7 @@ int main(int argc, char *argv[])
>  	if (!quiet)
>  		printf("\033[%dB", num_threads*2 + 2);
>  	else
> -		print_stat(stdout, receiver, sender, 0, 0);
> +		print_stat(receiver, sender, 0, 0);
>  
>  	for (i = 0; i < num_threads; i++) {
>  		receiver[i].shutdown = 1;
> -- 
> 2.43.0
> 
> 
> 

I tested this with rteval, and it works fine
checkpatch in the linux kernel flags a few minor things.
Would you mind fixing those up before I apply the patch?

WARNING: Improper SPDX comment style for 'src/include/histogram.h', please 
use '/*' instead
#227: FILE: src/include/histogram.h:1:
+// SPDX-License-Identifier: GPL-2.0-or-later

(Yes, I know there are other files in rt-tests that also aren't using
the recommended commenting style, you probably copied from there.)

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#227: FILE: src/include/histogram.h:1:
+// SPDX-License-Identifier: GPL-2.0-or-later

WARNING: braces {} are not necessary for any arm of this statement
#333: FILE: src/lib/histogram.c:59:
+	if (h->oflow_magnitude + extra > h->oflow_magnitude) {
[...]
+	} else {
[...]

ERROR: trailing whitespace
#338: FILE: src/lib/histogram.c:64:
+^I^I$

WARNING: braces {} are not necessary for any arm of this statement
#340: FILE: src/lib/histogram.c:66:
+		if (h->oflow_count < h->oflow_bufsize) {
[...]
+		} else {
[...]

total: 1 errors, 5 warnings, 394 lines checked

Thanks

John Kacur

Re: [PATCH 4/4] rt-tests: cyclicdeadline: Add histogram support

[John Kacur]

On Wed, 6 Dec 2023, Crystal Wood wrote:

> Add support for the --histogram and --histfile options as in cyclictest.
> The short -h option is not supported, as cyclicdeadline already uses that
> for help.  -H/--histofall is not supported but could be easily added.
> 
> Signed-off-by: Crystal Wood <crwood@redhat.com>
> ---
> Given the recent request for oslat, do we want to be able to do ns-granularity
> histograms?  Or just stick with what cyclictest supports, as that's what rteval
> wants?

rteval will want what we teach it to want. If the hardware supports 
meaningful numbers with ns-granularity, it might make sense to include it.
start with just doing what cyclictest currently supports, but I wouldn't 
rule out adding this in the future.

> 
> Is histofall support needed?  rteval doesn't use it.

Not sure, we probably want one or the other, I suppose the idea was to not 
have the extra information if you're parsing it with another program like 
rteval but to include it otherwise. I ran it just ctrl-cing it and 
couldn't see a difference.

John

> ---
>  src/sched_deadline/cyclicdeadline.c | 113 +++++++++++++++++++++++++---
>  1 file changed, 103 insertions(+), 10 deletions(-)
> 
> diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
> index 097e2e5d4580..f45ec3d6394b 100644
> --- a/src/sched_deadline/cyclicdeadline.c
> +++ b/src/sched_deadline/cyclicdeadline.c
> @@ -33,6 +33,7 @@
>  #include "rt-utils.h"
>  #include "rt-sched.h"
>  #include "rt-error.h"
> +#include "histogram.h"
>  
>  #define _STR(x) #x
>  #define STR(x) _STR(x)
> @@ -40,6 +41,8 @@
>  #define MAXPATH 1024
>  #endif
>  
> +#define HIST_MAX	1000000
> +
>  #define CPUSET_ALL	"my_cpuset_all"
>  #define CPUSET_LOCAL	"my_cpuset"
>  
> @@ -56,16 +59,13 @@ struct thread_stat {
>  	long act;
>  	double avg;
>  	long *values;
> -	long *hist_array;
> -	long *outliers;
> +	struct histogram *hist;
>  	pthread_t thread;
>  	int threadstarted;
>  	int tid;
>  	long reduce;
>  	long redmax;
>  	long cycleofmax;
> -	long hist_overflow;
> -	long num_outliers;
>  };
>  
>  struct sched_data {
> @@ -84,6 +84,8 @@ static int info_enable;
>  static int debug_enable;
>  static int tracelimit;
>  static int trace_marker;
> +static int histogram;
> +static FILE *histfile;
>  static pthread_mutex_t break_thread_id_lock = PTHREAD_MUTEX_INITIALIZER;
>  static pid_t break_thread_id;
>  static uint64_t break_thread_value;
> @@ -97,6 +99,8 @@ static int mark_fd;
>  static int quiet;
>  static char jsonfile[MAX_PATH];
>  
> +static struct histoset hset;
> +
>  static int find_mount(const char *mount, char *debugfs)
>  {
>  	char type[100];
> @@ -691,6 +695,10 @@ static void usage(int error)
>  	       "                           Append 'm', 'h', or 'd' to specify minutes, hours or\n"
>  	       "                           days\n"
>  	       "-h       --help            Show this help menu.\n"
> +	       "         --histogram=US    dump a latency histogram to stdout after the run\n"
> +	       "                           US is the max latency time to be tracked in microseconds\n"
> +	       "			   This option runs all threads at the same priority.\n"
> +	       "	 --histfile=<path> dump the latency histogram to <path> instead of stdout\n"
>  	       "-i INTV  --interval        The shortest deadline for the tasks in us\n"
>  	       "                           (default 1000us).\n"
>  	       "         --json=FILENAME   write final results into FILENAME, JSON formatted\n"
> @@ -718,6 +726,55 @@ static u64 get_time_us(void)
>  	return time;
>  }
>  
> +static void print_hist(FILE *fp, struct sched_data *sd, int nthreads)
> +{
> +	int i;
> +	unsigned long maxmax, alloverflows;
> +
> +	fprintf(fp, "# Histogram\n");
> +	for (i = 0; i < histogram; i++) {
> +		unsigned long flags = 0;
> +
> +		fprintf(fp, "%06d ", i);
> +
> +		hset_print_bucket(&hset, fp, i, flags);
> +		fprintf(fp, "\n");
> +	}
> +	fprintf(fp, "# Min Latencies:");
> +	for (i = 0; i < nthreads; i++)
> +		fprintf(fp, " %05lu", sd[i].stat.min);
> +	fprintf(fp, "\n");
> +	fprintf(fp, "# Avg Latencies:");
> +	for (i = 0; i < nthreads; i++)
> +		fprintf(fp, " %05lu", sd[i].stat.cycles ?
> +		       (long)(sd[i].stat.avg/sd[i].stat.cycles) : 0);
> +	fprintf(fp, "\n");
> +	fprintf(fp, "# Max Latencies:");
> +	maxmax = 0;
> +	for (i = 0; i < nthreads; i++) {
> +		fprintf(fp, " %05lu", sd[i].stat.max);
> +		if (sd[i].stat.max > maxmax)
> +			maxmax = sd[i].stat.max;
> +	}
> +	fprintf(fp, "\n");
> +	fprintf(fp, "# Histogram Overflows:");
> +	alloverflows = 0;
> +	for (i = 0; i < nthreads; i++) {
> +		fprintf(fp, " %05lu", sd[i].stat.hist->oflow_count);
> +		alloverflows += sd[i].stat.hist->oflow_count;
> +	}
> +	fprintf(fp, "\n");
> +
> +	fprintf(fp, "# Histogram Overflow at cycle number:\n");
> +	for (i = 0; i < nthreads; i++) {
> +		fprintf(fp, "# Thread %d: ", i);
> +		hist_print_oflows(sd[i].stat.hist, fp);
> +		fprintf(fp, "\n");
> +	}
> +
> +	fprintf(fp, "\n");
> +}
> +
>  static void print_stat(FILE *fp, struct sched_data *sd, int index, int verbose, int quiet)
>  {
>  	struct thread_stat *stat = &sd->stat;
> @@ -784,6 +841,9 @@ static u64 do_runtime(struct sched_data *sd, u64 period)
>  	stat->act = diff;
>  	stat->avg += (double) diff;
>  
> +	if (histogram)
> +		hist_sample(stat->hist, diff);
> +
>  	stat->cycles++;
>  
>  	return next_period;
> @@ -1058,8 +1118,13 @@ static void loop(struct sched_data *sched_data, int nr_threads)
>  	if (!quiet) {
>  		printf("\033[%dB", nr_threads + 2);
>  	} else {
> -		for (i = 0; i < nr_threads; ++i)
> -			print_stat(stdout, &sched_data[i], i, 0, 0);
> +		if (histogram) {
> +			FILE *out = histfile ? histfile : stdout;
> +			print_hist(out, sched_data, nr_threads);
> +		} else {
> +			for (i = 0; i < nr_threads; ++i)
> +				print_stat(stdout, &sched_data[i], i, 0, 0);
> +		}
>  	}
>  }
>  
> @@ -1075,10 +1140,14 @@ static void write_stats(FILE *f, void *data)
>  	for (i = 0; i < nr_threads; i++) {
>  		s = &sd[i].stat;
>  		fprintf(f, "    \"%u\": {\n", i);
> -		fprintf(f, "	 \"cycles\": %ld,\n", s->cycles);
> -		fprintf(f, "	 \"min\": %ld,\n", s->min);
> -		fprintf(f, "	 \"max\": %ld,\n", s->max);
> -		fprintf(f, "	 \"avg\": %.2f\n", s->avg/s->cycles);
> +
> +		fprintf(f, "      \"histogram\": {");
> +		hist_print_json(s->hist, f);
> +		fprintf(f, "      },\n");
> +		fprintf(f, "      \"cycles\": %ld,\n", s->cycles);
> +		fprintf(f, "      \"min\": %ld,\n", s->min);
> +		fprintf(f, "      \"max\": %ld,\n", s->max);
> +		fprintf(f, "      \"avg\": %.2f\n", s->avg/s->cycles);
>  		fprintf(f, "    }%s\n", i == nr_threads - 1 ? "" : ",");
>  	}
>  	fprintf(f, "  }\n");
> @@ -1088,6 +1157,7 @@ enum options_values {
>  	OPT_AFFINITY=1, OPT_DURATION, OPT_HELP, OPT_INTERVAL,
>  	OPT_JSON, OPT_STEP, OPT_THREADS, OPT_QUIET,
>  	OPT_BREAKTRACE, OPT_TRACEMARK, OPT_INFO, OPT_DEBUG,
> +	OPT_HISTOGRAM, OPT_HISTFILE
>  };
>  
>  int main(int argc, char **argv)
> @@ -1130,6 +1200,8 @@ int main(int argc, char **argv)
>  			{ "tracemark",	     no_argument,	NULL, OPT_TRACEMARK },
>  			{ "verbose",	no_argument,	NULL,	OPT_INFO},
>  			{ "debug",	no_argument, 	NULL, 	OPT_DEBUG},
> +			{ "histogram",	required_argument, NULL, OPT_HISTOGRAM },
> +			{ "histfile",	required_argument, NULL, OPT_HISTFILE },
>  			{ NULL,		0,			NULL,	0   },
>  		};
>  		c = getopt_long(argc, argv, "a::c:D:hi:s:t:b:q", options, NULL);
> @@ -1188,6 +1260,17 @@ int main(int argc, char **argv)
>  		case OPT_DEBUG:
>  			debug_enable = 1;
>  			break;
> +		case OPT_HISTOGRAM:
> +			histogram = atoi(optarg);
> +			if (histogram <= 0 || histogram > HIST_MAX)
> +				usage(1);
> +			break;
> +		case OPT_HISTFILE:
> +			histfile = fopen(optarg, "w");
> +			if (!histfile)
> +				fatal("Couldn\'t open histfile %s: %s\n",
> +				      optarg, strerror(errno));
> +			break;
>  		default:
>  			usage(1);
>  		}
> @@ -1233,6 +1316,10 @@ int main(int argc, char **argv)
>  	if (!thread || !sched_data)
>  		fatal("allocating threads");
>  
> +	if (histogram && hset_init(&hset, nr_threads, 1, histogram, histogram))
> +		fatal("failed to allocate histogram of size %d for %d threads\n",
> +		      histogram, nr_threads);
> +
>  	if (nr_threads > nr_cpus) {
>  		/*
>  		 * More threads than CPUs, then have the total be
> @@ -1262,6 +1349,9 @@ int main(int argc, char **argv)
>  		sd->runtime_us = runtime;
>  		sd->deadline_us = interval;
>  
> +		if (histogram)
> +			sd->stat.hist = &hset.histos[i];
> +
>  		info(info_enable, "interval: %lld:%lld\n", sd->runtime_us, sd->deadline_us);
>  
>  		/* Make sure that we can make our deadlines */
> @@ -1356,6 +1446,9 @@ int main(int argc, char **argv)
>  		free(setcpu_buf);
>  	free(thread);
>  	free(sched_data);
> +	if (histfile)
> +		fclose(histfile);
> +	hset_destroy(&hset);
>  
>  	return 0;
>  }
> -- 
> 2.43.0
> 
> 
> 

Re: [PATCH 1/4] rt-tests: Fix warnings

[John Kacur]

On Thu, 14 Dec 2023, John Kacur wrote:

> 
> 
> On Wed, 6 Dec 2023, Crystal Wood wrote:
> 
> > Numerous places threw sign comparison warnings; we could fix them but
> > it's kind of an obnoxious warning that requires casts to deal with things
> > such as ARRAY_SIZE() while still being able to check for the user
> > entering a negative number.
> > 
> > -Wunused-parameter is another obnoxious warning as it flags perfectly
> > reasonable code that takes unneeded parameters in order to comply with
> > a function pointer interface or similar; however, all of the instances
> > that were flagged here were actual dead parameters, so just fix them.
> > 
> > Add volatile to timer_started in hackbench so that it doesn't get
> > clobbered by longjmp().
> > 
> > Signed-off-by: Crystal Wood <crwood@redhat.com>
> > --
> > Let me know if you'd rather I fix the sign warnings.
> > ---
> >  Makefile                            |  2 +-
> >  src/hackbench/hackbench.c           |  2 +-
> >  src/sched_deadline/cyclicdeadline.c |  6 +++---
> >  src/sched_deadline/deadline_test.c  | 10 +++++-----
> >  src/sigwaittest/sigwaittest.c       |  6 +++---
> >  5 files changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 2808c212058a..ad481a73cf93 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -31,7 +31,7 @@ prefix  ?= /usr/local
> >  bindir  ?= $(prefix)/bin
> >  mandir	?= $(prefix)/share/man
> >  
> > -CFLAGS ?= -Wall -Wno-nonnull -Wextra
> > +CFLAGS ?= -Wall -Wno-nonnull -Wextra -Wno-sign-compare
> >  CPPFLAGS += -D_GNU_SOURCE -Isrc/include
> >  LDFLAGS ?=
> >  
> > diff --git a/src/hackbench/hackbench.c b/src/hackbench/hackbench.c
> > index 69dd5f087fb6..4430db0e4ed6 100644
> > --- a/src/hackbench/hackbench.c
> > +++ b/src/hackbench/hackbench.c
> > @@ -494,7 +494,7 @@ int main(int argc, char *argv[])
> >  	struct timeval start, stop, diff;
> >  	int readyfds[2], wakefds[2];
> >  	char dummy;
> > -	int timer_started = 0;
> > +	volatile int timer_started = 0;
> >  	struct sched_param sp;
> >  
> >  	process_options (argc, argv);
> > diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
> > index 9bdc4b5deaf1..097e2e5d4580 100644
> > --- a/src/sched_deadline/cyclicdeadline.c
> > +++ b/src/sched_deadline/cyclicdeadline.c
> > @@ -750,7 +750,7 @@ static void print_stat(FILE *fp, struct sched_data *sd, int index, int verbose,
> >  	}
> >  }
> >  
> > -static u64 do_runtime(long tid, struct sched_data *sd, u64 period)
> > +static u64 do_runtime(struct sched_data *sd, u64 period)
> >  {
> >  	struct thread_stat *stat = &sd->stat;
> >  	u64 next_period = period + sd->deadline_us;
> > @@ -833,7 +833,7 @@ void *run_deadline(void *data)
> >  	period = get_time_us();
> >  
> >  	while (!shutdown) {
> > -		period = do_runtime(tid, sd, period);
> > +		period = do_runtime(sd, period);
> >  		if (tracelimit && (stat->max > tracelimit)) {
> >  			shutdown++;
> >  			pthread_mutex_lock(&break_thread_id_lock);
> > @@ -1266,7 +1266,7 @@ int main(int argc, char **argv)
> >  
> >  		/* Make sure that we can make our deadlines */
> >  		start_period = get_time_us();
> > -		do_runtime(gettid(), sd, start_period);
> > +		do_runtime(sd, start_period);
> >  		end_period = get_time_us();
> >  		if (end_period - start_period > sd->runtime_us)
> >  			fatal("Failed to perform task within runtime: Missed by %lld us\n",
> > diff --git a/src/sched_deadline/deadline_test.c b/src/sched_deadline/deadline_test.c
> > index cd8ef01f7d68..ca2da476ec95 100644
> > --- a/src/sched_deadline/deadline_test.c
> > +++ b/src/sched_deadline/deadline_test.c
> > @@ -1181,7 +1181,7 @@ static int read_ctx_switches(int *vol, int *nonvol, int *migrate)
> >   *  @data->total_time - Total time it took to complete all loops
> >   *  @data->nr_periods - Number of periods that were executed.
> >   */
> > -static u64 do_runtime(long tid, struct sched_data *data, u64 period)
> > +static u64 do_runtime(struct sched_data *data, u64 period)
> >  {
> >  	u64 next_period = period + data->deadline_us;
> >  	u64 now = get_time_us();
> > @@ -1354,7 +1354,7 @@ void *run_deadline(void *data)
> >  	period = get_time_us();
> >  
> >  	while (!done) {
> > -		period = do_runtime(tid, sched_data, period);
> > +		period = do_runtime(sched_data, period);
> >  		sched_yield();
> >  	}
> >  	ret = sched_getattr(0, &attr, sizeof(attr), 0);
> > @@ -1714,7 +1714,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
> >  	do_sleep(1000);
> >  
> >  	start = get_time_us();
> > -	do_runtime(0, &sd, start + sd.deadline_us);
> > +	do_runtime(&sd, start + sd.deadline_us);
> >  	end = get_time_us();
> >  
> >  	diff = end - start;
> > @@ -1743,7 +1743,7 @@ static u64 calculate_loops_per_ms(u64 *overhead)
> >  	do_sleep(1000);
> >  
> >  	start = get_time_us();
> > -	do_runtime(0, &sd, start + sd.deadline_us);
> > +	do_runtime(&sd, start + sd.deadline_us);
> >  	end = get_time_us();
> >  
> >  	odiff = end - start;
> > @@ -1962,7 +1962,7 @@ int main(int argc, char **argv)
> >  
> >  		/* Make sure that we can make our deadlines */
> >  		start_period = get_time_us();
> > -		do_runtime(gettid(), sd, start_period);
> > +		do_runtime(sd, start_period);
> >  		end_period = get_time_us();
> >  		if (end_period - start_period > sd->runtime_us) {
> >  			printf("Failed to perform task within runtime: Missed by %lld us\n",
> > diff --git a/src/sigwaittest/sigwaittest.c b/src/sigwaittest/sigwaittest.c
> > index 55855769c63b..8c1c16fb3081 100644
> > --- a/src/sigwaittest/sigwaittest.c
> > +++ b/src/sigwaittest/sigwaittest.c
> > @@ -375,7 +375,7 @@ static void sighand(int sig __attribute__ ((unused)))
> >  	mustshutdown = 1;
> >  }
> >  
> > -static void print_stat(FILE *fp, struct params *receiver, struct params *sender,
> > +static void print_stat(struct params *receiver, struct params *sender,
> >  		       int verbose __attribute__ ((unused)), int quiet)
> >  {
> >  	int i;
> > @@ -644,7 +644,7 @@ int main(int argc, char *argv[])
> >  			    sender[i].shutdown;
> >  
> >  		if (receiver[0].samples > oldsamples || mustshutdown) {
> > -			print_stat(stdout, receiver, sender, 0, quiet);
> > +			print_stat(receiver, sender, 0, quiet);
> >  			if (!quiet)
> >  				printf("\033[%dA", num_threads*2);
> >  		}
> > @@ -664,7 +664,7 @@ int main(int argc, char *argv[])
> >  	if (!quiet)
> >  		printf("\033[%dB", num_threads*2 + 2);
> >  	else
> > -		print_stat(stdout, receiver, sender, 0, 0);
> > +		print_stat(receiver, sender, 0, 0);
> >  
> >  	for (i = 0; i < num_threads; i++) {
> >  		receiver[i].shutdown = 1;
> > -- 
> > 2.43.0
> > 
> > 
> > 
> 
> I tested this with rteval, and it works fine
> checkpatch in the linux kernel flags a few minor things.
> Would you mind fixing those up before I apply the patch?
> 
> WARNING: Improper SPDX comment style for 'src/include/histogram.h', please 
> use '/*' instead
> #227: FILE: src/include/histogram.h:1:
> +// SPDX-License-Identifier: GPL-2.0-or-later
> 
> (Yes, I know there are other files in rt-tests that also aren't using
> the recommended commenting style, you probably copied from there.)
> 
> WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> #227: FILE: src/include/histogram.h:1:
> +// SPDX-License-Identifier: GPL-2.0-or-later
> 
> WARNING: braces {} are not necessary for any arm of this statement
> #333: FILE: src/lib/histogram.c:59:
> +	if (h->oflow_magnitude + extra > h->oflow_magnitude) {
> [...]
> +	} else {
> [...]
> 
> ERROR: trailing whitespace
> #338: FILE: src/lib/histogram.c:64:
> +^I^I$
> 
> WARNING: braces {} are not necessary for any arm of this statement
> #340: FILE: src/lib/histogram.c:66:
> +		if (h->oflow_count < h->oflow_bufsize) {
> [...]
> +		} else {
> [...]
> 
> total: 1 errors, 5 warnings, 394 lines checked
> 
> Thanks
> 
> John Kacur
> 

Sorry, the comments are for patch number 3
John

Re: [PATCH 1/4] rt-tests: Fix warnings

[Crystal Wood]

On Thu, 2023-12-14 at 10:48 -0500, John Kacur wrote:
> 
> I tested this with rteval, and it works fine
> checkpatch in the linux kernel flags a few minor things.
> Would you mind fixing those up before I apply the patch?
> 
> WARNING: Improper SPDX comment style for 'src/include/histogram.h', please
> use '/*' instead
> #227: FILE: src/include/histogram.h:1:
> +// SPDX-License-Identifier: GPL-2.0-or-later
> 
> (Yes, I know there are other files in rt-tests that also aren't using
> the recommended commenting style, you probably copied from there.)

This confused me for quite a bit, until I saw in
Documentation/process/license-rules.rst that *sources* get // but *headers*
get /*.  The rationale seems to be based on the behavior of old toolchains
when processing assembly code, linker scripts, etc.  Doesn't seem terribly
relevant to non-kernel projects...

I can still "fix" it if you want.


> 
> ERROR: trailing whitespace
> #338: FILE: src/lib/histogram.c:64:
> +^I^I$

One of these days I need to teach the joe editor not to do this...

-Crystal

Re: [PATCH 1/4] rt-tests: Fix warnings

[John Kacur]

On Mon, 18 Dec 2023, Crystal Wood wrote:

> On Thu, 2023-12-14 at 10:48 -0500, John Kacur wrote:
> > 
> > I tested this with rteval, and it works fine
> > checkpatch in the linux kernel flags a few minor things.
> > Would you mind fixing those up before I apply the patch?
> > 
> > WARNING: Improper SPDX comment style for 'src/include/histogram.h', please
> > use '/*' instead
> > #227: FILE: src/include/histogram.h:1:
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > 
> > (Yes, I know there are other files in rt-tests that also aren't using
> > the recommended commenting style, you probably copied from there.)
> 
> This confused me for quite a bit, until I saw in
> Documentation/process/license-rules.rst that *sources* get // but *headers*
> get /*.  The rationale seems to be based on the behavior of old toolchains
> when processing assembly code, linker scripts, etc.  Doesn't seem terribly
> relevant to non-kernel projects...
> 
> I can still "fix" it if you want.
> 
> 
> > 
> > ERROR: trailing whitespace
> > #338: FILE: src/lib/histogram.c:64:
> > +^I^I$
> 
> One of these days I need to teach the joe editor not to do this...
> 
> -Crystal
> 
> 
> 
No, your explanation works for me! Just fix the warnings about
the brace styles and I'll apply it.

John