From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] fix race condition between libvirtd event handling and libxl fd deregister Date: Tue, 15 Jan 2013 10:40:38 +0000 Message-ID: <1358246438.15691.45.camel@zakaz.uk.xensource.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <50F05115.7040108@suse.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: Jim Fehlig Cc: "xen-devel@lists.xen.org" , Ian Jackson , Bamvor Jian Zhang List-Id: xen-devel@lists.xenproject.org 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). Without those extra references I think the problem you describe below doesn't happen. > 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. > Some other thoughts included: 1) an API to remove fd/timers from libxl, > 2) ensure no callbacks are invoked from libxl_ctx_free(). > > Thanks! > Jim >