Markers fixes
cover Markers fixes
Hi Ingo,
Here are the marker fixes which reenable batch markers registration/unregistration. They apply to -tip.
Thanks,
Mathieu
Signature · 3 lines
Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
PATCH 1/5 Markers reenable fast batch registration
Lai Jiangshan discovered a reentrancy issue with markers and fixed it by adding synchronize_sched() calls at each registration/unregistraiton. It works, but it removes the ability to do batch registration/unregistration and can cause registration of ~100 markers to take about 30 seconds on a loaded machine (synchronize_sched() is much slower on such workloads). This patch implements a version of the fix which won't slow down marker batch registration/unregistration. It also go back to the original non-synchronized reg/unreg, and thus needs the following markers-synchronize_marker_unregister.patch patch and friends.
--- kernel/marker.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 6 deletions(-)
kernel/marker.c +52 -6
@@ -60,6 +60,9 @@ struct marker_entry {struct marker_probe_closure single;struct marker_probe_closure *multi;int refcount;/* Number of times armed. 0 if disarmed. */ +struct rcu_head rcu; +void *oldptr; + unsigned char rcu_pending:1; unsigned char ptype:1;char name[0 ];/* Contains name'\0'format'\0' */ };
@@ -196,6 +199,16 @@ void marker_probe_cb_noarg(const struct }EXPORT_SYMBOL_GPL (marker_probe_cb_noarg); +static void free_old_closure (struct rcu_head *head) +{ +struct marker_entry *entry =container_of (head, + struct marker_entry, rcu); +kfree (entry->oldptr); +/* Make sure we free the data before setting the pending flag to 0 */ +smp_wmb (); + entry->rcu_pending =0 ; +} +static void debug_print_probes (struct marker_entry *entry) {int i;
@@ -404,6 +417,7 @@ static struct marker_entry *add_marker(c e->multi =NULL ; e->ptype =0 ; e->refcount =0 ; + e->rcu_pending =0 ;hlist_add_head (&e->hlist, head);return e; }
@@ -433,6 +447,9 @@ static int remove_marker(const char *namif (e->single.func != __mark_empty_function)return -EBUSY ;hlist_del (&e->hlist); +/* Make sure the call_rcu has been executed */ +if (e->rcu_pending) +rcu_barrier_sched ();kfree (e);return 0 ; }
@@ -462,8 +479,12 @@ static int marker_set_format(struct mark e->multi = (*entry)->multi; e->ptype = (*entry)->ptype; e->refcount = (*entry)->refcount; + e->rcu_pending =0 ;hlist_add_before (&e->hlist, &(*entry)->hlist);hlist_del (&(*entry)->hlist); +/* Make sure the call_rcu has been executed */ +if ((*entry)->rcu_pending) +rcu_barrier_sched ();kfree (*entry); *entry = e; trace_mark(core_marker_format, "name %s format %s",
@@ -637,6 +658,12 @@ int marker_probe_register(const char *na goto end; } } +/* +* If we detect that a call_rcu is pending for this marker, +* make sure it's executed now. +*/ +if (entry->rcu_pending) +rcu_barrier_sched (); old =marker_entry_add_probe (entry, probe, probe_private);if (IS_ERR (old)) { ret =PTR_ERR (old);
@@ -644,11 +671,16 @@ int marker_probe_register(const char *na }mutex_unlock (&markers_mutex);marker_update_probes ();/* may update entry */ -synchronize_sched (); -kfree (old);mutex_lock (&markers_mutex); entry =get_marker (name);WARN_ON (!entry); +if (entry->rcu_pending) +rcu_barrier_sched (); + entry->oldptr = old; + entry->rcu_pending =1 ; +/* write rcu_pending before calling the RCU callback */ +smp_wmb (); +call_rcu_sched (&entry->rcu, free_old_closure); end:mutex_unlock (&markers_mutex);return ret;
@@ -678,15 +710,22 @@ int marker_probe_unregister(const char * entry =get_marker (name);if (!entry) goto end; +if (entry->rcu_pending) +rcu_barrier_sched (); old =marker_entry_remove_probe (entry, probe, probe_private);mutex_unlock (&markers_mutex);marker_update_probes ();/* may update entry */ -synchronize_sched (); -kfree (old);mutex_lock (&markers_mutex); entry =get_marker (name);if (!entry) goto end; +if (entry->rcu_pending) +rcu_barrier_sched (); + entry->oldptr = old; + entry->rcu_pending =1 ; +/* write rcu_pending before calling the RCU callback */ +smp_wmb (); +call_rcu_sched (&entry->rcu, free_old_closure);remove_marker (name);/* Ignore busy error message */ ret =0 ; end:
@@ -752,14 +791,21 @@ int marker_probe_unregister_private_data ret = -ENOENT ; goto end; } +if (entry->rcu_pending) +rcu_barrier_sched (); old =marker_entry_remove_probe (entry,NULL , probe_private);mutex_unlock (&markers_mutex);marker_update_probes ();/* may update entry */ -synchronize_sched (); -kfree (old);mutex_lock (&markers_mutex); entry =get_marker_from_private_data (probe, probe_private);WARN_ON (!entry); +if (entry->rcu_pending) +rcu_barrier_sched (); + entry->oldptr = old; + entry->rcu_pending =1 ; +/* write rcu_pending before calling the RCU callback */ +smp_wmb (); +call_rcu_sched (&entry->rcu, free_old_closure);remove_marker (entry->name);/* Ignore busy error message */ end:mutex_unlock (&markers_mutex);
Signature · 3 lines
Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
PATCH 2/5 Markers : marker_synchronize_unregister()
In reply to Mathieu DesnoyersCreate marker_synchronize_unregister() which must be called before the end of exit() to make sure every probe callers have exited the non preemptible section and thus are not executing the probe code anymore.
--- include/linux/marker.h | 7 +++++++ 1 file changed, 7 insertions(+)
include/linux/marker.h +7 -0
@@ -142,4 +142,11 @@ extern int marker_probe_unregister_privaextern void *marker_get_private_data (const char *name,marker_probe_func *probe,int num); +/* +* marker_synchronize_unregister must be called between the last marker probe +* unregistration and the end of module exit to make sure there is no caller +* executing a probe when it is freed. +*/ +#define marker_synchronize_unregister () synchronize_sched() +#endif
Signature · 3 lines
Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
PATCH 3/5 Markers : probe example fix teardown
In reply to Mathieu DesnoyersNeed a marker_synchronize_unregister() before the end of exit() to make sure every probe callers have exited the non preemptible section and thus are not executing the probe code anymore.
--- samples/markers/probe-example.c | 1 + 1 file changed, 1 insertion(+)
samples/markers/probe-example.c +1 -0
@@ -81,6 +81,7 @@ static void __exit probe_fini(void) probe_array[i].probe_func, &probe_array[i]);printk (KERN_INFO "Number of event b : %u\n" ,atomic_read (&eventb_count)); +marker_synchronize_unregister (); }module_init (probe_init);
Signature · 3 lines
Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
PATCH 4/5 Markers : documentation fix teardown
In reply to Mathieu DesnoyersDocument the need for a marker_synchronize_unregister() before the end of exit() to make sure every probe callers have exited the non preemptible section and thus are not executing the probe code anymore.
--- Documentation/markers.txt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Documentation/markers.txt +6 -4
@@ -50,10 +50,12 @@ Connecting a function (probe) to a marke to call) for the specific marker through marker_probe_register() and can be activated by calling marker_arm(). Marker deactivation can be done by calling marker_disarm() as many times as marker_arm() has been called. Removing a probe -is done through marker_probe_unregister(); it will disarm the probe and make -sure there is no caller left using the probe when it returns. Probe removal is -preempt-safe because preemption is disabled around the probe call. See the -"Probe example" section below for a sample probe module. +is done through marker_probe_unregister(); it will disarm the probe. +marker_synchronize_unregister() must be called before the end of the module exit +function to make sure there is no caller left using the probe. This, and the +fact that preemption is disabled around the probe call, make sure that probe +removal and module unload are safe. See the "Probe example" section below for a +sample probe module. The marker mechanism supports inserting multiple instances of the same marker. Markers can be put in inline functions, inlined static functions, and
Signature · 3 lines
Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
PATCH 5/5 sputrace : use marker_synchronize_unregister()
In reply to Mathieu DesnoyersWe need a marker_synchronize_unregister() before the end of exit() to make sure every probe callers have exited the non preemptible section and thus are not executing the probe code anymore.
--- arch/powerpc/platforms/cell/spufs/sputrace.c | 1 + 1 file changed, 1 insertion(+)
arch/powerpc/platforms/cell/spufs/sputrace.c +1 -0
@@ -233,6 +233,7 @@ static void __exit sputrace_exit(void)remove_proc_entry ("sputrace" ,NULL );kfree (sputrace_log); +marker_synchronize_unregister (); }module_init (sputrace_init);
Signature · 3 lines
Mathieu Desnoyers Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Quoted 4 lines · Mathieu Desnoyers
Hi Ingo,
Here are the marker fixes which reenable batch markers registration/unregistration. They apply to -tip.
hm, only the first patch applies to tip/master - the rest is already applied. So i applied the first patch to tip/tracing/markers:
# a2b4a6c: markers: re-enable fast batch registration
Could you please check latest tip/master:
http://people.redhat.com/mingo/tip.git/README
to make sure that i sorted them out correctly. Thanks,
Ingo