[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 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 4e80831..65f2aec 100644
--- a/src/cyclictest/cyclictest.c
+++ b/src/cyclictest/cyclictest.c
@@ -532,6 +532,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)
@@ -647,14 +660,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);
-		}

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

[John Kacur]

On Thu, 17 Mar 2016, Luiz Capitulino wrote:

> 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 4e80831..65f2aec 100644
> --- a/src/cyclictest/cyclictest.c
> +++ b/src/cyclictest/cyclictest.c
> @@ -532,6 +532,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)
> @@ -647,14 +660,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);
> -		}
> -
> +		open_tracemark_fd();
>  	} else {
>  		setkernvar("trace_all_cpus", "1");
>  		setkernvar("trace_freerunning", "1");
> -- 
> 2.1.0

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

But, once again, it didn't apply cleanly, and you should have included it
with the next set off patches where it is required, because I was 
scratching my head as to why I should include this until I looked there.

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

[Luiz Capitulino]

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

> 
> 
> On Thu, 17 Mar 2016, Luiz Capitulino wrote:
> 
> > 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 4e80831..65f2aec 100644
> > --- a/src/cyclictest/cyclictest.c
> > +++ b/src/cyclictest/cyclictest.c
> > @@ -532,6 +532,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)
> > @@ -647,14 +660,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);
> > -		}
> > -
> > +		open_tracemark_fd();
> >  	} else {
> >  		setkernvar("trace_all_cpus", "1");
> >  		setkernvar("trace_freerunning", "1");
> > -- 
> > 2.1.0
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> 
> But, once again, it didn't apply cleanly,

Yeah, I didn't know I had to use the devel branch. Do you want me
to re-send or did git solved the conflicts?

> and you should have included it
> with the next set off patches where it is required, because I was 
> scratching my head as to why I should include this until I looked there.

Which set of patches?