On Tue, Jan 08, 2013 at 12:57:42PM +0000, Zhang, Yang Z wrote:
> 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()
Why not just
ioapic writer / map updater context
----------------------------------
ioapic_write()
make_all_vcpus_request()
(no special lock taken)
vcpu context, entry
------------------
if(check_request(KVM_REQ_, ....)) {
ioapic_lock(); (*)
update local EOI exit bitmap from IOAPIC
ioapic_unlock();
}
(*) plus any other lock that paths that update the map take
>
> 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
