lkml.org 
[lkml]   [2008]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC patch 01/12] Kernel Tracepoints
* Masami Hiramatsu (mhiramat@redhat.com) wrote:
> Hi Mathieu,
>
> Masami Hiramatsu wrote:
> [...]
> >> +int tracepoint_probe_register(const char *name, void *probe)
> >> +{
> >> + struct tracepoint_entry *entry;
> >> + int ret = 0;
> >> + void *old;
> >> +
> >> + mutex_lock(&tracepoints_mutex);
> >> + entry = get_tracepoint(name);
> >> + if (!entry) {
> >> + entry = add_tracepoint(name);
> >> + if (IS_ERR(entry)) {
> >> + ret = PTR_ERR(entry);
> >> + goto end;
> >> + }
> >> + }
> >> + /*
> >> + * If we detect that a call_rcu is pending for this tracepoint,
> >> + * make sure it's executed now.
> >> + */
> >> + if (entry->rcu_pending)
> >> + rcu_barrier();
> >> + old = tracepoint_entry_add_probe(entry, probe);
> >> + if (IS_ERR(old)) {
> >> + ret = PTR_ERR(old);
> >> + goto end;
> >> + }
> >> + mutex_unlock(&tracepoints_mutex);
> >> + tracepoint_update_probes(); /* may update entry */
> >> + mutex_lock(&tracepoints_mutex);
> >> + entry = get_tracepoint(name);
> >> + WARN_ON(!entry);
>
> As I said in another patch, you might have to check
> old != NULL here, because tracepoint_entry_add_probe() will
> return NULL when you add a first probe to the entry.
>
> >> + entry->oldptr = old;
> >> + entry->rcu_pending = 1;
> >> + /* write rcu_pending before calling the RCU callback */
> >> + smp_wmb();
> >> +#ifdef CONFIG_PREEMPT_RCU
> >> + synchronize_sched(); /* Until we have the call_rcu_sched() */
> >> +#endif
> >> + call_rcu(&entry->rcu, free_old_closure);
> >> +end:
> >> + mutex_unlock(&tracepoints_mutex);
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tracepoint_probe_register);
> >> +
> >> +/**
> >> + * tracepoint_probe_unregister - Disconnect a probe from a tracepoint
> >> + * @name: tracepoint name
> >> + * @probe: probe function pointer
> >> + *
> >> + * We do not need to call a synchronize_sched to make sure the probes have
> >> + * finished running before doing a module unload, because the module unload
> >> + * itself uses stop_machine(), which insures that every preempt disabled section
> >> + * have finished.
> >> + */
> >> +int tracepoint_probe_unregister(const char *name, void *probe)
> >> +{
> >> + struct tracepoint_entry *entry;
> >> + void *old;
> >> + int ret = -ENOENT;
> >> +
> >> + mutex_lock(&tracepoints_mutex);
> >> + entry = get_tracepoint(name);
> >> + if (!entry)
> >> + goto end;
> >> + if (entry->rcu_pending)
> >> + rcu_barrier();
> >> + old = tracepoint_entry_remove_probe(entry, probe);
> >> + mutex_unlock(&tracepoints_mutex);
> >> + tracepoint_update_probes(); /* may update entry */
> >> + mutex_lock(&tracepoints_mutex);
> >> + entry = get_tracepoint(name);
> >> + if (!entry)
> >> + goto end;
> >> + entry->oldptr = old;
> >> + entry->rcu_pending = 1;
> >> + /* write rcu_pending before calling the RCU callback */
> >> + smp_wmb();
> >> +#ifdef CONFIG_PREEMPT_RCU
> >> + synchronize_sched(); /* Until we have the call_rcu_sched() */
> >> +#endif
> >> + call_rcu(&entry->rcu, free_old_closure);
> >> + remove_tracepoint(name); /* Ignore busy error message */
> >> + ret = 0;
> >> +end:
> >> + mutex_unlock(&tracepoints_mutex);
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(tracepoint_probe_unregister);
> >> +
>
> On the other hand, tracepoint_entry_remove_probe() doesn't return NULL,
> however, I think it might be better to introduce tracepoint_entry_free_old()
> and simplify both of tracepoint_probe_register/unregister.
>

Yes, the cleanup makes sense and removes an unnecessary call to call_rcu
(which in fact simply kfree a NULL pointer, which is pointless). I'll
integrate this change.

Thanks,

Mathieu

> Thank you,
> --
> Masami Hiramatsu
>
> Software Engineer
> Hitachi Computer Products (America) Inc.
> Software Solutions Division
>
> e-mail: mhiramat@redhat.com
>

--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68


\
 
 \ /
  Last update: 2008-07-09 05:05    [W:0.258 / U:0.788 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site