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

[Gleb Natapov <gleb@redhat.com>]

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.

(*) not exists yet as far as I see.

--
			Gleb.