| Message ID | 20190319155536.19381-6-tstoyanov@vmware.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | trace-cmd: Timetamps sync between host and guest machines, relying on vsock events. | expand |
> On Mar 19, 2019, at 8:55 AM, Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote: > > In order to reuse the code inside the trace-cmd application, few > functions from trace-record.c are refactored: > - make_instances() and tracecmd_remove_instances() are splited. > New ones are created: tracecmd_make_instance() and tracecmd_remove_instance(), > which are visible outside the trace-record.c > - Following functions are made non-static: tracecmd_init_instance() > get_instance_dir(), write_instance_file(), write_tracing_on(), > tracecmd_set_clock() > - New function is implemented: tracecmd_local_cpu_count(), an internal > API to get local_cpu_count. > > Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com> > --- > tracecmd/include/trace-local.h | 9 ++++ > tracecmd/trace-record.c | 87 +++++++++++++++++++--------------- > 2 files changed, 58 insertions(+), 38 deletions(-) > > diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h > index d7bdb1f..8413054 100644 > --- a/tracecmd/include/trace-local.h > +++ b/tracecmd/include/trace-local.h > @@ -235,6 +235,15 @@ void update_first_instance(struct buffer_instance *instance, int topt); > void show_instance_file(struct buffer_instance *instance, const char *name); > > int count_cpus(void); > +void write_tracing_on(struct buffer_instance *instance, int on); > +char *get_instance_dir(struct buffer_instance *instance); > +int write_instance_file(struct buffer_instance *instance, > + const char *file, const char *str, const char *type); > +void tracecmd_init_instance(struct buffer_instance *instance); > +void tracecmd_make_instance(struct buffer_instance *instance); > +int tracecmd_local_cpu_count(void); > +void tracecmd_set_clock(struct buffer_instance *instance); > +void tracecmd_remove_instance(struct buffer_instance *instance); > > /* No longer in event-utils.h */ > void __noreturn die(const char *fmt, ...); /* Can be overriden */ > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > index 177060d..3950242 100644 > --- a/tracecmd/trace-record.c > +++ b/tracecmd/trace-record.c > <snipped> > -static void set_clock(struct buffer_instance *instance) > +void tracecmd_set_clock(struct buffer_instance *instance) > { > char *path; > char *content; > @@ -4316,49 +4314,57 @@ static void clear_func_filters(void) > } > } > > -static void make_instances(void) > +void tracecmd_make_instance(struct buffer_instance *instance) > { > - struct buffer_instance *instance; > struct stat st; > char *path; > int ret; > > + path = get_instance_dir(instance); > + ret = stat(path, &st); > + if (ret < 0) { > + ret = mkdir(path, 0777); Here is the mkdir ... > + if (ret < 0) > + die("mkdir %s", path); > + } else > + /* Don't delete instances that already exist */ > + instance->flags |= BUFFER_FL_KEEP; > + tracecmd_put_tracing_file(path); > + > +} > + > +static void make_instances(void) > +{ > + struct buffer_instance *instance; > + > for_each_instance(instance) { > if (is_guest(instance)) > continue; > + tracecmd_make_instance(instance); > + } > +} > > - path = get_instance_dir(instance); > - ret = stat(path, &st); > - if (ret < 0) { > - ret = mkdir(path, 0777); > - if (ret < 0) > - die("mkdir %s", path); > - } else > - /* Don't delete instances that already exist */ > - instance->flags |= BUFFER_FL_KEEP; > - tracecmd_put_tracing_file(path); > +void tracecmd_remove_instance(struct buffer_instance *instance) > +{ > + char *path; > + > + if (instance->tracing_on_fd > 0) { > + close(instance->tracing_on_fd); > + instance->tracing_on_fd = 0; > } > + path = get_instance_dir(instance); > + tracecmd_put_tracing_file(path); > } > > void tracecmd_remove_instances(void) > { > struct buffer_instance *instance; > - char *path; > - int ret; > > for_each_instance(instance) { > /* Only delete what we created */ > if (is_guest(instance) || (instance->flags & BUFFER_FL_KEEP)) > continue; > - if (instance->tracing_on_fd > 0) { > - close(instance->tracing_on_fd); > - instance->tracing_on_fd = 0; > - } > - path = get_instance_dir(instance); > - ret = rmdir(path); But this rmdir is not moved to tracecmd_remove_instance() — it’s lost. This looks like it’s introducing a bug. > - if (ret < 0) > - die("rmdir %s", path); > - tracecmd_put_tracing_file(path); > + tracecmd_remove_instance(instance); > } > } <snipped> The rest looks fine. Cheers, -Matt Helsley
On Tue, Mar 19, 2019 at 8:21 PM Matt Helsley <mhelsley@vmware.com> wrote: > > > > > On Mar 19, 2019, at 8:55 AM, Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote: > > > > In order to reuse the code inside the trace-cmd application, few > > functions from trace-record.c are refactored: > > - make_instances() and tracecmd_remove_instances() are splited. > > New ones are created: tracecmd_make_instance() and tracecmd_remove_instance(), > > which are visible outside the trace-record.c > > - Following functions are made non-static: tracecmd_init_instance() > > get_instance_dir(), write_instance_file(), write_tracing_on(), > > tracecmd_set_clock() > > - New function is implemented: tracecmd_local_cpu_count(), an internal > > API to get local_cpu_count. > > > > Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com> > > --- > > tracecmd/include/trace-local.h | 9 ++++ > > tracecmd/trace-record.c | 87 +++++++++++++++++++--------------- > > 2 files changed, 58 insertions(+), 38 deletions(-) > > > > diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h > > index d7bdb1f..8413054 100644 > > --- a/tracecmd/include/trace-local.h > > +++ b/tracecmd/include/trace-local.h > > @@ -235,6 +235,15 @@ void update_first_instance(struct buffer_instance *instance, int topt); > > void show_instance_file(struct buffer_instance *instance, const char *name); > > > > int count_cpus(void); > > +void write_tracing_on(struct buffer_instance *instance, int on); > > +char *get_instance_dir(struct buffer_instance *instance); > > +int write_instance_file(struct buffer_instance *instance, > > + const char *file, const char *str, const char *type); > > +void tracecmd_init_instance(struct buffer_instance *instance); > > +void tracecmd_make_instance(struct buffer_instance *instance); > > +int tracecmd_local_cpu_count(void); > > +void tracecmd_set_clock(struct buffer_instance *instance); > > +void tracecmd_remove_instance(struct buffer_instance *instance); > > > > /* No longer in event-utils.h */ > > void __noreturn die(const char *fmt, ...); /* Can be overriden */ > > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c > > index 177060d..3950242 100644 > > --- a/tracecmd/trace-record.c > > +++ b/tracecmd/trace-record.c > > > > <snipped> > > > -static void set_clock(struct buffer_instance *instance) > > +void tracecmd_set_clock(struct buffer_instance *instance) > > { > > char *path; > > char *content; > > @@ -4316,49 +4314,57 @@ static void clear_func_filters(void) > > } > > } > > > > -static void make_instances(void) > > +void tracecmd_make_instance(struct buffer_instance *instance) > > { > > - struct buffer_instance *instance; > > struct stat st; > > char *path; > > int ret; > > > > + path = get_instance_dir(instance); > > + ret = stat(path, &st); > > + if (ret < 0) { > > + ret = mkdir(path, 0777); > > Here is the mkdir ... > > > + if (ret < 0) > > + die("mkdir %s", path); > > + } else > > + /* Don't delete instances that already exist */ > > + instance->flags |= BUFFER_FL_KEEP; > > + tracecmd_put_tracing_file(path); > > + > > +} > > + > > +static void make_instances(void) > > +{ > > + struct buffer_instance *instance; > > + > > for_each_instance(instance) { > > if (is_guest(instance)) > > continue; > > + tracecmd_make_instance(instance); > > + } > > +} > > > > - path = get_instance_dir(instance); > > - ret = stat(path, &st); > > - if (ret < 0) { > > - ret = mkdir(path, 0777); > > - if (ret < 0) > > - die("mkdir %s", path); > > - } else > > - /* Don't delete instances that already exist */ > > - instance->flags |= BUFFER_FL_KEEP; > > - tracecmd_put_tracing_file(path); > > +void tracecmd_remove_instance(struct buffer_instance *instance) > > +{ > > + char *path; > > + > > + if (instance->tracing_on_fd > 0) { > > + close(instance->tracing_on_fd); > > + instance->tracing_on_fd = 0; > > } > > + path = get_instance_dir(instance); > > + tracecmd_put_tracing_file(path); > > } > > > > void tracecmd_remove_instances(void) > > { > > struct buffer_instance *instance; > > - char *path; > > - int ret; > > > > for_each_instance(instance) { > > /* Only delete what we created */ > > if (is_guest(instance) || (instance->flags & BUFFER_FL_KEEP)) > > continue; > > - if (instance->tracing_on_fd > 0) { > > - close(instance->tracing_on_fd); > > - instance->tracing_on_fd = 0; > > - } > > - path = get_instance_dir(instance); > > - ret = rmdir(path); > > But this rmdir is not moved to tracecmd_remove_instance() — it’s lost. This looks like it’s introducing a bug. Thanks Matt, this one is from v1 of the patchset, I didn't notice it. I'll add the rmdir() call in the next version. There is some general problem with deleting instances, or (most probably) I cannot figure out what it the proper way to delete a ftrace instance - even after the call to rmdir() in tracecmd_remove_instance(), the instance dir is still there. I tried to run "rm -rf path_to_instance_dir" from the shell (at root), after the trace-cmd is executed, got a lot of "Operation not permitted" errors and the instance dir is not deleted. > > > - if (ret < 0) > > - die("rmdir %s", path); > > - tracecmd_put_tracing_file(path); > > + tracecmd_remove_instance(instance); > > } > > } > > <snipped> > > The rest looks fine. > > Cheers, > -Matt Helsley >
On Wed, 20 Mar 2019 14:14:03 +0000 Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote: > > But this rmdir is not moved to tracecmd_remove_instance() — it’s lost. This looks like it’s introducing a bug. > > Thanks Matt, this one is from v1 of the patchset, I didn't notice it. > I'll add the rmdir() call in the next version. > There is some general problem with deleting instances, or (most > probably) I cannot figure out what it the proper way > to delete a ftrace instance - even after the call to rmdir() in > tracecmd_remove_instance(), the instance dir is still there. > I tried to run "rm -rf path_to_instance_dir" from the shell (at root), > after the trace-cmd is executed, got a lot of > "Operation not permitted" errors and the instance dir is not deleted. Make sure you reset the instance before trying to remove it. There's some operations that can be started that wont let you remove the instance until they are cleaned up. But if you are still having issues with that, let me know. It could be a bug in the instance removal code in the kernel. -- Steve
On Wed, Mar 20, 2019 at 4:35 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 20 Mar 2019 14:14:03 +0000 > Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote: > > > > But this rmdir is not moved to tracecmd_remove_instance() — it’s lost. This looks like it’s introducing a bug. > > > > Thanks Matt, this one is from v1 of the patchset, I didn't notice it. > > I'll add the rmdir() call in the next version. > > There is some general problem with deleting instances, or (most > > probably) I cannot figure out what it the proper way > > to delete a ftrace instance - even after the call to rmdir() in > > tracecmd_remove_instance(), the instance dir is still there. > > I tried to run "rm -rf path_to_instance_dir" from the shell (at root), > > after the trace-cmd is executed, got a lot of > > "Operation not permitted" errors and the instance dir is not deleted. > > Make sure you reset the instance before trying to remove it. There's > some operations that can be started that wont let you remove the > instance until they are cleaned up. > > But if you are still having issues with that, let me know. It could be > a bug in the instance removal code in the kernel. I still get the issue, checked that the instance is set with default values. Attached is a simple script, which does the same - just run it as root > > -- Steve
On Thu, 21 Mar 2019 15:06:03 +0000 Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote: > #!/bin/bash > > INST_NAME=vtest > TRACE_INST_DIR=/sys/kernel/debug/tracing/instances > > mkdir $TRACE_INST_DIR/$INST_NAME > > echo "vmx_read_l1_tsc_offset" > $TRACE_INST_DIR/$INST_NAME/set_ftrace_filter > echo "function" > $TRACE_INST_DIR/$INST_NAME/current_tracer > echo "1" > $TRACE_INST_DIR/$INST_NAME/events/vsock/virtio_transport_recv_pkt/enable > echo "len!=0" > $TRACE_INST_DIR/$INST_NAME/events/vsock/virtio_transport_recv_pkt/filter > echo "mono_raw" > $TRACE_INST_DIR/$INST_NAME/trace_clock > echo "1" > $TRACE_INST_DIR/$INST_NAME/tracing_on > > sleep 2 > > echo "0" > $TRACE_INST_DIR/$INST_NAME/tracing_on > echo "" > $TRACE_INST_DIR/$INST_NAME/set_ftrace_filter > echo "nop" > $TRACE_INST_DIR/$INST_NAME/current_tracer > echo "0" > $TRACE_INST_DIR/$INST_NAME/events/vsock/virtio_transport_recv_pkt/enable > echo '0' > $TRACE_INST_DIR/$INST_NAME/events/vsock/virtio_transport_recv_pkt/filter > echo "local" > $TRACE_INST_DIR/$INST_NAME/trace_clock > > rm -rf $TRACE_INST_DIR/$INST_NAME Ah, that's because you are trying to remove the individual files. These are not real files, and can not be removed. But you can remove the directory. rmdir $TRACE_INST_DIR/$INST_NAME works as expected. -- Steve
diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h index d7bdb1f..8413054 100644 --- a/tracecmd/include/trace-local.h +++ b/tracecmd/include/trace-local.h @@ -235,6 +235,15 @@ void update_first_instance(struct buffer_instance *instance, int topt); void show_instance_file(struct buffer_instance *instance, const char *name); int count_cpus(void); +void write_tracing_on(struct buffer_instance *instance, int on); +char *get_instance_dir(struct buffer_instance *instance); +int write_instance_file(struct buffer_instance *instance, + const char *file, const char *str, const char *type); +void tracecmd_init_instance(struct buffer_instance *instance); +void tracecmd_make_instance(struct buffer_instance *instance); +int tracecmd_local_cpu_count(void); +void tracecmd_set_clock(struct buffer_instance *instance); +void tracecmd_remove_instance(struct buffer_instance *instance); /* No longer in event-utils.h */ void __noreturn die(const char *fmt, ...); /* Can be overriden */ diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c index 177060d..3950242 100644 --- a/tracecmd/trace-record.c +++ b/tracecmd/trace-record.c @@ -184,7 +184,7 @@ static inline int no_top_instance(void) return first_instance != &top_instance; } -static void init_instance(struct buffer_instance *instance) +void tracecmd_init_instance(struct buffer_instance *instance) { instance->event_next = &instance->events; } @@ -308,7 +308,7 @@ static void reset_save_file_cond(const char *file, int prio, */ void add_instance(struct buffer_instance *instance, int cpu_count) { - init_instance(instance); + tracecmd_init_instance(instance); instance->next = buffer_instances; if (first_instance == buffer_instances) first_instance = instance; @@ -495,7 +495,7 @@ static void add_event(struct buffer_instance *instance, struct event_list *event static void reset_event_list(struct buffer_instance *instance) { instance->events = NULL; - init_instance(instance); + tracecmd_init_instance(instance); } static char *get_temp_file(struct buffer_instance *instance, int cpu) @@ -791,8 +791,7 @@ get_instance_file(struct buffer_instance *instance, const char *file) return path; } -static char * -get_instance_dir(struct buffer_instance *instance) +char *get_instance_dir(struct buffer_instance *instance) { char *buf; char *path; @@ -1668,9 +1667,8 @@ static int write_file(const char *file, const char *str, const char *type) return ret; } -static int -write_instance_file(struct buffer_instance *instance, - const char *file, const char *str, const char *type) +int write_instance_file(struct buffer_instance *instance, + const char *file, const char *str, const char *type) { char *path; int ret; @@ -1946,7 +1944,7 @@ static int open_tracing_on(struct buffer_instance *instance) return fd; } -static void write_tracing_on(struct buffer_instance *instance, int on) +void write_tracing_on(struct buffer_instance *instance, int on) { int ret; int fd; @@ -2262,7 +2260,7 @@ void tracecmd_enable_events(void) enable_events(first_instance); } -static void set_clock(struct buffer_instance *instance) +void tracecmd_set_clock(struct buffer_instance *instance) { char *path; char *content; @@ -4316,49 +4314,57 @@ static void clear_func_filters(void) } } -static void make_instances(void) +void tracecmd_make_instance(struct buffer_instance *instance) { - struct buffer_instance *instance; struct stat st; char *path; int ret; + path = get_instance_dir(instance); + ret = stat(path, &st); + if (ret < 0) { + ret = mkdir(path, 0777); + if (ret < 0) + die("mkdir %s", path); + } else + /* Don't delete instances that already exist */ + instance->flags |= BUFFER_FL_KEEP; + tracecmd_put_tracing_file(path); + +} + +static void make_instances(void) +{ + struct buffer_instance *instance; + for_each_instance(instance) { if (is_guest(instance)) continue; + tracecmd_make_instance(instance); + } +} - path = get_instance_dir(instance); - ret = stat(path, &st); - if (ret < 0) { - ret = mkdir(path, 0777); - if (ret < 0) - die("mkdir %s", path); - } else - /* Don't delete instances that already exist */ - instance->flags |= BUFFER_FL_KEEP; - tracecmd_put_tracing_file(path); +void tracecmd_remove_instance(struct buffer_instance *instance) +{ + char *path; + + if (instance->tracing_on_fd > 0) { + close(instance->tracing_on_fd); + instance->tracing_on_fd = 0; } + path = get_instance_dir(instance); + tracecmd_put_tracing_file(path); } void tracecmd_remove_instances(void) { struct buffer_instance *instance; - char *path; - int ret; for_each_instance(instance) { /* Only delete what we created */ if (is_guest(instance) || (instance->flags & BUFFER_FL_KEEP)) continue; - if (instance->tracing_on_fd > 0) { - close(instance->tracing_on_fd); - instance->tracing_on_fd = 0; - } - path = get_instance_dir(instance); - ret = rmdir(path); - if (ret < 0) - die("rmdir %s", path); - tracecmd_put_tracing_file(path); + tracecmd_remove_instance(instance); } } @@ -4853,7 +4859,7 @@ void trace_stop(int argc, char **argv) int topt = 0; struct buffer_instance *instance = &top_instance; - init_instance(instance); + tracecmd_init_instance(instance); for (;;) { int c; @@ -4894,7 +4900,7 @@ void trace_restart(int argc, char **argv) int topt = 0; struct buffer_instance *instance = &top_instance; - init_instance(instance); + tracecmd_init_instance(instance); for (;;) { int c; @@ -4936,7 +4942,7 @@ void trace_reset(int argc, char **argv) int topt = 0; struct buffer_instance *instance = &top_instance; - init_instance(instance); + tracecmd_init_instance(instance); /* if last arg is -a, then -b and -d apply to all instances */ int last_specified_all = 0; @@ -5014,11 +5020,16 @@ static void init_common_record_context(struct common_record_context *ctx, memset(ctx, 0, sizeof(*ctx)); ctx->instance = &top_instance; ctx->curr_cmd = curr_cmd; - init_instance(ctx->instance); + tracecmd_init_instance(ctx->instance); local_cpu_count = count_cpus(); ctx->instance->cpu_count = local_cpu_count; } +int tracecmd_local_cpu_count(void) +{ + return local_cpu_count; +} + #define IS_EXTRACT(ctx) ((ctx)->curr_cmd == CMD_extract) #define IS_START(ctx) ((ctx)->curr_cmd == CMD_start) #define IS_STREAM(ctx) ((ctx)->curr_cmd == CMD_stream) @@ -5589,7 +5600,7 @@ static void record_trace(int argc, char **argv, tracecmd_disable_all_tracing(1); for_all_instances(instance) - set_clock(instance); + tracecmd_set_clock(instance); /* Record records the date first */ if (ctx->date &&
In order to reuse the code inside the trace-cmd application, few functions from trace-record.c are refactored: - make_instances() and tracecmd_remove_instances() are splited. New ones are created: tracecmd_make_instance() and tracecmd_remove_instance(), which are visible outside the trace-record.c - Following functions are made non-static: tracecmd_init_instance() get_instance_dir(), write_instance_file(), write_tracing_on(), tracecmd_set_clock() - New function is implemented: tracecmd_local_cpu_count(), an internal API to get local_cpu_count. Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com> --- tracecmd/include/trace-local.h | 9 ++++ tracecmd/trace-record.c | 87 +++++++++++++++++++--------------- 2 files changed, 58 insertions(+), 38 deletions(-)