From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753878AbdA3Rk4 (ORCPT ); Mon, 30 Jan 2017 12:40:56 -0500 Received: from outprodmail02.cc.columbia.edu ([128.59.72.51]:54968 "EHLO outprodmail02.cc.columbia.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753813AbdA3Rky (ORCPT ); Mon, 30 Jan 2017 12:40:54 -0500 MIME-Version: 1.0 In-Reply-To: <2453da50-d827-0aa5-1dea-beb35117d1ae@arm.com> References: <1485479100-4966-1-git-send-email-jintack@cs.columbia.edu> <1485479100-4966-3-git-send-email-jintack@cs.columbia.edu> <867f5e9j4y.fsf@arm.com> <20170130144550.GA16459@cbox> <2453da50-d827-0aa5-1dea-beb35117d1ae@arm.com> From: Jintack Lim Date: Mon, 30 Jan 2017 12:40:50 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC v2 02/10] KVM: arm/arm64: Move cntvoff to each timer context To: Marc Zyngier Cc: Christoffer Dall , Paolo Bonzini , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , linux@armlinux.org.uk, Catalin Marinas , Will Deacon , Andre Przywara , KVM General , arm-mail-list , kvmarm@lists.cs.columbia.edu, lkml - Kernel Mailing List Content-Type: text/plain; charset=UTF-8 X-No-Spam-Score: Local Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 30, 2017 at 9:51 AM, Marc Zyngier wrote: > On 30/01/17 14:45, Christoffer Dall wrote: >> On Sun, Jan 29, 2017 at 11:54:05AM +0000, Marc Zyngier wrote: >>> On Fri, Jan 27 2017 at 01:04:52 AM, Jintack Lim wrote: >>>> Make cntvoff per each timer context. This is helpful to abstract kvm >>>> timer functions to work with timer context without considering timer >>>> types (e.g. physical timer or virtual timer). >>>> >>>> This also would pave the way for ever doing adjustments of the cntvoff >>>> on a per-CPU basis if that should ever make sense. >>>> >>>> Signed-off-by: Jintack Lim >>>> --- >>>> arch/arm/include/asm/kvm_host.h | 6 +++--- >>>> arch/arm64/include/asm/kvm_host.h | 4 ++-- >>>> include/kvm/arm_arch_timer.h | 8 +++----- >>>> virt/kvm/arm/arch_timer.c | 26 ++++++++++++++++++++------ >>>> virt/kvm/arm/hyp/timer-sr.c | 3 +-- >>>> 5 files changed, 29 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >>>> index d5423ab..f5456a9 100644 >>>> --- a/arch/arm/include/asm/kvm_host.h >>>> +++ b/arch/arm/include/asm/kvm_host.h >>>> @@ -60,9 +60,6 @@ struct kvm_arch { >>>> /* The last vcpu id that ran on each physical CPU */ >>>> int __percpu *last_vcpu_ran; >>>> >>>> - /* Timer */ >>>> - struct arch_timer_kvm timer; >>>> - >>>> /* >>>> * Anything that is not used directly from assembly code goes >>>> * here. >>>> @@ -75,6 +72,9 @@ struct kvm_arch { >>>> /* Stage-2 page table */ >>>> pgd_t *pgd; >>>> >>>> + /* A lock to synchronize cntvoff among all vtimer context of vcpus */ >>>> + spinlock_t cntvoff_lock; >>> >>> Is there any condition where we need this to be a spinlock? I would have >>> thought that a mutex should have been enough, as this should only be >>> updated on migration or initialization. Not that it matters much in this >>> case, but I wondered if there is something I'm missing. >>> >> >> I would think the critical section is small enough that a spinlock makes >> sense, but what I don't think we need is to add the additional lock. >> >> I think just taking the kvm->lock should be sufficient, which happens to >> be a mutex, and while that may be a bit slower to take than the >> spinlock, it's not in the critical path so let's just keep things >> simple. >> >> Perhaps this what Marc also meant. > > That would be the logical conclusion, assuming that we can sleep on this > path. All right. I'll take kvm->lock there. Thanks, Jintack > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... >