Messages in this thread | | | From | Mark Asselstine <> | Subject | Re: [PATCH 3/3] trace-cmd: setting plugin to 'nop' clears data before its recorded | Date | Fri, 6 Apr 2012 08:06:06 -0400 |
| |
On Thursday 05 April 2012 17:37:46 Steven Rostedt wrote: > On Thu, 2012-04-05 at 15:19 -0400, Mark Asselstine wrote: > > Hi Mark, > > Thanks for the updates. > > > commit e09a5db1a929ab668c273b87c4f0a32b81e1c21a > > [trace-cmd: Add trace-cmd record --date option] > > > > moved the call to disable_all() in trace_record() from after > > record_data() to before it. disable_all() sets 'nop' in > > 'current_tracer' which has the side affect of clearing 'trace', thus > > all the latency tracer reports are empty/useless. By moving > > set_plugin() out of disable_all() and rather calling it after > > record_data() we still achieve the desired goals of commit e09a5db1a9 > > while fixing latency tracing reports. > > > > Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com> > > --- > > > > trace-record.c | 13 ++++++++++--- > > 1 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/trace-record.c b/trace-record.c > > index 1c56fa9..6887874 100644 > > --- a/trace-record.c > > +++ b/trace-record.c > > @@ -897,11 +897,10 @@ static void disable_tracing(void) > > > > write_tracing_on(0); > > > > } > > > > -static void disable_all(void) > > +static void disable_all_but_plugin(void) > > Hmm, I don't really care for this name. Maybe it would be better to have > this take a parameter called "plugins", and if it is set, then you > disable plugins, otherwise you don't. This keeps it a single function > and not two different ones where one is slightly different than the > other. That is usually only done when a function is used by lots of > other areas and it is just simpler to make another function do something > slightly different and have the original call it. But this isn't used > much, so just changing the way it works, I think would be better. >
Works for me. I see you make use of gboolean elsewhere so I assume you prefer the use of this rather than using int?
> Also, and this has nothing to do with you or your patches, I simply hate > the fact that I called these plugins. They should have been called > "tracers" but I think I'm stuck with it. The term now is ambiguous as it > means both a tracer (as it does here) and it means actual plugins that > you can load into trace-cmd itself. >
To stop the proliferation do you want me to name the bool 'disable_current_tracer' rather than 'disable_plugin' or 'plugin'?
> > { > > > > disable_tracing(); > > > > - set_plugin("nop"); > > > > reset_events(); > > > > /* Force close and reset of ftrace pid file */ > > > > @@ -911,6 +910,12 @@ static void disable_all(void) > > > > clear_trace(); > > > > } > > > > +static void disable_all(void) > > +{ > > + disable_all_but_plugin(); > > + set_plugin("nop"); > > +} > > + > > > > static void > > update_sched_event(struct event_list **event, const char *file, > > > > const char *pid_filter, const char *field_filter) > > > > @@ -2227,7 +2232,7 @@ void trace_record (int argc, char **argv) > > > > } > > > > if (!keep) > > > > - disable_all(); > > + disable_all_but_plugin(); > > > > printf("Kernel buffer statistics:\n" > > > > " Note: \"entries\" are the entries left in the > > kernel ring buffer and are not\n"> > > @@ -2245,6 +2250,8 @@ void trace_record (int argc, char **argv) > > > > record_data(date2ts); > > delete_thread_data(); > > > > + if (!keep) > > + set_plugin("nop"); > > > > if (keep) > > > > exit(0); > > I think the above would have been better if you did: > > if (keep) > exit(0); > else > set_plugin(nop); > > -- Steve
Wow, I wasn't being too bright there was I? Agreed and the 'else' isn't necessary either, so this is a good cleanup.
I will get this fixed up later today. Do you want a V2 of the full patch series or just an update of this one patch?
Regards, Mark
| |