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

[Gleb Natapov <gleb@redhat.com>]

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.