markers: avoid call_rcu_sched if old is NULL

PATCH In review Masami Hiramatsu 3 participants 5 messages Jul 8, 2008 – Jul 13, 2008

Share & tools

Masami Hiramatsu

author

2008-07-08 20:31 UTC #0 lore raw

PATCH 1/1 markers: avoid call_rcu_sched if old is NULL

Introduce marker_entry_free_old() and check old pointer is NULL before setting call_rcu_sched(), because marker_entry_remove/add_probe() can return NULL.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/marker.c |   29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Mathieu, I think this might be a bug. Tracepoint also has same bug...

kernel/marker.c +14 ~~-15~~

Index: 2.6.26-rc8-mm1/kernel/marker.c
===================================================================
@@ -201,6 +201,17 @@ static void free_old_closure(struct rcu_
 	entry->rcu_pending = 0;
 }
 
+static void marker_entry_free_old(struct marker_entry *entry, void *old)
+{
+	if (!old)
+		return;
+	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);
+}
+
 static void debug_print_probes(struct marker_entry *entry)
 {
 	int i;

@@ -666,11 +677,7 @@ int marker_probe_register(const char *na
 	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);
+	marker_entry_free_old(entry, old);
 end:
 	mutex_unlock(&markers_mutex);
 	return ret;

@@ -709,11 +716,7 @@ int marker_probe_unregister(const char *
 	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);
+	marker_entry_free_old(entry, old);
 	remove_marker(name);	/* Ignore busy error message */
 	ret = 0;
 end:

@@ -787,11 +790,7 @@ int marker_probe_unregister_private_data
 	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);
+	marker_entry_free_old(entry, old);
 	remove_marker(entry->name);	/* Ignore busy error message */
 end:
 	mutex_unlock(&markers_mutex);

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Mathieu Desnoyers

Acked-by on v1 1/1

2008-07-09 03:02 UTC #1 lore raw

Quoted 4 lines · * Masami Hiramatsu ([email protected]) wrote:

Introduce marker_entry_free_old() and check old pointer is NULL before setting call_rcu_sched(), because marker_entry_remove/add_probe() can return NULL.

Hi Masami,

I doubt this is a bug per se, because kfree accepts NULL pointers (and kfree is the only action done on the oldptr by free_old_closure).

This cleans up the code, so I think it's good to merge your patch, but I would definitely not classify this as a bugfix.

Acked-by Mathieu Desnoyers <[email protected]>

Mathieu

Quoted 78 lines

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/marker.c |   29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Mathieu, I think this might be a bug. Tracepoint also has same bug...

Index: 2.6.26-rc8-mm1/kernel/marker.c
===================================================================

kernel/marker.c +14 ~~-15~~

@@ -201,6 +201,17 @@ static void free_old_closure(struct rcu_
 	entry->rcu_pending = 0;
 }
 
+static void marker_entry_free_old(struct marker_entry *entry, void *old)
+{
+	if (!old)
+		return;
+	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);
+}
+
 static void debug_print_probes(struct marker_entry *entry)
 {
 	int i;

@@ -666,11 +677,7 @@ int marker_probe_register(const char *na
 	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);
+	marker_entry_free_old(entry, old);
 end:
 	mutex_unlock(&markers_mutex);
 	return ret;

@@ -709,11 +716,7 @@ int marker_probe_unregister(const char *
 	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);
+	marker_entry_free_old(entry, old);
 	remove_marker(name);	/* Ignore busy error message */
 	ret = 0;
 end:

@@ -787,11 +790,7 @@ int marker_probe_unregister_private_data
 	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);
+	marker_entry_free_old(entry, old);
 	remove_marker(entry->name);	/* Ignore busy error message */
 end:
 	mutex_unlock(&markers_mutex);

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Signature · 2 lines

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

Mathieu Desnoyers

2008-07-09 04:25 UTC #2 lore raw

Quoted 8 lines · * Mathieu Desnoyers ([email protected]) wrote:

Quoted 4 lines · * Masami Hiramatsu ([email protected]) wrote:
Introduce marker_entry_free_old() and check old pointer is NULL before setting call_rcu_sched(), because marker_entry_remove/add_probe() can return NULL.
Hi Masami,

Quoted 8 lines

I doubt this is a bug per se, because kfree accepts NULL pointers (and kfree is the only action done on the oldptr by free_old_closure).
This cleans up the code, so I think it's good to merge your patch, but I would definitely not classify this as a bugfix.
Acked-by Mathieu Desnoyers <[email protected]>

Hrm, nope, finally there is a problem with your fix. Nack. Here is why :

Lets say we have two probes to connect on the marker, each with its own private_data.

If we pass from 1 connected probe (probe A) to 2 (Probes A and B), and then back to one (probe B), we want to make sure that readers (instrumentation sites) will see coherent probes (matching callback and private data). I remind you that 0 or 1 probes connected does not use an array with the markers. Only if 2 to N probes are connected do we use an array to store the callback and private data pairs. Therefore, special care must be taken of the callback and private data update in the 1 probe connected case (when there are 0 probes connected, the private data is not used and therefore we leave it as is at its old value).

However, because your cleanup actually removes the rcu_barrier() done before the operation following the 2nd probe addition (the barrier is only done if there are rcu calls pending), we can end up doing :

Probes
A
{A, B}
B

within a single RCU period, which means that a reader can see :

A
B

Those are "single probes" which involves updates of two pointers : the callback and the private data. They must never be mixed within a single rcu period : valid transitions go from 0 to 1, 1 to 2, 2 N and from N to 2, 2 to 1, 1 to 0. The modification you are doing here adds transitions 1 to 1 (as my example show) which should never happen.

Final reminder : this "special case" of 0 and 1 probes connected is done to remove a pointer dereference and to minimize memory allocation in the "common case" where only 1 probe is connected to a marker. It's not used in the new tracepoint infrastructure because we have to iterate on the callbacks in the instrumentation sites. The markers can do this in a wrapper function, so we don't suffer from added instructions to every call sites. So your cleanup makes sense for the tracepoint infrastructure, but not for the markers.

Mathieu

Quoted 84 lines

Mathieu

Quoted 78 lines

Signed-off-by Masami Hiramatsu <[email protected]>

---
 kernel/marker.c |   29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

Mathieu, I think this might be a bug. Tracepoint also has same bug...

Index: 2.6.26-rc8-mm1/kernel/marker.c
===================================================================

kernel/marker.c +14 ~~-15~~

@@ -201,6 +201,17 @@ static void free_old_closure(struct rcu_
 	entry->rcu_pending = 0;
 }
 
+static void marker_entry_free_old(struct marker_entry *entry, void *old)
+{
+	if (!old)
+		return;
+	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);
+}
+
 static void debug_print_probes(struct marker_entry *entry)
 {
 	int i;

@@ -666,11 +677,7 @@ int marker_probe_register(const char *na
 	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);
+	marker_entry_free_old(entry, old);
 end:
 	mutex_unlock(&markers_mutex);
 	return ret;

@@ -709,11 +716,7 @@ int marker_probe_unregister(const char *
 	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);
+	marker_entry_free_old(entry, old);
 	remove_marker(name);	/* Ignore busy error message */
 	ret = 0;
 end:

@@ -787,11 +790,7 @@ int marker_probe_unregister_private_data
 	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);
+	marker_entry_free_old(entry, old);
 	remove_marker(entry->name);	/* Ignore busy error message */
 end:
 	mutex_unlock(&markers_mutex);

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Signature · 2 lines

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

Signature · 2 lines

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

Andrew Morton

2008-07-11 19:22 UTC #3 lore raw

In reply to Mathieu Desnoyers

Quoted 8 lines · On Tue, 8 Jul 2008 23:02:01 -0400 Mathieu Desnoyers…

Quoted 4 lines · * Masami Hiramatsu ([email protected]) wrote:
Introduce marker_entry_free_old() and check old pointer is NULL before setting call_rcu_sched(), because marker_entry_remove/add_probe() can return NULL.
Hi Masami,

Quoted 7 lines

I doubt this is a bug per se, because kfree accepts NULL pointers (and kfree is the only action done on the oldptr by free_old_closure).
This cleans up the code, so I think it's good to merge your patch, but I would definitely not classify this as a bugfix.
Acked-by Mathieu Desnoyers <[email protected]>

I cannot get this to apply on the rather dated tree which I have on this rather not-on-the-internet machine. Please merge this patch locally, test, rewrite the changelog and resend it to someone ;)

Mathieu Desnoyers

2008-07-13 04:28 UTC #4 lore raw

Quoted 13 lines · * Andrew Morton ([email protected]) wrote:

Quoted 11 lines · On Tue, 8 Jul 2008 23:02:01 -0400 Mathieu Desnoyers…
Quoted 4 lines · * Masami Hiramatsu ([email protected]) wrote:
Introduce marker_entry_free_old() and check old pointer is NULL before setting call_rcu_sched(), because marker_entry_remove/add_probe() can return NULL.
Hi Masami,
I doubt this is a bug per se, because kfree accepts NULL pointers (and kfree is the only action done on the oldptr by free_old_closure).

Quoted 9 lines

Quoted 4 lines
This cleans up the code, so I think it's good to merge your patch, but I would definitely not classify this as a bugfix.
Acked-by Mathieu Desnoyers <[email protected]>
I cannot get this to apply on the rather dated tree which I have on this rather not-on-the-internet machine. Please merge this patch locally, test, rewrite the changelog and resend it to someone ;)

Hi Andrew,

As I pointed out in a reply to this email, I NACK this patch because it removes a necessary quiescent state wait from the marker code. The other reply here explains why I changed my mind about this patch.

http://linux.derkeiler.com/Mailing-Lists/Kernel/2008-07/msg03514.html

Mathieu

Signature · 2 lines

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