ftrace/perf_event leak

I recently added perf_event support to kvm_stat, to display kvm tracepoints as statistics (I'd like to fold this to tools/perf eventually, but that's another story). However I'm seeing a resource leak - after I quit the tool, there are quite a few references into the kvm module:

kvm_intel 43655 0
kvm 272984 269 kvm_intel

The tool is just a python script that reads /sys/kernel/debug/tracing/events/kvm to find out which events are available, uses perf_event_open() to create one group per cpu to which a lot of events are attached. The only special thing I can think of is that we use an ioctl to attach a filter to many perf_event descriptors.

You can find the source at
http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=blob_plain;f=kvm/kvm_stat;hb=5bd5f131b50cb373ff4e2a3632c6dad00a1f0b55.
All it needs are the kvm modules loaded; no need to actually run a guest. Run as root.

Does something like the below cure that?

I seem to remember C doesn't make any promises about the order of logic statements, hence we need to explicitly pull out that try_module_get() so that it evaluates after the rest of the conditions.

Quoted 3 lines · Peter Zijlstra

Does something like the below cure that?

Unfortunately not.

Then try this:

Quoted 2 lines · Li Zefan

Then try this:

Tested-by Avi Kivity <[email protected]>

Quoted 6 lines · Avi Kivity

Quoted 2 lines · Li Zefan

Then try this:

Tested-by Avi Kivity <[email protected]>

Thanks, queued as:

---
Subject: perf, trace: Fix module leak
From: Li Zefan <[email protected]>
Date: Wed Sep 01 12:58:43 CEST 2010

Commit 1c024eca (perf, trace: Optimize tracepoints by using per-tracepoint-per-cpu hlist to track events) caused a module refcount leak.

Tested-by Avi Kivity <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

LKML-Reference: <[email protected]>

Quoted 6 lines · Avi Kivity

Quoted 2 lines · Li Zefan

Then try this:

Tested-by Avi Kivity <[email protected]>

Thanks, queued as:

---
Subject: perf, trace: Fix module leak
From: Li Zefan <[email protected]>
Date: Wed Sep 01 12:58:43 CEST 2010

Commit 1c024eca (perf, trace: Optimize tracepoints by using per-tracepoint-per-cpu hlist to track events) caused a module refcount leak.

Tested-by Avi Kivity <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

LKML-Reference: <[email protected]>

---
 kernel/trace/trace_event_perf.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6/kernel/trace/trace_event_perf.c
===================================================================

kernel/trace/trace_event_perf.c +2 -0

@@ -91,6 +91,8 @@ int perf_trace_init(struct perf_event *p
 		    tp_event->class && tp_event->class->reg &&
 		    try_module_get(tp_event->mod)) {
 			ret = perf_trace_event_init(tp_event, p_event);
+			if (ret)
+				module_put(tp_event->mod);
 			break;
 		}
 	}

Thanks for fixing this.

However, can we split this in two patches to ease the backport?

The lack of a module_put() after perf_trace_init() failure is there for a while (the backport needs to start in 2.6.32).

But the lack of a module_put in the destroy path needs a .35 backport only.

Quoted 4 lines · Frederic Weisbecker

Thanks for fixing this.

However, can we split this in two patches to ease the backport?

Quoted 4 lines

The lack of a module_put() after perf_trace_init() failure is there for a while (the backport needs to start in 2.6.32).

But the lack of a module_put in the destroy path needs a .35 backport only.

I don't think it really needs two patches. Just notify stable (and Greg KH in particular) about the backport requirements. Greg can handle it ;)

Quoted 30 lines

Subject: perf, trace: Fix module leak
From: Li Zefan <[email protected]>
Date: Wed Sep 01 12:58:43 CEST 2010

Commit 1c024eca (perf, trace: Optimize tracepoints by using per-tracepoint-per-cpu hlist to track events) caused a module refcount leak.

Tested-by Avi Kivity <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

LKML-Reference: <[email protected]>

---
 kernel/trace/trace_event_perf.c |    3 +++
 1 file changed, 3 insertions(+)

Index: linux-2.6/kernel/trace/trace_event_perf.c
===================================================================

kernel/trace/trace_event_perf.c +2 -0

@@ -91,6 +91,8 @@ int perf_trace_init(struct perf_event *p
 		    tp_event->class && tp_event->class->reg &&
 		    try_module_get(tp_event->mod)) {
 			ret = perf_trace_event_init(tp_event, p_event);
+			if (ret)
+				module_put(tp_event->mod);
 			break;
 		}
 	}

kernel/trace/trace_event_perf.c void perf_trace_destroy(struct perf_even · line 155

Quoted 7 lines

kernel/trace/trace_event_perf.c +1 -0

@@ -147,6 +149,7 @@ void perf_trace_destroy(struct perf_even
 		}
 	}
 out:
+	module_put(tp_event->mod);
 	mutex_unlock(&event_mutex);
 }
 

Thanks for fixing this.

However, can we split this in two patches to ease the backport?

The lack of a module_put() after perf_trace_init() failure is there for a while (the backport needs to start in 2.6.32).

But the lack of a module_put in the destroy path needs a .35 backport only.

Quoted 3 lines · Peter Zijlstra

Does something like the below cure that?

Unfortunately not.