On 11/10/2017 10:08 AM, Erik Skultety wrote:
> On Tue, Nov 07, 2017 at 09:39:54PM -0500, John Ferlan wrote:
>> Whether the @srv/@srvAdm is added to the dmn->servers list or not,
>> the reference kept for the allocation can be dropped leaving just the
>> reference for being on the dmn->servers list be the sole deciding
>> factor when to really free the associated memory. The @dmn dispose
>> function (virNetDaemonDispose) will handle the Unref of the objects
>> during the virHashFree.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> daemon/libvirtd.c | 15 +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
>> index 73f24915df..5c47e49d48 100644
>> --- a/daemon/libvirtd.c
>> +++ b/daemon/libvirtd.c
>> @@ -1065,6 +1065,7 @@ int main(int argc, char **argv) {
>> char *remote_config_file = NULL;
>> int statuswrite = -1;
>> int ret = 1;
>> + int rc;
>> int pid_file_fd = -1;
>> char *pid_file = NULL;
>> char *sock_file = NULL;
>> @@ -1319,7 +1320,11 @@ int main(int argc, char **argv) {
>> goto cleanup;
>> }
>>
>> - if (virNetDaemonAddServer(dmn, srv) < 0) {
>> + /* Add @srv to @dmn servers hash table and Unref to allow removal from
>> + * hash table at @dmn disposal to decide when to free @srv */
>> + rc = virNetDaemonAddServer(dmn, srv);
>
> <rant>
> Sorry for the delay, I've been playing with LXC for 2 days, trying to either
> debug or just use valgrind on lxc_controller to get the info I need
> (explanation below), but after those 2 days, I just give up, not worth any more
> time, if someone knows how I can actually hit the cleanup section in
> lxc_controller within gdb telling me that it suddenly temporarily disabled
> breakpoints at the moment it's supposed to stop at one that it had let me to set
> earlier, I'm all ears. Btw to my biggest surprise valgrind still doesn't handle
> setns syscall which has probably been the reason why we forbade running LXC
> under valgrind in 2009, unbelievable. </rant>
I feel your pain ;-) Chasing these memory things is challenging
especially when variable names change or things just aren't consistent.
They're a real time sync too - that whole NetDaemon, NetServer,
NetServerClient, etc. code is a beast to understand. I only have
figured out a small piece of it...
>
> Anyhow, based purely on a visual perception (sigh...), it looks like that the
> LXC "server" is leaked in lxc_controller.c because virLXCControllerSetupServer
> creates the object, calls virNetDaemonAddServer which increments @refcnt, but I
> don't see two corresponding virObjectUnrefs (that's the thing, once you don't
> run gdb or valgrind, you can't be sure whether this suspicion is right or
> not ...).
So let's see - I see the 2 REF's on @srv as you do in main(). The
ctrl->daemon->servers will be UNREF'd in virNetDaemonDispose when the
last Unref of ctrl->daemon is done in virLXCControllerFree via the call
to virHashFree... BTW: Chasing that is painful, but once the server is
removed from the hash table it does end up calling an Unref when
virObjectFreeHashData is called (e.g. the 2nd arg to virHashCreate).
This may be one of those cases where you need to add "temporary" debug
code to virObjectRef and virObjectUnref along with the address of the
object in order to see "when" it's Ref'd and Unref'd and what the count
is (I've done this using VIR_WARN and just perusing generated output).
It is painful... Don't forget to add debug code to this function that
would print the addr of the daemon and server object's so that you can
output everything to a file and then comb through the output looking for
when things are ref/unref'd and most importantly some free of an object
that didn't have an object it contains being free'd. Once you see the
daemon object free'd, but the @srv object still hanging on - you know
you're pretty safe in assuming there's a missing Unref...
Anyway, I agree with you upon visual inspection - there's a leak since
@srv is never Unref'd. Once it's added to the Server hash table, then
there should be a virObjectUnref(srv).
As for other consumers.... The reason that log_daemon doesn't do the
virObjectUnref is that it saves @srv and eventually will Unref it later.
Unlike the lock_daemon code which uses virNetDaemonGetServer to find the
@srv; whereas, log_daemon goes direct to logDaemon->srv.
>
> Now the actual review.
> virNetDaemonAddServer is only used when spawning a new daemon or setting up LXC
> controller. The function essentially does:
>
> lock(@dmn)
> hash_table_add(@srv)
> ref(@srv)
> unlock(@dmn)
>
> and then you unref @dmn right upon the completion of adding the server to the
> hash table, what's the purpose of the extra ref when you discard it
> immediately?
The REF is for the Hash Table. We've added something to the hash table -
we should increment the refcnt - nothing more nothing less. The UNREF
for that occurs as part of virHashFree because virHashCreate uses
virObjectFreeHashData.
The UNREF is for the allocation (or virNetServerNew) and an optimization
because we know it's in the Hash Table and wouldn't be removed from the
Hash Table until the virNetDaemonDispose calls virHashFree once the last
@dmn is UNREF'd.
Dropping this patch and the next one is fine by me, but the Unref's
below would be reinstated.
> Unless I'm failing to see something, I'd prefer to just drop the
> extra ref in virNetDaemonAddServer and leave the proposed hunks unchanged - you
> have 1 ref which you got by creating the object, now it should be just simply
> inserted into the hash table, the additional ref after insertion doesn't make
> sense, if someone managed to unref it before inserting it into the hash table,
> hash insert would most likely fail, if not, hashFree surely would, not
> mentioning that at that point there's no entity that is touching servers.
That messes up log_daemon as it's stores @srv in logDamon->srv and then
will virObjectUnref it virLogDaemonFree. If log_daemon didn't save a ref
of the object in logDaemon->srv, then it too could do a virObjectUnref
right away too.
>
>> + virObjectUnref(srv);
>> + if (rc < 0) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> @@ -1393,7 +1398,11 @@ int main(int argc, char **argv) {
>> goto cleanup;
>> }
>>
>> - if (virNetDaemonAddServer(dmn, srvAdm) < 0) {
>> + /* Add @srvAdm to @dmn servers hash table and Unref to allow removal from
>> + * hash table at @dmn disposal to decide when to free @srvAdm */
>> + rc = virNetDaemonAddServer(dmn, srvAdm);
>> + virObjectUnref(srvAdm);
>> + if (rc < 0) {
>> ret = VIR_DAEMON_ERR_INIT;
>> goto cleanup;
>> }
>> @@ -1509,8 +1518,6 @@ int main(int argc, char **argv) {
>> virObjectUnref(qemuProgram);
>> virObjectUnref(adminProgram);
>> virNetDaemonClose(dmn);
>> - virObjectUnref(srv);
>> - virObjectUnref(srvAdm);
>
> ^This hunk is of course fine.
>
> Erik
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
