On Tue, Jan 08, 2013 at 11:57:59AM -0200, Marcelo Tosatti wrote:
> 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();
> }
>
Fine by me. Looks simpler.
>
>
> (*) plus any other lock that paths that update the map take
>
>
>
>
>
> >
> > Best regards,
> > Yang
--
Gleb.
--
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
