11:10 a.m.
Hi List,
Please find below a patch that should correct two memory leaks within
the udev device handling code.
The issue is triggered by 'add' udev calls and will slowly cause
libvirtd to consume the majority of system memory.
The first part of the patch deals with udevAddOneDevice() and frees
the memory allocated to 'def' if the function would return -1 (an
error condition) due to the inability to look up udev properties (for
example, if the udev device is already removed).
The second part of the patch deals with a remaining 8kB/cycle memory
leak, in which the udev device reference isn't released at the end of
running udevEventHandleCallback().
I'm happy to answer any questions about the patch, there is also a bit
more background at
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/571093
---
From 6c8183e83fbfeb031b16cf9ae2d41b16e3145378 Mon Sep 17 00:00:00 2001
From: Nigel Jones <dev(a)nigelj.com>
Date: Mon, 24 May 2010 15:05:53 +0000
Subject: [PATCH] Patch 2 memory leaks.
1. Ensure that memory is free'd from udevAddOneDevice() if the return
value will be non-zero
2. Release udev device reference in udevEventHandleCallback()
similar to the release of the reference in udevProcessDeviceListEntry()
---
src/node_device/node_device_udev.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index a1ced87..4d0effa 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1296,6 +1296,9 @@ static int udevAddOneDevice(struct udev_device *device)
ret = 0;
out:
+ if (ret != 0) {
+ virNodeDeviceDefFree(def); /* Free assigned memory to prevent leaks */
+ }
return ret;
}
@@ -1426,6 +1429,7 @@ static void udevEventHandleCallback(int watch
ATTRIBUTE_UNUSED,
}
out:
+ udev_device_unref(device);
return;
}
--
1.7.0.4
11:32 a.m.
On 05/24/2010 12:10 PM, Nigel Jones wrote:
>From 6c8183e83fbfeb031b16cf9ae2d41b16e3145378 Mon Sep 17 00:00:00
2001
From: Nigel Jones <dev(a)nigelj.com>
Date: Mon, 24 May 2010 15:05:53 +0000
Subject: [PATCH] Patch 2 memory leaks.
1. Ensure that memory is free'd from udevAddOneDevice() if the return
value will be non-zero
2. Release udev device reference in udevEventHandleCallback()
similar to the release of the reference in udevProcessDeviceListEntry()
---
src/node_device/node_device_udev.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index a1ced87..4d0effa 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1296,6 +1296,9 @@ static int udevAddOneDevice(struct udev_device *device)
ret = 0;
out:
+ if (ret != 0) {
+ virNodeDeviceDefFree(def); /* Free assigned memory to prevent leaks */
+ }
return ret;
}
@@ -1426,6 +1429,7 @@ static void udevEventHandleCallback(int watch
ATTRIBUTE_UNUSED,
}
out:
+ udev_device_unref(device);
return;
}
Good catch on both of these. I had to convince myself that udev_device_unref()
would handle a NULL device (in the case of error before we allocated device), but it
does.
ACK
--
Chris Lalancette
2:15 p.m.
On 05/24/2010 12:32 PM, Chris Lalancette wrote:
On 05/24/2010 12:10 PM, Nigel Jones wrote:
> >From 6c8183e83fbfeb031b16cf9ae2d41b16e3145378 Mon Sep 17 00:00:00 2001
> From: Nigel Jones <dev(a)nigelj.com>
> Date: Mon, 24 May 2010 15:05:53 +0000
> Subject: [PATCH] Patch 2 memory leaks.
>
> 1. Ensure that memory is free'd from udevAddOneDevice() if the return
> value will be non-zero
> 2. Release udev device reference in udevEventHandleCallback()
> similar to the release of the reference in udevProcessDeviceListEntry()
> ---
> src/node_device/node_device_udev.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/src/node_device/node_device_udev.c
> b/src/node_device/node_device_udev.c
> index a1ced87..4d0effa 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1296,6 +1296,9 @@ static int udevAddOneDevice(struct udev_device *device)
> ret = 0;
>
> out:
> + if (ret != 0) {
> + virNodeDeviceDefFree(def); /* Free assigned memory to prevent leaks */
> + }
> return ret;
> }
>
> @@ -1426,6 +1429,7 @@ static void udevEventHandleCallback(int watch
> ATTRIBUTE_UNUSED,
> }
>
> out:
> + udev_device_unref(device);
> return;
> }
>
Good catch on both of these. I had to convince myself that udev_device_unref()
would handle a NULL device (in the case of error before we allocated device), but it
does.
OK, so after talking to David Allan, who originally wrote this code, it seems like
this is right approach, but there is one piece left that we have to worry about.
Namely, on a udev "change" event, we have to make sure we remove and re-add the
device
to the node-device list. Now, this *may* already be happening, but I don't quite
understand the semantics of this enough to say for sure. I'm going to hold off
pushing this into the repo until Dave has had a chance to look at it and comment.
--
Chris Lalancette
10:13 p.m.
On Tue, May 25, 2010 at 04:10:27AM +1200, Nigel Jones wrote:
Hi List,
Please find below a patch that should correct two memory leaks within
the udev device handling code.
The issue is triggered by 'add' udev calls and will slowly cause
libvirtd to consume the majority of system memory.
The first part of the patch deals with udevAddOneDevice() and frees
the memory allocated to 'def' if the function would return -1 (an
error condition) due to the inability to look up udev properties (for
example, if the udev device is already removed).
The second part of the patch deals with a remaining 8kB/cycle memory
leak, in which the udev device reference isn't released at the end of
running udevEventHandleCallback().
I'm happy to answer any questions about the patch, there is also a bit
more background at
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/571093
Hi Nigel,
Thanks for reporting this, and the patch. While reviewing your patch,
I found an additional leak in the remove case which I have also
addressed in the attached patch, which is otherwise just a slight
modification of yours. I was able to reproduce the leaks by running a
device add/remove cycle, and I do not see memory leakage after
applying this patch. Let me know if it fixes it for you as well.
Dave
12:22 p.m.
On 05/28/10 - 11:13:29PM, Dave Allan wrote:
diff --git a/src/node_device/node_device_udev.c
b/src/node_device/node_device_udev.c
index 6e3ecd7..5193f5b 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1211,6 +1211,8 @@ static int udevRemoveOneDevice(struct udev_device *device)
}
nodeDeviceUnlock(driverState);
+ udev_device_unref(device);
+
return ret;
}
Instead of adding the udev_device_unref here and in in udevAddOneDevice, I
think we should probably just put it in the udevEventHandleCallback() and
udevProcessDeviceListEntry(). Besides the fact that it is easier to read and
verify that you've dropped the reference, if udev ever grows new actions
besides "add", "change", or "remove", we'll go back to
leaking the reference.
--
Chris Lalancette
12:23 p.m.
New subject: [libvirt] [PATCH 0/1] Fix udev memory leak v3
Here's a new version of the fix for the udev memory leak based on
Chris Lalancette's feedback. I've tested it with valgrind as well as
through an extremely long cycle of device additions and removals, so
I'm confident in it, but I'd appreciate an ack that it fixes the
reported problem.
Dave
David Allan (1):
Fix leaks in udev device add/remove v3
src/node_device/node_device_udev.c | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)
12:23 p.m.
New subject: [libvirt] [PATCH 1/1] Fix leaks in udev device add/remove v3
* This patch is a modification of a patch submitted by Nigel Jones.
It fixes several memory leaks on device addition/removal:
1. Free the virNodeDeviceDefPtr in udevAddOneDevice if the return
value is non-zero
2. Always release the node device reference after the device has been
processed.
* Refactored for better readability per the suggestion of clalance
---
src/node_device/node_device_udev.c | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 6e3ecd7..73217c5 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1309,13 +1309,14 @@ static int udevAddOneDevice(struct udev_device *device)
goto out;
}
+ /* If this is a device change, the old definition will be freed
+ * and the current definition will take its place. */
nodeDeviceLock(driverState);
dev = virNodeDeviceAssignDef(&driverState->devs, def);
nodeDeviceUnlock(driverState);
if (dev == NULL) {
VIR_ERROR(_("Failed to create device for '%s'"),
def->name);
- virNodeDeviceDefFree(def);
goto out;
}
@@ -1324,6 +1325,10 @@ static int udevAddOneDevice(struct udev_device *device)
ret = 0;
out:
+ if (ret != 0) {
+ virNodeDeviceDefFree(def);
+ }
+
return ret;
}
@@ -1338,15 +1343,17 @@ static int udevProcessDeviceListEntry(struct udev *udev,
name = udev_list_entry_get_name(list_entry);
device = udev_device_new_from_syspath(udev, name);
+
if (device != NULL) {
if (udevAddOneDevice(device) != 0) {
VIR_INFO("Failed to create node device for udev device
'%s'",
name);
}
- udev_device_unref(device);
ret = 0;
}
+ udev_device_unref(device);
+
return ret;
}
@@ -1454,6 +1461,7 @@ static void udevEventHandleCallback(int watch ATTRIBUTE_UNUSED,
}
out:
+ udev_device_unref(device);
return;
}
--
1.7.0.1
1:38 p.m.
New subject: [libvirt] [PATCH 1/1] Fix leaks in udev device add/remove v3
On 06/07/2010 11:23 AM, David Allan wrote:
* This patch is a modification of a patch submitted by Nigel Jones.
It fixes several memory leaks on device addition/removal:
1. Free the virNodeDeviceDefPtr in udevAddOneDevice if the return
value is non-zero
2. Always release the node device reference after the device has been
processed.
* Refactored for better readability per the suggestion of clalance
Yes, in comparing the two versions, I can see the improvement. ACK on
this version.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:46 p.m.
New subject: [libvirt] [PATCH 1/1] Fix leaks in udev device add/remove v3
On Tue, Jun 08, 2010 at 12:38:47PM -0600, Eric Blake wrote:
On 06/07/2010 11:23 AM, David Allan wrote:
> * This patch is a modification of a patch submitted by Nigel Jones.
> It fixes several memory leaks on device addition/removal:
>
> 1. Free the virNodeDeviceDefPtr in udevAddOneDevice if the return
> value is non-zero
>
> 2. Always release the node device reference after the device has been
> processed.
>
> * Refactored for better readability per the suggestion of clalance
Yes, in comparing the two versions, I can see the improvement. ACK on
this version.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Thanks, pushed.
Dave
5257
days inactive
5272
days old
8 comments
5 participants
participants (5)
-
Chris Lalancette -
Dave Allan -
David Allan
-
Eric Blake -
Nigel Jones