[PATCH rt-tests 0/9] more rt-tests cleanups and a cyclictest feature

[PATCH rt-tests 0/9] more rt-tests cleanups and a cyclictest feature

[Josh Cartwright]

Hey Clark, John-

Here's another set, this one contains eight patches are very minor cleanups
(found mostly via code inspection and by building with warnings cranked up).

The last patch introduces an option to cyclictest to control the dumping of
histogram data into a separate file, it's useful for when you want to see the
'normal' cyclictest live-updating, but also want to store away the generated
histogram for later analysis.

(My goal was to present a net-negative diffstat...I came so close!)

Thanks!
  Josh

Gratian Crisan (1):
  cyclictest: Add option for dumping the histogram in a file

Josh Cartwright (8):
  cyclictest: consistently make all functions 'static'
  cyclictest: fixup documentation for --priority option
  cyclictest: drop unnecessary numa_on_and_available() check
  signaltest: drop unused tsnorm()
  cyclictest: use correct type when allocating cpu bitmask size
  cyclictest: drop impossible use_fifo conditional
  cyclictest: fail if use_fifo && thread creation failed
  error: mark fatal, err_exit, err_quit as being noreturn

 src/cyclictest/cyclictest.c | 120 ++++++++++++++++++++++++++------------------
 src/cyclictest/rt_numa.h    |  11 +---
 src/include/error.h         |   6 +--
 src/signaltest/signaltest.c |   8 ---
 4 files changed, 74 insertions(+), 71 deletions(-)

-- 
2.5.0

[PATCH rt-tests 1/9] cyclictest: consistently make all functions 'static'

[Josh Cartwright]

Most functions in cyclictest were already 'static', with a few
exceptions.  Fixup those exceptions, in the interest of consistency,
optimization, etc.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/cyclictest/cyclictest.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 34053c5..9aebe32 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -392,7 +392,7 @@ static inline int64_t calcdiff_ns(struct timespec t1, struct timespec t2)
 	return diff;
 }
 
-void traceopt(char *option)
+static void traceopt(char *option)
 {
 	char *ptr;
 	if (traceopt_count + 1 > traceopt_size) {
@@ -442,7 +442,7 @@ static void tracemark(char *fmt, ...)
 
 
 
-void tracing(int on)
+static void tracing(int on)
 {
 	if (on) {
 		switch (kernelversion) {
@@ -638,7 +638,7 @@ static void setup_tracer(void)
  *
  * the return value is a value in seconds
  */
-int parse_time_string(char *val)
+static int parse_time_string(char *val)
 {
 	char *end;
 	int t = strtol(val, &end, 10);
@@ -749,7 +749,7 @@ try_again:
  * - CLOCK_REALTIME
  *
  */
-void *timerthread(void *param)
+static void *timerthread(void *param)
 {
 	struct thread_param *par = param;
 	struct sched_param schedp;
@@ -1766,7 +1766,7 @@ static void print_stat(FILE *fp, struct thread_param *par, int index, int verbos
  * thread that creates a named fifo and hands out run stats when someone
  * reads from the fifo.
  */
-void *fifothread(void *param)
+static void *fifothread(void *param)
 {
 	int ret;
 	int fd;
-- 
2.5.0

Re: [PATCH rt-tests 1/9] cyclictest: consistently make all functions 'static'

[John Kacur]

On Mon, 31 Aug 2015, Josh Cartwright wrote:

> Most functions in cyclictest were already 'static', with a few
> exceptions.  Fixup those exceptions, in the interest of consistency,
> optimization, etc.
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/cyclictest/cyclictest.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 34053c5..9aebe32 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -392,7 +392,7 @@ static inline int64_t calcdiff_ns(struct timespec t1, struct timespec t2)
>  	return diff;
>  }
>  
> -void traceopt(char *option)
> +static void traceopt(char *option)
>  {
>  	char *ptr;
>  	if (traceopt_count + 1 > traceopt_size) {
> @@ -442,7 +442,7 @@ static void tracemark(char *fmt, ...)
>  
>  
>  
> -void tracing(int on)
> +static void tracing(int on)
>  {
>  	if (on) {
>  		switch (kernelversion) {
> @@ -638,7 +638,7 @@ static void setup_tracer(void)
>   *
>   * the return value is a value in seconds
>   */
> -int parse_time_string(char *val)
> +static int parse_time_string(char *val)
>  {
>  	char *end;
>  	int t = strtol(val, &end, 10);
> @@ -749,7 +749,7 @@ try_again:
>   * - CLOCK_REALTIME
>   *
>   */
> -void *timerthread(void *param)
> +static void *timerthread(void *param)
>  {
>  	struct thread_param *par = param;
>  	struct sched_param schedp;
> @@ -1766,7 +1766,7 @@ static void print_stat(FILE *fp, struct thread_param *par, int index, int verbos
>   * thread that creates a named fifo and hands out run stats when someone
>   * reads from the fifo.
>   */
> -void *fifothread(void *param)
> +static void *fifothread(void *param)
>  {
>  	int ret;
>  	int fd;
> -- 
> 2.5.0
> 
> --

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

[PATCH rt-tests 2/9] cyclictest: fixup documentation for --priority option

[Josh Cartwright]

Commit 15c6d81986b6 ("[cyclictest] added priority spreading option
--priospread") introduced an additional option with the prefix 'prio';
this means that it is no longer possible for a user to specify '--prio'
instead of the full '--priority'.  Doing so results in the following
error:

  $ cyclictest --prio=4
  cyclictest: option '--prio=4' is ambiguous; possibilities: '--priority' '--priospread'

Fix this up by updating the 'help' output.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/cyclictest/cyclictest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 9aebe32..4ced67f 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -1062,7 +1062,7 @@ static void display_help(int error)
 	       "-N       --nsecs           print results in ns instead of us (default us)\n"
 	       "-o RED   --oscope=RED      oscilloscope mode, reduce verbose output by RED\n"
 	       "-O TOPT  --traceopt=TOPT   trace option\n"
-	       "-p PRIO  --prio=PRIO       priority of highest prio thread\n"
+	       "-p PRIO  --priority=PRIO   priority of highest prio thread\n"
 	       "-P       --preemptoff      Preempt off tracing (used with -b)\n"
 	       "-q       --quiet           print only a summary on exit\n"
 	       "	 --priospread       spread priority levels starting at specified value\n"
-- 
2.5.0

Re: [PATCH rt-tests 2/9] cyclictest: fixup documentation for --priority option

[John Kacur]

On Mon, 31 Aug 2015, Josh Cartwright wrote:

> Commit 15c6d81986b6 ("[cyclictest] added priority spreading option
> --priospread") introduced an additional option with the prefix 'prio';
> this means that it is no longer possible for a user to specify '--prio'
> instead of the full '--priority'.  Doing so results in the following
> error:
> 
>   $ cyclictest --prio=4
>   cyclictest: option '--prio=4' is ambiguous; possibilities: '--priority' '--priospread'
> 
> Fix this up by updating the 'help' output.
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/cyclictest/cyclictest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 9aebe32..4ced67f 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -1062,7 +1062,7 @@ static void display_help(int error)
>  	       "-N       --nsecs           print results in ns instead of us (default us)\n"
>  	       "-o RED   --oscope=RED      oscilloscope mode, reduce verbose output by RED\n"
>  	       "-O TOPT  --traceopt=TOPT   trace option\n"
> -	       "-p PRIO  --prio=PRIO       priority of highest prio thread\n"
> +	       "-p PRIO  --priority=PRIO   priority of highest prio thread\n"
>  	       "-P       --preemptoff      Preempt off tracing (used with -b)\n"
>  	       "-q       --quiet           print only a summary on exit\n"
>  	       "	 --priospread       spread priority levels starting at specified value\n"
> -- 
> 2.5.0

This was already fixed in v0.93
417ddb5db0ab cyclictest: Fix long priority help text option

Thanks> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH rt-tests 2/9] cyclictest: fixup documentation for --priority option

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]

On Tue, Sep 01, 2015 at 11:03:08PM +0200, John Kacur wrote:
> On Mon, 31 Aug 2015, Josh Cartwright wrote:
> 
> > Commit 15c6d81986b6 ("[cyclictest] added priority spreading option
> > --priospread") introduced an additional option with the prefix 'prio';
> > this means that it is no longer possible for a user to specify '--prio'
> > instead of the full '--priority'.  Doing so results in the following
> > error:
> > 
> >   $ cyclictest --prio=4
> >   cyclictest: option '--prio=4' is ambiguous; possibilities: '--priority' '--priospread'
> > 
> > Fix this up by updating the 'help' output.
> > 
> > Signed-off-by: Josh Cartwright <joshc@ni.com>
> > ---
> >  src/cyclictest/cyclictest.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> > index 9aebe32..4ced67f 100644
> > --- a/src/cyclictest/cyclictest.c
> > +++ b/src/cyclictest/cyclictest.c
> > @@ -1062,7 +1062,7 @@ static void display_help(int error)
> >  	       "-N       --nsecs           print results in ns instead of us (default us)\n"
> >  	       "-o RED   --oscope=RED      oscilloscope mode, reduce verbose output by RED\n"
> >  	       "-O TOPT  --traceopt=TOPT   trace option\n"
> > -	       "-p PRIO  --prio=PRIO       priority of highest prio thread\n"
> > +	       "-p PRIO  --priority=PRIO   priority of highest prio thread\n"
> >  	       "-P       --preemptoff      Preempt off tracing (used with -b)\n"
> >  	       "-q       --quiet           print only a summary on exit\n"
> >  	       "	 --priospread       spread priority levels starting at specified value\n"
> > -- 
> > 2.5.0
> 
> This was already fixed in v0.93
> 417ddb5db0ab cyclictest: Fix long priority help text option

Ahh, so it was.  Sorry about that, I had my git remote pointed at
clrkwllms/rt-tests.git instead of rt-tests/rt-tests.git which hadn't
been updated.

The rest should still be relevant.

Thanks,

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH rt-tests 3/9] cyclictest: drop unnecessary numa_on_and_available() check

[Josh Cartwright]

The condition that numa_on_and_available() checks is impossible to hit,
as it's already being checked during process_options().

Removal of numa_on_and_available() also has the side effect of removing
the following warning during build:

src/cyclictest/rt_numa.h:259:15: warning: implicit declaration of function ‘numa_available’ [-Wimplicit-function-declaration]

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/cyclictest/cyclictest.c | 3 ---
 src/cyclictest/rt_numa.h    | 9 ---------
 2 files changed, 12 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 4ced67f..dc754fd 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -1816,9 +1816,6 @@ int main(int argc, char **argv)
 	if (verbose)
 		printf("Max CPUs = %d\n", max_cpus);
 
-	/* Checks if numa is on, program exits if numa on but not available */
-	numa_on_and_available();
-
 	/* lock all memory (prevent swapping) */
 	if (lockall)
 		if (mlockall(MCL_CURRENT|MCL_FUTURE) == -1) {
diff --git a/src/cyclictest/rt_numa.h b/src/cyclictest/rt_numa.h
index 06c9420..caa80e6 100644
--- a/src/cyclictest/rt_numa.h
+++ b/src/cyclictest/rt_numa.h
@@ -251,15 +251,6 @@ static inline void rt_bitmask_free(struct bitmask *mask)
 
 #endif	/* NUMA */
 
-/*
- * Any behavioral differences above are transparent to these functions
- */
-static void numa_on_and_available()
-{
-	if (numa && (numa_available() == -1))
-		fatal("--numa specified and numa functions not available.\n");
-}

Re: [PATCH rt-tests 3/9] cyclictest: drop unnecessary numa_on_and_available() check

[John Kacur]

[-- Attachment #1: Type: text/plain, Size: 5367 bytes --]



On Mon, 31 Aug 2015, Josh Cartwright wrote:

> The condition that numa_on_and_available() checks is impossible to hit,
> as it's already being checked during process_options().
> 
> Removal of numa_on_and_available() also has the side effect of removing
> the following warning during build:
> 
> src/cyclictest/rt_numa.h:259:15: warning: implicit declaration of function ‘numa_available’ [-Wimplicit-function-declaration]
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/cyclictest/cyclictest.c | 3 ---
>  src/cyclictest/rt_numa.h    | 9 ---------
>  2 files changed, 12 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 4ced67f..dc754fd 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -1816,9 +1816,6 @@ int main(int argc, char **argv)
>  	if (verbose)
>  		printf("Max CPUs = %d\n", max_cpus);
>  
> -	/* Checks if numa is on, program exits if numa on but not available */
> -	numa_on_and_available();
> -
>  	/* lock all memory (prevent swapping) */
>  	if (lockall)
>  		if (mlockall(MCL_CURRENT|MCL_FUTURE) == -1) {
> diff --git a/src/cyclictest/rt_numa.h b/src/cyclictest/rt_numa.h
> index 06c9420..caa80e6 100644
> --- a/src/cyclictest/rt_numa.h
> +++ b/src/cyclictest/rt_numa.h
> @@ -251,15 +251,6 @@ static inline void rt_bitmask_free(struct bitmask *mask)
>  
>  #endif	/* NUMA */
>  
> -/*
> - * Any behavioral differences above are transparent to these functions
> - */
> -static void numa_on_and_available()
> -{
> -	if (numa && (numa_available() == -1))
> -		fatal("--numa specified and numa functions not available.\n");
> -}
> -
>  /** Returns number of bits set in mask. */
>  static inline unsigned int rt_numa_bitmask_count(const struct bitmask *mask)
>  {
> -- 
> 2.5.0
> 
> --

I would prefer not to drop this function, since I think it cleanly 
documents a necessary initialization for using numa, althought here is 
something to be said for how cleanly your patch changes it. Proposing the 
following patch for now instead. Further clean-ups are possible.

>From c74b5d3d0e8a4a298dd0480f832e43ac886fdca0 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Tue, 15 Sep 2015 23:37:12 +0200
Subject: [PATCH] numa_on_and_available: Remove from main in cyclictest

We don't support building without numa libs anymore, although we of
course support running on machines without numa. Never-the-less I
created two versions of numa_on_and_available, one for if you build with
the unsupported NUMA=0 and one for if you build with NUMA=1, which is
the default.

I would prefer not to drop this function, since I think it cleanly
documents the fact that numa_available must be called before any other
numa library functions are defined.

As Josh Cartwright reported though, there was no need to call it from
main() since it was already tested in process_options(), so I tested it
there.

Tested by building with NUMA=0, NUMA=1 and with the non-standard
-Wimplicit-function-declaration

Reported-by: Signed-off-by: Josh Cartwright <joshc@ni.com>
Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/cyclictest/cyclictest.c |  7 ++-----
 src/cyclictest/rt_numa.h    | 18 +++++++++++++-----
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 213c527fd714..b3abfcc67407 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -1451,12 +1451,11 @@ static void process_options (int argc, char *argv[], int max_cpus)
 			setvbuf(stdout, NULL, _IONBF, 0); break;
 		case 'U':
 		case OPT_NUMA: /* NUMA testing */
+			numa = 1;	/* Turn numa on */
 			if (smp)
 				fatal("numa and smp options are mutually exclusive\n");
+			numa_on_and_available();
 #ifdef NUMA
-			if (numa_available() == -1)
-				fatal("NUMA functionality not available!");
-			numa = 1;
 			num_threads = max_cpus;
 			setaffinity = AFFINITY_USEALL;
 			use_nanosleep = MODE_CLOCK_NANOSLEEP;
@@ -1816,8 +1815,6 @@ int main(int argc, char **argv)
 	if (verbose)
 		printf("Max CPUs = %d\n", max_cpus);
 
-	/* Checks if numa is on, program exits if numa on but not available */
-	numa_on_and_available();
 
 	/* lock all memory (prevent swapping) */
 	if (lockall)
diff --git a/src/cyclictest/rt_numa.h b/src/cyclictest/rt_numa.h
index 06c9420e53cc..c1b3f942762d 100644
--- a/src/cyclictest/rt_numa.h
+++ b/src/cyclictest/rt_numa.h
@@ -192,6 +192,12 @@ static inline void rt_bitmask_free(struct bitmask *mask)
 
 #endif	/* LIBNUMA_API_VERSION */
 
+static void numa_on_and_available()
+{
+	if (numa && (numa_available() == -1))
+		fatal("--numa specified and numa functions not available.\n");
+}
+
 #else /* ! NUMA */
 
 struct bitmask {
@@ -249,17 +255,19 @@ static inline void rt_bitmask_free(struct bitmask *mask)
 	free(mask);
 }
 
-#endif	/* NUMA */
 
-/*
- * Any behavioral differences above are transparent to these functions
- */
 static void numa_on_and_available()
 {
-	if (numa && (numa_available() == -1))
+	if (numa) /* NUMA is not defined here */
 		fatal("--numa specified and numa functions not available.\n");
 }
 
+#endif	/* NUMA */
+
+/*
+ * Any behavioral differences above are transparent to these functions
+ */
+
 /** Returns number of bits set in mask. */
 static inline unsigned int rt_numa_bitmask_count(const struct bitmask *mask)
 {
-- 
2.4.3

Re: [PATCH rt-tests 3/9] cyclictest: drop unnecessary numa_on_and_available() check

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]

On Tue, Sep 15, 2015 at 11:50:16PM +0200, John Kacur wrote:
> On Mon, 31 Aug 2015, Josh Cartwright wrote:
> 
> > The condition that numa_on_and_available() checks is impossible to hit,
> > as it's already being checked during process_options().
> > 
> > Removal of numa_on_and_available() also has the side effect of removing
> > the following warning during build:
> > 
> > src/cyclictest/rt_numa.h:259:15: warning: implicit declaration of function ???numa_available??? [-Wimplicit-function-declaration]
> > 
> > Signed-off-by: Josh Cartwright <joshc@ni.com>

[..]

> We don't support building without numa libs anymore, although we of
> course support running on machines without numa. Never-the-less I
> created two versions of numa_on_and_available, one for if you build with
> the unsupported NUMA=0 and one for if you build with NUMA=1, which is
> the default.

:(.  You risk plenty of embedded folks who don't much care about NUMA.

> I would prefer not to drop this function, since I think it cleanly
> documents the fact that numa_available must be called before any other
> numa library functions are defined.
> 
> As Josh Cartwright reported though, there was no need to call it from
> main() since it was already tested in process_options(), so I tested it
> there.
> 
> Tested by building with NUMA=0, NUMA=1 and with the non-standard
> -Wimplicit-function-declaration
> 
> Reported-by: Signed-off-by: Josh Cartwright <joshc@ni.com>

s/Signed-off-by: /

:)

> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  src/cyclictest/cyclictest.c |  7 ++-----
>  src/cyclictest/rt_numa.h    | 18 +++++++++++++-----
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 213c527fd714..b3abfcc67407 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -1451,12 +1451,11 @@ static void process_options (int argc, char *argv[], int max_cpus)
>  			setvbuf(stdout, NULL, _IONBF, 0); break;
>  		case 'U':
>  		case OPT_NUMA: /* NUMA testing */
> +			numa = 1;	/* Turn numa on */
>  			if (smp)
>  				fatal("numa and smp options are mutually exclusive\n");
> +			numa_on_and_available();

...except numa is always on here.  So why check if NUMA is enabled
again?  Seems like a waste.

>  #ifdef NUMA
> -			if (numa_available() == -1)
> -				fatal("NUMA functionality not available!");

Perhaps a middle ground should be to keep this check around and define a
static inline numa_available() in the NUMA:=0 case which unconditionally
returns -1.

Thanks for taking a look,
  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH rt-tests 3/9] cyclictest: drop unnecessary numa_on_and_available() check

[John Kacur]

On Wed, 16 Sep 2015, Josh Cartwright wrote:

> On Tue, Sep 15, 2015 at 11:50:16PM +0200, John Kacur wrote:
> > On Mon, 31 Aug 2015, Josh Cartwright wrote:
> > 
> > > The condition that numa_on_and_available() checks is impossible to hit,
> > > as it's already being checked during process_options().
> > > 
> > > Removal of numa_on_and_available() also has the side effect of removing
> > > the following warning during build:
> > > 
> > > src/cyclictest/rt_numa.h:259:15: warning: implicit declaration of function ???numa_available??? [-Wimplicit-function-declaration]
> > > 
> > > Signed-off-by: Josh Cartwright <joshc@ni.com>
> 
> [..]
> 
> > We don't support building without numa libs anymore, although we of
> > course support running on machines without numa. Never-the-less I
> > created two versions of numa_on_and_available, one for if you build with
> > the unsupported NUMA=0 and one for if you build with NUMA=1, which is
> > the default.
> 
> :(.  You risk plenty of embedded folks who don't much care about NUMA.

Sorry, there is some confusion here, and it's probably partly on my side.
We do of course still support building without NUMA. All that I was really 
referring to was, that if you are building on platforms that support NUMA,
we are making the development libs a requirement for compiling, in our 
distro spec files, even you are running without NUMA
But please continue to test with NUMA=0 builds as well as the default, and 
report / or fix any builds that don't work with NUMA=0

> 
> > I would prefer not to drop this function, since I think it cleanly
> > documents the fact that numa_available must be called before any other
> > numa library functions are defined.
> > 
> > As Josh Cartwright reported though, there was no need to call it from
> > main() since it was already tested in process_options(), so I tested it
> > there.
> > 
> > Tested by building with NUMA=0, NUMA=1 and with the non-standard
> > -Wimplicit-function-declaration
> > 
> > Reported-by: Signed-off-by: Josh Cartwright <joshc@ni.com>
> 
> s/Signed-off-by: /
> 

Oops, okay, fixing that msg.

> :)
> 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> > ---
> >  src/cyclictest/cyclictest.c |  7 ++-----
> >  src/cyclictest/rt_numa.h    | 18 +++++++++++++-----
> >  2 files changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> > index 213c527fd714..b3abfcc67407 100644
> > --- a/src/cyclictest/cyclictest.c
> > +++ b/src/cyclictest/cyclictest.c
> > @@ -1451,12 +1451,11 @@ static void process_options (int argc, char *argv[], int max_cpus)
> >  			setvbuf(stdout, NULL, _IONBF, 0); break;
> >  		case 'U':
> >  		case OPT_NUMA: /* NUMA testing */
> > +			numa = 1;	/* Turn numa on */
> >  			if (smp)
> >  				fatal("numa and smp options are mutually exclusive\n");
> > +			numa_on_and_available();
> 
> ...except numa is always on here.  So why check if NUMA is enabled
> again?  Seems like a waste.
> 
> >  #ifdef NUMA
> > -			if (numa_available() == -1)
> > -				fatal("NUMA functionality not available!");
> 
> Perhaps a middle ground should be to keep this check around and define a
> static inline numa_available() in the NUMA:=0 case which unconditionally
> returns -1.
> 
> Thanks for taking a look,

You are of course correct here, but it's just one extra check at init 
time, so the impact is pretty close to zero.

Will consider more clean-ups in the future, but it's probably not worth 
obsessing about. Like I said, I like the api more as documentation for the 
process of setting numa up.

Thanks

John

[PATCH rt-tests 4/9] signaltest: drop unused tsnorm()

[Josh Cartwright]

tsnorm() is not used at all in signaltest.  Remove it.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/signaltest/signaltest.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/src/signaltest/signaltest.c b/src/signaltest/signaltest.c
index 9454a26..c6d1cfd 100644
--- a/src/signaltest/signaltest.c
+++ b/src/signaltest/signaltest.c
@@ -69,14 +69,6 @@ static int shutdown;
 static int tracelimit = 0;
 static int oldtrace = 0;
 
-static inline void tsnorm(struct timespec *ts)
-{
-	while (ts->tv_nsec >= NSEC_PER_SEC) {
-		ts->tv_nsec -= NSEC_PER_SEC;
-		ts->tv_sec++;
-	}
-}

Re: [PATCH rt-tests 4/9] signaltest: drop unused tsnorm()

[John Kacur]

On Mon, 31 Aug 2015, Josh Cartwright wrote:

> tsnorm() is not used at all in signaltest.  Remove it.
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/signaltest/signaltest.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/src/signaltest/signaltest.c b/src/signaltest/signaltest.c
> index 9454a26..c6d1cfd 100644
> --- a/src/signaltest/signaltest.c
> +++ b/src/signaltest/signaltest.c
> @@ -69,14 +69,6 @@ static int shutdown;
>  static int tracelimit = 0;
>  static int oldtrace = 0;
>  
> -static inline void tsnorm(struct timespec *ts)
> -{
> -	while (ts->tv_nsec >= NSEC_PER_SEC) {
> -		ts->tv_nsec -= NSEC_PER_SEC;
> -		ts->tv_sec++;
> -	}
> -}
> -
>  static inline long calcdiff(struct timespec t1, struct timespec t2)
>  {
>  	long diff;
> -- 
> 2.5.0
> 
> --

As you can see, many of the programs in this suite are modeled after 
cyclictest, but many of them don't unfortunately receive nearly the 
amount of testing that cyclictest does. Now the fact that this function 
was copied from cyclictest, but not used, sends off alarm bells in my 
head. We have various struct timespec in signaltest, could the fact that 
we are not calling tsnorm mean that there are some hidden potential 
defects? Rather than removing this function, I'd like to spend some time 
auditing the use of timespec here until I'm convinced.

Thanks

John

Re: [PATCH rt-tests 4/9] signaltest: drop unused tsnorm()

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

On Wed, Sep 16, 2015 at 12:00:10AM +0200, John Kacur wrote:
> On Mon, 31 Aug 2015, Josh Cartwright wrote:
> 
> > tsnorm() is not used at all in signaltest.  Remove it.
> > 
> > Signed-off-by: Josh Cartwright <joshc@ni.com>
> > ---
> >  src/signaltest/signaltest.c | 8 --------
> >  1 file changed, 8 deletions(-)
> > 
> > diff --git a/src/signaltest/signaltest.c b/src/signaltest/signaltest.c
> > index 9454a26..c6d1cfd 100644
> > --- a/src/signaltest/signaltest.c
> > +++ b/src/signaltest/signaltest.c
> > @@ -69,14 +69,6 @@ static int shutdown;
> >  static int tracelimit = 0;
> >  static int oldtrace = 0;
> >  
> > -static inline void tsnorm(struct timespec *ts)
> > -{
> > -	while (ts->tv_nsec >= NSEC_PER_SEC) {
> > -		ts->tv_nsec -= NSEC_PER_SEC;
> > -		ts->tv_sec++;
> > -	}
> > -}
> > -
> >  static inline long calcdiff(struct timespec t1, struct timespec t2)
> >  {
> >  	long diff;
> > -- 
> > 2.5.0
> > 
> > --
> 
> As you can see, many of the programs in this suite are modeled after 
> cyclictest, but many of them don't unfortunately receive nearly the 
> amount of testing that cyclictest does. Now the fact that this function 
> was copied from cyclictest, but not used, sends off alarm bells in my 
> head. We have various struct timespec in signaltest, could the fact that 
> we are not calling tsnorm mean that there are some hidden potential 
> defects? Rather than removing this function, I'd like to spend some time 
> auditing the use of timespec here until I'm convinced.

Yeah, I did the cheap-and-easy thing here; but you raise a good point,
we should audit signaltest to see where any timespec math is done to
ensure proper normalization.

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH rt-tests 5/9] cyclictest: use correct type when allocating cpu bitmask size

[Josh Cartwright]

On any sane platform sizeof(long) == sizeof(unsigned long), so this
does not actually fix a real bug, but the code should at least be
consistent.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/cyclictest/rt_numa.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cyclictest/rt_numa.h b/src/cyclictest/rt_numa.h
index caa80e6..9da9fd8 100644
--- a/src/cyclictest/rt_numa.h
+++ b/src/cyclictest/rt_numa.h
@@ -229,7 +229,7 @@ static inline struct bitmask* rt_numa_parse_cpustring(const char* s,
 			 * max_cpus bits */
 			int nlongs = (max_cpus+BITS_PER_LONG-1)/BITS_PER_LONG;
 
-			mask->maskp = calloc(nlongs, sizeof(long));
+			mask->maskp = calloc(nlongs, sizeof(unsigned long));
 			if (mask->maskp) {
 				mask->maskp[cpu/BITS_PER_LONG] |=
 					(1UL << (cpu % BITS_PER_LONG));
-- 
2.5.0

Re: [PATCH rt-tests 5/9] cyclictest: use correct type when allocating cpu bitmask size

[John Kacur]

On Mon, 31 Aug 2015, Josh Cartwright wrote:

> On any sane platform sizeof(long) == sizeof(unsigned long), so this
> does not actually fix a real bug, but the code should at least be
> consistent.
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/cyclictest/rt_numa.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/cyclictest/rt_numa.h b/src/cyclictest/rt_numa.h
> index caa80e6..9da9fd8 100644
> --- a/src/cyclictest/rt_numa.h
> +++ b/src/cyclictest/rt_numa.h
> @@ -229,7 +229,7 @@ static inline struct bitmask* rt_numa_parse_cpustring(const char* s,
>  			 * max_cpus bits */
>  			int nlongs = (max_cpus+BITS_PER_LONG-1)/BITS_PER_LONG;
>  
> -			mask->maskp = calloc(nlongs, sizeof(long));
> +			mask->maskp = calloc(nlongs, sizeof(unsigned long));
>  			if (mask->maskp) {
>  				mask->maskp[cpu/BITS_PER_LONG] |=
>  					(1UL << (cpu % BITS_PER_LONG));
> -- 
> 2.5.0
> 
> --

Suspect this isn't the only inconsistency here between longs and unsigned 
longs, but
Signed-off-by: John Kacur <jkacur@redhat.com>

[PATCH rt-tests 6/9] cyclictest: drop impossible use_fifo conditional

[Josh Cartwright]

The fifothread is only created when use_fifo is set; having the thread
itself perform a check is redundant and unnecessary.  Drop it.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/cyclictest/cyclictest.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index dc754fd..896cdd7 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -1773,9 +1773,6 @@ static void *fifothread(void *param)
 	FILE *fp;
 	int i;
 
-	if (use_fifo == 0)
-		return NULL;

Re: [PATCH rt-tests 6/9] cyclictest: drop impossible use_fifo conditional

[John Kacur]

On Mon, 31 Aug 2015, Josh Cartwright wrote:

> The fifothread is only created when use_fifo is set; having the thread
> itself perform a check is redundant and unnecessary.  Drop it.
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/cyclictest/cyclictest.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index dc754fd..896cdd7 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -1773,9 +1773,6 @@ static void *fifothread(void *param)
>  	FILE *fp;
>  	int i;
>  
> -	if (use_fifo == 0)
> -		return NULL;
> -
>  	unlink(fifopath);
>  	ret = mkfifo(fifopath, 0666);
>  	if (ret) {
> -- 
> 2.5.0
> 
> --
Signed-off-by: John Kacur <jkacur@redhat.com>

[PATCH rt-tests 7/9] cyclictest: fail if use_fifo && thread creation failed

[Josh Cartwright]

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/cyclictest/cyclictest.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 896cdd7..2c44f8e 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -2058,8 +2058,11 @@ int main(int argc, char **argv)
 			fatal("failed to create thread %d: %s\n", i, strerror(status));
 
 	}
-	if (use_fifo)
+	if (use_fifo) {
 		status = pthread_create(&fifo_threadid, NULL, fifothread, NULL);
+		if (status)
+			fatal("failed to create fifo thread: %s\n", strerror(status));
+	}
 
 	while (!shutdown) {
 		char lavg[256];
-- 
2.5.0

Re: [PATCH rt-tests 7/9] cyclictest: fail if use_fifo && thread creation failed

[John Kacur]

On Mon, 31 Aug 2015, Josh Cartwright wrote:

> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/cyclictest/cyclictest.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 896cdd7..2c44f8e 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -2058,8 +2058,11 @@ int main(int argc, char **argv)
>  			fatal("failed to create thread %d: %s\n", i, strerror(status));
>  
>  	}
> -	if (use_fifo)
> +	if (use_fifo) {
>  		status = pthread_create(&fifo_threadid, NULL, fifothread, NULL);
> +		if (status)
> +			fatal("failed to create fifo thread: %s\n", strerror(status));
> +	}
>  
>  	while (!shutdown) {
>  		char lavg[256];
> -- 
> 2.5.0
> 
> --

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

[PATCH rt-tests 8/9] error: mark fatal, err_exit, err_quit as being noreturn

[Josh Cartwright]

These functions never return to their caller.  Mark them as such to aide
in code generation and help out static analysis.

Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/include/error.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/include/error.h b/src/include/error.h
index ae05a2e..4acff49 100644
--- a/src/include/error.h
+++ b/src/include/error.h
@@ -6,14 +6,14 @@
 #include <stdarg.h>
 #include <string.h>
 
-void err_exit(int err, char *fmt, ...);
+void err_exit(int err, char *fmt, ...) __attribute__((noreturn));
 void err_msg(char *fmt, ...);
 void err_msg_n(int err, char *fmt, ...);
-void err_quit(char *fmt, ...);
+void err_quit(char *fmt, ...) __attribute__((noreturn));
 void debug(char *fmt, ...);
 void info(char *fmt, ...);
 void warn(char *fmt, ...);
-void fatal(char *fmt, ...);
+void fatal(char *fmt, ...) __attribute__((noreturn));
 void err_doit(int err, const char *fmt, va_list ap);
 
 #endif	/* __ERROR_H */
-- 
2.5.0

Re: [PATCH rt-tests 8/9] error: mark fatal, err_exit, err_quit as being noreturn

[John Kacur]

On Mon, 31 Aug 2015, Josh Cartwright wrote:

> These functions never return to their caller.  Mark them as such to aide
> in code generation and help out static analysis.
> 
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/include/error.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/include/error.h b/src/include/error.h
> index ae05a2e..4acff49 100644
> --- a/src/include/error.h
> +++ b/src/include/error.h
> @@ -6,14 +6,14 @@
>  #include <stdarg.h>
>  #include <string.h>
>  
> -void err_exit(int err, char *fmt, ...);
> +void err_exit(int err, char *fmt, ...) __attribute__((noreturn));
>  void err_msg(char *fmt, ...);
>  void err_msg_n(int err, char *fmt, ...);
> -void err_quit(char *fmt, ...);
> +void err_quit(char *fmt, ...) __attribute__((noreturn));
>  void debug(char *fmt, ...);
>  void info(char *fmt, ...);
>  void warn(char *fmt, ...);
> -void fatal(char *fmt, ...);
> +void fatal(char *fmt, ...) __attribute__((noreturn));
>  void err_doit(int err, const char *fmt, va_list ap);
>  
>  #endif	/* __ERROR_H */
> -- 
> 2.5.0
> 
> --
Signed-off-by: John Kacur <jkacur@redhat.com>

[PATCH rt-tests 9/9] cyclictest: add option for dumping the histogram in a file

[Josh Cartwright]

From: Gratian Crisan <gratian.crisan@ni.com>

Add an option '-J' or '--histfile' to dump the latency histogram to <path>
instead of stdout. This allows for live update of the current min, avg, and max
numbers while retaining the option to save histogram data for later analysis.

Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/cyclictest/cyclictest.c | 97 ++++++++++++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 37 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 2c44f8e..ec1ed17 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -188,6 +188,7 @@ static int aligned = 0;
 static int secaligned = 0;
 static int offset = 0;
 static int laptop = 0;
+static int use_histfile = 0;
 
 static pthread_cond_t refresh_on_max_cond = PTHREAD_COND_INITIALIZER;
 static pthread_mutex_t refresh_on_max_lock = PTHREAD_MUTEX_INITIALIZER;
@@ -210,6 +211,7 @@ static char *procfileprefix = "/proc/sys/kernel/";
 static char *fileprefix;
 static char tracer[MAX_PATH];
 static char fifopath[MAX_PATH];
+static char histfile[MAX_PATH];
 static char **traceptr;
 static int traceopt_count;
 static int traceopt_size;
@@ -1049,6 +1051,7 @@ static void display_help(int error)
 	       "                           (with same priority about many threads)\n"
 	       "                           US is the max time to be be tracked in microseconds\n"
 	       "-H       --histofall=US    same as -h except with an additional summary column\n"
+	       "-J       --histfile=<path> dump the latency histogram to <path> instead of stdout\n"
 	       "-i INTV  --interval=INTV   base interval of thread in us default=1000\n"
 	       "-I       --irqsoff         Irqsoff tracing (used with -b)\n"
 	       "-l LOOPS --loops=LOOPS     number of loops: default=0(endless)\n"
@@ -1214,13 +1217,13 @@ static char *policyname(int policy)
 enum option_values {
 	OPT_AFFINITY=1, OPT_NOTRACE, OPT_BREAKTRACE, OPT_PREEMPTIRQ, OPT_CLOCK,
 	OPT_CONTEXT, OPT_DISTANCE, OPT_DURATION, OPT_LATENCY, OPT_EVENT,
-	OPT_FTRACE, OPT_FIFO, OPT_HISTOGRAM, OPT_HISTOFALL, OPT_INTERVAL,
-	OPT_IRQSOFF, OPT_LOOPS, OPT_MLOCKALL, OPT_REFRESH, OPT_NANOSLEEP,
-	OPT_NSECS, OPT_OSCOPE, OPT_TRACEOPT, OPT_PRIORITY, OPT_PREEMPTOFF,
-	OPT_QUIET, OPT_PRIOSPREAD, OPT_RELATIVE, OPT_RESOLUTION, OPT_SYSTEM,
-	OPT_SMP, OPT_THREADS, OPT_TRACER, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE,
-	OPT_WAKEUP, OPT_WAKEUPRT, OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS,
-	OPT_ALIGNED, OPT_LAPTOP, OPT_SECALIGNED,
+	OPT_FTRACE, OPT_FIFO, OPT_HISTOGRAM, OPT_HISTOFALL, OPT_HISTFILE,
+	OPT_INTERVAL, OPT_IRQSOFF, OPT_LOOPS, OPT_MLOCKALL, OPT_REFRESH, OPT_NANOSLEEP,
+	OPT_NSECS, OPT_OSCOPE, OPT_TRACEOPT, OPT_PRIORITY, OPT_PREEMPTOFF, OPT_QUIET,
+	OPT_PRIOSPREAD, OPT_RELATIVE, OPT_RESOLUTION, OPT_SYSTEM, OPT_SMP, OPT_THREADS,
+	OPT_TRACER, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE, OPT_WAKEUP, OPT_WAKEUPRT,
+	OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS, OPT_ALIGNED, OPT_LAPTOP,
+	OPT_SECALIGNED,
 };
 
 /* Process commandline options */
@@ -1251,6 +1254,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
 			{"fifo",             required_argument, NULL, OPT_FIFO },
 			{"histogram",        required_argument, NULL, OPT_HISTOGRAM },
 			{"histofall",        required_argument, NULL, OPT_HISTOFALL },
+			{"histfile",	     required_argument, NULL, OPT_HISTFILE },
 			{"interval",         required_argument, NULL, OPT_INTERVAL },
 			{"irqsoff",          no_argument,       NULL, OPT_IRQSOFF },
 			{"laptop",	     no_argument,	NULL, OPT_LAPTOP },
@@ -1348,6 +1352,11 @@ static void process_options (int argc, char *argv[], int max_cpus)
 		case 'h':
 		case OPT_HISTOGRAM:
 			histogram = atoi(optarg); break;
+		case 'J':
+		case OPT_HISTFILE:
+			use_histfile = 1;
+			strncpy(histfile, optarg, strlen(optarg));
+			break;
 		case 'i':
 		case OPT_INTERVAL:
 			interval = atoi(optarg); break;
@@ -1653,74 +1662,88 @@ 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));
 
-	printf("# Histogram\n");
+	if (use_histfile) {
+		fd = fopen(histfile, "w");
+		if (!fd) {
+			perror("opening histogram file:");
+			return;
+		}
+	} else {
+		fd = stdout;
+	}
+
+	fprintf(fd, "# Histogram\n");
 	for (i = 0; i < histogram; i++) {
 		unsigned long long int allthreads = 0;
 
-		printf("%06d ", i);
+		fprintf(fd, "%06d ", i);
 
 		for (j = 0; j < nthreads; j++) {
 			unsigned long curr_latency=par[j]->stats->hist_array[i];
-			printf("%06lu", curr_latency);
+			fprintf(fd, "%06lu", curr_latency);
 			if (j < nthreads - 1)
-				printf("\t");
+				fprintf(fd, "\t");
 			log_entries[j] += curr_latency;
 			allthreads += curr_latency;
 		}
 		if (histofall && nthreads > 1) {
-			printf("\t%06llu", allthreads);
+			fprintf(fd, "\t%06llu", allthreads);
 			log_entries[nthreads] += allthreads;
 		}
-		printf("\n");
+		fprintf(fd, "\n");
 	}
-	printf("# Total:");
+	fprintf(fd, "# Total:");
 	for (j = 0; j < nthreads; j++)
-		printf(" %09llu", log_entries[j]);
+		fprintf(fd, " %09llu", log_entries[j]);
 	if (histofall && nthreads > 1)
-		printf(" %09llu", log_entries[nthreads]);
-	printf("\n");
-	printf("# Min Latencies:");
+		fprintf(fd, " %09llu", log_entries[nthreads]);
+	fprintf(fd, "\n");
+	fprintf(fd, "# Min Latencies:");
 	for (j = 0; j < nthreads; j++)
-		printf(" %05lu", par[j]->stats->min);
-	printf("\n");
-	printf("# Avg Latencies:");
+		fprintf(fd, " %05lu", par[j]->stats->min);
+	fprintf(fd, "\n");
+	fprintf(fd, "# Avg Latencies:");
 	for (j = 0; j < nthreads; j++)
-		printf(" %05lu", par[j]->stats->cycles ?
+		fprintf(fd, " %05lu", par[j]->stats->cycles ?
 		       (long)(par[j]->stats->avg/par[j]->stats->cycles) : 0);
-	printf("\n");
-	printf("# Max Latencies:");
+	fprintf(fd, "\n");
+	fprintf(fd, "# Max Latencies:");
 	maxmax = 0;
 	for (j = 0; j < nthreads; j++) {
-		printf(" %05lu", par[j]->stats->max);
+		fprintf(fd, " %05lu", par[j]->stats->max);
 		if (par[j]->stats->max > maxmax)
 			maxmax = par[j]->stats->max;
 	}
 	if (histofall && nthreads > 1)
-		printf(" %05lu", maxmax);
-	printf("\n");
-	printf("# Histogram Overflows:");
+		fprintf(fd, " %05lu", maxmax);
+	fprintf(fd, "\n");
+	fprintf(fd, "# Histogram Overflows:");
 	alloverflows = 0;
 	for (j = 0; j < nthreads; j++) {
-		printf(" %05lu", par[j]->stats->hist_overflow);
+		fprintf(fd, " %05lu", par[j]->stats->hist_overflow);
 		alloverflows += par[j]->stats->hist_overflow;
 	}
 	if (histofall && nthreads > 1)
-		printf(" %05lu", alloverflows);
-	printf("\n");
+		fprintf(fd, " %05lu", alloverflows);
+	fprintf(fd, "\n");
 
-	printf("# Histogram Overflow at cycle number:\n");
+	fprintf(fd, "# Histogram Overflow at cycle number:\n");
 	for (i = 0; i < nthreads; i++) {
-		printf("# Thread %d:", i);
+		fprintf(fd, "# Thread %d:", i);
 		for (j = 0; j < par[i]->stats->num_outliers; j++)
-			printf(" %05lu", par[i]->stats->outliers[j]);
+			fprintf(fd, " %05lu", par[i]->stats->outliers[j]);
 		if (par[i]->stats->num_outliers < par[i]->stats->hist_overflow)
-			printf(" # %05lu others", par[i]->stats->hist_overflow - par[i]->stats->num_outliers);
-		printf("\n");
+			fprintf(fd, " # %05lu others", par[i]->stats->hist_overflow - par[i]->stats->num_outliers);
+		fprintf(fd, "\n");
 	}
-	printf("\n");
+	fprintf(fd, "\n");
+
+	if (use_histfile)
+		fclose(fd);
 }
 
 static void print_stat(FILE *fp, struct thread_param *par, int index, int verbose, int quiet)
-- 
2.5.0

Re: [PATCH rt-tests 9/9] cyclictest: add option for dumping the histogram in a file

[John Kacur]

On Mon, 31 Aug 2015, Josh Cartwright wrote:

> From: Gratian Crisan <gratian.crisan@ni.com>
> 
> Add an option '-J' or '--histfile' to dump the latency histogram to <path>
> instead of stdout. This allows for live update of the current min, avg, and max
> numbers while retaining the option to save histogram data for later analysis.
> 
> Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/cyclictest/cyclictest.c | 97 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 60 insertions(+), 37 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 2c44f8e..ec1ed17 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -188,6 +188,7 @@ static int aligned = 0;
>  static int secaligned = 0;
>  static int offset = 0;
>  static int laptop = 0;
> +static int use_histfile = 0;
>  
>  static pthread_cond_t refresh_on_max_cond = PTHREAD_COND_INITIALIZER;
>  static pthread_mutex_t refresh_on_max_lock = PTHREAD_MUTEX_INITIALIZER;
> @@ -210,6 +211,7 @@ static char *procfileprefix = "/proc/sys/kernel/";
>  static char *fileprefix;
>  static char tracer[MAX_PATH];
>  static char fifopath[MAX_PATH];
> +static char histfile[MAX_PATH];
>  static char **traceptr;
>  static int traceopt_count;
>  static int traceopt_size;
> @@ -1049,6 +1051,7 @@ static void display_help(int error)
>  	       "                           (with same priority about many threads)\n"
>  	       "                           US is the max time to be be tracked in microseconds\n"
>  	       "-H       --histofall=US    same as -h except with an additional summary column\n"
> +	       "-J       --histfile=<path> dump the latency histogram to <path> instead of stdout\n"
>  	       "-i INTV  --interval=INTV   base interval of thread in us default=1000\n"
>  	       "-I       --irqsoff         Irqsoff tracing (used with -b)\n"
>  	       "-l LOOPS --loops=LOOPS     number of loops: default=0(endless)\n"
> @@ -1214,13 +1217,13 @@ static char *policyname(int policy)
>  enum option_values {
>  	OPT_AFFINITY=1, OPT_NOTRACE, OPT_BREAKTRACE, OPT_PREEMPTIRQ, OPT_CLOCK,
>  	OPT_CONTEXT, OPT_DISTANCE, OPT_DURATION, OPT_LATENCY, OPT_EVENT,
> -	OPT_FTRACE, OPT_FIFO, OPT_HISTOGRAM, OPT_HISTOFALL, OPT_INTERVAL,
> -	OPT_IRQSOFF, OPT_LOOPS, OPT_MLOCKALL, OPT_REFRESH, OPT_NANOSLEEP,
> -	OPT_NSECS, OPT_OSCOPE, OPT_TRACEOPT, OPT_PRIORITY, OPT_PREEMPTOFF,
> -	OPT_QUIET, OPT_PRIOSPREAD, OPT_RELATIVE, OPT_RESOLUTION, OPT_SYSTEM,
> -	OPT_SMP, OPT_THREADS, OPT_TRACER, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE,
> -	OPT_WAKEUP, OPT_WAKEUPRT, OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS,
> -	OPT_ALIGNED, OPT_LAPTOP, OPT_SECALIGNED,
> +	OPT_FTRACE, OPT_FIFO, OPT_HISTOGRAM, OPT_HISTOFALL, OPT_HISTFILE,
> +	OPT_INTERVAL, OPT_IRQSOFF, OPT_LOOPS, OPT_MLOCKALL, OPT_REFRESH, OPT_NANOSLEEP,
> +	OPT_NSECS, OPT_OSCOPE, OPT_TRACEOPT, OPT_PRIORITY, OPT_PREEMPTOFF, OPT_QUIET,
> +	OPT_PRIOSPREAD, OPT_RELATIVE, OPT_RESOLUTION, OPT_SYSTEM, OPT_SMP, OPT_THREADS,
> +	OPT_TRACER, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE, OPT_WAKEUP, OPT_WAKEUPRT,
> +	OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS, OPT_ALIGNED, OPT_LAPTOP,
> +	OPT_SECALIGNED,
>  };
>  
>  /* Process commandline options */
> @@ -1251,6 +1254,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
>  			{"fifo",             required_argument, NULL, OPT_FIFO },
>  			{"histogram",        required_argument, NULL, OPT_HISTOGRAM },
>  			{"histofall",        required_argument, NULL, OPT_HISTOFALL },
> +			{"histfile",	     required_argument, NULL, OPT_HISTFILE },
>  			{"interval",         required_argument, NULL, OPT_INTERVAL },
>  			{"irqsoff",          no_argument,       NULL, OPT_IRQSOFF },
>  			{"laptop",	     no_argument,	NULL, OPT_LAPTOP },
> @@ -1348,6 +1352,11 @@ static void process_options (int argc, char *argv[], int max_cpus)
>  		case 'h':
>  		case OPT_HISTOGRAM:
>  			histogram = atoi(optarg); break;
> +		case 'J':
> +		case OPT_HISTFILE:
> +			use_histfile = 1;
> +			strncpy(histfile, optarg, strlen(optarg));
> +			break;
>  		case 'i':
>  		case OPT_INTERVAL:
>  			interval = atoi(optarg); break;
> @@ -1653,74 +1662,88 @@ 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));
>  
> -	printf("# Histogram\n");
> +	if (use_histfile) {
> +		fd = fopen(histfile, "w");
> +		if (!fd) {
> +			perror("opening histogram file:");
> +			return;
> +		}
> +	} else {
> +		fd = stdout;
> +	}
> +
> +	fprintf(fd, "# Histogram\n");
>  	for (i = 0; i < histogram; i++) {
>  		unsigned long long int allthreads = 0;
>  
> -		printf("%06d ", i);
> +		fprintf(fd, "%06d ", i);
>  
>  		for (j = 0; j < nthreads; j++) {
>  			unsigned long curr_latency=par[j]->stats->hist_array[i];
> -			printf("%06lu", curr_latency);
> +			fprintf(fd, "%06lu", curr_latency);
>  			if (j < nthreads - 1)
> -				printf("\t");
> +				fprintf(fd, "\t");
>  			log_entries[j] += curr_latency;
>  			allthreads += curr_latency;
>  		}
>  		if (histofall && nthreads > 1) {
> -			printf("\t%06llu", allthreads);
> +			fprintf(fd, "\t%06llu", allthreads);
>  			log_entries[nthreads] += allthreads;
>  		}
> -		printf("\n");
> +		fprintf(fd, "\n");
>  	}
> -	printf("# Total:");
> +	fprintf(fd, "# Total:");
>  	for (j = 0; j < nthreads; j++)
> -		printf(" %09llu", log_entries[j]);
> +		fprintf(fd, " %09llu", log_entries[j]);
>  	if (histofall && nthreads > 1)
> -		printf(" %09llu", log_entries[nthreads]);
> -	printf("\n");
> -	printf("# Min Latencies:");
> +		fprintf(fd, " %09llu", log_entries[nthreads]);
> +	fprintf(fd, "\n");
> +	fprintf(fd, "# Min Latencies:");
>  	for (j = 0; j < nthreads; j++)
> -		printf(" %05lu", par[j]->stats->min);
> -	printf("\n");
> -	printf("# Avg Latencies:");
> +		fprintf(fd, " %05lu", par[j]->stats->min);
> +	fprintf(fd, "\n");
> +	fprintf(fd, "# Avg Latencies:");
>  	for (j = 0; j < nthreads; j++)
> -		printf(" %05lu", par[j]->stats->cycles ?
> +		fprintf(fd, " %05lu", par[j]->stats->cycles ?
>  		       (long)(par[j]->stats->avg/par[j]->stats->cycles) : 0);
> -	printf("\n");
> -	printf("# Max Latencies:");
> +	fprintf(fd, "\n");
> +	fprintf(fd, "# Max Latencies:");
>  	maxmax = 0;
>  	for (j = 0; j < nthreads; j++) {
> -		printf(" %05lu", par[j]->stats->max);
> +		fprintf(fd, " %05lu", par[j]->stats->max);
>  		if (par[j]->stats->max > maxmax)
>  			maxmax = par[j]->stats->max;
>  	}
>  	if (histofall && nthreads > 1)
> -		printf(" %05lu", maxmax);
> -	printf("\n");
> -	printf("# Histogram Overflows:");
> +		fprintf(fd, " %05lu", maxmax);
> +	fprintf(fd, "\n");
> +	fprintf(fd, "# Histogram Overflows:");
>  	alloverflows = 0;
>  	for (j = 0; j < nthreads; j++) {
> -		printf(" %05lu", par[j]->stats->hist_overflow);
> +		fprintf(fd, " %05lu", par[j]->stats->hist_overflow);
>  		alloverflows += par[j]->stats->hist_overflow;
>  	}
>  	if (histofall && nthreads > 1)
> -		printf(" %05lu", alloverflows);
> -	printf("\n");
> +		fprintf(fd, " %05lu", alloverflows);
> +	fprintf(fd, "\n");
>  
> -	printf("# Histogram Overflow at cycle number:\n");
> +	fprintf(fd, "# Histogram Overflow at cycle number:\n");
>  	for (i = 0; i < nthreads; i++) {
> -		printf("# Thread %d:", i);
> +		fprintf(fd, "# Thread %d:", i);
>  		for (j = 0; j < par[i]->stats->num_outliers; j++)
> -			printf(" %05lu", par[i]->stats->outliers[j]);
> +			fprintf(fd, " %05lu", par[i]->stats->outliers[j]);
>  		if (par[i]->stats->num_outliers < par[i]->stats->hist_overflow)
> -			printf(" # %05lu others", par[i]->stats->hist_overflow - par[i]->stats->num_outliers);
> -		printf("\n");
> +			fprintf(fd, " # %05lu others", par[i]->stats->hist_overflow - par[i]->stats->num_outliers);
> +		fprintf(fd, "\n");
>  	}
> -	printf("\n");
> +	fprintf(fd, "\n");
> +
> +	if (use_histfile)
> +		fclose(fd);
>  }
>  
>  static void print_stat(FILE *fp, struct thread_param *par, int index, int verbose, int quiet)
> -- 
> 2.5.0
> 
> --

We worked really hard to remove the large amount of options, and we may 
have been over zealous in some cases (Carsten?). If I were to accept this 
patch, I would at least like you to remove the short form option, and just 
have it in the long form.

Thanks

John Kacur

Re: [PATCH rt-tests 9/9] cyclictest: add option for dumping the histogram in a file

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 8907 bytes --]

On Wed, Sep 16, 2015 at 01:05:51AM +0200, John Kacur wrote:
> On Mon, 31 Aug 2015, Josh Cartwright wrote:
> > From: Gratian Crisan <gratian.crisan@ni.com>
> > 
> > Add an option '-J' or '--histfile' to dump the latency histogram to <path>
> > instead of stdout. This allows for live update of the current min, avg, and max
> > numbers while retaining the option to save histogram data for later analysis.
> > 
> > Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
> > Signed-off-by: Josh Cartwright <joshc@ni.com>
[..]
> 
> We worked really hard to remove the large amount of options, and we may 
> have been over zealous in some cases (Carsten?).

Fair enough, cyclictest has way too many knobs.  Regardless, we've at
least found this particular option useful.

> If I were to accept this patch, I would at least like you to remove
> the short form option, and just have it in the long form.

Here is a v2 with the short form -J dropped.

Thanks,
  Josh

-- 8< --
From: Gratian Crisan <gratian.crisan@ni.com>
Subject: [PATCH v2] cyclictest: add option for dumping the histogram in a file

Add an option '--histfile' to dump the latency histogram to <path>
instead of stdout. This allows for live update of the current min, avg,
and max numbers while retaining the option to save histogram data for
later analysis.

Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
Signed-off-by: Josh Cartwright <joshc@ni.com>
---
 src/cyclictest/cyclictest.c | 96 ++++++++++++++++++++++++++++-----------------
 1 file changed, 59 insertions(+), 37 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 62f7cd9..5c9600c 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -188,6 +188,7 @@ static int aligned = 0;
 static int secaligned = 0;
 static int offset = 0;
 static int laptop = 0;
+static int use_histfile = 0;
 
 static pthread_cond_t refresh_on_max_cond = PTHREAD_COND_INITIALIZER;
 static pthread_mutex_t refresh_on_max_lock = PTHREAD_MUTEX_INITIALIZER;
@@ -210,6 +211,7 @@ static char *procfileprefix = "/proc/sys/kernel/";
 static char *fileprefix;
 static char tracer[MAX_PATH];
 static char fifopath[MAX_PATH];
+static char histfile[MAX_PATH];
 static char **traceptr;
 static int traceopt_count;
 static int traceopt_size;
@@ -1049,6 +1051,7 @@ static void display_help(int error)
 	       "                           (with same priority about many threads)\n"
 	       "                           US is the max time to be be tracked in microseconds\n"
 	       "-H       --histofall=US    same as -h except with an additional summary column\n"
+	       "	 --histfile=<path> dump the latency histogram to <path> instead of stdout\n"
 	       "-i INTV  --interval=INTV   base interval of thread in us default=1000\n"
 	       "-I       --irqsoff         Irqsoff tracing (used with -b)\n"
 	       "-l LOOPS --loops=LOOPS     number of loops: default=0(endless)\n"
@@ -1214,13 +1217,13 @@ static char *policyname(int policy)
 enum option_values {
 	OPT_AFFINITY=1, OPT_NOTRACE, OPT_BREAKTRACE, OPT_PREEMPTIRQ, OPT_CLOCK,
 	OPT_CONTEXT, OPT_DISTANCE, OPT_DURATION, OPT_LATENCY, OPT_EVENT,
-	OPT_FTRACE, OPT_FIFO, OPT_HISTOGRAM, OPT_HISTOFALL, OPT_INTERVAL,
-	OPT_IRQSOFF, OPT_LOOPS, OPT_MLOCKALL, OPT_REFRESH, OPT_NANOSLEEP,
-	OPT_NSECS, OPT_OSCOPE, OPT_TRACEOPT, OPT_PRIORITY, OPT_PREEMPTOFF,
-	OPT_QUIET, OPT_PRIOSPREAD, OPT_RELATIVE, OPT_RESOLUTION, OPT_SYSTEM,
-	OPT_SMP, OPT_THREADS, OPT_TRACER, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE,
-	OPT_WAKEUP, OPT_WAKEUPRT, OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS,
-	OPT_ALIGNED, OPT_LAPTOP, OPT_SECALIGNED,
+	OPT_FTRACE, OPT_FIFO, OPT_HISTOGRAM, OPT_HISTOFALL, OPT_HISTFILE,
+	OPT_INTERVAL, OPT_IRQSOFF, OPT_LOOPS, OPT_MLOCKALL, OPT_REFRESH, OPT_NANOSLEEP,
+	OPT_NSECS, OPT_OSCOPE, OPT_TRACEOPT, OPT_PRIORITY, OPT_PREEMPTOFF, OPT_QUIET,
+	OPT_PRIOSPREAD, OPT_RELATIVE, OPT_RESOLUTION, OPT_SYSTEM, OPT_SMP, OPT_THREADS,
+	OPT_TRACER, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE, OPT_WAKEUP, OPT_WAKEUPRT,
+	OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS, OPT_ALIGNED, OPT_LAPTOP,
+	OPT_SECALIGNED,
 };
 
 /* Process commandline options */
@@ -1251,6 +1254,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
 			{"fifo",             required_argument, NULL, OPT_FIFO },
 			{"histogram",        required_argument, NULL, OPT_HISTOGRAM },
 			{"histofall",        required_argument, NULL, OPT_HISTOFALL },
+			{"histfile",	     required_argument, NULL, OPT_HISTFILE },
 			{"interval",         required_argument, NULL, OPT_INTERVAL },
 			{"irqsoff",          no_argument,       NULL, OPT_IRQSOFF },
 			{"laptop",	     no_argument,	NULL, OPT_LAPTOP },
@@ -1348,6 +1352,10 @@ static void process_options (int argc, char *argv[], int max_cpus)
 		case 'h':
 		case OPT_HISTOGRAM:
 			histogram = atoi(optarg); break;
+		case OPT_HISTFILE:
+			use_histfile = 1;
+			strncpy(histfile, optarg, strlen(optarg));
+			break;
 		case 'i':
 		case OPT_INTERVAL:
 			interval = atoi(optarg); break;
@@ -1653,74 +1661,88 @@ 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));
 
-	printf("# Histogram\n");
+	if (use_histfile) {
+		fd = fopen(histfile, "w");
+		if (!fd) {
+			perror("opening histogram file:");
+			return;
+		}
+	} else {
+		fd = stdout;
+	}
+
+	fprintf(fd, "# Histogram\n");
 	for (i = 0; i < histogram; i++) {
 		unsigned long long int allthreads = 0;
 
-		printf("%06d ", i);
+		fprintf(fd, "%06d ", i);
 
 		for (j = 0; j < nthreads; j++) {
 			unsigned long curr_latency=par[j]->stats->hist_array[i];
-			printf("%06lu", curr_latency);
+			fprintf(fd, "%06lu", curr_latency);
 			if (j < nthreads - 1)
-				printf("\t");
+				fprintf(fd, "\t");
 			log_entries[j] += curr_latency;
 			allthreads += curr_latency;
 		}
 		if (histofall && nthreads > 1) {
-			printf("\t%06llu", allthreads);
+			fprintf(fd, "\t%06llu", allthreads);
 			log_entries[nthreads] += allthreads;
 		}
-		printf("\n");
+		fprintf(fd, "\n");
 	}
-	printf("# Total:");
+	fprintf(fd, "# Total:");
 	for (j = 0; j < nthreads; j++)
-		printf(" %09llu", log_entries[j]);
+		fprintf(fd, " %09llu", log_entries[j]);
 	if (histofall && nthreads > 1)
-		printf(" %09llu", log_entries[nthreads]);
-	printf("\n");
-	printf("# Min Latencies:");
+		fprintf(fd, " %09llu", log_entries[nthreads]);
+	fprintf(fd, "\n");
+	fprintf(fd, "# Min Latencies:");
 	for (j = 0; j < nthreads; j++)
-		printf(" %05lu", par[j]->stats->min);
-	printf("\n");
-	printf("# Avg Latencies:");
+		fprintf(fd, " %05lu", par[j]->stats->min);
+	fprintf(fd, "\n");
+	fprintf(fd, "# Avg Latencies:");
 	for (j = 0; j < nthreads; j++)
-		printf(" %05lu", par[j]->stats->cycles ?
+		fprintf(fd, " %05lu", par[j]->stats->cycles ?
 		       (long)(par[j]->stats->avg/par[j]->stats->cycles) : 0);
-	printf("\n");
-	printf("# Max Latencies:");
+	fprintf(fd, "\n");
+	fprintf(fd, "# Max Latencies:");
 	maxmax = 0;
 	for (j = 0; j < nthreads; j++) {
-		printf(" %05lu", par[j]->stats->max);
+		fprintf(fd, " %05lu", par[j]->stats->max);
 		if (par[j]->stats->max > maxmax)
 			maxmax = par[j]->stats->max;
 	}
 	if (histofall && nthreads > 1)
-		printf(" %05lu", maxmax);
-	printf("\n");
-	printf("# Histogram Overflows:");
+		fprintf(fd, " %05lu", maxmax);
+	fprintf(fd, "\n");
+	fprintf(fd, "# Histogram Overflows:");
 	alloverflows = 0;
 	for (j = 0; j < nthreads; j++) {
-		printf(" %05lu", par[j]->stats->hist_overflow);
+		fprintf(fd, " %05lu", par[j]->stats->hist_overflow);
 		alloverflows += par[j]->stats->hist_overflow;
 	}
 	if (histofall && nthreads > 1)
-		printf(" %05lu", alloverflows);
-	printf("\n");
+		fprintf(fd, " %05lu", alloverflows);
+	fprintf(fd, "\n");
 
-	printf("# Histogram Overflow at cycle number:\n");
+	fprintf(fd, "# Histogram Overflow at cycle number:\n");
 	for (i = 0; i < nthreads; i++) {
-		printf("# Thread %d:", i);
+		fprintf(fd, "# Thread %d:", i);
 		for (j = 0; j < par[i]->stats->num_outliers; j++)
-			printf(" %05lu", par[i]->stats->outliers[j]);
+			fprintf(fd, " %05lu", par[i]->stats->outliers[j]);
 		if (par[i]->stats->num_outliers < par[i]->stats->hist_overflow)
-			printf(" # %05lu others", par[i]->stats->hist_overflow - par[i]->stats->num_outliers);
-		printf("\n");
+			fprintf(fd, " # %05lu others", par[i]->stats->hist_overflow - par[i]->stats->num_outliers);
+		fprintf(fd, "\n");
 	}
-	printf("\n");
+	fprintf(fd, "\n");
+
+	if (use_histfile)
+		fclose(fd);
 }
 
 static void print_stat(FILE *fp, struct thread_param *par, int index, int verbose, int quiet)
-- 
2.5.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH rt-tests 9/9] cyclictest: add option for dumping the histogram in a file

[John Kacur]

On Wed, 16 Sep 2015, Josh Cartwright wrote:

> On Wed, Sep 16, 2015 at 01:05:51AM +0200, John Kacur wrote:
> > On Mon, 31 Aug 2015, Josh Cartwright wrote:
> > > From: Gratian Crisan <gratian.crisan@ni.com>
> > > 
> > > Add an option '-J' or '--histfile' to dump the latency histogram to <path>
> > > instead of stdout. This allows for live update of the current min, avg, and max
> > > numbers while retaining the option to save histogram data for later analysis.
> > > 
> > > Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
> > > Signed-off-by: Josh Cartwright <joshc@ni.com>
> [..]
> > 
> > We worked really hard to remove the large amount of options, and we may 
> > have been over zealous in some cases (Carsten?).
> 
> Fair enough, cyclictest has way too many knobs.  Regardless, we've at
> least found this particular option useful.
> 
> > If I were to accept this patch, I would at least like you to remove
> > the short form option, and just have it in the long form.
> 
> Here is a v2 with the short form -J dropped.
> 
> Thanks,
>   Josh
> 
> -- 8< --
> From: Gratian Crisan <gratian.crisan@ni.com>
> Subject: [PATCH v2] cyclictest: add option for dumping the histogram in a file
> 
> Add an option '--histfile' to dump the latency histogram to <path>
> instead of stdout. This allows for live update of the current min, avg,
> and max numbers while retaining the option to save histogram data for
> later analysis.
> 
> Signed-off-by: Gratian Crisan <gratian.crisan@ni.com>
> Signed-off-by: Josh Cartwright <joshc@ni.com>
> ---
>  src/cyclictest/cyclictest.c | 96 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 59 insertions(+), 37 deletions(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 62f7cd9..5c9600c 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -188,6 +188,7 @@ static int aligned = 0;
>  static int secaligned = 0;
>  static int offset = 0;
>  static int laptop = 0;
> +static int use_histfile = 0;
>  
>  static pthread_cond_t refresh_on_max_cond = PTHREAD_COND_INITIALIZER;
>  static pthread_mutex_t refresh_on_max_lock = PTHREAD_MUTEX_INITIALIZER;
> @@ -210,6 +211,7 @@ static char *procfileprefix = "/proc/sys/kernel/";
>  static char *fileprefix;
>  static char tracer[MAX_PATH];
>  static char fifopath[MAX_PATH];
> +static char histfile[MAX_PATH];
>  static char **traceptr;
>  static int traceopt_count;
>  static int traceopt_size;
> @@ -1049,6 +1051,7 @@ static void display_help(int error)
>  	       "                           (with same priority about many threads)\n"
>  	       "                           US is the max time to be be tracked in microseconds\n"
>  	       "-H       --histofall=US    same as -h except with an additional summary column\n"
> +	       "	 --histfile=<path> dump the latency histogram to <path> instead of stdout\n"
>  	       "-i INTV  --interval=INTV   base interval of thread in us default=1000\n"
>  	       "-I       --irqsoff         Irqsoff tracing (used with -b)\n"
>  	       "-l LOOPS --loops=LOOPS     number of loops: default=0(endless)\n"
> @@ -1214,13 +1217,13 @@ static char *policyname(int policy)
>  enum option_values {
>  	OPT_AFFINITY=1, OPT_NOTRACE, OPT_BREAKTRACE, OPT_PREEMPTIRQ, OPT_CLOCK,
>  	OPT_CONTEXT, OPT_DISTANCE, OPT_DURATION, OPT_LATENCY, OPT_EVENT,
> -	OPT_FTRACE, OPT_FIFO, OPT_HISTOGRAM, OPT_HISTOFALL, OPT_INTERVAL,
> -	OPT_IRQSOFF, OPT_LOOPS, OPT_MLOCKALL, OPT_REFRESH, OPT_NANOSLEEP,
> -	OPT_NSECS, OPT_OSCOPE, OPT_TRACEOPT, OPT_PRIORITY, OPT_PREEMPTOFF,
> -	OPT_QUIET, OPT_PRIOSPREAD, OPT_RELATIVE, OPT_RESOLUTION, OPT_SYSTEM,
> -	OPT_SMP, OPT_THREADS, OPT_TRACER, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE,
> -	OPT_WAKEUP, OPT_WAKEUPRT, OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS,
> -	OPT_ALIGNED, OPT_LAPTOP, OPT_SECALIGNED,
> +	OPT_FTRACE, OPT_FIFO, OPT_HISTOGRAM, OPT_HISTOFALL, OPT_HISTFILE,
> +	OPT_INTERVAL, OPT_IRQSOFF, OPT_LOOPS, OPT_MLOCKALL, OPT_REFRESH, OPT_NANOSLEEP,
> +	OPT_NSECS, OPT_OSCOPE, OPT_TRACEOPT, OPT_PRIORITY, OPT_PREEMPTOFF, OPT_QUIET,
> +	OPT_PRIOSPREAD, OPT_RELATIVE, OPT_RESOLUTION, OPT_SYSTEM, OPT_SMP, OPT_THREADS,
> +	OPT_TRACER, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE, OPT_WAKEUP, OPT_WAKEUPRT,
> +	OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS, OPT_ALIGNED, OPT_LAPTOP,
> +	OPT_SECALIGNED,
>  };
>  
>  /* Process commandline options */
> @@ -1251,6 +1254,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
>  			{"fifo",             required_argument, NULL, OPT_FIFO },
>  			{"histogram",        required_argument, NULL, OPT_HISTOGRAM },
>  			{"histofall",        required_argument, NULL, OPT_HISTOFALL },
> +			{"histfile",	     required_argument, NULL, OPT_HISTFILE },
>  			{"interval",         required_argument, NULL, OPT_INTERVAL },
>  			{"irqsoff",          no_argument,       NULL, OPT_IRQSOFF },
>  			{"laptop",	     no_argument,	NULL, OPT_LAPTOP },
> @@ -1348,6 +1352,10 @@ static void process_options (int argc, char *argv[], int max_cpus)
>  		case 'h':
>  		case OPT_HISTOGRAM:
>  			histogram = atoi(optarg); break;
> +		case OPT_HISTFILE:
> +			use_histfile = 1;
> +			strncpy(histfile, optarg, strlen(optarg));
> +			break;
>  		case 'i':
>  		case OPT_INTERVAL:
>  			interval = atoi(optarg); break;
> @@ -1653,74 +1661,88 @@ 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));
>  
> -	printf("# Histogram\n");
> +	if (use_histfile) {
> +		fd = fopen(histfile, "w");
> +		if (!fd) {
> +			perror("opening histogram file:");
> +			return;
> +		}
> +	} else {
> +		fd = stdout;
> +	}
> +
> +	fprintf(fd, "# Histogram\n");
>  	for (i = 0; i < histogram; i++) {
>  		unsigned long long int allthreads = 0;
>  
> -		printf("%06d ", i);
> +		fprintf(fd, "%06d ", i);
>  
>  		for (j = 0; j < nthreads; j++) {
>  			unsigned long curr_latency=par[j]->stats->hist_array[i];
> -			printf("%06lu", curr_latency);
> +			fprintf(fd, "%06lu", curr_latency);
>  			if (j < nthreads - 1)
> -				printf("\t");
> +				fprintf(fd, "\t");
>  			log_entries[j] += curr_latency;
>  			allthreads += curr_latency;
>  		}
>  		if (histofall && nthreads > 1) {
> -			printf("\t%06llu", allthreads);
> +			fprintf(fd, "\t%06llu", allthreads);
>  			log_entries[nthreads] += allthreads;
>  		}
> -		printf("\n");
> +		fprintf(fd, "\n");
>  	}
> -	printf("# Total:");
> +	fprintf(fd, "# Total:");
>  	for (j = 0; j < nthreads; j++)
> -		printf(" %09llu", log_entries[j]);
> +		fprintf(fd, " %09llu", log_entries[j]);
>  	if (histofall && nthreads > 1)
> -		printf(" %09llu", log_entries[nthreads]);
> -	printf("\n");
> -	printf("# Min Latencies:");
> +		fprintf(fd, " %09llu", log_entries[nthreads]);
> +	fprintf(fd, "\n");
> +	fprintf(fd, "# Min Latencies:");
>  	for (j = 0; j < nthreads; j++)
> -		printf(" %05lu", par[j]->stats->min);
> -	printf("\n");
> -	printf("# Avg Latencies:");
> +		fprintf(fd, " %05lu", par[j]->stats->min);
> +	fprintf(fd, "\n");
> +	fprintf(fd, "# Avg Latencies:");
>  	for (j = 0; j < nthreads; j++)
> -		printf(" %05lu", par[j]->stats->cycles ?
> +		fprintf(fd, " %05lu", par[j]->stats->cycles ?
>  		       (long)(par[j]->stats->avg/par[j]->stats->cycles) : 0);
> -	printf("\n");
> -	printf("# Max Latencies:");
> +	fprintf(fd, "\n");
> +	fprintf(fd, "# Max Latencies:");
>  	maxmax = 0;
>  	for (j = 0; j < nthreads; j++) {
> -		printf(" %05lu", par[j]->stats->max);
> +		fprintf(fd, " %05lu", par[j]->stats->max);
>  		if (par[j]->stats->max > maxmax)
>  			maxmax = par[j]->stats->max;
>  	}
>  	if (histofall && nthreads > 1)
> -		printf(" %05lu", maxmax);
> -	printf("\n");
> -	printf("# Histogram Overflows:");
> +		fprintf(fd, " %05lu", maxmax);
> +	fprintf(fd, "\n");
> +	fprintf(fd, "# Histogram Overflows:");
>  	alloverflows = 0;
>  	for (j = 0; j < nthreads; j++) {
> -		printf(" %05lu", par[j]->stats->hist_overflow);
> +		fprintf(fd, " %05lu", par[j]->stats->hist_overflow);
>  		alloverflows += par[j]->stats->hist_overflow;
>  	}
>  	if (histofall && nthreads > 1)
> -		printf(" %05lu", alloverflows);
> -	printf("\n");
> +		fprintf(fd, " %05lu", alloverflows);
> +	fprintf(fd, "\n");
>  
> -	printf("# Histogram Overflow at cycle number:\n");
> +	fprintf(fd, "# Histogram Overflow at cycle number:\n");
>  	for (i = 0; i < nthreads; i++) {
> -		printf("# Thread %d:", i);
> +		fprintf(fd, "# Thread %d:", i);
>  		for (j = 0; j < par[i]->stats->num_outliers; j++)
> -			printf(" %05lu", par[i]->stats->outliers[j]);
> +			fprintf(fd, " %05lu", par[i]->stats->outliers[j]);
>  		if (par[i]->stats->num_outliers < par[i]->stats->hist_overflow)
> -			printf(" # %05lu others", par[i]->stats->hist_overflow - par[i]->stats->num_outliers);
> -		printf("\n");
> +			fprintf(fd, " # %05lu others", par[i]->stats->hist_overflow - par[i]->stats->num_outliers);
> +		fprintf(fd, "\n");
>  	}
> -	printf("\n");
> +	fprintf(fd, "\n");
> +
> +	if (use_histfile)
> +		fclose(fd);
>  }
>  
>  static void print_stat(FILE *fp, struct thread_param *par, int index, int verbose, int quiet)
> -- 
> 2.5.1
> 
> 

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