From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= Subject: Re: [PATCH v1 12/12] libxl: add device backend listener in order to launch backends Date: Wed, 6 Nov 2013 10:41:55 +0100 Message-ID: <527A0EE3.1040603@citrix.com> References: <1380705874-58491-1-git-send-email-roger.pau@citrix.com> <1380705874-58491-13-git-send-email-roger.pau@citrix.com> <21105.15127.428243.701439@mariner.uk.xensource.com> <5277D362.4040300@citrix.com> <21111.55145.501074.162529@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vdzcc-00039F-Uf for xen-devel@lists.xenproject.org; Wed, 06 Nov 2013 09:41:59 +0000 In-Reply-To: <21111.55145.501074.162529@mariner.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 Jackson Cc: xen-devel@lists.xenproject.org, Ian Campbell List-Id: xen-devel@lists.xenproject.org On 04/11/13 18:20, Ian Jackson wrote: > Roger Pau Monn=E9 writes ("Re: [PATCH v1 12/12] libxl: add device backend= listener in order to launch backends"): >> So if I got it right, this new libxl__nested_ao_create will return a new >> ao (with a new gc), that I could use in conjunction with the >> long-running ao that I use in the main xs_watch loop, right? > = > Yes. It would give you a new psuedo-ao, which you can use for > per-event memory allocation. It's a psuedo-ao in the sense that you > mustn't call libxl__ao_abort or libxl__ao_complete on it, but it would > have the right type and in particular you could stuff it in > sub-operations' ao fields, call STATE_AO_GC on it and so on. I could > make it possible to call libxl__ao_inprogress and have that reflected > to the underlyhing real ao. > = >> That sounds like a good solution to my problem, I wouldn't mind if you >> write that :) > = > OK, watch this space. > = >> I'm wondering if there are also other memory problems, even when using >> this approach, for example I register a xswatch callback, and the >> callback gets called with a watch_path and an event_path arguments, does >> the internal libxl event handler machinery reuse those (or allocate and >> free them after each loop)? > = > The event machinery gets those from a different gc which is > per-system-event, so that's not a problem. (Otherwise waiting for a a > particular thing in xenstore would involve memory growing endlessly > with calls to read from xenstore, ec.) > = >>>> + case LIBXL__DEVICE_KIND_VBD: >>>> + case LIBXL__DEVICE_KIND_VIF: >>>> + if (dev->backend_kind =3D=3D LIBXL__DEVICE_KIND_VBD) dguest->= num_vbds--; >>>> + if (dev->backend_kind =3D=3D LIBXL__DEVICE_KIND_VIF) dguest->= num_vifs--; >>> >>> Is it really safe to decrement these already ? What if something else >>> comes along in the meantime and makes num_devs 0 (below) and removes >>> everything while this operation is still running and liable to be >>> reentered on completion ? >> >> That's the point of decrementing it here, so that we get to 0 (if this >> is the last device), and remove the libxl__ddomain_guest and >> libxl__ddomain_device. Then, when the remove AO finishes, the AO >> callback will take care of removing the associated libxl__device. >> >> I thought backend_watch_callback could not be called concurrently, but >> maybe that's not true? (and if that's the case ignore everything above >> because it's completely wrong) > = > While you are _actually in this function_, you hold the Big Lock. So > nothing else can come along find the wrong value of num_*. > = > But what you actually do is call initiate_device_remove and then > return - ie, you return to the event loop. That gives up the lock, > obviously. So while the device removal is proceeding, other events > can occur. > = > If backend_watch_callback happens then, I think you may find that it > seems num_*=3D=3D0 and decides to tear down the state for that domain. The cleanup for the domain already happened, after we decrement num_* we return to the main backend_watch_callback (all holding the Big Lock), and libxl proceeds with the removal of the libxl__ddomain_guest if sum(num_*) =3D=3D 0. What happens in backend_watch_callback (with the Big Lock hold) is basically: - Decrement num_* - Check if sum(num_*) =3D=3D 0 -> cleanup all data for the domain - END If another device is added to the domain after the domain has been removed from the list (because sum(num_*) =3D=3D 0), a new libxl__ddomain_guest will be created, just like when a device for a new domain is added. > That would at the very least involve messing about in xenstore with > the device which is still being removed. Tearing down the domain just involves removing it's associated data structures, nothing is written to xenstore. > Then, later, the device removal will complete and device_complete will > be called. device_complete doesn't make use of either libxl__ddomain_device or libxl__ddomain_guest, during normal program flow device_complete will be called with both of the above data structures already freed. > I think you need to do the decrement in device_complete, and that > means you need a kind of "perhaps tidy up domain" function which you > can call from both there and backend_watch_callback. And you probably > need to provide some more useful pointers to device_complete. It's quite possible that I'm completely wrong, but I don't see a race with the current program flow.