[PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Frederic Weisbecker]

Adapt mmiotrace to the new print_line type.
By default, it ignores (and consumes) types it doesn't support.

Acked-by: Pekka Paalanen <pq@iki.fi>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/trace/trace_mmiotrace.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
index a108c32..be0a6b0 100644
--- a/kernel/trace/trace_mmiotrace.c
+++ b/kernel/trace/trace_mmiotrace.c
@@ -171,7 +171,7 @@ print_out:
 	return (ret == -EBUSY) ? 0 : ret;
 }
 
-static int mmio_print_rw(struct trace_iterator *iter)
+static enum print_line_t mmio_print_rw(struct trace_iterator *iter)
 {
 	struct trace_entry *entry = iter->ent;
 	struct mmiotrace_rw *rw	= &entry->field.mmiorw;
@@ -209,11 +209,11 @@ static int mmio_print_rw(struct trace_iterator *iter)
 		break;
 	}
 	if (ret)
-		return 1;
-	return 0;
+		return TRACE_TYPE_HANDLED;
+	return TRACE_TYPE_PARTIAL_LINE;
 }
 
-static int mmio_print_map(struct trace_iterator *iter)
+static enum print_line_t mmio_print_map(struct trace_iterator *iter)
 {
 	struct trace_entry *entry = iter->ent;
 	struct mmiotrace_map *m	= &entry->field.mmiomap;
@@ -221,7 +221,7 @@ static int mmio_print_map(struct trace_iterator *iter)
 	unsigned long long t	= ns2usecs(entry->field.t);
 	unsigned long usec_rem	= do_div(t, 1000000ULL);
 	unsigned secs		= (unsigned long)t;
-	int ret = 1;
+	int ret;
 
 	switch (entry->field.mmiorw.opcode) {
 	case MMIO_PROBE:
@@ -241,11 +241,11 @@ static int mmio_print_map(struct trace_iterator *iter)
 		break;
 	}
 	if (ret)
-		return 1;
-	return 0;
+		return TRACE_TYPE_HANDLED;
+	return TRACE_TYPE_PARTIAL_LINE;
 }
 
-static int mmio_print_mark(struct trace_iterator *iter)
+static enum print_line_t mmio_print_mark(struct trace_iterator *iter)
 {
 	struct trace_entry *entry = iter->ent;
 	const char *msg		= entry->field.print.buf;
@@ -258,16 +258,15 @@ static int mmio_print_mark(struct trace_iterator *iter)
 	/* The trailing newline must be in the message. */
 	ret = trace_seq_printf(s, "MARK %lu.%06lu %s", secs, usec_rem, msg);
 	if (!ret)
-		return 0;
+		return TRACE_TYPE_PARTIAL_LINE;
 
 	if (entry->field.flags & TRACE_FLAG_CONT)
 		trace_seq_print_cont(s, iter);
 
-	return 1;
+	return TRACE_TYPE_HANDLED;
 }
 
-/* return 0 to abort printing without consuming current entry in pipe mode */
-static int mmio_print_line(struct trace_iterator *iter)
+static enum print_line_t mmio_print_line(struct trace_iterator *iter)
 {
 	switch (iter->ent->type) {
 	case TRACE_MMIO_RW:
@@ -277,7 +276,7 @@ static int mmio_print_line(struct trace_iterator *iter)
 	case TRACE_PRINT:
 		return mmio_print_mark(iter);
 	default:
-		return 1; /* ignore unknown entries */
+		return TRACE_TYPE_HANDLED; /* ignore unknown entries */
 	}
 }
 

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Pekka Paalanen]

On Mon, 29 Sep 2008 20:27:42 +0200
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Adapt mmiotrace to the new print_line type.
> By default, it ignores (and consumes) types it doesn't support.
> 
> Acked-by: Pekka Paalanen <pq@iki.fi>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

Ack!
All four patches looking good.


Cheers.

> ---
>  kernel/trace/trace_mmiotrace.c |   25 ++++++++++++-------------
>  1 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> index a108c32..be0a6b0 100644
> --- a/kernel/trace/trace_mmiotrace.c
> +++ b/kernel/trace/trace_mmiotrace.c
> @@ -171,7 +171,7 @@ print_out:
>  	return (ret == -EBUSY) ? 0 : ret;
>  }
>  
> -static int mmio_print_rw(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_rw(struct trace_iterator *iter)
>  {
>  	struct trace_entry *entry = iter->ent;
>  	struct mmiotrace_rw *rw	= &entry->field.mmiorw;
> @@ -209,11 +209,11 @@ static int mmio_print_rw(struct trace_iterator *iter)
>  		break;
>  	}
>  	if (ret)
> -		return 1;
> -	return 0;
> +		return TRACE_TYPE_HANDLED;
> +	return TRACE_TYPE_PARTIAL_LINE;
>  }
>  
> -static int mmio_print_map(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_map(struct trace_iterator *iter)
>  {
>  	struct trace_entry *entry = iter->ent;
>  	struct mmiotrace_map *m	= &entry->field.mmiomap;
> @@ -221,7 +221,7 @@ static int mmio_print_map(struct trace_iterator *iter)
>  	unsigned long long t	= ns2usecs(entry->field.t);
>  	unsigned long usec_rem	= do_div(t, 1000000ULL);
>  	unsigned secs		= (unsigned long)t;
> -	int ret = 1;
> +	int ret;
>  
>  	switch (entry->field.mmiorw.opcode) {
>  	case MMIO_PROBE:
> @@ -241,11 +241,11 @@ static int mmio_print_map(struct trace_iterator *iter)
>  		break;
>  	}
>  	if (ret)
> -		return 1;
> -	return 0;
> +		return TRACE_TYPE_HANDLED;
> +	return TRACE_TYPE_PARTIAL_LINE;
>  }
>  
> -static int mmio_print_mark(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_mark(struct trace_iterator *iter)
>  {
>  	struct trace_entry *entry = iter->ent;
>  	const char *msg		= entry->field.print.buf;
> @@ -258,16 +258,15 @@ static int mmio_print_mark(struct trace_iterator *iter)
>  	/* The trailing newline must be in the message. */
>  	ret = trace_seq_printf(s, "MARK %lu.%06lu %s", secs, usec_rem, msg);
>  	if (!ret)
> -		return 0;
> +		return TRACE_TYPE_PARTIAL_LINE;
>  
>  	if (entry->field.flags & TRACE_FLAG_CONT)
>  		trace_seq_print_cont(s, iter);
>  
> -	return 1;
> +	return TRACE_TYPE_HANDLED;
>  }
>  
> -/* return 0 to abort printing without consuming current entry in pipe mode */
> -static int mmio_print_line(struct trace_iterator *iter)
> +static enum print_line_t mmio_print_line(struct trace_iterator *iter)
>  {
>  	switch (iter->ent->type) {
>  	case TRACE_MMIO_RW:
> @@ -277,7 +276,7 @@ static int mmio_print_line(struct trace_iterator *iter)
>  	case TRACE_PRINT:
>  		return mmio_print_mark(iter);
>  	default:
> -		return 1; /* ignore unknown entries */
> +		return TRACE_TYPE_HANDLED; /* ignore unknown entries */
>  	}
>  }
>  
> 


-- 
Pekka Paalanen
http://www.iki.fi/pq/

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Ingo Molnar]

* Pekka Paalanen <pq@iki.fi> wrote:

> On Mon, 29 Sep 2008 20:27:42 +0200
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Adapt mmiotrace to the new print_line type.
> > By default, it ignores (and consumes) types it doesn't support.
> > 
> > Acked-by: Pekka Paalanen <pq@iki.fi>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> 
> Ack!
> All four patches looking good.

thanks guys!

Steve's latest trace-ringbuffer code against -tip complicates life a 
little bit.

We dont yet know whether tip/tracing/ring-buffer is ready for wider 
consumption, so it's in a separate feature branch. These four patches 
conflict with the ring-buffer code quite widely.

So ... i did an optimistic merge approach: i merged these 4 patches 
ontop of the new tip/tracing/ring-buffer code, and have put it into a 
new tip/tracing/pipe branch:

 # f19d495: tracing/ftrace: adapt the boot tracer to the new print_line type
 # 92261f6: tracing/ftrace: adapt mmiotrace to the new type of print_line
 # 157190e: tracing/ftrace: fix pipe breaking
 # 92e359a: tracing/ftrace: change the type of the print_line callback

i'm merging this into tip/master for a bit of testing, but if it breaks 
something (or if i havent pushed it out yet) you can merge it locally 
too, by doing this ontop of tip/master:

  git-merge tip/tracing/pipe

it should merge up cleanly and you should be able to check the end 
result. (and send me any fixes for merge mistakes)

( Once it's all in reasonably well-tested shape it will all show up in 
  tip/master and you can stop dealing with tip/tracing/* sub-branches.)

	Ingo

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Frédéric Weisbecker]

2008/9/30 Ingo Molnar <mingo@elte.hu>:
> thanks guys!
>
> Steve's latest trace-ringbuffer code against -tip complicates life a
> little bit.
>
> We dont yet know whether tip/tracing/ring-buffer is ready for wider
> consumption, so it's in a separate feature branch. These four patches
> conflict with the ring-buffer code quite widely.
>
> So ... i did an optimistic merge approach: i merged these 4 patches
> ontop of the new tip/tracing/ring-buffer code, and have put it into a
> new tip/tracing/pipe branch:
>
>  # f19d495: tracing/ftrace: adapt the boot tracer to the new print_line type
>  # 92261f6: tracing/ftrace: adapt mmiotrace to the new type of print_line
>  # 157190e: tracing/ftrace: fix pipe breaking
>  # 92e359a: tracing/ftrace: change the type of the print_line callback
>
> i'm merging this into tip/master for a bit of testing, but if it breaks
> something (or if i havent pushed it out yet) you can merge it locally
> too, by doing this ontop of tip/master:
>
>  git-merge tip/tracing/pipe
>
> it should merge up cleanly and you should be able to check the end
> result. (and send me any fixes for merge mistakes)
>
> ( Once it's all in reasonably well-tested shape it will all show up in
>  tip/master and you can stop dealing with tip/tracing/* sub-branches.)
>
>        Ingo
>

Thanks Ingo.

I'm currently testing tip/master, since tracing/pipe is merged into.
It seems there are some locking issues that I hadn't when I tested the patches.
Perhaps it's because of the merge with the new ring buffer.

I just launched the sched_switch tracer and it worked well until 4
seconds of execution. And after that it blocked.
I tested too a marker message adding and it is never displayed. Worse:
I can't break the pipe with Ctrl + C after that.

When I will have some time, I will try to debug all what I find.

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Steven Rostedt]

On Tue, 30 Sep 2008, Fr?d?ric Weisbecker wrote:
> 
> Thanks Ingo.
> 
> I'm currently testing tip/master, since tracing/pipe is merged into.
> It seems there are some locking issues that I hadn't when I tested the patches.
> Perhaps it's because of the merge with the new ring buffer.
> 
> I just launched the sched_switch tracer and it worked well until 4
> seconds of execution. And after that it blocked.
> I tested too a marker message adding and it is never displayed. Worse:
> I can't break the pipe with Ctrl + C after that.
> 
> When I will have some time, I will try to debug all what I find.
> 

OK, I'll nuke the ring_buffer_lock :-/

The trace_pipe calls that and then calls print_trace_line which will 
eventually lock the buffer again. This is a bug on my part. I'll fix that 
today.

Thanks,

-- Steve

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Frédéric Weisbecker]

2008/9/30 Steven Rostedt <rostedt@goodmis.org>:
>
> OK, I'll nuke the ring_buffer_lock :-/
>
> The trace_pipe calls that and then calls print_trace_line which will
> eventually lock the buffer again. This is a bug on my part. I'll fix that
> today.
>
> Thanks,

Strange, I can't see any case where print_trace_line could call the
ring_buffer_lock.
Hmm, I will see in your patch.

Ingo, I just saw one damage from the merging, trace_empty() returns
TRACE_TYPE_HANDLED. The type is wrong, it should return 1.
It's not urgent since the value is the same. Should I send a patch for
such a small error?

Thanks.

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Steven Rostedt]

On Tue, 30 Sep 2008, Fr?d?ric Weisbecker wrote:

> 2008/9/30 Steven Rostedt <rostedt@goodmis.org>:
> >
> > OK, I'll nuke the ring_buffer_lock :-/
> >
> > The trace_pipe calls that and then calls print_trace_line which will
> > eventually lock the buffer again. This is a bug on my part. I'll fix that
> > today.
> >
> > Thanks,
> 
> Strange, I can't see any case where print_trace_line could call the
> ring_buffer_lock.
> Hmm, I will see in your patch.

Opps, I mean the find_next_entry_inc will do the lock.

The ring_buffer_lock locks all CPU buffers, and the find_next_entry_inc
will read from the ring buffer which will also lock the buffer. Hence
a deadlock.

-- Steve

> 
> Ingo, I just saw one damage from the merging, trace_empty() returns
> TRACE_TYPE_HANDLED. The type is wrong, it should return 1.
> It's not urgent since the value is the same. Should I send a patch for
> such a small error?

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Ingo Molnar]

* Frédéric Weisbecker <fweisbec@gmail.com> wrote:

> 2008/9/30 Steven Rostedt <rostedt@goodmis.org>:
> >
> > OK, I'll nuke the ring_buffer_lock :-/
> >
> > The trace_pipe calls that and then calls print_trace_line which will
> > eventually lock the buffer again. This is a bug on my part. I'll fix that
> > today.
> >
> > Thanks,
> 
> Strange, I can't see any case where print_trace_line could call the
> ring_buffer_lock.
> Hmm, I will see in your patch.
> 
> Ingo, I just saw one damage from the merging, trace_empty() returns 
> TRACE_TYPE_HANDLED. The type is wrong, it should return 1. It's not 
> urgent since the value is the same. Should I send a patch for such a 
> small error?

yes, please do. One too many patch is far better than one too few ;-)

	Ingo

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Frederic Weisbecker]

Ingo Molnar wrote:
> * Frédéric Weisbecker <fweisbec@gmail.com> wrote:
> 
>> 2008/9/30 Steven Rostedt <rostedt@goodmis.org>:
>>> OK, I'll nuke the ring_buffer_lock :-/
>>>
>>> The trace_pipe calls that and then calls print_trace_line which will
>>> eventually lock the buffer again. This is a bug on my part. I'll fix that
>>> today.
>>>
>>> Thanks,
>> Strange, I can't see any case where print_trace_line could call the
>> ring_buffer_lock.
>> Hmm, I will see in your patch.
>>
>> Ingo, I just saw one damage from the merging, trace_empty() returns 
>> TRACE_TYPE_HANDLED. The type is wrong, it should return 1. It's not 
>> urgent since the value is the same. Should I send a patch for such a 
>> small error?
> 
> yes, please do. One too many patch is far better than one too few ;-)
> 
> 	Ingo
> 

:)

Here it is!

Subject: [PATCH -tip] Tracing/ftrace: correct return value of trace_empty

Correct the value's type of trace_empty function

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6a1c76b..da3789d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1686,7 +1686,7 @@ static int trace_empty(struct trace_iterator *iter)
 		if (!ring_buffer_iter_empty(iter->buffer_iter[cpu]))
 			return 0;
 	}
-	return TRACE_TYPE_HANDLED;
+	return 1;
 }
 
 static enum print_line_t print_trace_line(struct trace_iterator *iter)


diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 6a1c76b..da3789d 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1686,7 +1686,7 @@ static int trace_empty(struct trace_iterator *iter)
 		if (!ring_buffer_iter_empty(iter->buffer_iter[cpu]))
 			return 0;
 	}
-	return TRACE_TYPE_HANDLED;
+	return 1;
 }
 
 static enum print_line_t print_trace_line(struct trace_iterator *iter)

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Frédéric Weisbecker]

Oops sorry...
Cut in the middle....

2008/9/30 Frederic Weisbecker <fweisbec@gmail.com>:
> Ingo Molnar wrote:
>> * Frédéric Weisbecker <fweisbec@gmail.com> wrote:
>>
>>> 2008/9/30 Steven Rostedt <rostedt@goodmis.org>:
>>>> OK, I'll nuke the ring_buffer_lock :-/
>>>>
>>>> The trace_pipe calls that and then calls print_trace_line which will
>>>> eventually lock the buffer again. This is a bug on my part. I'll fix that
>>>> today.
>>>>
>>>> Thanks,
>>> Strange, I can't see any case where print_trace_line could call the
>>> ring_buffer_lock.
>>> Hmm, I will see in your patch.
>>>
>>> Ingo, I just saw one damage from the merging, trace_empty() returns
>>> TRACE_TYPE_HANDLED. The type is wrong, it should return 1. It's not
>>> urgent since the value is the same. Should I send a patch for such a
>>> small error?
>>
>> yes, please do. One too many patch is far better than one too few ;-)
>>
>>       Ingo
>>
>
> :)
>
> Here it is!
>
> Subject: [PATCH -tip] Tracing/ftrace: correct return value of trace_empty
>
> Correct the value's type of trace_empty function
>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6a1c76b..da3789d 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1686,7 +1686,7 @@ static int trace_empty(struct trace_iterator *iter)
>                if (!ring_buffer_iter_empty(iter->buffer_iter[cpu]))
>                        return 0;
>        }
> -       return TRACE_TYPE_HANDLED;
> +       return 1;
>  }
>
>  static enum print_line_t print_trace_line(struct trace_iterator *iter)
>
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6a1c76b..da3789d 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1686,7 +1686,7 @@ static int trace_empty(struct trace_iterator *iter)
>                if (!ring_buffer_iter_empty(iter->buffer_iter[cpu]))
>                        return 0;
>        }
> -       return TRACE_TYPE_HANDLED;
> +       return 1;
>  }
>
>  static enum print_line_t print_trace_line(struct trace_iterator *iter)
>

Re: [PATCH V2 -tip 3/4] Tracing/ftrace: Adapt mmiotrace to the new type of print_line

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Subject: [PATCH -tip] Tracing/ftrace: correct return value of trace_empty
> 
> Correct the value's type of trace_empty function
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 6a1c76b..da3789d 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1686,7 +1686,7 @@ static int trace_empty(struct trace_iterator *iter)
>  		if (!ring_buffer_iter_empty(iter->buffer_iter[cpu]))
>  			return 0;
>  	}
> -	return TRACE_TYPE_HANDLED;
> +	return 1;

applied the commit below to tip/tracing/ring-buffer, thanks Frederic! 
(had to do manual merging due to flux in that function due to the 
locking changes - please double-check that i got it right.)

btw., we still have the mmiotrace type casting buglet unfixed:

> > >   struct trace_entry *entry = iter->ent;
> > > - struct mmiotrace_map *m = &entry->field.mmiomap;
> > > + struct mmiotrace_map *m = (struct mmiotrace_map *)entry;

	Ingo

------------>
>From a2e221682b91ff83dc8a5e7fbb60a9d87a4e83f2 Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <fweisbec@gmail.com>
Date: Tue, 30 Sep 2008 18:13:45 +0200
Subject: [PATCH] tracing/ftrace: Adapt mmiotrace to the new type of print_line, fix

Correct the value's type of trace_empty function

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/trace/trace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index b542f88..c163406 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1750,7 +1750,7 @@ static int trace_empty(struct trace_iterator *iter)
 		}
 	}
 
-	return TRACE_TYPE_HANDLED;
+	return 1;
 }
 
 static enum print_line_t print_trace_line(struct trace_iterator *iter)