On Thu, Aug 04, 2011 at 05:26:31PM +0200, Matthias Bolte wrote:
> 2011/8/2 Daniel P. Berrange <berrange@xxxxxxxxxx>:
> > On Mon, Aug 01, 2011 at 06:35:21PM +0200, Matthias Bolte wrote:
> >> 2011/8/1 Eric Blake <eblake@xxxxxxxxxx>:
> >> > On 07/28/2011 12:07 PM, Matthias Bolte wrote:
> >> >> 2011/7/27 Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>:
> >> >>> 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.
Oh yes, we'd have to change that code too, to delegate to the
background event loop thread.
> > 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.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list