From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: KVM: test for LAPIC timer migration request after disabling IRQ's Date: Mon, 19 May 2008 14:32:03 +0300 Message-ID: <48316533.4010608@qumranet.com> References: <20080517142550.GA9738@dmt> <482FCE22.6000508@qumranet.com> <20080518164742.GB10613@dmt> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Chris Wright To: Marcelo Tosatti Return-path: Received: from bzq-179-150-194.static.bezeqint.net ([212.179.150.194]:40793 "EHLO il.qumranet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754899AbYESLcH (ORCPT ); Mon, 19 May 2008 07:32:07 -0400 In-Reply-To: <20080518164742.GB10613@dmt> Sender: kvm-owner@vger.kernel.org List-ID: Marcelo Tosatti wrote: > On Sun, May 18, 2008 at 09:35:14AM +0300, Avi Kivity wrote: > >> Marcelo Tosatti wrote: >> >>> (resending to kvm@vger.kernel.org) >>> >>> A guest vcpu instance can be scheduled to a different physical CPU >>> between the test for KVM_REQ_MIGRATE_TIMER and local_irq_disable(). >>> >>> If that happens, the timer will only be migrated to the current pCPU on >>> the next exit, meaning that guest LAPIC timer event can be delayed until >>> a host timer interrupt is triggered. >>> >>> Fix that by testing the migration request after local_irq_disable(), >>> similarly to what is done for KVM_REQ_MMU_RELOAD. >>> >>> >>> >> An alternative fix is to have __apic_timer_fn() call >> smp_call_function_single() to the current vcpu->cpu with a dummy >> function (not saying it's better). >> > > smp_call_function_single() can't function from IRQ context (there is a > global lock protecting internal data and waiting for a remote CPU to > finish a function from IRQ context does not sound good). Remember the > vcpu_kick() in the pit timer handler? :) > > Right you are. >> How about >> >> >> if (vcpu->requests) { >> local_irq_enable(); >> preempt_enable(); >> r = 1; >> goto out; >> } >> >> ? >> >> It ensures that we never enter guest mode with _any_ pending request, so >> long as request bits are set on vcpu->cpu or are followed by an IPI. I >> think it fixes a similar race with KVM_REQ_TLB_FLUSH. >> > > Yes doing it for any pending requests sounds much better. > > As for KVM_REQ_TLB_FLUSH, the check happens after local_irq_disable() > already. Which is sort of problematic in that requests arriving after > entering the IRQ-disabled section but before the test_and_clear_bit() > will run ->tlb_flush but won't clear the pending IPI, so there's an exit > for nothing. Probably uncommon though. > > True. The main advantage of my suggestion is that control flow is simpler, not any performance or correctness benefit. -- error compiling committee.c: too many arguments to function