Hi Jovi, I'll try to read this patch carefully tomorrow.
Looks fine at first glance, but some nits below. On 06/24, zhangwei(Jovi) wrote: > > static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs) > { > - if (!is_ret_probe(tu)) > - uprobe_trace_print(tu, 0, regs); > + struct event_file_link *link; > + > + if (is_ret_probe(tu)) > + return 0; > + > + rcu_read_lock(); > + > + list_for_each_entry(link, &tu->files, list) > + uprobe_trace_print(tu, 0, regs, link->file); > + > + rcu_read_unlock(); Purely cosmetic and I won't argue, but why the empty lines around list_for_each_entry() ? > 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; > > - if (is_trace_uprobe_enabled(tu)) > + /* > + * Currently TP_FLAG_TRACE/TP_FLAG_PROFILE are mutually exclusive > + * for uprobe(filter argument issue), this need to fix in future. > + */ > + if ((file && (tu->flags & TP_FLAG_PROFILE)) || > + (!file && (tu->flags & TP_FLAG_TRACE))) > return -EINTR; Well, this looks confusing and overcomplicated, see below. > + /* Currently we cannot call uprobe_register twice for same tu */ > + if (is_trace_uprobe_enabled(tu)) > + enabled = 1; The comment is wrong. It is not that we can't do this "Currently". We must not do uprobe_register(..., consumer) twice, consumer/uprobe are linked together. > + if (file) { > + struct event_file_link *link; > + Just add if (TP_FLAG_PROFILE) return -EINTR; here and kill the complicated check below. Same for the "else" branch. > +static void > +probe_event_disable(struct trace_uprobe *tu, struct ftrace_event_file *file) > +{ > + if (file) { > + struct event_file_link *link; > + > + link = find_event_file_link(tu, file); > + if (!link) > + return; > + > + list_del_rcu(&link->list); > + /* synchronize with uprobe_trace_func/uretprobe_trace_func */ > + synchronize_sched(); > + kfree(link); > + > + if (!list_empty(&tu->files)) > + return; > + > + tu->flags &= ~TP_FLAG_TRACE; > + } else > + tu->flags &= ~TP_FLAG_PROFILE; > + > > WARN_ON(!uprobe_filter_is_empty(&tu->filter)); > > - uprobe_unregister(tu->inode, tu->offset, &tu->consumer); > - tu->flags &= ~flag; > + if (!is_trace_uprobe_enabled(tu)) > + uprobe_unregister(tu->inode, tu->offset, &tu->consumer); Well, this is not exactly right... Currently this is fine, but still. It would be better to clear TP_FLAG_TRACE/TP_FLAG_PROFILE after uprobe_unregister(), when we can't race with the running handler which can check ->flags. And I'd suggest you to send the soft-enable/disable change in a separate (and trivial) patch. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/