Messages in this thread | | | Date | Wed, 26 Aug 2009 14:17:58 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload |
| |
* Mathieu Desnoyers (mathieu.desnoyers@polymtl.ca) wrote: > * Steven Rostedt (rostedt@goodmis.org) wrote: > > > > This patch solves the problem that Li originally reported. If something > > registers a trace point belonging to a module, then it ups the ref count > > of the module. This prevents a process from registering a probe to a > > tracepoint belonging to a module and then having the module disappear. > > > > Doing the example with perf in Li's original post, now errors on the > > rmmod, with "ERROR: Module trace_events_sample is in use". > > > > Mathieu, can I have your acked-by on this? > > > > Sorry, it looks buggy. > > It does not deal with the fact that tracepoints with the same name and > arguments can be present in more than one module, or in a combination of > kernel core and modules. > > The struct tracepoint_entry is specific to a a tracepoint name, used for > registration, but is eventually tied to all tracepoint instrumentation > instances for this tracepoint name. >
Looking at the original post: http://lkml.org/lkml/2009/8/24/5
the problem seems to be caused by the fact that the trace_event_profile.c keeps some knowledge of modules in internal data structures, but does not get notified of module unloads. Why don't we fix that instead ?
A quick glance at it seems to indicate that it lazily discovers new modules when the tracepoints are hit. Using module load/unload notifiers would be more appropriate. Or maybe adding a notifier call to tracepoint.c, calling notification callbacks for probe modules which need to know when the connected tracepoints are changing (when they are connected/disconnected) would probably be even more appropriate. As a result, it would remove the dynamic verification cost implied by lazy data structure lookup and check each time the probe is fired.
Mathieu
> Mathieu > > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org> > > > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > > index 0341f2e..055275b 100644 > > --- a/include/linux/tracepoint.h > > +++ b/include/linux/tracepoint.h > > @@ -109,8 +109,9 @@ struct tracepoint { > > #define EXPORT_TRACEPOINT_SYMBOL(name) \ > > EXPORT_SYMBOL(__tracepoint_##name) > > > > -extern void tracepoint_update_probe_range(struct tracepoint *begin, > > - struct tracepoint *end); > > +extern void tracepoint_update_probe_range(struct module *, > > + struct tracepoint *begin, > > + struct tracepoint *end); > > > > #else /* !CONFIG_TRACEPOINTS */ > > #define DECLARE_TRACE_WITH_CALLBACK(name, proto, args, reg, unreg) \ > > diff --git a/kernel/module.c b/kernel/module.c > > index b182143..a8e69fa 100644 > > --- a/kernel/module.c > > +++ b/kernel/module.c > > @@ -2974,7 +2974,7 @@ void module_update_tracepoints(void) > > mutex_lock(&module_mutex); > > list_for_each_entry(mod, &modules, list) > > if (!mod->taints) > > - tracepoint_update_probe_range(mod->tracepoints, > > + tracepoint_update_probe_range(mod, mod->tracepoints, > > mod->tracepoints + mod->num_tracepoints); > > mutex_unlock(&module_mutex); > > } > > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > > index 06f165a..089e6f9 100644 > > --- a/kernel/tracepoint.c > > +++ b/kernel/tracepoint.c > > @@ -54,6 +54,7 @@ static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; > > */ > > struct tracepoint_entry { > > struct hlist_node hlist; > > + struct module *mod; > > void **funcs; > > int refcount; /* Number of times armed. 0 if disarmed. */ > > char name[0]; > > @@ -221,6 +222,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name) > > memcpy(&e->name[0], name, name_len); > > e->funcs = NULL; > > e->refcount = 0; > > + e->mod = NULL; /* Will be assigned in tracepoint_update_probe_range */ > > hlist_add_head(&e->hlist, head); > > return e; > > } > > @@ -231,6 +233,8 @@ static struct tracepoint_entry *add_tracepoint(const char *name) > > */ > > static inline void remove_tracepoint(struct tracepoint_entry *e) > > { > > + if (e->mod) > > + module_put(e->mod); > > hlist_del(&e->hlist); > > kfree(e); > > } > > @@ -274,7 +278,8 @@ static void disable_tracepoint(struct tracepoint *elem) > > * Updates the probe callback corresponding to a range of tracepoints. > > */ > > void > > -tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end) > > +tracepoint_update_probe_range(struct module *mod, > > + struct tracepoint *begin, struct tracepoint *end) > > { > > struct tracepoint *iter; > > struct tracepoint_entry *mark_entry; > > @@ -286,9 +291,15 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end) > > for (iter = begin; iter < end; iter++) { > > mark_entry = get_tracepoint(iter->name); > > if (mark_entry) { > > + if (mod && !mark_entry->mod) { > > + if (!try_module_get(mod)) > > + goto disable; > > + mark_entry->mod = mod; > > + } > > set_tracepoint(&mark_entry, iter, > > !!mark_entry->refcount); > > } else { > > + disable: > > disable_tracepoint(iter); > > } > > } > > @@ -301,7 +312,7 @@ tracepoint_update_probe_range(struct tracepoint *begin, struct tracepoint *end) > > static void tracepoint_update_probes(void) > > { > > /* Core kernel tracepoints */ > > - tracepoint_update_probe_range(__start___tracepoints, > > + tracepoint_update_probe_range(NULL, __start___tracepoints, > > __stop___tracepoints); > > /* tracepoints in modules. */ > > module_update_tracepoints(); > > @@ -556,7 +567,7 @@ int tracepoint_module_notify(struct notifier_block *self, > > switch (val) { > > case MODULE_STATE_COMING: > > case MODULE_STATE_GOING: > > - tracepoint_update_probe_range(mod->tracepoints, > > + tracepoint_update_probe_range(mod, mod->tracepoints, > > mod->tracepoints + mod->num_tracepoints); > > break; > > } > > > > > > -- > Mathieu Desnoyers > OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
-- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
| |