Re: [Qemu-devel] [PATCH 0/4] Tracetool cleanup
Stefan Hajnoczi writes: > On Wed, Feb 19, 2014 at 04:19:10PM +0100, Lluís Vilanova wrote: >> Stefan Hajnoczi writes: >> >> > On Mon, Feb 17, 2014 at 08:36:19PM +0100, Lluís Vilanova wrote: >> >> Minimizes the amount of backend code, making it simpler to add >> >> new/different >> >> backends. >> >> >> >> Also performs other cleanups all around. >> >> >> >> Signed-off-by: Lluís Vilanova >> >> --- >> >> >> >> Lluís Vilanova (4): >> >> trace: [tracetool] Add method 'Event.api' to build event names >> >> trace: [tracetool,trivial] Style changes >> >> trace: [tracetool] Identify formats directly used by QEMU >> >> trace: [tracetool] Minimize the amount of per-backend code >> >> > I think we stretched the concepts of backends and formats too far. >> > There are formats that only work with one backend (like 'stap'). And >> > there are backends that behave differently from all other backends. >> >> > As a result we're trying to abstract and make common a bunch of stuff >> > that isn't really common. This problem existed before this patch >> > series, but I feel we're going further down a direction that >> > increasingly seems to be wrong. >> >> > It's simpler if we turn the design inside-out. Instead of making >> > backends export all sorts of interfaces and flags, tracetool should just >> > parse trace-events and hand the list over to the backend. >> >> > Let the backend do whatever it wants. The format option simply becomes >> > an option telling the backend which type of output to generate >> > (events.h, events.c, .stp, etc). >> >> > Common behavior should live in plain old Python modules/functions. >> >> > TL;DR moving to a library design would simplify and clean up more than >> > trying to improve the current framework design >> >> > What do you think? >> >> This series moves into that direction; some of the formats are simply not >> handled by backends. For example, the "stap", "events_c" and "events_h" >> formats >> have no backend-specific contents. >> >> Also, having common code for the "format", and then relying on backends for a >> small piece of the contents simplifies later patches like the multi-backend >> tracing. >> >> The thing here is that maybe we should change format to "file", since it >> actually refers to a specific output file. > You have a point. tracetool needs to output a particular file (e.g. > generated-events.c, generated-tracers.h), which is kind of what a > "format" has become. > So the "format" is the primary piece of code that emits output. But if > it needs to do something backend-specific (like generated-tracers.h) > then it should call into backend modules. Exactly. > I am still concerned about the weird and wonderful interfaces that we're > creating (like the "API" field in this patch series). They make it > harder to understand the code and add new backends. Will think about > this more when reviewing the next revision of this series. Right. To be honest, the "API" sounded like a nice idea when I wrote it, but the only thing it buys you is that non-API formats will only receive non-disabled events. A really small benefit for adding more inter-module semantics. I will probably remove it after rebasing the series. Thanks, Lluis -- "And it's much the same thing with knowledge, for whenever you learn something new, the whole world becomes that much richer." -- The Princess of Pure Reason, as told by Norton Juster in The Phantom Tollbooth
Re: [Qemu-devel] [PATCH 0/4] Tracetool cleanup
On Wed, Feb 19, 2014 at 04:19:10PM +0100, Lluís Vilanova wrote: > Stefan Hajnoczi writes: > > > On Mon, Feb 17, 2014 at 08:36:19PM +0100, Lluís Vilanova wrote: > >> Minimizes the amount of backend code, making it simpler to add > >> new/different > >> backends. > >> > >> Also performs other cleanups all around. > >> > >> Signed-off-by: Lluís Vilanova > >> --- > >> > >> Lluís Vilanova (4): > >> trace: [tracetool] Add method 'Event.api' to build event names > >> trace: [tracetool,trivial] Style changes > >> trace: [tracetool] Identify formats directly used by QEMU > >> trace: [tracetool] Minimize the amount of per-backend code > > > I think we stretched the concepts of backends and formats too far. > > There are formats that only work with one backend (like 'stap'). And > > there are backends that behave differently from all other backends. > > > As a result we're trying to abstract and make common a bunch of stuff > > that isn't really common. This problem existed before this patch > > series, but I feel we're going further down a direction that > > increasingly seems to be wrong. > > > It's simpler if we turn the design inside-out. Instead of making > > backends export all sorts of interfaces and flags, tracetool should just > > parse trace-events and hand the list over to the backend. > > > Let the backend do whatever it wants. The format option simply becomes > > an option telling the backend which type of output to generate > > (events.h, events.c, .stp, etc). > > > Common behavior should live in plain old Python modules/functions. > > > TL;DR moving to a library design would simplify and clean up more than > > trying to improve the current framework design > > > What do you think? > > This series moves into that direction; some of the formats are simply not > handled by backends. For example, the "stap", "events_c" and "events_h" > formats > have no backend-specific contents. > > Also, having common code for the "format", and then relying on backends for a > small piece of the contents simplifies later patches like the multi-backend > tracing. > > The thing here is that maybe we should change format to "file", since it > actually refers to a specific output file. You have a point. tracetool needs to output a particular file (e.g. generated-events.c, generated-tracers.h), which is kind of what a "format" has become. So the "format" is the primary piece of code that emits output. But if it needs to do something backend-specific (like generated-tracers.h) then it should call into backend modules. I am still concerned about the weird and wonderful interfaces that we're creating (like the "API" field in this patch series). They make it harder to understand the code and add new backends. Will think about this more when reviewing the next revision of this series. Stefan
Re: [Qemu-devel] [PATCH 0/4] Tracetool cleanup
Stefan Hajnoczi writes: > On Mon, Feb 17, 2014 at 08:36:19PM +0100, Lluís Vilanova wrote: >> Minimizes the amount of backend code, making it simpler to add new/different >> backends. >> >> Also performs other cleanups all around. >> >> Signed-off-by: Lluís Vilanova >> --- >> >> Lluís Vilanova (4): >> trace: [tracetool] Add method 'Event.api' to build event names >> trace: [tracetool,trivial] Style changes >> trace: [tracetool] Identify formats directly used by QEMU >> trace: [tracetool] Minimize the amount of per-backend code > I think we stretched the concepts of backends and formats too far. > There are formats that only work with one backend (like 'stap'). And > there are backends that behave differently from all other backends. > As a result we're trying to abstract and make common a bunch of stuff > that isn't really common. This problem existed before this patch > series, but I feel we're going further down a direction that > increasingly seems to be wrong. > It's simpler if we turn the design inside-out. Instead of making > backends export all sorts of interfaces and flags, tracetool should just > parse trace-events and hand the list over to the backend. > Let the backend do whatever it wants. The format option simply becomes > an option telling the backend which type of output to generate > (events.h, events.c, .stp, etc). > Common behavior should live in plain old Python modules/functions. > TL;DR moving to a library design would simplify and clean up more than > trying to improve the current framework design > What do you think? This series moves into that direction; some of the formats are simply not handled by backends. For example, the "stap", "events_c" and "events_h" formats have no backend-specific contents. Also, having common code for the "format", and then relying on backends for a small piece of the contents simplifies later patches like the multi-backend tracing. The thing here is that maybe we should change format to "file", since it actually refers to a specific output file. Thanks, Lluis -- "And it's much the same thing with knowledge, for whenever you learn something new, the whole world becomes that much richer." -- The Princess of Pure Reason, as told by Norton Juster in The Phantom Tollbooth
Re: [Qemu-devel] [PATCH 0/4] Tracetool cleanup
On Mon, Feb 17, 2014 at 08:36:19PM +0100, Lluís Vilanova wrote: > Minimizes the amount of backend code, making it simpler to add new/different > backends. > > Also performs other cleanups all around. > > Signed-off-by: Lluís Vilanova > --- > > Lluís Vilanova (4): > trace: [tracetool] Add method 'Event.api' to build event names > trace: [tracetool,trivial] Style changes > trace: [tracetool] Identify formats directly used by QEMU > trace: [tracetool] Minimize the amount of per-backend code I think we stretched the concepts of backends and formats too far. There are formats that only work with one backend (like 'stap'). And there are backends that behave differently from all other backends. As a result we're trying to abstract and make common a bunch of stuff that isn't really common. This problem existed before this patch series, but I feel we're going further down a direction that increasingly seems to be wrong. It's simpler if we turn the design inside-out. Instead of making backends export all sorts of interfaces and flags, tracetool should just parse trace-events and hand the list over to the backend. Let the backend do whatever it wants. The format option simply becomes an option telling the backend which type of output to generate (events.h, events.c, .stp, etc). Common behavior should live in plain old Python modules/functions. TL;DR moving to a library design would simplify and clean up more than trying to improve the current framework design What do you think? Stefan
[Qemu-devel] [PATCH 0/4] Tracetool cleanup
Minimizes the amount of backend code, making it simpler to add new/different backends. Also performs other cleanups all around. Signed-off-by: Lluís Vilanova --- Lluís Vilanova (4): trace: [tracetool] Add method 'Event.api' to build event names trace: [tracetool,trivial] Style changes trace: [tracetool] Identify formats directly used by QEMU trace: [tracetool] Minimize the amount of per-backend code scripts/simpletrace.py|6 -- scripts/tracetool/__init__.py | 53 +++-- scripts/tracetool/backend/__init__.py | 74 --- scripts/tracetool/backend/dtrace.py | 81 ++-- scripts/tracetool/backend/events.py | 23 -- scripts/tracetool/backend/ftrace.py | 56 ++ scripts/tracetool/backend/simple.py | 132 - scripts/tracetool/backend/stderr.py | 43 --- scripts/tracetool/backend/ust.py | 44 +-- scripts/tracetool/format/__init__.py | 61 +++ scripts/tracetool/format/c.py | 16 +++- scripts/tracetool/format/d.py | 30 +++- scripts/tracetool/format/events_c.py | 18 ++--- scripts/tracetool/format/events_h.py | 18 ++--- scripts/tracetool/format/h.py | 33 +--- scripts/tracetool/format/stap.py | 42 ++- trace/Makefile.objs |4 + 17 files changed, 343 insertions(+), 391 deletions(-) delete mode 100644 scripts/tracetool/backend/events.py To: qemu-devel@nongnu.org Cc: Stefan Hajnoczi Cc: Kazuya Saito
