Messages in this thread | | | Date | Fri, 14 Jun 2013 16:44:42 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] tracing/uprobes: Support ftrace_event_file base multibuffer |
| |
On 06/14, zhangwei(Jovi) wrote: > > Support multi-buffer on uprobe-based dynamic events by > using ftrace_event_file. > > The code change is based on kprobe-based dynamic events > multibuffer support work commited by Masami(commit 41a7dd420c)
And the change in probe_event_enable() doesn't look right, but let me repeat I didn't read the patch carefully yet.
> +static DEFINE_MUTEX(uprobe_enable_lock); > + > static inline bool is_trace_uprobe_enabled(struct trace_uprobe *tu) > { > return tu->flags & (TP_FLAG_TRACE | TP_FLAG_PROFILE); > @@ -607,33 +655,123 @@ typedef bool (*filter_func_t)(struct uprobe_consumer *self, > struct mm_struct *mm); > > static int > -probe_event_enable(struct trace_uprobe *tu, int flag, filter_func_t filter) > +probe_event_enable(struct trace_uprobe *tu, struct ftrace_event_file *file, > + filter_func_t filter) > { > + int enabled = 0; > int ret = 0; > > + mutex_lock(&uprobe_enable_lock);
Do we really need this? Can't we really on mutex_event hold by the caller?
> if (is_trace_uprobe_enabled(tu)) > - return -EINTR; > + enabled = 1; > + > + if (file) { > + struct ftrace_event_file **new, **old; > + int n = trace_uprobe_nr_files(tu); > + > + old = rcu_dereference_raw(tu->files); > + /* 1 is for new one and 1 is for stopper */ > + new = kzalloc((n + 2) * sizeof(struct ftrace_event_file *), > + GFP_KERNEL); > + if (!new) { > + ret = -ENOMEM; > + goto out_unlock; > + } > + memcpy(new, old, n * sizeof(struct ftrace_event_file *)); > + new[n] = file; > + /* The last one keeps a NULL */ > + > + rcu_assign_pointer(tu->files, new); > + tu->flags |= TP_FLAG_TRACE; > + > + if (old) { > + /* Make sure the probe is done with old files */ > + synchronize_sched(); > + kfree(old); > + } > + } else > + tu->flags |= TP_FLAG_PROFILE;
So it can set both TP_FLAG_TRACE and TP_FLAG_PROFILE, yes?
If yes, this is not right. Until we change the pre-filtering at least. Currently TP_FLAG_TRACE/TP_FLAG_PROFILE are mutually exclusive.
I think it makes sense to remove this limitation anyway, and in fact I do not remember why I didn't do this... But this needs a separate change.
Oleg.
| |