Michael S. Tsirkin wrote: > On Thu, Jun 18, 2009 at 12:29:31PM -0400, Gregory Haskins wrote: > >> Michael S. Tsirkin wrote: >> >>> On Thu, Jun 18, 2009 at 10:03:36AM -0400, Gregory Haskins wrote: >>> >>> >>>> 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? >>>> >>>> >>> Right. >>> >>> >>> >>>> All this talk does give me an idea, tho. Ill make a patch. >>>> >>>> >>> OK, but ask yourself whether this bag of tricks is worth it, and whether >>> we'll find another hole later. Let's reserve the trickiness for >>> fast-path, where it's needed, and keep at least the assign/deassign simple. >>> >>> >> Understood. OTOH, going back to the model where two steps are needed >> for close() is ugly too, so I don't want to just give up and revert that >> fix too easily. At some point we will call it one way or the other, but >> I am not there quite yet. >> >>> >>> >>>>> >>>>> >>>>> >>>>>>> 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. >>>> >>>> >>> Yes but that's not live migration. For live migration, the trick is that >>> you are running locally but send changes to remote guest. For that, we >>> need to put qemu in the middle between the device and the guest, so it >>> can detect activity and update the remote side. >>> >>> And the best way to do that is to take poll eventfd that device assigns >>> and push eventfd that kvm polls. To switch between this setup >>> and the one where kvm polls the ventfd from device directly, >>> you need deassign. >>> >>> >> So its still not clear why the distinction between >> deassign-the-gsi-but-leave-the-fd-valid is needed over a simple >> close(). Can you elaborate? >> > > > The fd needs to be left assigned to the device, so that we can poll > the fd and get events, then forward them to kvm. > Ah, ok. Now I get what you are trying to do. Well, per the PM I sent you this morning, I figured out the magic to resolve the locking issues. So we should be able to add DEASSIGN logic soon, I hope. -Greg
Attachment:
signature.asc
Description: OpenPGP digital signature
