Re: [libvirt] [PATCH v2 2/2] remote: free client all event callbacks at remoteClientCloseFunc

On 11/11/2017 03:20 AM, xinhua.Cao wrote: > > 在 2017/11/11 5:43, John Ferlan 写道: >> On 11/06/2017 12:47 AM, xinhua.Cao wrote: >>> base on commit fe8f1c8b8650c8ab5e7d27338f4538582651bd14, we solve >>> libvirt coredump problem, but it introduce a memory leak sense: >>> >>> 1. one client register a domain event such as reboot event >>> 2. and client was terminated unexpectly, such as kill -9, >> unexpectedly >> >>> then this client object will not free at libvirtd service program. >>> >>> remoteDispatchConnectDomainEventCallbackRegisterAny reference the >>> client, >>> but when client was terminated before it call deRegisterAny,the >>> reference >>> of client will not reduced to zero. so the memory leak take place. >> What's the difference between the path that would terminate normally and >> the one that terminates abnormally. It seems to me there might be (an) >> extra Ref(s) on the client which prevents virNetServerClientDispose from >> calling it's @privateDataFreeFunc which would call remoteClientFreeFunc >> since remoteClientInitHook worked properly. >> >> That's what needs to be found. > If client terminate normally, client would call > virConnectDomainEventDeregisterAny to > clear eventCallbacks. Then libvirt daemon will call > remoteDispatchConnectDomainEventDeregisterAny > -> remoteEventCallbackFree -> virObjectUnref(callback->client) to free > eventCallbacks and unRef client. > just think if client don't call  virConnectDomainEventDeregisterAny and > be killed. then there is no way to call call > remoteDispatchConnectDomainEventDeregisterAny -> remoteEventCallbackFree > -> virObjectUnref(callback->client) > so client's ref can't down to zero. so remoteClientFreeFunc can't be > called. then client object memory leak. So the client/sample code is not calling Deregister because it has no "handling" of an abnormal exit. That is no signal handler to call Deregister in the event some outside force kills the client/sample code while it's processing. So a different solution would be to add signal handling to the code in order to handle the kill and make the Deregister (and close) calls.

> The difference is call remoteDispatchConnectDomainEventDeregisterAny or > don't call remoteDispatchConnectDomainEventDeregisterAny > to Unref(callback->client). > IIUC, the whole reason the Deregister code is in the Free Func is to "handle" the case where that last client reference was Unref'd, but someone didn't perform the actual Deregister. The logic predates my libvirt involvement... Still because of commit id 'fe8f1c8b' where we generate a REF for the Register and that's transparent to the consumer (e.g. how would they know they need to ensure that Deregister is called), thus the purpose of this patch is to find a way to Deregister if it's determined that the consumer hasn't by the time of the "last" REF we'd have [trying to write a commit message here too]. This solution to this problem is to alter the processing to have the remoteClientCloseFunc handle performing the Deregister calls instead of the remoteClientFreeFunc because there's no way FreeFunc would be called unless the Deregister was already called. In fact, if you think about it if the CloseFunc does the Deregister why would the FreeFunc also include that call? IOW: It should only need to be in one or the other. I know you've already posted patch v3; however, I also think you need to read commit id '8294aa0c'. If you're moving the Deregister handling to the close func, then I would think that the "reason" for that commit still applies. When you generate a v4, my suggestion is to generate 2 patches. 1. Just the remoteClientFreePrivateCallbacks separation including the sysident handling. From my read of the code, the virConnectClose doesn't have the same issue.

2. Move the call to remoteClientFreePrivateCallbacks from FreeFunc to CloseFunc with a commit message that explains why this is necessary since commit id 'fe8f1c8b' (BTW: No need to supply the complete hash, 8 significant digits are usually plenty).

I supplied a bit more details w/r/t what I found for REF/UNREF - I assume it essentially matches what you've found. But having it here certainly helps - so thanks for providing it along with the example. > here is my test code > > #include<stdio.h> > #include<pthread.h> > #include<libvirt/libvirt.h> > > int _DomainStatusCallback(virConnectPtr conn, virDomainPtr dom, int event, >         int detail, void *opaque); > void timeOutCallback(int timer, void *opaque); > > int run = 1; > > int _DomainStatusCallback(virConnectPtr conn, virDomainPtr dom, int event, >         int detail, void *opaque) > { >     char uuid[128] = { 0 }; >     (void) virDomainGetUUIDString(dom, uuid); >     printf("domain %s come a event %d\n", uuid, event); > } > > void _timeOutCallback(int timer, void *opaque) > { >     printf("time out is comming\n"); > } > > void *event_thread(void *opaque) > { >     int i =0; >     if (virEventRegisterDefaultImpl() < 0) >     { >         return NULL; >     } > >     virConnectPtr conn = virConnectOpen("qemu:///system"); >     if (conn == NULL) >     { >         return NULL; >     } > >     int callback = virConnectDomainEventRegisterAny(conn, NULL, > VIR_DOMAIN_EVENT_ID_LIFECYCLE, >             _DomainStatusCallback, NULL, NULL); >     int timeout = virEventAddTimeout(1000,_timeOutCallback,NULL,NULL); > >     while (run && virConnectIsAlive(conn) == 1) >     { >        virEventRunDefaultImpl(); >     } > >     sleep(5); >     virConnectDomainEventDeregisterAny(conn, callback); >     virConnectClose(conn); > } > > > int main(int argc, char *argv[]) > { >     pthread_t pid; >     int ret = 0; >     ret = pthread_create(&pid, NULL, event_thread, NULL); >     sleep(100); >     run = 0; >     sleep(10); > } > > when I don't call remoteClientFreePrivateCallbacks at > remoteClientCloseFunc, > here is client refs change when I kill the client. > (gdb) c > Continuing. > Hardware watchpoint 6: *(int *) 0x5637edc888f4 > > Old value = 4 > New value = 5 > virObjectRef (anyobj=anyobj@entry=0x5637edc888f0) at util/virobject.c:296 > 296         PROBE(OBJECT_REF, "obj=%p", obj); > (gdb) bt > #0  virObjectRef (anyobj=anyobj@entry=0x5637edc888f0) at > util/virobject.c:296 > #1  0x00005637ec61d643 in > remoteDispatchConnectDomainEventCallbackRegisterAny (server=<optimized > out>, msg=<optimized out>, >     ret=0x7f054c0008e0, args=0x7f054c0008c0, rerr=0x7f057ba6cc90, > client=0x5637edc888f0) at remote.c:4418 Although the line numbers are different with top of tree, this is the RegisterAny REF on the client... [...] So that says to me we generally run with 3-4 REFs (without any Register Event occurring). AFAICT, the following is the list: 1. Adding to the srv->clients (virNetServerAddClient) 2. Adding IO Callback (virNetServerClientRegisterEvent) 3. Adding Keep Alive (virNetServerClientInitKeepAlive and virKeepAliveStart) The 4th is a bit trickier, but I believe it's REF'd during the call to virNetServerClientDispatchRead... > Old value = 5 > New value = 4 > virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259 > 259         PROBE(OBJECT_UNREF, "obj=%p", obj); > Continuing. > [Switching to Thread 0x7f058eb1f840 (LWP 3878)] > Hardware watchpoint 9: *(int *) 0x5637edc88634 I'm assuming this comes from the opposite end of the DispatchRead - there's no trace here to prove it, but that's not necessarily surprising either as it's not easy to find where the Unref would naturally occur without an error path occurring.... > Old value = 4 > New value = 5 > virObjectRef (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:296 > 296         PROBE(OBJECT_REF, "obj=%p", obj); > #0  virObjectRef (anyobj=anyobj@entry=0x5637edc88630) at > util/virobject.c:296 > #1  0x00005637ec65299d in virNetServerClientClose > (client=0x5637edc88630) at rpc/virnetserverclient.c:978 [...] > Old value = 5 > New value = 4 > virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:259 > 259         PROBE(OBJECT_UNREF, "obj=%p", obj); > #0  virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at > util/virobject.c:259 > #1  0x00005637ec6529bd in virNetServerClientClose > (client=0x5637edc88630) at rpc/virnetserverclient.c:982 This pair of REF/UNREF in virNetServerClientClose when client->keepalive is set. [...] > Old value = 4 > New value = 5 > virObjectRef (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:296 > 296         PROBE(OBJECT_REF, "obj=%p", obj); > #0  virObjectRef (anyobj=anyobj@entry=0x5637edc88630) at > util/virobject.c:296 > #1  0x00005637ec6529d1 in virNetServerClientClose > (client=0x5637edc88630) at rpc/virnetserverclient.c:987 [...] > Old value = 5 > New value = 4 > virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:259 > 259         PROBE(OBJECT_UNREF, "obj=%p", obj); > #0  virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at > util/virobject.c:259 > #1  0x00005637ec6529ee in virNetServerClientClose > (client=0x5637edc88630) at rpc/virnetserverclient.c:991 This pair of REF/UNREF in virNetServerClientClose when client->privateDataCloseFunc is set. [...] > Old value = 4 > New value = 3 > virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at util/virobject.c:259 > 259         PROBE(OBJECT_UNREF, "obj=%p", obj); > #0  virObjectUnref (anyobj=anyobj@entry=0x5637edc88630) at > util/virobject.c:259 > #1  0x00005637ec650705 in virNetServerProcessClients > (srv=0x5637edc6bf10) at rpc/virnetserver.c:873 Again my line numbers are off a bit, but this would be the for the UNREF after VIR_DELETE_ELEMENT when removed from srv->clients [...] > Old value = 3 > New value = 2 > virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259 > 259         PROBE(OBJECT_UNREF, "obj=%p", obj); > #0  virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259 > #1  0x00007f058de4199b in virObjectUnref (anyobj=<optimized out>) at > util/virobject.c:265 > #2  0x00007f058de0db34 in virEventPollCleanupTimeouts () at > util/vireventpoll.c:543 [...] This one is the opposite end of the virObjectRef done for the keep alive timer. See virNetServerClientInitKeepAlive and virKeepAliveStart usage of virEventAddTimeout. > Old value = 2 > New value = 1 > virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259 > 259         PROBE(OBJECT_UNREF, "obj=%p", obj); > #0  virObjectUnref (anyobj=0x5637edc88630) at util/virobject.c:259 > #1  0x00005637ec656bee in virNetSocketEventFree > (opaque=opaque@entry=0x5637edc8a5b0) at rpc/virnetsocket.c:2152 > #2  0x00007f058de0dd34 in virEventPollCleanupHandles () at > util/vireventpoll.c:592 [...] This one is the opposite end of the virObjectRef done in virNetServerClientRegisterEvent prior to virNetSocketAddIOCallback which calls virEventAddHandle. Theoretically the "last" REF... John > Continuing. > > so the client refs alway have 1 when I register one event callbacks. >>> this patch will deRegister all event callbacks when client was close. >>> --- >> FWIW: This commit message was a bit difficult to follow - I know it's >> primarily a language barrier thing - so maybe if you just construct some >> function code flow for both initialization/open and destruction/close >> it'd help... Paths that work and the path that you think is broken... >> That can be done in this section after the "---" and before the changed >> API's. >> >> I assume you've done a lot of research, but the details of that research >> can be difficult to just "pick up on".  Typically someone will provide >> some sort of valgrind output as an example. >> >> I just have a sense that this could boil down to a path (or two) that >> are doing the virObjectRef without the expected virObjectUnref occurring >> - something perhaps to do with how @wantClose is handled in the abnormal >> termination case. Nothing jumps out at me though even after looking at >> the code for a while. >> >> >>>   daemon/remote.c | 46 ++++++++++++++++++++++++++++------------------ >>>   1 file changed, 28 insertions(+), 18 deletions(-) >>> >>> diff --git a/daemon/remote.c b/daemon/remote.c >>> index cbcb6e8..466b2e7 100644 >>> --- a/daemon/remote.c >>> +++ b/daemon/remote.c >>> @@ -1689,23 +1689,10 @@ void >>> remoteRelayConnectionClosedEvent(virConnectPtr conn ATTRIBUTE_UNUSED, >>> int r >>>           neventCallbacks = 0; \ >>>       } while (0); >>>   -/* >>> - * You must hold lock for at least the client >>> - * We don't free stuff here, merely disconnect the client's >>> - * network socket & resources. >>> - * We keep the libvirt connection open until any async >>> - * jobs have finished, then clean it up elsewhere >>> - */ >>> -void remoteClientFreeFunc(void *data) >>> +static void >>> +remoteClientFreePrivateCallbacks(struct daemonClientPrivate *priv) >>>   { >>> -    struct daemonClientPrivate *priv = data; >>> - >>> -    /* Deregister event delivery callback */ >>> -    if (priv->conn) { >>> -        virIdentityPtr sysident = virIdentityGetSystem(); >>> - >>> -        virIdentitySetCurrent(sysident); >>> - >>> +    if (priv && priv->conn) { >> Well each of the callers has already dereferenced @priv so it's a bit >> too late to check it now! > yes, I also think this check is too late. this placement can only check > priv->conn. >>>           DEREG_CB(priv->conn, priv->domainEventCallbacks, >>>                    priv->ndomainEventCallbacks, >>>                    virConnectDomainEventDeregisterAny, "domain"); >>> @@ -1730,16 +1717,38 @@ void remoteClientFreeFunc(void *data) >>> >>> remoteRelayConnectionClosedEvent) < 0) >>>                   VIR_WARN("unexpected close callback event >>> deregister failure"); >>>           } >>> +    } >>> +} >>> +#undef DEREG_CB >>> + >> There should be another blank line here. > OK, I lost a blank line. >>> +/* >>> + * You must hold lock for at least the client >>> + * We don't free stuff here, merely disconnect the client's >>> + * network socket & resources. >>> + * We keep the libvirt connection open until any async >>> + * jobs have finished, then clean it up elsewhere >>> + */ >>> +void remoteClientFreeFunc(void *data) >>> +{ >>> +    struct daemonClientPrivate *priv = data; >>> + >>> +    /* Deregister event delivery callback */ >>> +    if (priv->conn) { >>> +        virIdentityPtr sysident = virIdentityGetSystem(); >>> + >>> +        virIdentitySetCurrent(sysident); >>> + >>> +        remoteClientFreePrivateCallbacks(priv); >>>             virConnectClose(priv->conn); >>>             virIdentitySetCurrent(NULL); >>>           virObjectUnref(sysident); >>> -    } >>>   +    } >>>       VIR_FREE(priv); >>> + >>>   } >>> -#undef DEREG_CB >>>       static void remoteClientCloseFunc(virNetServerClientPtr client) >>> @@ -1747,6 +1756,7 @@ static void >>> remoteClientCloseFunc(virNetServerClientPtr client) >>>       struct daemonClientPrivate *priv = >>> virNetServerClientGetPrivateData(client); >>>         daemonRemoveAllClientStreams(priv->streams); >>> +    remoteClientFreePrivateCallbacks(priv); >> Perhaps this just gets a "if (priv->conn)" before calling... >> >> Still if as I assume the missing Unref is found (someday), then >> theoretically remoteClientFreeFunc would be called - of course the >> nEventCallbacks = 0 will ensure the Dereg's dont' get called again. So >> it shoudn't be a problem > In my opinion, it is safe to check point is Null. But check everywhere > is stupid. > so it is hard to determine where need check.:-) >> John >> >>>   } >>> >> . >> > .