From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jim Fehlig Subject: Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister Date: Tue, 15 Jan 2013 10:02:40 -0700 Message-ID: <50F58BB0.1040904@suse.com> References: <0451e6041bdd88c90eee.1353395794@linux-bjrd.bjz> <20661.3989.258191.396175@mariner.uk.xensource.com> <1354101923.25834.16.camel@zakaz.uk.xensource.com> <20674.16214.934271.479230@mariner.uk.xensource.com> <1355134766.31710.119.camel@zakaz.uk.xensource.com> <20677.47995.298291.120095@mariner.uk.xensource.com> <20678.5159.946248.90947@mariner.uk.xensource.com> <1357904481.20328.42.camel@zakaz.uk.xensource.com> <50F05115.7040108@suse.com> <1358246438.15691.45.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1358246438.15691.45.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xen.org" , Ian Jackson , Bamvor Jian Zhang List-Id: xen-devel@lists.xenproject.org Ian Campbell wrote: > On Fri, 2013-01-11 at 17:51 +0000, Jim Fehlig wrote: > >> Ian Campbell wrote: >> >>> On Mon, 2012-12-10 at 16:56 +0000, Ian Jackson wrote: >>> >>> >>>> Ian Jackson writes ("Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister"): >>>> >>>> >>>>> I'm not surprised that the original patch makes Bamvor's symptoms go >>>>> away. Bamvor had one of the possible races (the fd-related one) but >>>>> not the other. >>>>> >>>>> >>>> Here (followups to this message, shortly) is v3 of my two-patch series >>>> which after conversation with Ian C I think fully fixes the race, and >>>> which I have tested now. >>>> >>>> >>> Is this version now tested and ready to be applied? >>> >>> >> Hi Ian, >> >> I have been doing quite a bit of testing with this version, but have one >> remaining issue wrt races between the libvirt libxl driver and libxl. >> Earlier in this thread you mentioned this potential solution >> >> "The other scheme which springs to mind is to do reference counting, with >> the application holding a reference whenever the event is present in its >> event loop (such that there is any chance of the event being generated) >> and libxl holding a reference while it considers the event to be active" >> >> I thought this was a good approach, particularly since libvirt has >> excellent support for it. When libxl registers an fd/timer, I create an >> object containing the details with an initial reference count of 1. If >> the fd/timer is successfully injected into libvirt's event loop, I take >> another reference on the object. The object is only destroyed after >> libxl has deregistered the fd/timer *and* it has been removed from >> libvirt's event loop. For each fd/timer object, I also increment the >> reference count on my libxl_ctx object. This approach works well IMO. >> It ensures the libxl_ctx exists for the life of all fd/timer objects. >> > > Is taking a reference count on the ctx for each fd/timer strictly > necessary? > > You can guarantee that the ctx lifetime is greater than the fd/timer > lifetime because if you were to destroy the ctx then it would teardown > the fd/timer as part of ctx_free (I think? More of an Ian J question). > Yes, but the teardown of timers in particular is asynchronous. libxl calls the modify timeout hook with abs_t of {0,0}, the timer fires on next iteration of event loop invoking the callback, which calls libxl_osevent_occurred_timeout() to finally cleanup the timeout on the libxl side. But in the meantime, the associated ctx has been freed. Taking a ref count on the ctx avoids this race. > Without those extra references I think the problem you describe below > doesn't happen. > Right, but then the ctx disappears before all fds/timers have been cleaned up. > >> The only wrench in this machinery is that watch_efd is not deregistered >> until calling libxl_ctx_free(). But I never get to that point since >> that fd registration holds a reference on my libxl_ctx :(. My first >> thought was to cleanup/deregister that fd on domain death, but I didn't >> have much success creating a patch. Perhaps I should look at that again... >> > > I'd be worried about libxl internal uses of this watch which you cannot > easily control preventing you from doing this. > Agreed :/. Jim