[Patch -tip 3/3] Tracing/ftrace: Don't consume entries unhandled by mmiotrace

[Patch -tip 3/3] Tracing/ftrace: Don't consume entries unhandled by mmiotrace

[Frédéric Weisbecker]

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

When mmiotrace can't handle an entry output, it returns 1.
It should return 0 to relay on other output functions.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---

[-- Attachment #2: 3.diff --]
[-- Type: text/plain, Size: 433 bytes --]

diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
index a108c32..6fe1315 100644
--- a/kernel/trace/trace_mmiotrace.c
+++ b/kernel/trace/trace_mmiotrace.c
@@ -277,7 +277,8 @@ static int mmio_print_line(struct trace_iterator *iter)
 	case TRACE_PRINT:
 		return mmio_print_mark(iter);
 	default:
-		return 1; /* ignore unknown entries */
+		return 0; /* ignore unknown entries whithout consuming */
 	}
 }
 

Re: [Patch -tip 3/3] Tracing/ftrace: Don't consume entries unhandled by mmiotrace

[Ingo Molnar]

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

> When mmiotrace can't handle an entry output, it returns 1.
> It should return 0 to relay on other output functions.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>

applied to tip/tracing/mmiotrace, thanks Frédéric!

	Ingo

Re: [Patch -tip 3/3] Tracing/ftrace: Don't consume entries unhandled by mmiotrace

[Pekka Paalanen]

On Thu, 25 Sep 2008 13:28:56 +0100
Frédéric Weisbecker <fweisbec@gmail.com> wrote:

> When mmiotrace can't handle an entry output, it returns 1.
> It should return 0 to relay on other output functions.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
> diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> index a108c32..6fe1315 100644
> --- a/kernel/trace/trace_mmiotrace.c
> +++ b/kernel/trace/trace_mmiotrace.c
> @@ -277,7 +277,8 @@ static int mmio_print_line(struct trace_iterator *iter)
>  	case TRACE_PRINT:
>  		return mmio_print_mark(iter);
>  	default:
> -		return 1; /* ignore unknown entries */
> +		return 0; /* ignore unknown entries whithout consuming */
>  	}
>  }

NACK.

mmiotrace's log output is stricly specified, the standard/default
printing functions may NOT be used. The output is supposed to be
machine readable in addition to human readable.

The ftrace infrastructure assumes there is only one active tracer
at a time, therefore destroying unhandled entries is not a problem.

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

Re: [Patch -tip 3/3] Tracing/ftrace: Don't consume entries unhandled by mmiotrace

[Frédéric Weisbecker]

2008/9/25 Pekka Paalanen <pq@iki.fi>:
> NACK.
>
> mmiotrace's log output is stricly specified, the standard/default
> printing functions may NOT be used. The output is supposed to be
> machine readable in addition to human readable.
>
> The ftrace infrastructure assumes there is only one active tracer
> at a time, therefore destroying unhandled entries is not a problem.
>
> --
> Pekka Paalanen
> http://www.iki.fi/pq/
>

Hi Pekka.

It's up to you.
I just guessed that one would trace IO and stack for example.
Or why not IO and functions: This sounds to me very useful (who
performed this io..?)

As you want....

Regards...

Re: [Patch -tip 3/3] Tracing/ftrace: Don't consume entries unhandled by mmiotrace

[Pekka Paalanen]

On Thu, 25 Sep 2008 16:44:08 +0200
"Frédéric Weisbecker" <fweisbec@gmail.com> wrote:

> 2008/9/25 Pekka Paalanen <pq@iki.fi>:
> > NACK.
> >
> > mmiotrace's log output is stricly specified, the standard/default
> > printing functions may NOT be used. The output is supposed to be
> > machine readable in addition to human readable.
> >
> > The ftrace infrastructure assumes there is only one active tracer
> > at a time, therefore destroying unhandled entries is not a problem.
> 
> Hi Pekka.
> 
> It's up to you.
> I just guessed that one would trace IO and stack for example.
> Or why not IO and functions: This sounds to me very useful (who
> performed this io..?)

We can (must) return to this if/when multiple tracers are allowed
to be active simultaneously. But in the current situation, I do not
see a way to trace more than one thing, but I haven't really looked
into the other tracers.

OTOH, mmiotrace log format has a pid field, which is pretty much unused
currently. It could be used to record the current process, if one exists.
That can be added, should the need arise, but so far the field has been
reserved for tracing accesses done in user space - but those are not
caught yet.

I'm not sure what stack trace collects, but tracing functions is useless
or harmful in the usual use case of mmiotrace, i.e. tracing a binary-only
proprietary driver. The functions will be anonymous anyway, and we can't
disassemble the driver due to possible legal reasons. That's why mmiotrace
does not even collect instruction pointers by default, although it
supports them (or did, can't recall if it works now, since it hasn't been
needed). I assume the goal is to create a free open driver without any
legal issues by watching what the proprietary driver does.

Even if the log would be cluttered with MMIO from other, uninteresting
drivers, you can use the physical address to filter the entries by
device (and therefore driver) afterwards. This is one reason why
an mmiotrace log starts with the contents of /proc/bus/pci/devices.

Was there some other scenario you were thinking about?

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

Re: [Patch -tip 3/3] Tracing/ftrace: Don't consume entries unhandled by mmiotrace

[Frédéric Weisbecker]

2008/9/25 Pekka Paalanen <pq@iki.fi>:
> We can (must) return to this if/when multiple tracers are allowed
> to be active simultaneously. But in the current situation, I do not
> see a way to trace more than one thing, but I haven't really looked
> into the other tracers.

You can enable another tracer simultaneously. I never tried it but you can
launch one from debugfs and another from kernel code for example.
The new boot tracer will have to launch the sched_switch tracer.
And since the functions which display the sched_switch entries are
already inside
trace.c I wanted to relay the display to the "default display
functions". That's one of
the reason of my patch (+ the bug).
But I could proceed in a another way than default relaying. Why not
using a function exported
by the tracing engine to display sched_switches.

> but tracing functions is useless
> or harmful in the usual use case of mmiotrace, i.e. tracing a binary-only
> proprietary driver. The functions will be anonymous anyway, and we can't
> disassemble the driver due to possible legal reasons. That's why mmiotrace
> does not even collect instruction pointers by default, although it
> supports them (or did, can't recall if it works now, since it hasn't been
> needed). I assume the goal is to create a free open driver without any
> legal issues by watching what the proprietary driver does.
>
> Even if the log would be cluttered with MMIO from other, uninteresting
> drivers, you can use the physical address to filter the entries by
> device (and therefore driver) afterwards. This is one reason why
> an mmiotrace log starts with the contents of /proc/bus/pci/devices.
>
> Was there some other scenario you were thinking about?

When I wanted to help on the ath5k project, I used to compare madwifi
traces with the ath5k traces. The goal was to find where was the
trouble when support for ar2425 cards didn't worked.
And I wanted to see the name of the functions where the code entered
to see where I was in the traces. That's why I wanted to have
an mmiotrace_printk function primarily :)
But I think one could use it for debugging too. Just to know where we
are in the trace.

Re: [Patch -tip 3/3] Tracing/ftrace: Don't consume entries unhandled by mmiotrace

[Ingo Molnar]

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

> 2008/9/25 Pekka Paalanen <pq@iki.fi>:
> > We can (must) return to this if/when multiple tracers are allowed
> > to be active simultaneously. But in the current situation, I do not
> > see a way to trace more than one thing, but I haven't really looked
> > into the other tracers.
> 
> You can enable another tracer simultaneously. I never tried it but you 
> can launch one from debugfs and another from kernel code for example. 
> The new boot tracer will have to launch the sched_switch tracer.

hm, right now i can see a lot of problems with that approach - the 
buffers are partly per tracer and partly global.

the new trace-ringbuffer design Steve is working on would abstract away 
this complication - then we could make tracers truly independent 
entities.

	Ingo

Re: [Patch -tip 3/3] Tracing/ftrace: Don't consume entries unhandled by mmiotrace

[Steven Rostedt]

On Sat, 27 Sep 2008, Ingo Molnar wrote:
> 
> the new trace-ringbuffer design Steve is working on would abstract away 
> this complication - then we could make tracers truly independent 
> entities.

Yep, I even started implementing this. What is nice about this too, is 
that we could even mix the tracer buffers together in the merge sort. 
Since all buffers use the same timestamping, we can keep the separate 
buffers out, turn on different tracers, and have the output show them all 
together.

This would mean that we can keep the function tracer buffer large, and the 
other tracer buffers smaller. And merge them together when needed.

I'll have to recreate it, since the buffer code changed quite a bit since 
I first started making ftrace do this. Also, I want to make the changes to 
ftrace as incremental as possible. That way we could easily diagnose 
problems that arise.

-- Steve