Gleb Natapov wrote on 2013-01-08:
> 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
In my patch, it traverses IOAPIC entry once and only updates target vcpus's eoi exit bitmap. Then make request for all vcpus.
With your suggestion , all vcpus will traverse all IOAPIC entries. Though ioapic entry write is rare, it's still not reasonable.
>> ioapic_unlock();
>> }
>>
> Fine by me. Looks simpler.
>
>>
>>
>> (*) plus any other lock that paths that update the map take
>>
>>
>>
>>
>>
>>>
>>> Best regards,
>>> Yang
>
> --
> Gleb.
Best regards,
Yang
--
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
