From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v8 2/3] x86, apicv: add virtual interrupt delivery support Date: Tue, 8 Jan 2013 17:43:14 +0200 Message-ID: <20130108154314.GA7005@redhat.com> References: <1357524157-4666-1-git-send-email-yang.z.zhang@intel.com> <1357524157-4666-3-git-send-email-yang.z.zhang@intel.com> <20130107135221.GA25775@amt.cnet> <20130107174843.GA4872@redhat.com> <20130107213239.GB31677@amt.cnet> <20130108100332.GI21250@redhat.com> <20130108135759.GB5121@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "Zhang, Yang Z" , "kvm@vger.kernel.org" , "Shan, Haitao" , "Tian, Kevin" To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:21229 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756683Ab3AHPnR (ORCPT ); Tue, 8 Jan 2013 10:43:17 -0500 Content-Disposition: inline In-Reply-To: <20130108135759.GB5121@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 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.