[PATCH] Markers : fix reentrancy

[PATCH] Markers : fix reentrancy

[Mathieu Desnoyers]

Lai Jiangshan <laijs@cn.fujitsu.com> wrote :
> 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.
> 

Ah, you're right. I looked again through your patch description and I
think using a :


if (entry->rcu_pending)
                rcu_barrier_sched();

After each time the markers_mutex is taken would keep the fast
batch registration/unregistration and fix the reentrancy problem.
The following patch should address the problem.

Thanks,

Mathieu


Lai Jiangshan discovered a reentrancy issue with markers. This patch implements
a version of the fix which won't slow down marker batch
registration/unregistration.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
CC: Ingo Molnar <mingo@elte.hu>
CC: Lai Jiangshan <laijs@cn.fujitsu.com>
CC: "Frank Ch. Eigler" <fche@redhat.com>
---
 kernel/marker.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-2.6-lttng/kernel/marker.c
===================================================================
--- linux-2.6-lttng.orig/kernel/marker.c	2008-09-30 01:29:18.000000000 -0400
+++ linux-2.6-lttng/kernel/marker.c	2008-09-30 01:31:28.000000000 -0400
@@ -674,6 +674,8 @@ int marker_probe_register(const char *na
 	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 */
@@ -717,6 +719,8 @@ int marker_probe_unregister(const char *
 	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 */
@@ -795,6 +799,8 @@ int marker_probe_unregister_private_data
 	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 */

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

Re: [PATCH] Markers : fix reentrancy

[Ingo Molnar]

* Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:

> Lai Jiangshan <laijs@cn.fujitsu.com> wrote :
> > 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.
> > 
> 
> Ah, you're right. I looked again through your patch description and I
> think using a :
> 
> 
> if (entry->rcu_pending)
>                 rcu_barrier_sched();
> 
> After each time the markers_mutex is taken would keep the fast batch 
> registration/unregistration and fix the reentrancy problem. The 
> following patch should address the problem.

could you please send a delta patch against tip/master? Lai's patch is 
already applied and is tested. Thanks,

	Ingo

Re: [PATCH] Markers : fix reentrancy

[Mathieu Desnoyers]

* Ingo Molnar (mingo@elte.hu) wrote:
> 
> * Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> 
> > Lai Jiangshan <laijs@cn.fujitsu.com> wrote :
> > > 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.
> > > 
> > 
> > Ah, you're right. I looked again through your patch description and I
> > think using a :
> > 
> > 
> > if (entry->rcu_pending)
> >                 rcu_barrier_sched();
> > 
> > After each time the markers_mutex is taken would keep the fast batch 
> > registration/unregistration and fix the reentrancy problem. The 
> > following patch should address the problem.
> 
> could you please send a delta patch against tip/master? Lai's patch is 
> already applied and is tested. Thanks,
> 
> 	Ingo

Hrm,

Most of Lai "simplifications" to use synchronize_sched() instead of
call_rcu_sched() will make batch registration/unregistration much
slower (very noticeable on ~50 markers on a loaded machine). It also
contains two different fixes in one, one of which has been nacked.

The correct list of changes against tip now becomes :

Revert commit d587284c1d26b796bf52a6e3d3f783db4e462119
Apply patch "Markers : marker_synchronize_unregister()"
Apply patch "Markers : probe example fix teardown"
Apply patch "Markers : documentation fix teardown"
Apply patch "sputrace : use marker_synchronize_unregister()"
Apply patch "Markers : fix reentrancy"

Sorry, but rebasing those marker fixes will just make all those changes
really messy. I think the cleanest way is to just revert. :-/

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