2011/7/27 Matthias Bolte <matthias.bolte(a)googlemail.com&gt;: > doRemoteClose doesn't free the virNetClientPtr and this creates a > 260kb leak per remote connection. This happens because > virNetClientFree doesn't remove the last ref, because virNetClientNew > creates the virNetClientPtr with a refcount of 2. >

The memory leak I saw was due to virsh calling virEventRegisterDefaultImpl, but not calling virEventRunDefaultImpl. Because the event loop is initialized, the call to virNetSocketAddIOCallback in virNetClientNew succeeds. But virsh not driving the event loop results in not removing callbacks that were marked for deletion. Finally this leaks the virNetClientPtr using in the remote driver. I used a virsh -c qemu:///system to test. I was able fix this by calling virEventRunDefaultImpl after virConnectClose in virsh. But I don't think that this is the correct general solution.

To me the general problem seems to be the entanglement between the virNetClientPtr refcounting and the event loop. I'm not sure how to improve this in general. Maybe should have a thread calling virEventRunDefaultImpl as the comment on virEventRunDefaultImpl suggest.

2011/8/1 Eric Blake <eblake(a)redhat.com&gt;: > On 07/28/2011 12:07 PM, Matthias Bolte wrote: >> 2011/7/27 Matthias Bolte <matthias.bolte(a)googlemail.com&gt;: >>> doRemoteClose doesn't free the virNetClientPtr and this creates a >>> 260kb leak per remote connection. This happens because >>> virNetClientFree doesn't remove the last ref, because virNetClientNew >>> creates the virNetClientPtr with a refcount of 2. >>> > >> The memory leak I saw was due to virsh calling >> virEventRegisterDefaultImpl, but not calling virEventRunDefaultImpl. >> Because the event loop is initialized, the call to >> virNetSocketAddIOCallback in virNetClientNew succeeds. But virsh not >> driving the event loop results in not removing callbacks that were >> marked for deletion. Finally this leaks the virNetClientPtr using in >> the remote driver. I used a virsh -c qemu:///system to test. >> >> I was able fix this by calling virEventRunDefaultImpl after >> virConnectClose in virsh. But I don't think that this is the correct >> general solution. > > Where do we stand with 0.9.4?  Is this something where we need the > general fix before the release, or is just the virsh hack to call > virEventRunDefaultImpl good enough for the release, or is this problem > not severe enough and we can release as-is? virsh is a bit special here as it registers/initializes the default even loop but doesn't drive it properly. It only drives it for the console command. That's the problem in virsh. This can be avoided by calling virEventRunDefaultImpl after virConnectClose iff it's a remote connection, in other cases virEventRunDefaultImpl might block. Therefore, we shouldn't be calling it in general after a virConnectClose in virsh. If we assume that an application that registers an event loop will drive it properly then this problem is not critical, as it doesn't exist. Just virsh is a special case that leaks 260kb per closed remote connection. When we assume that a typical virsh user uses a single connection per virsh invocation then we can view this as a static leak.

2011/8/2 Daniel P. Berrange <berrange(a)redhat.com&gt;: > On Mon, Aug 01, 2011 at 06:35:21PM +0200, Matthias Bolte wrote: >> 2011/8/1 Eric Blake <eblake(a)redhat.com&gt;: >> > On 07/28/2011 12:07 PM, Matthias Bolte wrote: >> >> 2011/7/27 Matthias Bolte <matthias.bolte(a)googlemail.com&gt;: >> >>> doRemoteClose doesn't free the virNetClientPtr and this creates a >> >>> 260kb leak per remote connection. This happens because >> >>> virNetClientFree doesn't remove the last ref, because virNetClientNew >> >>> creates the virNetClientPtr with a refcount of 2. >> >>> >> > >> >> The memory leak I saw was due to virsh calling >> >> virEventRegisterDefaultImpl, but not calling virEventRunDefaultImpl. >> >> Because the event loop is initialized, the call to >> >> virNetSocketAddIOCallback in virNetClientNew succeeds. But virsh not >> >> driving the event loop results in not removing callbacks that were >> >> marked for deletion. Finally this leaks the virNetClientPtr using in >> >> the remote driver. I used a virsh -c qemu:///system to test. >> >> >> >> I was able fix this by calling virEventRunDefaultImpl after >> >> virConnectClose in virsh. But I don't think that this is the correct >> >> general solution. >> > >> > Where do we stand with 0.9.4?  Is this something where we need the >> > general fix before the release, or is just the virsh hack to call >> > virEventRunDefaultImpl good enough for the release, or is this problem >> > not severe enough and we can release as-is? >> >> virsh is a bit special here as it registers/initializes the default >> even loop but doesn't drive it properly. It only drives it for the >> console command. That's the problem in virsh. This can be avoided by >> calling virEventRunDefaultImpl after virConnectClose iff it's a remote >> connection, in other cases virEventRunDefaultImpl might block. >> Therefore, we shouldn't be calling it in general after a >> virConnectClose in virsh. >> >> If we assume that an application that registers an event loop will >> drive it properly then this problem is not critical, as it doesn't >> exist. Just virsh is a special case that leaks 260kb per closed remote >> connection. When we assume that a typical virsh user uses a single >> connection per virsh invocation then we can view this as a static >> leak. > > Yeah, this is a case I never thought of. The "easy" fix is to just > make virsh spawn a new thread to run the event loop in the background. The problem here will probably be the console command with this lines while (!con->quit) { if (virEventRunDefaultImpl() < 0) break; } in console.c. Having two threads calling virEventRunDefaultImpl probably isn't a good idea.

> The "hard" fix is to make virsh I/O entirely event driven, so that > we don't just sit in a blocking read of stdin waiting for input, > but instead use the event loop to process stdin.