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

Zhang, Yang Z
2013-01-08T12:57:45Z

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()

Best regards,
Yang