From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH v8 2/3] x86, apicv: add virtual interrupt delivery support Date: Tue, 8 Jan 2013 11:57:59 -0200 Message-ID: <20130108135759.GB5121@amt.cnet> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Gleb Natapov , "kvm@vger.kernel.org" , "Shan, Haitao" , "Tian, Kevin" To: "Zhang, Yang Z" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:14064 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756181Ab3AHN6D (ORCPT ); Tue, 8 Jan 2013 08:58:03 -0500 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 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(); } (*) plus any other lock that paths that update the map take > > Best regards, > Yang