From patchwork Fri Feb 14 11:24:42 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Bader X-Patchwork-Id: 3652051 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 4F0D2BF13A for ; Fri, 14 Feb 2014 11:26:04 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 661AB20221 for ; Fri, 14 Feb 2014 11:26:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7CF1820160 for ; Fri, 14 Feb 2014 11:25:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752220AbaBNLZK (ORCPT ); Fri, 14 Feb 2014 06:25:10 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:58861 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751772AbaBNLZH (ORCPT ); Fri, 14 Feb 2014 06:25:07 -0500 Received: from hsi-kbw-078-043-113-216.hsi4.kabel-badenwuerttemberg.de ([78.43.113.216] helo=[192.168.2.5]) by youngberry.canonical.com with esmtpsa (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1WEGst-00020U-LT; Fri, 14 Feb 2014 11:24:43 +0000 Message-ID: <52FDFCFA.906@canonical.com> Date: Fri, 14 Feb 2014 12:24:42 +0100 From: Stefan Bader User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Peter Zijlstra CC: Borislav Petkov , Paolo Bonzini , Linux Kernel Mailing List , kvm@vger.kernel.org, Marcelo Tosatti , MASAO TAKAHASHI , Joerg Roedel Subject: Re: Another preempt folding issue? References: <52FA6D4B.7020709@canonical.com> <20140211194553.GZ9987@twins.programming.kicks-ass.net> <52FB2EC8.4080602@canonical.com> <20140212103713.GE3545@laptop.programming.kicks-ass.net> <20140212104017.GA5121@pd.tnic> <52FB5669.7090506@canonical.com> <20140212115412.GW27965@twins.programming.kicks-ass.net> <52FCFA23.4060701@canonical.com> <20140213182522.GB14089@laptop.programming.kicks-ass.net> In-Reply-To: <20140213182522.GB14089@laptop.programming.kicks-ass.net> X-Enigmail-Version: 1.5.2 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 13.02.2014 19:25, Peter Zijlstra wrote: > On Thu, Feb 13, 2014 at 06:00:19PM +0100, Stefan Bader wrote: >> On 12.02.2014 12:54, Peter Zijlstra wrote: >>> On Wed, Feb 12, 2014 at 12:09:29PM +0100, Stefan Bader wrote: >>>> Something else here I run a kernel with CONFIG_PREEMPT not set and NR_CPUS >>>> limited to 8 (for the 32bit kernel). So the default apic driver is used. Since >>>> default_send_IPI_mask_logical is only used from there, I assume the trace you >>>> got does the same. Maybe something there is wrong which would explain why we >>>> only see it on 32bit hosts. >>> >>> Can you try with a different APIC driver to test this? >>> >> I don't think I can. And I think the statement about this only be used for 32bit >> could be wrong. I got mislead to think so because those are only defined in >> probe_32 but the 64bit counterpart isn't containing much aside that. >> >> Anyway, I played around with tracing a bit more. So with this change: >> >> if (need_resched()) { >> srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx); >> if (need_resched() != should_resched()) { >> + trace_printk("need(%i) != should(%i)\n", >> + need_resched(), should_resched()); >> + trace_printk("exit_reason=%u\n", >> + vcpu->run->exit_reason); >> + trace_printk("preempt_count=%lx\n", >> + __this_cpu_read_4(__preempt_count)); >> + tracing_stop(); >> + printk(KERN_ERR "Stopped tracing, due to >> inconsistent state.\n"); >> } >> + schedule(); >> - cond_reschedule(); >> vcpu->srcu_idx = srcu_read_lock(&kvm->srcu); >> } >> >> I get the following (weird) output: >> >> Xorg-1078 [001] d... 71.270251: native_smp_send_reschedule >> <-resched_task >> Xorg-1078 [001] d... 71.270251: default_send_IPI_mask_logical >> <-native_smp_send_reschedule >> bamfdaemon-2318 [001] d... 71.270465: resched_task <-check_preempt_wakeup >> bamfdaemon-2318 [001] d... 71.270539: resched_task <-check_preempt_wakeup >> compiz-2365 [001] d... 71.270689: resched_task <-check_preempt_wakeup >> compiz-2365 [001] d... 71.270827: resched_task <-check_preempt_wakeup >> compiz-2365 [001] d... 71.270940: resched_task <-check_preempt_wakeup >> qemu-system-x86-2679 [000] dn.. 71.270999: smp_reschedule_interrupt >> <-reschedule_interrupt >> qemu-system-x86-2679 [000] dn.. 71.270999: scheduler_ipi >> <-smp_reschedule_interrupt >> qemu-system-x86-2679 [000] .N.. 71.271001: kvm_arch_vcpu_ioctl_run: need(1) >> != should(0) >> qemu-system-x86-2679 [000] .N.. 71.271002: kvm_arch_vcpu_ioctl_run: >> exit_reason=2 >> qemu-system-x86-2679 [000] .N.. 71.271003: kvm_arch_vcpu_ioctl_run: >> preempt_count=0 >> >> So am I reading this right, that the interrupt did get delivered to cpu#0 while >> the thread info already had the resched flag set. So this really should have >> cleared the bit in preempt_count. But while the trace info shows 'N' for some >> reason should_reschedule returns false but at the same time reading the preempt >> count manually shows it 0? > > So the assembly merges the first and second should_resched(), so its > possible that load got before the interrupt(). > > The 3rd preempt_count load gets re-issued and so that would show the > 'true' value again. > > If you want to force a reload after the condition; put in a barrier(). > > In any case; this looks like a false-positive. Please try again until > you get one where the interrupt doesn't happen and we stay in 'n' state. > Oh and one thing I was wondering. Not sure I do understand it right... When initially converting to percpu counts, you changed the 32bit assembly like that: This seems to say if preempt_count was 0 then then if the thread flag was set and interrupts were not off(?) it would do a preempt ipi and then come back to re-check the thread flag. This would now be if preempt_count is 0 only... and I wonder whether that would change from doing that loop... --- a/arch/x86/kernel/entry_32.S +++ b/arch/x86/kernel/entry_32.S @@ -362,12 +362,9 @@ END(ret_from_exception) #ifdef CONFIG_PREEMPT ENTRY(resume_kernel) DISABLE_INTERRUPTS(CLBR_ANY) - cmpl $0,TI_preempt_count(%ebp) # non-zero preempt_count ? - jnz restore_all need_resched: - movl TI_flags(%ebp), %ecx # need_resched set ? - testb $_TIF_NEED_RESCHED, %cl - jz restore_all + cmpl $0,PER_CPU_VAR(__preempt_count) + jnz restore_all testl $X86_EFLAGS_IF,PT_EFLAGS(%esp) # interrupts off (exception path jz restore_all call preempt_schedule_irq