On Tue, Jan 08, 2013 at 12:03:32PM +0200, Gleb Natapov wrote:
> 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() | <------- (1)
XXX
> 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 simplification is you don't remotely update the EOI exit bitmaps.
Its obvious that updating data locally (that is, the context that
updates data is the only user of that data) is simpler.
Yes, ioapic_update_exitmap is the same implementation, but there is no
concern over the state of the remote vcpu.
For example, you don't have to worry whether code such as this
+static void vmx_update_exitmap_start(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_vmx *vmx = to_vmx(vcpu);
+
+ memset(vmx->eoi_exit_bitmap, 0, 32);
+ spin_lock(&vmx->eoi_bitmap_lock);
+}
is safe.
Because eoi_exit_bitmap is only accessed by the vcpu itself.
Does that make sense?
> 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.
--
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
