(2013/06/17 2:21), Oleg Nesterov wrote: > I think that "ftrace_event_file *trace_probe[]" complicates the > code for no reason, turn it into list_head to simplify the code. > enable_trace_probe() no longer needs synchronize_sched().
Looks cleaner :) > > This needs the extra sizeof(list_head) memory for every attached > ftrace_event_file, hopefully not a problem in this case. I think it's no problem, because the number depends on the instances and it could not be so much. :) Thanks! Acked-by: Masami Hiramatsu <[email protected]> > > Signed-off-by: Oleg Nesterov <[email protected]> > --- > kernel/trace/trace_kprobe.c | 138 > ++++++++++++------------------------------- > 1 files changed, 37 insertions(+), 101 deletions(-) > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 5a73de0..b95f683 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -35,12 +35,17 @@ struct trace_probe { > const char *symbol; /* symbol name */ > struct ftrace_event_class class; > struct ftrace_event_call call; > - struct ftrace_event_file * __rcu *files; > + struct list_head files; > ssize_t size; /* trace entry size */ > unsigned int nr_args; > struct probe_arg args[]; > }; > > +struct event_file_link { > + struct ftrace_event_file *file; > + struct list_head list; > +}; > + > #define SIZEOF_TRACE_PROBE(n) \ > (offsetof(struct trace_probe, args) + \ > (sizeof(struct probe_arg) * (n))) > @@ -150,6 +155,7 @@ static struct trace_probe *alloc_trace_probe(const char > *group, > goto error; > > INIT_LIST_HEAD(&tp->list); > + INIT_LIST_HEAD(&tp->files); > return tp; > error: > kfree(tp->call.name); > @@ -184,22 +190,6 @@ static struct trace_probe *find_trace_probe(const char > *event, > } > > /* > - * This and enable_trace_probe/disable_trace_probe rely on event_mutex > - * held by the caller, __ftrace_set_clr_event(). > - */ > -static int trace_probe_nr_files(struct trace_probe *tp) > -{ > - struct ftrace_event_file **file = rcu_dereference_raw(tp->files); > - int ret = 0; > - > - if (file) > - while (*(file++)) > - ret++; > - > - return ret; > -} > - > -/* > * Enable trace_probe > * if the file is NULL, enable "perf" handler, or enable "trace" handler. > */ > @@ -209,29 +199,18 @@ enable_trace_probe(struct trace_probe *tp, struct > ftrace_event_file *file) > int ret = 0; > > if (file) { > - struct ftrace_event_file **new, **old; > - int n = trace_probe_nr_files(tp); > - > - old = rcu_dereference_raw(tp->files); > - /* 1 is for new one and 1 is for stopper */ > - new = kzalloc((n + 2) * sizeof(struct ftrace_event_file *), > - GFP_KERNEL); > - if (!new) { > + struct event_file_link *link; > + > + link = kmalloc(sizeof(*link), GFP_KERNEL); > + if (!link) { > ret = -ENOMEM; > goto out; > } > - memcpy(new, old, n * sizeof(struct ftrace_event_file *)); > - new[n] = file; > - /* The last one keeps a NULL */ > > - rcu_assign_pointer(tp->files, new); > - tp->flags |= TP_FLAG_TRACE; > + link->file = file; > + list_add_rcu(&link->list, &tp->files); > > - if (old) { > - /* Make sure the probe is done with old files */ > - synchronize_sched(); > - kfree(old); > - } > + tp->flags |= TP_FLAG_TRACE; > } else > tp->flags |= TP_FLAG_PROFILE; > > @@ -246,24 +225,16 @@ enable_trace_probe(struct trace_probe *tp, struct > ftrace_event_file *file) > return ret; > } > > -static int > -trace_probe_file_index(struct trace_probe *tp, struct ftrace_event_file > *file) > +static struct event_file_link * > +find_event_file_link(struct trace_probe *tp, struct ftrace_event_file *file) > { > - struct ftrace_event_file **files; > - int i; > + struct event_file_link *link; > > - /* > - * Since all tp->files updater is protected by probe_enable_lock, > - * we don't need to lock an rcu_read_lock. > - */ > - files = rcu_dereference_raw(tp->files); > - if (files) { > - for (i = 0; files[i]; i++) > - if (files[i] == file) > - return i; > - } > + list_for_each_entry(link, &tp->files, list) > + if (link->file == file) > + return link; > > - return -1; > + return NULL; > } > > /* > @@ -276,38 +247,23 @@ disable_trace_probe(struct trace_probe *tp, struct > ftrace_event_file *file) > int ret = 0; > > if (file) { > - struct ftrace_event_file **new, **old; > - int n = trace_probe_nr_files(tp); > - int i, j; > + struct event_file_link *link; > > - old = rcu_dereference_raw(tp->files); > - if (n == 0 || trace_probe_file_index(tp, file) < 0) { > + link = find_event_file_link(tp, file); > + if (!link) { > ret = -EINVAL; > goto out; > } > > - if (n == 1) { /* Remove the last file */ > - tp->flags &= ~TP_FLAG_TRACE; > - new = NULL; > - } else { > - new = kzalloc(n * sizeof(struct ftrace_event_file *), > - GFP_KERNEL); > - if (!new) { > - ret = -ENOMEM; > - goto out; > - } > - > - /* This copy & check loop copies the NULL stopper too */ > - for (i = 0, j = 0; j < n && i < n + 1; i++) > - if (old[i] != file) > - new[j++] = old[i]; > - } > + list_del_rcu(&link->list); > + /* synchronize with kprobe_trace_func/kretprobe_trace_func */ > + synchronize_sched(); > + kfree(link); > > - rcu_assign_pointer(tp->files, new); > + if (!list_empty(&tp->files)) > + goto out; > > - /* Make sure the probe is done with old files */ > - synchronize_sched(); > - kfree(old); > + tp->flags &= ~TP_FLAG_TRACE; > } else > tp->flags &= ~TP_FLAG_PROFILE; > > @@ -872,20 +828,10 @@ __kprobe_trace_func(struct trace_probe *tp, struct > pt_regs *regs, > static __kprobes void > kprobe_trace_func(struct trace_probe *tp, struct pt_regs *regs) > { > - /* > - * Note: preempt is already disabled around the kprobe handler. > - * However, we still need an smp_read_barrier_depends() corresponding > - * to smp_wmb() in rcu_assign_pointer() to access the pointer. > - */ > - struct ftrace_event_file **file = rcu_dereference_raw(tp->files); > - > - if (unlikely(!file)) > - return; > + struct event_file_link *link; > > - while (*file) { > - __kprobe_trace_func(tp, regs, *file); > - file++; > - } > + list_for_each_entry_rcu(link, &tp->files, list) > + __kprobe_trace_func(tp, regs, link->file); > } > > /* Kretprobe handler */ > @@ -932,20 +878,10 @@ static __kprobes void > kretprobe_trace_func(struct trace_probe *tp, struct kretprobe_instance *ri, > struct pt_regs *regs) > { > - /* > - * Note: preempt is already disabled around the kprobe handler. > - * However, we still need an smp_read_barrier_depends() corresponding > - * to smp_wmb() in rcu_assign_pointer() to access the pointer. > - */ > - struct ftrace_event_file **file = rcu_dereference_raw(tp->files); > - > - if (unlikely(!file)) > - return; > + struct event_file_link *link; > > - while (*file) { > - __kretprobe_trace_func(tp, ri, regs, *file); > - file++; > - } > + list_for_each_entry_rcu(link, &tp->files, list) > + __kretprobe_trace_func(tp, ri, regs, link->file); > } > > /* Event entry printers */ > -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: [email protected] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
