From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Borntraeger Subject: Re: [PATCH v2] arm/arm64: KVM: Properly account for guest CPU time Date: Mon, 01 Jun 2015 09:47:46 +0200 Message-ID: <556C0E22.9090401@de.ibm.com> References: <1432838950-28774-1-git-send-email-christoffer.dall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 7bit Cc: Marc Zyngier , Paolo Bonzini To: Christoffer Dall , kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org Return-path: Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:54689 "EHLO e06smtp12.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751464AbbFAHru (ORCPT ); Mon, 1 Jun 2015 03:47:50 -0400 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 1 Jun 2015 08:47:48 +0100 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 3E3A61B08074 for ; Mon, 1 Jun 2015 08:48:41 +0100 (BST) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t517lluD24182800 for ; Mon, 1 Jun 2015 07:47:47 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t517lkRN024203 for ; Mon, 1 Jun 2015 03:47:47 -0400 In-Reply-To: <1432838950-28774-1-git-send-email-christoffer.dall@linaro.org> Sender: kvm-owner@vger.kernel.org List-ID: Am 28.05.2015 um 20:49 schrieb Christoffer Dall: > Until now we have been calling kvm_guest_exit after re-enabling > interrupts when we come back from the guest, but this has the > unfortunate effect that CPU time accounting done in the context of timer > interrupts occurring while the guest is running doesn't properly notice > that the time since the last tick was spent in the guest. Can you verify that a CPU bound guest has almost zero guest time? Assuming that your answer is "yes" your patch make sense as host timer interrupts should be the only reasons for guest exits then. > Inspired by the comment in the x86 code, move the kvm_guest_exit() call > below the local_irq_enable() call and change __kvm_guest_exit() to > kvm_guest_exit(), because we are now calling this function with > interrupts enabled. We have to now explicitly disable preemption and > not enable preemption before we've called kvm_guest_exit(), since > otherwise we could be preempted and everything happening before we > eventually get scheduled again would be accounted for as guest time. > > At the same time, move the trace_kvm_exit() call outside of the atomic > section, since there is no reason for us to do that with interrupts > disabled. > > Signed-off-by: Christoffer Dall > --- > This patch is based on kvm/queue, because it has the kvm_guest_enter/exit > rework recently posted by Christian Borntraeger. I hope I got the logic > of this right, there were 2 slightly worrying facts about this: > > First, we now enable and disable and enable interrupts on each exit > path, but I couldn't see any performance overhead on hackbench - yes the > only benchmark we care about. This should be somewhat similar to the situation before my patch. There it was 1: "disable", "guest", "disable again and save", "restore to disable", "enable" and now it is 2: "disable", "guest", "enable" and with your patch it is 3: "disable", "guest", "enable", "disable, "enable" I assume that 3 and 1 are similar in its costs, so this is probably ok. > > Second, looking at the ppc and mips code, they seem to also call > kvm_guest_exit() before enabling interrupts, so I don't understand how > guest CPU time accounting works on those architectures. Not an expert here, but I assume mips has the same logic as arm so if your patch is right for arm its probably also for mips. powerpc looks similar to what s390 does (not using the tick, instead it uses a hw-timer) so this should be fine. From mboxrd@z Thu Jan 1 00:00:00 1970 From: borntraeger@de.ibm.com (Christian Borntraeger) Date: Mon, 01 Jun 2015 09:47:46 +0200 Subject: [PATCH v2] arm/arm64: KVM: Properly account for guest CPU time In-Reply-To: <1432838950-28774-1-git-send-email-christoffer.dall@linaro.org> References: <1432838950-28774-1-git-send-email-christoffer.dall@linaro.org> Message-ID: <556C0E22.9090401@de.ibm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Am 28.05.2015 um 20:49 schrieb Christoffer Dall: > Until now we have been calling kvm_guest_exit after re-enabling > interrupts when we come back from the guest, but this has the > unfortunate effect that CPU time accounting done in the context of timer > interrupts occurring while the guest is running doesn't properly notice > that the time since the last tick was spent in the guest. Can you verify that a CPU bound guest has almost zero guest time? Assuming that your answer is "yes" your patch make sense as host timer interrupts should be the only reasons for guest exits then. > Inspired by the comment in the x86 code, move the kvm_guest_exit() call > below the local_irq_enable() call and change __kvm_guest_exit() to > kvm_guest_exit(), because we are now calling this function with > interrupts enabled. We have to now explicitly disable preemption and > not enable preemption before we've called kvm_guest_exit(), since > otherwise we could be preempted and everything happening before we > eventually get scheduled again would be accounted for as guest time. > > At the same time, move the trace_kvm_exit() call outside of the atomic > section, since there is no reason for us to do that with interrupts > disabled. > > Signed-off-by: Christoffer Dall > --- > This patch is based on kvm/queue, because it has the kvm_guest_enter/exit > rework recently posted by Christian Borntraeger. I hope I got the logic > of this right, there were 2 slightly worrying facts about this: > > First, we now enable and disable and enable interrupts on each exit > path, but I couldn't see any performance overhead on hackbench - yes the > only benchmark we care about. This should be somewhat similar to the situation before my patch. There it was 1: "disable", "guest", "disable again and save", "restore to disable", "enable" and now it is 2: "disable", "guest", "enable" and with your patch it is 3: "disable", "guest", "enable", "disable, "enable" I assume that 3 and 1 are similar in its costs, so this is probably ok. > > Second, looking at the ppc and mips code, they seem to also call > kvm_guest_exit() before enabling interrupts, so I don't understand how > guest CPU time accounting works on those architectures. Not an expert here, but I assume mips has the same logic as arm so if your patch is right for arm its probably also for mips. powerpc looks similar to what s390 does (not using the tick, instead it uses a hw-timer) so this should be fine.