[PATCH 0/4] cyclictest: improve running under trace-cmd

[PATCH 0/4] cyclictest: improve running under trace-cmd

[Luiz Capitulino]

This is just a repost of my RFC posting with no changes.
I have tested it a bit more and I think it's good
for inclusion.

Original intro:

In short, this series allows you to run cyclictest under
trace-cmd and still get trace marks when the latency
specified with -b is execeded. More details in patch 4/4.

This series is RFC because I'm not completely sure this
is the right thing to do. I'm wondering if we shouldn't
ditch all tracing support from cyclictest...

Luiz Capitulino (4):
  cyclictest: tracing(): check for notrace
  cyclictest: move debugfs init code to its own function
  cyclictest: move tracemark_fd handling to its own function
  cyclictest: add --tracemark option

 src/cyclictest/cyclictest.c | 62 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 15 deletions(-)

-- 
2.1.0

[PATCH 1/4] cyclictest: tracing(): check for notrace

[Luiz Capitulino]

If you pass -b and --notrace to cyclictest today, it will
write to tracing_on when -b latency is reached.

Fix this by making tracing() check notrace.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 src/cyclictest/cyclictest.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 34053c5..54fc033 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -444,6 +444,9 @@ static void tracemark(char *fmt, ...)
 
 void tracing(int on)
 {
+	if (notrace)
+		return;
+
 	if (on) {
 		switch (kernelversion) {
 		case KV_26_LT18: gettimeofday(0,(struct timezone *)1); break;
@@ -1634,7 +1637,7 @@ static void sighand(int sig)
 	shutdown = 1;
 	if (refresh_on_max)
 		pthread_cond_signal(&refresh_on_max_cond);
-	if (tracelimit && !notrace)
+	if (tracelimit)
 		tracing(0);
 }
 
@@ -2164,7 +2167,7 @@ int main(int argc, char **argv)
 	}
  out:
 	/* ensure that the tracer is stopped */
-	if (tracelimit && !notrace)
+	if (tracelimit)
 		tracing(0);
 
 
-- 
2.1.0

[PATCH 2/4] cyclictest: move debugfs init code to its own function

[Luiz Capitulino]

A function added by a future commit will want to call
this code too.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 src/cyclictest/cyclictest.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 54fc033..f543c34 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -479,11 +479,8 @@ static int settracer(char *tracer)
 	return -1;
 }
 
-static void setup_tracer(void)
+static void debugfs_prepare(void)
 {
-	if (!tracelimit || notrace)
-		return;

[PATCH 3/4] cyclictest: move tracemark_fd handling to its own function

[Luiz Capitulino]

A function added by the next commit will want to call
this code too.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 src/cyclictest/cyclictest.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index f543c34..e35088b 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -497,6 +497,19 @@ static void debugfs_prepare(void)
 		fileprefix = procfileprefix;
 }
 
+static void open_tracemark_fd(void)
+{
+	char path[MAX_PATH];
+
+	if (tracemark_fd >= 0)
+		return;
+
+	sprintf(path, "%s/%s", fileprefix, "trace_marker");
+	tracemark_fd = open(path, O_WRONLY);
+	if (tracemark_fd < 0)
+		warn("unable to open trace_marker file: %s\n", path);
+}
+
 static void setup_tracer(void)
 {
 	if (!tracelimit || notrace)
@@ -612,14 +625,7 @@ static void setup_tracer(void)
 				fatal("unable to open %s for tracing", path);
 		}
 
-		/* open the tracemark file descriptor */
-		if (tracemark_fd == -1) {
-			char path[MAX_PATH];
-			strcat(strcpy(path, fileprefix), "trace_marker");
-			if ((tracemark_fd = open(path, O_WRONLY)) == -1)
-				warn("unable to open trace_marker file: %s\n", path);
-		}

[PATCH 4/4] cyclictest: add --tracemark option

[Luiz Capitulino]

cyclictest will only write to /sys/kernel/debug/tracing/trace_maker
if it's also setup to do tracing. This conflicts with
running cyclictest under trace-cmd.

The --tracemark option tells cyclictest to write to the
trace_marker file even when it's not doing tracing.

It's can be used like this:

 # trace-cmd record [...] cyclictest [...] -bX --tracemark --notrace

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 src/cyclictest/cyclictest.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index e35088b..5a261e2 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -167,6 +167,7 @@ struct thread_stat {
 static int shutdown;
 static int tracelimit = 0;
 static int notrace = 0;
+static int trace_marker = 0;
 static int ftrace = 0;
 static int kernelversion;
 static int verbose = 0;
@@ -510,6 +511,18 @@ static void open_tracemark_fd(void)
 		warn("unable to open trace_marker file: %s\n", path);
 }
 
+static void enable_trace_mark(void)
+{
+	if (!trace_marker)
+		return;
+
+	if (!tracelimit)
+		fatal("--tracemark requires -b\n");
+
+	debugfs_prepare();
+	open_tracemark_fd();
+}
+
 static void setup_tracer(void)
 {
 	if (!tracelimit || notrace)
@@ -1234,7 +1247,7 @@ enum option_values {
 	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_ALIGNED, OPT_LAPTOP, OPT_SECALIGNED, OPT_TRACEMARK,
 };
 
 /* Process commandline options */
@@ -1285,6 +1298,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
 			{"system",           no_argument,       NULL, OPT_SYSTEM },
 			{"smp",              no_argument,       NULL, OPT_SMP },
 			{"threads",          optional_argument, NULL, OPT_THREADS },
+			{"tracemark",        no_argument,       NULL, OPT_TRACEMARK },
 			{"tracer",           required_argument, NULL, OPT_TRACER },
 			{"unbuffered",       no_argument,       NULL, OPT_UNBUFFERED },
 			{"numa",             no_argument,       NULL, OPT_NUMA },
@@ -1509,6 +1523,8 @@ static void process_options (int argc, char *argv[], int max_cpus)
 			ct_debug = 1; break;
 		case OPT_LAPTOP:
 			laptop = 1; break;
+		case OPT_TRACEMARK:
+			trace_marker = 1; break;
 		}
 	}
 
@@ -1850,6 +1866,8 @@ int main(int argc, char **argv)
 
 	setup_tracer();
 
+	enable_trace_mark();
+
 	if (check_timer())
 		warn("High resolution timers not available\n");
 
-- 
2.1.0

Re: [PATCH 0/4] cyclictest: improve running under trace-cmd

[Luiz Capitulino]

On Thu, 25 Feb 2016 11:29:13 -0500
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> 
> This is just a repost of my RFC posting with no changes.
> I have tested it a bit more and I think it's good
> for inclusion.

ping?

> 
> Original intro:
> 
> In short, this series allows you to run cyclictest under
> trace-cmd and still get trace marks when the latency
> specified with -b is execeded. More details in patch 4/4.
> 
> This series is RFC because I'm not completely sure this
> is the right thing to do. I'm wondering if we shouldn't
> ditch all tracing support from cyclictest...
> 
> Luiz Capitulino (4):
>   cyclictest: tracing(): check for notrace
>   cyclictest: move debugfs init code to its own function
>   cyclictest: move tracemark_fd handling to its own function
>   cyclictest: add --tracemark option
> 
>  src/cyclictest/cyclictest.c | 62 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 15 deletions(-)
> 

Re: [PATCH 0/4] cyclictest: improve running under trace-cmd

[John Kacur]

On Wed, 16 Mar 2016, Luiz Capitulino wrote:

> On Thu, 25 Feb 2016 11:29:13 -0500
> Luiz Capitulino <lcapitulino@redhat.com> wrote:
> 
> > 
> > This is just a repost of my RFC posting with no changes.
> > I have tested it a bit more and I think it's good
> > for inclusion.
> 
> ping?
> 
> > 
> > Original intro:
> > 
> > In short, this series allows you to run cyclictest under
> > trace-cmd and still get trace marks when the latency
> > specified with -b is execeded. More details in patch 4/4.
> > 
> > This series is RFC because I'm not completely sure this
> > is the right thing to do. I'm wondering if we shouldn't
> > ditch all tracing support from cyclictest...
> > 
> > Luiz Capitulino (4):
> >   cyclictest: tracing(): check for notrace
> >   cyclictest: move debugfs init code to its own function
> >   cyclictest: move tracemark_fd handling to its own function
> >   cyclictest: add --tracemark option
> > 
> >  src/cyclictest/cyclictest.c | 62 ++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 47 insertions(+), 15 deletions(-)
> > 
> 
> --

I'll have a closer look tomorrow, but these look okay.

John

Re: [PATCH 0/4] cyclictest: improve running under trace-cmd

[Luiz Capitulino]

On Wed, 16 Mar 2016 23:37:21 +0100 (CET)
John Kacur <jkacur@redhat.com> wrote:

> 
> 
> On Wed, 16 Mar 2016, Luiz Capitulino wrote:
> 
> > On Thu, 25 Feb 2016 11:29:13 -0500
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > 
> > > 
> > > This is just a repost of my RFC posting with no changes.
> > > I have tested it a bit more and I think it's good
> > > for inclusion.
> > 
> > ping?
> > 
> > > 
> > > Original intro:
> > > 
> > > In short, this series allows you to run cyclictest under
> > > trace-cmd and still get trace marks when the latency
> > > specified with -b is execeded. More details in patch 4/4.
> > > 
> > > This series is RFC because I'm not completely sure this
> > > is the right thing to do. I'm wondering if we shouldn't
> > > ditch all tracing support from cyclictest...
> > > 
> > > Luiz Capitulino (4):
> > >   cyclictest: tracing(): check for notrace
> > >   cyclictest: move debugfs init code to its own function
> > >   cyclictest: move tracemark_fd handling to its own function
> > >   cyclictest: add --tracemark option
> > > 
> > >  src/cyclictest/cyclictest.c | 62 ++++++++++++++++++++++++++++++++++-----------
> > >  1 file changed, 47 insertions(+), 15 deletions(-)
> > > 
> > 
> > --
> 
> I'll have a closer look tomorrow, but these look okay.

There's a small conflict due to the older repo I was using. I'll
rebase it, re-test and re-post.

[PATCH 4/4] cyclictest: add --tracemark option

[Luiz Capitulino]

cyclictest will only write to /sys/kernel/debug/tracing/trace_maker
if it's also setup to do tracing. This conflicts with
running cyclictest under trace-cmd.

The --tracemark option tells cyclictest to write to the
trace_marker file even when it's not doing tracing.

It can be used like this:

 # trace-cmd record [...] cyclictest [...] -bX --tracemark --notrace

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 src/cyclictest/cyclictest.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
index 65f2aec..75af721 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -192,6 +192,7 @@ static void trigger_update(struct thread_param *par, int diff, int64_t ts);
 static int shutdown;
 static int tracelimit = 0;
 static int notrace = 0;
+static int trace_marker = 0;
 static int ftrace = 0;
 static int kernelversion;
 static int verbose = 0;
@@ -545,6 +546,18 @@ static void open_tracemark_fd(void)
 		warn("unable to open trace_marker file: %s\n", path);
 }
 
+static void enable_trace_mark(void)
+{
+	if (!trace_marker)
+		return;
+
+	if (!tracelimit)
+		fatal("--tracemark requires -b\n");
+
+	debugfs_prepare();
+	open_tracemark_fd();
+}
+
 static void setup_tracer(void)
 {
 	if (!tracelimit || notrace)
@@ -1286,7 +1299,7 @@ enum option_values {
 	OPT_SYSTEM, OPT_SMP, OPT_THREADS, OPT_TRACER, OPT_TRIGGER,
 	OPT_TRIGGER_NODES, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE, OPT_WAKEUP,
 	OPT_WAKEUPRT, OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS,
-	OPT_ALIGNED, OPT_SECALIGNED, OPT_LAPTOP,
+	OPT_ALIGNED, OPT_SECALIGNED, OPT_LAPTOP, OPT_TRACEMARK,
 };
 
 /* Process commandline options */
@@ -1340,6 +1353,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
 			{"spike",	     required_argument, NULL, OPT_TRIGGER },
 			{"spike-nodes",	     required_argument, NULL, OPT_TRIGGER_NODES },
 			{"threads",          optional_argument, NULL, OPT_THREADS },
+			{"tracemark",        no_argument,       NULL, OPT_TRACEMARK },
 			{"tracer",           required_argument, NULL, OPT_TRACER },
 			{"unbuffered",       no_argument,       NULL, OPT_UNBUFFERED },
 			{"numa",             no_argument,       NULL, OPT_NUMA },
@@ -1574,6 +1588,8 @@ static void process_options (int argc, char *argv[], int max_cpus)
 			ct_debug = 1; break;
 		case OPT_LAPTOP:
 			laptop = 1; break;
+		case OPT_TRACEMARK:
+			trace_marker = 1; break;
 		}
 	}
 
@@ -1983,6 +1999,8 @@ int main(int argc, char **argv)
 
 	setup_tracer();
 
+	enable_trace_mark();
+
 	if (check_timer())
 		warn("High resolution timers not available\n");
 
-- 
2.1.0

Re: [PATCH 4/4] cyclictest: add --tracemark option

[John Kacur]

On Thu, 17 Mar 2016, Luiz Capitulino wrote:

> cyclictest will only write to /sys/kernel/debug/tracing/trace_maker
> if it's also setup to do tracing. This conflicts with
> running cyclictest under trace-cmd.
> 
> The --tracemark option tells cyclictest to write to the
> trace_marker file even when it's not doing tracing.
> 
> It can be used like this:
> 
>  # trace-cmd record [...] cyclictest [...] -bX --tracemark --notrace
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  src/cyclictest/cyclictest.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> index 65f2aec..75af721 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -192,6 +192,7 @@ static void trigger_update(struct thread_param *par, int diff, int64_t ts);
>  static int shutdown;
>  static int tracelimit = 0;
>  static int notrace = 0;
> +static int trace_marker = 0;
>  static int ftrace = 0;
>  static int kernelversion;
>  static int verbose = 0;
> @@ -545,6 +546,18 @@ static void open_tracemark_fd(void)
>  		warn("unable to open trace_marker file: %s\n", path);
>  }
>  
> +static void enable_trace_mark(void)
> +{
> +	if (!trace_marker)
> +		return;
> +
> +	if (!tracelimit)
> +		fatal("--tracemark requires -b\n");
> +
> +	debugfs_prepare();
> +	open_tracemark_fd();
> +}
> +
>  static void setup_tracer(void)
>  {
>  	if (!tracelimit || notrace)
> @@ -1286,7 +1299,7 @@ enum option_values {
>  	OPT_SYSTEM, OPT_SMP, OPT_THREADS, OPT_TRACER, OPT_TRIGGER,
>  	OPT_TRIGGER_NODES, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE, OPT_WAKEUP,
>  	OPT_WAKEUPRT, OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS,
> -	OPT_ALIGNED, OPT_SECALIGNED, OPT_LAPTOP,
> +	OPT_ALIGNED, OPT_SECALIGNED, OPT_LAPTOP, OPT_TRACEMARK,
>  };
>  
>  /* Process commandline options */
> @@ -1340,6 +1353,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
>  			{"spike",	     required_argument, NULL, OPT_TRIGGER },
>  			{"spike-nodes",	     required_argument, NULL, OPT_TRIGGER_NODES },
>  			{"threads",          optional_argument, NULL, OPT_THREADS },
> +			{"tracemark",        no_argument,       NULL, OPT_TRACEMARK },
>  			{"tracer",           required_argument, NULL, OPT_TRACER },
>  			{"unbuffered",       no_argument,       NULL, OPT_UNBUFFERED },
>  			{"numa",             no_argument,       NULL, OPT_NUMA },
> @@ -1574,6 +1588,8 @@ static void process_options (int argc, char *argv[], int max_cpus)
>  			ct_debug = 1; break;
>  		case OPT_LAPTOP:
>  			laptop = 1; break;
> +		case OPT_TRACEMARK:
> +			trace_marker = 1; break;
>  		}
>  	}
>  
> @@ -1983,6 +1999,8 @@ int main(int argc, char **argv)
>  
>  	setup_tracer();
>  
> +	enable_trace_mark();
> +
>  	if (check_timer())
>  		warn("High resolution timers not available\n");
>  
> -- 
> 2.1.0
> 

I've pushed all the fixed up to before this point to kernel.org
can you respin this patch against the latest code?

Once it applies cleanly then I'll take it

Thanks

John

Re: [PATCH 4/4] cyclictest: add --tracemark option

[Luiz Capitulino]

On Tue, 22 Mar 2016 15:57:45 +0100 (CET)
John Kacur <jkacur@redhat.com> wrote:

> 
> 
> On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> 
> > cyclictest will only write to /sys/kernel/debug/tracing/trace_maker
> > if it's also setup to do tracing. This conflicts with
> > running cyclictest under trace-cmd.
> > 
> > The --tracemark option tells cyclictest to write to the
> > trace_marker file even when it's not doing tracing.
> > 
> > It can be used like this:
> > 
> >  # trace-cmd record [...] cyclictest [...] -bX --tracemark --notrace
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  src/cyclictest/cyclictest.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/cyclictest/cyclictest.c b/src/cyclictest/cyclictest.c
> > index 65f2aec..75af721 100644
> > --- a/src/cyclictest/cyclictest.c
> > +++ b/src/cyclictest/cyclictest.c
> > @@ -192,6 +192,7 @@ static void trigger_update(struct thread_param *par, int diff, int64_t ts);
> >  static int shutdown;
> >  static int tracelimit = 0;
> >  static int notrace = 0;
> > +static int trace_marker = 0;
> >  static int ftrace = 0;
> >  static int kernelversion;
> >  static int verbose = 0;
> > @@ -545,6 +546,18 @@ static void open_tracemark_fd(void)
> >  		warn("unable to open trace_marker file: %s\n", path);
> >  }
> >  
> > +static void enable_trace_mark(void)
> > +{
> > +	if (!trace_marker)
> > +		return;
> > +
> > +	if (!tracelimit)
> > +		fatal("--tracemark requires -b\n");
> > +
> > +	debugfs_prepare();
> > +	open_tracemark_fd();
> > +}
> > +
> >  static void setup_tracer(void)
> >  {
> >  	if (!tracelimit || notrace)
> > @@ -1286,7 +1299,7 @@ enum option_values {
> >  	OPT_SYSTEM, OPT_SMP, OPT_THREADS, OPT_TRACER, OPT_TRIGGER,
> >  	OPT_TRIGGER_NODES, OPT_UNBUFFERED, OPT_NUMA, OPT_VERBOSE, OPT_WAKEUP,
> >  	OPT_WAKEUPRT, OPT_DBGCYCLIC, OPT_POLICY, OPT_HELP, OPT_NUMOPTS,
> > -	OPT_ALIGNED, OPT_SECALIGNED, OPT_LAPTOP,
> > +	OPT_ALIGNED, OPT_SECALIGNED, OPT_LAPTOP, OPT_TRACEMARK,
> >  };
> >  
> >  /* Process commandline options */
> > @@ -1340,6 +1353,7 @@ static void process_options (int argc, char *argv[], int max_cpus)
> >  			{"spike",	     required_argument, NULL, OPT_TRIGGER },
> >  			{"spike-nodes",	     required_argument, NULL, OPT_TRIGGER_NODES },
> >  			{"threads",          optional_argument, NULL, OPT_THREADS },
> > +			{"tracemark",        no_argument,       NULL, OPT_TRACEMARK },
> >  			{"tracer",           required_argument, NULL, OPT_TRACER },
> >  			{"unbuffered",       no_argument,       NULL, OPT_UNBUFFERED },
> >  			{"numa",             no_argument,       NULL, OPT_NUMA },
> > @@ -1574,6 +1588,8 @@ static void process_options (int argc, char *argv[], int max_cpus)
> >  			ct_debug = 1; break;
> >  		case OPT_LAPTOP:
> >  			laptop = 1; break;
> > +		case OPT_TRACEMARK:
> > +			trace_marker = 1; break;
> >  		}
> >  	}
> >  
> > @@ -1983,6 +1999,8 @@ int main(int argc, char **argv)
> >  
> >  	setup_tracer();
> >  
> > +	enable_trace_mark();
> > +
> >  	if (check_timer())
> >  		warn("High resolution timers not available\n");
> >  
> > -- 
> > 2.1.0
> > 
> 
> I've pushed all the fixed up to before this point to kernel.org
> can you respin this patch against the latest code?

Sure, which branch?

> 
> Once it applies cleanly then I'll take it
> 
> Thanks
> 
> John
>