Gleb Natapov wrote on 2013-01-08: > On Mon, Jan 07, 2013 at 07:32:39PM -0200, Marcelo Tosatti wrote: >> On Mon, Jan 07, 2013 at 07:48:43PM +0200, Gleb Natapov wrote: >>>> ioapic_write (or any other ioapic update) >>>> lock() >>>> perform update >>>> make_all_vcpus_request(KVM_REQ_UPDATE_EOI_BITMAP) (*) >>>> unlock() >>>> >>>> (*) Similarly to TLB flush. >>>> >>>> The advantage is that all work becomes vcpu local. The end result >>>> is much simpler code. >>> What complexity will it remove? >> >> Synchronization between multiple CPUs (except the KVM_REQ_ bit >> processing, which is infrastructure shared by other parts of KVM). >> > Synchronization is just a lock around bitmap access. Can be replaced > with RCU if it turns to be performance problem. > >> We agreed that performance is non issue here. > Yes, if the code is indeed simpler we can take the hit, although > recalculating bitmap 255 times instead of one for -smp 255 looks like a > little bit excessive, but I do not see considerable simplification (if > at all). > > So as far as I understand you are proposing: > > vcpu0 or io thread: | vcpu1: > ioapic_write (or other ioapic update) | > lock(exitbitmap) | > if (on vcpu) | > ioapic_update_my_eoi_exitmap() | > make_all_vcpus_request(update) | if (update requested) > | > ioapic_update_my_eoi_exitmap() > unlock(exitbitmap) | > The current patch logic is this: > > vcpu0 or io thread: | vcpu1: > ioapic_write (or other ioapic update) | > lock(exitbitmap) | > ioapic_update_all_eoi_exitmaps() | > make request on each vcpu | > kick each vcpu | if (update requested) > unlock(exitbitmap) | lock(exitbitmap) > | load_exitbitmap() > | unlock(exitbitmap) > If I described correctly what you are proposing I do not > see simplification since the bulk of the complexity is in the > ioapic_update_(my|all)_eoi_exitmap() and they will be the same in both > implementations. Actually I do see complication in your idea introduced > by the fact that the case when update is done from vcpu thread have to > be handled specially. > > The proposed patch may be simplified further by > make_all_vcpus_request_async(update)(*) instead of making request and > kicking each vcpu individually. In fact the way it is done now is buggy > since requests are made only for vcpus with bit set in their bitmask, > but if bit is cleared request is not made so vcpu can run with stale > bitmask. ok, how about the follow logic: ioapic_write() lock() clear_eoi_exitmap_on_all_vcpus() perform update(no make request) make_all_vcpus_request(like tlb flush) unlock() Best regards, Yang -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
