markers: fix unregister bug and reenter bug

PATCH Applied Lai Jiangshan 6 participants 25 messages Sep 29, 2008 – Oct 3, 2008 1 message not in the archive

Subsystems (5)

Documentation PowerPC RCU Cell broadband engine architecture +1 more subsystems

lore

Share & tools

lore

Lai Jiangshan

author

2008-09-29 08:03 UTC #0 lore raw

PATCH 1/1 markers: fix unregister bug and reenter bug

unregister bug:

codes using makers are typically calling marker_probe_unregister() and then destroying the data that marker_probe_func needs(or unloading this module). This is bug when the corresponding marker_probe_func is still running(on other cpus),
it is using the destroying/ed data.

we should call synchronize_sched() after marker_update_probes().

reenter bug:

marker_probe_register(), marker_probe_unregister() and marker_probe_unregister_private_data() are not reentrant safe functions. these 3 functions release markers_mutex and then require it again and do "entry->oldptr = old; ...", but entry->oldptr maybe is using now for these 3 functions may reenter when markers_mutex is released.

we use synchronize_sched() instead of call_rcu_sched() to fix this bug. actually we can do:
"
if (entry->rcu_pending)

		rcu_barrier_sched();

"
after require markers_mutex again. but synchronize_sched() is better and simpler. For these 3 functions are not critical path.

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/marker.c +6 ~~-46~~

index 7d1faec..9f76c4a 100644
@@ -60,9 +60,6 @@ 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' */
 };

@@ -199,16 +196,6 @@ void marker_probe_cb_noarg(const struct marker *mdata, void *call_private, ...)
 }
 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;

@@ -417,7 +404,6 @@ static struct marker_entry *add_marker(const char *name, const char *format)
 	e->multi = NULL;
 	e->ptype = 0;
 	e->refcount = 0;
-	e->rcu_pending = 0;
 	hlist_add_head(&e->hlist, head);
 	return e;
 }

@@ -447,9 +433,6 @@ static int remove_marker(const char *name)
 	if (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;
 }

@@ -479,12 +462,8 @@ static int marker_set_format(struct marker_entry **entry, const char *format)
 	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",

@@ -658,12 +637,6 @@ int marker_probe_register(const char *name, const char *format,
 			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);

@@ -671,14 +644,11 @@ int marker_probe_register(const char *name, const char *format,
 	}
 	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);
-	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;

@@ -708,20 +678,15 @@ int marker_probe_unregister(const char *name,
 	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;
-	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:

@@ -787,19 +752,14 @@ int marker_probe_unregister_private_data(marker_probe_func *probe,
 		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);
-	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);

Ingo Molnar

Applied

2008-09-29 08:28 UTC #1 lore raw

Quoted 23 lines · Lai Jiangshan

unregister bug:
codes using makers are typically calling marker_probe_unregister() and then destroying the data that marker_probe_func needs(or unloading this module). This is bug when the corresponding marker_probe_func is still running(on other cpus),
it is using the destroying/ed data.
we should call synchronize_sched() after marker_update_probes().
reenter bug:
marker_probe_register(), marker_probe_unregister() and marker_probe_unregister_private_data() are not reentrant safe functions. these 3 functions release markers_mutex and then require it again and do "entry->oldptr = old; ...", but entry->oldptr maybe is using now for these 3 functions may reenter when markers_mutex is released.
we use synchronize_sched() instead of call_rcu_sched() to fix this bug. actually we can do:
"
if (entry->rcu_pending)

Quoted 6 lines

		rcu_barrier_sched();
"
after require markers_mutex again. but synchronize_sched() is better and simpler. For these 3 functions are not critical path.
Signed-off-by Lai Jiangshan <[email protected]>

applied to tip/tracing/markers, thanks!

Mathieu, i've reactivated tip/tracing/markers for linux-next integration, it propagates into auto-ftrace-next. Tracepoints are what we use in the scheduler/etc. in tip/master, but until there's marker use elsewhere there's no reason not to apply fix patches.

Ingo

Mathieu Desnoyers

2008-09-29 15:04 UTC #2 lore raw

Quoted 31 lines · * Ingo Molnar ([email protected]) wrote:

Quoted 28 lines · Lai Jiangshan
unregister bug:
codes using makers are typically calling marker_probe_unregister() and then destroying the data that marker_probe_func needs(or unloading this module). This is bug when the corresponding marker_probe_func is still running(on other cpus),
it is using the destroying/ed data.
we should call synchronize_sched() after marker_update_probes().
reenter bug:
marker_probe_register(), marker_probe_unregister() and marker_probe_unregister_private_data() are not reentrant safe functions. these 3 functions release markers_mutex and then require it again and do "entry->oldptr = old; ...", but entry->oldptr maybe is using now for these 3 functions may reenter when markers_mutex is released.
we use synchronize_sched() instead of call_rcu_sched() to fix this bug. actually we can do:
"
if (entry->rcu_pending)
		rcu_barrier_sched();
"
after require markers_mutex again. but synchronize_sched() is better and simpler. For these 3 functions are not critical path.

Quoted 9 lines

Quoted 1 line
Signed-off-by Lai Jiangshan <[email protected]>
applied to tip/tracing/markers, thanks!
Mathieu, i've reactivated tip/tracing/markers for linux-next integration, it propagates into auto-ftrace-next. Tracepoints are what we use in the scheduler/etc. in tip/master, but until there's marker use elsewhere there's no reason not to apply fix patches.

nack for this patch. I'll my fix patchset.

Mathieu

Quoted 1 line

Ingo

Signature · 2 lines

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

Mathieu Desnoyers

2008-09-29 15:06 UTC #3 lore raw

PATCH 1/1 RCU read sched

In reply to Ingo Molnar

(probably already in -tip)

Add rcu_read_lock_sched() and rcu_read_unlock_sched() to rcupdate.h to match the recently added write-side call_rcu_sched() and rcu_barrier_sched(). They also match the no-so-recently-added synchronize_sched().

It will help following matching use of the update/read lock primitives. Those new read lock will replace preempt_disable()/enable() used in pair with RCU-classic synchronization.

Signed-off-by Mathieu Desnoyers <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

CC Paul E McKenney <[email protected]>

CC [email protected]

---
 include/linux/rcupdate.h |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

include/linux/rcupdate.h +20 -0

Index: linux-2.6-lttng/include/linux/rcupdate.h
===================================================================
@@ -132,6 +132,26 @@ struct rcu_head {
 #define rcu_read_unlock_bh() __rcu_read_unlock_bh()
 
 /**
+ * rcu_read_lock_sched - mark the beginning of a RCU-classic critical section
+ *
+ * Should be used with either
+ * - synchronize_sched()
+ * or
+ * - call_rcu_sched() and rcu_barrier_sched()
+ * on the write-side to insure proper synchronization.
+ */
+#define rcu_read_lock_sched() preempt_disable()
+
+/*
+ * rcu_read_unlock_sched - marks the end of a RCU-classic critical section
+ *
+ * See rcu_read_lock_sched for more information.
+ */
+#define rcu_read_unlock_sched() preempt_enable()
+
+
+
+/**
  * rcu_dereference - fetch an RCU-protected pointer in an
  * RCU read-side critical section.  This pointer may later
  * be safely dereferenced.

Signature · 2 lines

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

Ingo Molnar

2008-09-30 10:09 UTC #4 lore raw

Re: [PATCH] RCU read sched

Quoted 6 lines · Mathieu Desnoyers

(probably already in -tip)
Add rcu_read_lock_sched() and rcu_read_unlock_sched() to rcupdate.h to match the recently added write-side call_rcu_sched() and rcu_barrier_sched(). They also match the no-so-recently-added synchronize_sched().

Quoted 7 lines

It will help following matching use of the update/read lock primitives. Those new read lock will replace preempt_disable()/enable() used in pair with RCU-classic synchronization.
Signed-off-by Mathieu Desnoyers <[email protected]>
Acked-by Peter Zijlstra <[email protected]>
CC Paul E McKenney <[email protected]>

Paul, does this API extension look good to you?

i'll apply it to tip/core/rcu if yes.

Ingo

Paul E. McKenney

2008-09-30 13:12 UTC #5 lore raw

Re: [PATCH] RCU read sched

Quoted 13 lines · Ingo Molnar

Quoted 10 lines · Mathieu Desnoyers
(probably already in -tip)
Add rcu_read_lock_sched() and rcu_read_unlock_sched() to rcupdate.h to match the recently added write-side call_rcu_sched() and rcu_barrier_sched(). They also match the no-so-recently-added synchronize_sched().
It will help following matching use of the update/read lock primitives. Those new read lock will replace preempt_disable()/enable() used in pair with RCU-classic synchronization.

Quoted 5 lines

Quoted 3 lines
Signed-off-by Mathieu Desnoyers <[email protected]>
Acked-by Peter Zijlstra <[email protected]>
CC Paul E McKenney <[email protected]>
Paul, does this API extension look good to you?

Yep!!!

Quoted 1 line

i'll apply it to tip/core/rcu if yes.

Please do!

Thanx, Paul

Paul E. McKenney

Reviewed-by on v1 1/1

2008-09-30 13:10 UTC #6 lore raw

Re: [PATCH] RCU read sched

In reply to Mathieu Desnoyers

Quoted 9 lines · Mathieu Desnoyers

(probably already in -tip)
Add rcu_read_lock_sched() and rcu_read_unlock_sched() to rcupdate.h to match the recently added write-side call_rcu_sched() and rcu_barrier_sched(). They also match the no-so-recently-added synchronize_sched().
It will help following matching use of the update/read lock primitives. Those new read lock will replace preempt_disable()/enable() used in pair with RCU-classic synchronization.

Looks like a very good addition to me!

Thanx, Paul

Reviewed-by Paul E. McKenney <[email protected]>

Quoted 42 lines

Signed-off-by Mathieu Desnoyers <[email protected]>

Acked-by Peter Zijlstra <[email protected]>

CC Paul E McKenney <[email protected]>

CC [email protected]

---
 include/linux/rcupdate.h |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: linux-2.6-lttng/include/linux/rcupdate.h
===================================================================

include/linux/rcupdate.h +20 -0

@@ -132,6 +132,26 @@ struct rcu_head {
 #define rcu_read_unlock_bh() __rcu_read_unlock_bh()
 
 /**
+ * rcu_read_lock_sched - mark the beginning of a RCU-classic critical section
+ *
+ * Should be used with either
+ * - synchronize_sched()
+ * or
+ * - call_rcu_sched() and rcu_barrier_sched()
+ * on the write-side to insure proper synchronization.
+ */
+#define rcu_read_lock_sched() preempt_disable()
+
+/*
+ * rcu_read_unlock_sched - marks the end of a RCU-classic critical section
+ *
+ * See rcu_read_lock_sched for more information.
+ */
+#define rcu_read_unlock_sched() preempt_enable()
+
+
+
+/**
  * rcu_dereference - fetch an RCU-protected pointer in an
  * RCU read-side critical section.  This pointer may later
  * be safely dereferenced.

Signature · 2 lines

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

Mathieu Desnoyers

2008-09-29 15:08 UTC #7 lore raw

Re: [PATCH] Markers use rcu_read_lock_sched()

In reply to Ingo Molnar

Use the new rcu_read_lock_sched/unlock_sched() in marker code around the call site instead of preempt_disable/enable(). It helps reviewing the code more easily.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Ingo Molnar <[email protected]>

CC Peter Zijlstra <[email protected]>

CC Paul E McKenney <[email protected]>

CC [email protected]

---
 kernel/marker.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

kernel/marker.c +8 -8

Index: linux-2.6-lttng/kernel/marker.c
===================================================================
@@ -105,11 +105,11 @@ void marker_probe_cb(const struct marker
 	char ptype;
 
 	/*
-	 * preempt_disable does two things : disabling preemption to make sure
-	 * the teardown of the callbacks can be done correctly when they are in
-	 * modules and they insure RCU read coherency.
+	 * rcu_read_lock_sched does two things : disabling preemption to make
+	 * sure the teardown of the callbacks can be done correctly when they
+	 * are in modules and they insure RCU read coherency.
 	 */
-	preempt_disable();
+	rcu_read_lock_sched();
 	ptype = mdata->ptype;
 	if (likely(!ptype)) {
 		marker_probe_func *func;

@@ -146,7 +146,7 @@ void marker_probe_cb(const struct marker
 			va_end(args);
 		}
 	}
-	preempt_enable();
+	rcu_read_unlock_sched();
 }
 EXPORT_SYMBOL_GPL(marker_probe_cb);

@@ -165,7 +165,7 @@ void marker_probe_cb_noarg(const struct 
 	va_list args;	/* not initialized */
 	char ptype;
 
-	preempt_disable();
+	rcu_read_lock_sched();
 	ptype = mdata->ptype;
 	if (likely(!ptype)) {
 		marker_probe_func *func;

@@ -197,7 +197,7 @@ void marker_probe_cb_noarg(const struct 
 			multi[i].func(multi[i].probe_private, call_private, fmt,
 				&args);
 	}
-	preempt_enable();
+	rcu_read_unlock_sched();
 }
 EXPORT_SYMBOL_GPL(marker_probe_cb_noarg);

@@ -562,7 +562,7 @@ static int set_marker(struct marker_entr
  * Disable a marker and its probe callback.
  * Note: only waiting an RCU period after setting elem->call to the empty
  * function insures that the original callback is not used anymore. This insured
- * by preempt_disable around the call site.
+ * by rcu_read_lock_sched around the call site.
  */
 static void disable_marker(struct marker *elem)
 {

Signature · 2 lines

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

Ingo Molnar

2008-09-30 10:14 UTC #8 lore raw

Re: [PATCH] Markers use rcu_read_lock_sched()

Quoted 3 lines · Mathieu Desnoyers

Use the new rcu_read_lock_sched/unlock_sched() in marker code around the call site instead of preempt_disable/enable(). It helps reviewing the code more easily.

applied this cleanup on top of the fix from Lai Jiangshan, thanks Matthieu!

here are the new commits in tip/tracing/markers:

2396ac3: sputrace: use marker_synchronize_unregister()
a16801c: markers: documentation fix for teardown
b05d9cf: markers: probe example, fix teardown
4b0b16f: markers: fix unregister bug and reenter bug, cleanup 1638da5: markers: marker_synchronize_unregister()
b211730: Merge branch 'core/rcu' into tracing/markers 1c50b72: rcu: add rcu_read_lock_sched() / rcu_read_unlock_sched()
d587284: markers: fix unregister bug and reenter bug

Ingo

Mathieu Desnoyers

2008-09-29 15:09 UTC #9 lore raw

PATCH 1/1 Markers : probe example fix teardown

In reply to Ingo Molnar

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.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Ingo Molnar <[email protected]>

CC Rusty Russell <[email protected]>

CC [email protected]

CC "Frank Ch. Eigler" <[email protected]>

---
 samples/markers/probe-example.c |    1 +
 1 file changed, 1 insertion(+)

samples/markers/probe-example.c +1 -0

Index: linux-2.6-lttng/samples/markers/probe-example.c
===================================================================
@@ -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 · 2 lines

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

Mathieu Desnoyers

2008-09-29 15:10 UTC #10 lore raw

PATCH 1/1 Markers : marker_synchronize_unregister()

In reply to Ingo Molnar

Create 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.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Ingo Molnar <[email protected]>

CC Rusty Russell <[email protected]>

CC [email protected]

CC "Frank Ch. Eigler" <[email protected]>

---
 include/linux/marker.h |    7 +++++++
 1 file changed, 7 insertions(+)

include/linux/marker.h +7 -0

Index: linux-2.6-lttng/include/linux/marker.h
===================================================================
@@ -142,4 +142,11 @@ extern int marker_probe_unregister_priva
 extern 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 · 2 lines

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

Lai Jiangshan

author

2008-09-30 03:27 UTC #11 lore raw

Re: [PATCH] Markers : marker_synchronize_unregister()

Quoted 18 lines · Mathieu Desnoyers

Create 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.
Signed-off-by Mathieu Desnoyers <[email protected]>
CC Ingo Molnar <[email protected]>
CC Rusty Russell <[email protected]>
CC [email protected]
CC "Frank Ch. Eigler" <[email protected]>
---
 include/linux/marker.h |    7 +++++++
 1 file changed, 7 insertions(+)
Index: linux-2.6-lttng/include/linux/marker.h
===================================================================

include/linux/marker.h extern int marker_probe_unregister_priva · line 149

Quoted 8 lines

include/linux/marker.h +5 -0

@@ -142,4 +142,11 @@ extern int marker_probe_unregister_priva
 extern 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.
+ */

marker_synchronize_unregister must be called _also_ between unregistration and destruction the data that unregistration-ed probes need to make sure there is no caller executing a probe when it's data is destroyed.

Quoted 3 lines

include/linux/marker.h +2 -0

+#define marker_synchronize_unregister() synchronize_sched()
+
 #endif

A message that is not in the archive (sent off-list or not yet received): [email protected]

Mathieu Desnoyers

2008-10-03 15:53 UTC #13 lore raw

PATCH 1/1 Markers synchronize unregister static inline

Turn marker synchronize unregister into a static inline. There is no reason to keep it as a macro over a static inline.

Ingo, can you pull this into -tip on top of the previous "Markers synchronize unregister" patch ?

Thanks,

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Ingo Molnar <[email protected]>

CC Andrew Morton <[email protected]>

---
 include/linux/marker.h |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

include/linux/marker.h +5 -1

Index: linux-2.6-lttng/include/linux/marker.h
===================================================================
@@ -14,6 +14,7 @@
 
 #include <linux/immediate.h>
 #include <linux/types.h>
+#include <linux/rcupdate.h>
 
 struct module;
 struct task_struct;

@@ -218,7 +219,10 @@ extern void *marker_get_private_data(con
  * 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()
+static inline void marker_synchronize_unregister(void)
+{
+	synchronize_sched();
+}
 
 struct marker_iter {
 	struct module *module;

Signature · 2 lines

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

Ingo Molnar

Applied

2008-10-03 17:32 UTC #14 lore raw

Re: [PATCH] Markers synchronize unregister static inline

Quoted 5 lines · Mathieu Desnoyers

Turn marker synchronize unregister into a static inline. There is no reason to keep it as a macro over a static inline.
Ingo, can you pull this into -tip on top of the previous "Markers synchronize unregister" patch ?

applied to tip/tracing/markers, thanks Mathieu!

Ingo

Mathieu Desnoyers

2008-09-29 15:10 UTC #15 lore raw

PATCH 1/1 Markers : documentation fix teardown

In reply to Ingo Molnar

Document 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.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Ingo Molnar <[email protected]>

CC Rusty Russell <[email protected]>

CC [email protected]

CC "Frank Ch. Eigler" <[email protected]>

---
 Documentation/markers.txt |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Documentation/markers.txt +6 -4

Index: linux-2.6-lttng/Documentation/markers.txt
===================================================================
@@ -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 · 2 lines

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

Mathieu Desnoyers

2008-09-29 15:11 UTC #16 lore raw

PATCH 1/1 sputrace : use marker_synchronize_unregister()

In reply to Ingo Molnar

We 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.

Signed-off-by Mathieu Desnoyers <[email protected]>

CC Ingo Molnar <[email protected]>

CC Jeremy Kerr <[email protected]>

CC [email protected]

CC Christoph Hellwig <[email protected]>

---
 arch/powerpc/platforms/cell/spufs/sputrace.c |    1 +
 1 file changed, 1 insertion(+)

arch/powerpc/platforms/cell/spufs/sputrace.c +1 -0

Index: linux-2.6-lttng/arch/powerpc/platforms/cell/spufs/sputrace.c
===================================================================
@@ -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 · 2 lines

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

Christoph Hellwig

2008-09-29 15:13 UTC #17 lore raw

Re: [PATCH] sputrace : use marker_synchronize_unregister()

Quoted 3 lines · Mathieu Desnoyers

We 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.

Looks good.

Jeremy Kerr

2008-09-30 00:28 UTC #18 lore raw

Re: [PATCH] sputrace : use marker_synchronize_unregister()

In reply to Mathieu Desnoyers

Mathieu,

Quoted 3 lines

We 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.

Looks good - added to spufs.git.

Cheers,

Jeremy

Ingo Molnar

2008-09-30 09:56 UTC #19 lore raw

Re: [PATCH] sputrace : use marker_synchronize_unregister()

Quoted 7 lines · Jeremy Kerr

Mathieu,
Quoted 3 lines
We 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.
Looks good - added to spufs.git.

that wont work very well as the patch relies on the new marker_synchronize_unregister() facility.

Ingo

Jeremy Kerr

2008-09-30 11:22 UTC #20 lore raw

Re: [Cbe-oss-dev] [PATCH] sputrace : use marker_synchronize_unregister()

Ingo,

Quoted 2 lines

that wont work very well as the patch relies on the new marker_synchronize_unregister() facility.

d'oh, right you are. Should I leave this in your hands to merge?

Cheers,

Jeremy

Ingo Molnar

2008-09-30 11:30 UTC #21 lore raw

Re: [Cbe-oss-dev] [PATCH] sputrace : use marker_synchronize_unregister()

Quoted 6 lines · Jeremy Kerr

Ingo,
Quoted 2 lines
that wont work very well as the patch relies on the new marker_synchronize_unregister() facility.
d'oh, right you are. Should I leave this in your hands to merge?

would be nice if you could give your Acked-by for the sputrace bits, then we can merge it. It's a oneliner so it shouldnt cause merging trouble in linux-next.

Ingo

Jeremy Kerr

Acked-by

2008-09-30 11:34 UTC #22 lore raw

Re: [Cbe-oss-dev] [PATCH] sputrace : use marker_synchronize_unregister()

Ingo,

Quoted 3 lines

would be nice if you could give your Acked-by for the sputrace bits, then we can merge it. It's a oneliner so it shouldnt cause merging trouble in linux-next.

Sure!

Acked-by Jeremy Kerr <[email protected]>

Jeremy

Mathieu Desnoyers

2008-09-29 15:03 UTC #23 lore raw

In reply to Lai Jiangshan

Hi Lai,

I'll have to nack this fix :

One fix I already posted makes sure every marker unregister callers call synchronize_sched() _at some point_ before module unload. It thus makes sure we can do batch unregistration without doing multiple synchronize_sched calls.

Also, there is no need to do the synchronize_sched with the marker mutex held. call_rcu_sched takes care of making sure the previous quiescent state is over before calling kfree. This means that when we return from the register/unregister functions, there may still be markers "in flight" using the old markers. Again, why would it be a problem ?

Thanks,

Mathieu

P.S. : I'll send along the patches I am referring to. Ingo, those should probably be merged if they are not in -tip already.

Signature · 2 lines

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

Lai Jiangshan

author

2008-09-30 03:27 UTC #24 lore raw

Quoted 8 lines · Mathieu Desnoyers

Hi Lai,
I'll have to nack this fix :
One fix I already posted makes sure every marker unregister callers call synchronize_sched() _at some point_ before module unload. It thus makes sure we can do batch unregistration without doing multiple synchronize_sched calls.

1) the new API marker_synchronize_unregister() is ugly, it separate one thing into two steps.
user have to remember to call marker_synchronize_unregister() and have to know what he can do and what he can't do before marker_synchronize_unregister().

2) IMO, synchronous code is better than asynchronous in non-critical-path. we need synchronize_sched() for free(old).

you fixes haven't fix the reenter bug.

I recommend my fix.

Quoted 14 lines

Also, there is no need to do the synchronize_sched with the marker mutex held. call_rcu_sched takes care of making sure the previous quiescent state is over before calling kfree. This means that when we return from the register/unregister functions, there may still be markers "in flight" using the old markers. Again, why would it be a problem ?
Thanks,
Mathieu
P.S. : I'll send along the patches I am referring to. Ingo, those should probably be merged if they are not in -tip already.

Mathieu Desnoyers

2008-09-30 03:38 UTC #25 lore raw

Quoted 10 lines · * Lai Jiangshan ([email protected]) wrote:

Quoted 8 lines · Mathieu Desnoyers
Hi Lai,
I'll have to nack this fix :
One fix I already posted makes sure every marker unregister callers call synchronize_sched() _at some point_ before module unload. It thus makes sure we can do batch unregistration without doing multiple synchronize_sched calls.

Quoted 6 lines

1) the new API marker_synchronize_unregister() is ugly, it separate one thing into two steps.
user have to remember to call marker_synchronize_unregister() and have to know what he can do and what he can't do before marker_synchronize_unregister().

Hum, yes it does separate unregistration and synchronization in two steps for a very precise purpose : I don't want unregistration of 100 markers to take ~30 seconds on a heavily loaded machine.

Quoted 3 lines

2) IMO, synchronous code is better than asynchronous in non-critical-path. we need synchronize_sched() for free(old).

free(old) is only done in call_rcu. the rcu callback is forced by a rcu_barrier() if two consecutive operations are done on the same marker.

Quoted 2 lines

you fixes haven't fix the reenter bug.

I don't see any reentrancy bug here. Have you actually experienced such an issue ? Can you give me an example of a bogus execution trace (step-by-step operations) ?

Thanks,

Mathieu

Quoted 18 lines

I recommend my fix.
Quoted 14 lines
Also, there is no need to do the synchronize_sched with the marker mutex held. call_rcu_sched takes care of making sure the previous quiescent state is over before calling kfree. This means that when we return from the register/unregister functions, there may still be markers "in flight" using the old markers. Again, why would it be a problem ?
Thanks,
Mathieu
P.S. : I'll send along the patches I am referring to. Ingo, those should probably be merged if they are not in -tip already.

Signature · 2 lines

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