[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] fix race condition between libvirtd event handling and libxl fd deregister

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 :/.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.