Re: [PATCH v8 2/3] x86, apicv: add virtual interrupt delivery support

[Marcelo Tosatti <mtosatti@redhat.com>]

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