Re: [KVM PATCH v2 2/2] kvm: use POLLHUP to close an irqfd instead of an explicit ioctl

Gregory Haskins
2009-06-18T14:03:53Z

[-- Attachment #1: Type: text/plain, Size: 2632 bytes --]

Michael S. Tsirkin wrote:
> On Thu, Jun 18, 2009 at 08:00:39AM -0400, Gregory Haskins wrote:
>
>> Michael S. Tsirkin wrote:
>>
>>
>>> BTW, Gregory, this can be used to fix the race in the design: create a
>>> thread and let it drop the module reference with module_put_and_exit.
>>>
>>>
>> I had thought of doing something like this initially too, but I think
>> its racy as well. Ultimately, you need to make sure the eventfd
>> callback is completely out before its safe to run, and deferring to a
>> thread would not change this race. The only sane way I can see to do
>> that is to have the caller infrastructure annotate the event somehow
>> (either directly with a module_put(), or indirectly with some kind of
>> state transition that can be tracked with something like
>> synchronize_sched().
>>
>
> Here's what one could do: create a thread for each irqfd, and increment
> module ref count, put that thread to sleep. When done with
> irqfd, don't delete it and don't decrement module refcount, wake thread
> instead. thread kills irqfd and calls module_put_and_exit.
>
> I don't think it's racy

I believe it is. How would you prevent the thread from doing the
module_put_and_exit() before the eventfd callback thread is known to
have exited the relevant .text section?

All this talk does give me an idea, tho. Ill make a patch.

>
>>> Which will work, but I guess at this point we should ask ourselves
>>> whether all the hearburn with srcu, threads and module references is
>>> better than just asking the user to call and ioctl.
>>>
>>>
>> I am starting to agree with you, here. :)
>>
>> Note one thing: the SRCU stuff is mostly orthogonal from the rest of the
>> conversation re: the module_put() races. I only tied it into the
>> current thread because the eventfd_notifier_register() thread gave me a
>> convenient way to hook some other context to do the module_put(). In
>> the long term, the srcu changes are for the can_sleep() stuff. So on
>> that note, lets see if I can convince Davide that the srcu stuff is not
>> so evil before we revert the POLLHUP patches, since the module_put() fix
>> is trivial once that is in place.
>>
>
> Can this help with DEASSIGN as well? We need it for migration.
>

No, but afaict you do not need this for migration anyway. Migrate the
GSI and re-call kvm_irqfd() on the other side. Would the fd even be
relevant across a migration anyway? I would think not, but admittedly I
know little about how qemu/kvm migration actually works.

Regards,
-Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]