From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8A620C4332F for ; Tue, 11 Jan 2022 18:46:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346293AbiAKSqw (ORCPT ); Tue, 11 Jan 2022 13:46:52 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33132 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345929AbiAKSqv (ORCPT ); Tue, 11 Jan 2022 13:46:51 -0500 Received: from mail-yb1-xb32.google.com (mail-yb1-xb32.google.com [IPv6:2607:f8b0:4864:20::b32]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AB230C061748 for ; Tue, 11 Jan 2022 10:46:51 -0800 (PST) Received: by mail-yb1-xb32.google.com with SMTP id h14so32630773ybe.12 for ; Tue, 11 Jan 2022 10:46:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=aFmLILnvpHUVz7ssSaFntznd0hUHj6gSC616E6Fkf3U=; b=ePhCIqosOE3NORpI3w54xHfcf6puCuQ+ahHlVC9G+tLO1IGP+Wgo03UffIkLCD29C6 k0CRMWuU19THro9DlEhGP69zZBJFVhAePULjiWgpPP2+R248Tcm165947vP0aZi3s+z3 KJBzevL0NfgzA5F+Uc4oKmj+jYR+wZ55HEE+ti6ILLFigEbDJwP/lrOgXm0M24EPXvYm 4dtTAQF9+cItgcqoCCoSXLydPtnIrMJF1vzL8JO2vTdoooQUT9gSNNHz/Q2miLU8T7Ag MZlAK/xMpFZL9y3rVVJXsoY8rl5Y5BvRXzGKK3g1FVPPKtyr0P1MbZ2vI5P2ZX9v2Gz3 y+SQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=aFmLILnvpHUVz7ssSaFntznd0hUHj6gSC616E6Fkf3U=; b=xkBGYzZlIp316vHu4IlYHxn2kVkUquosVqQ9CakXBnWIATqXL43PaMjhT0JXgAzTIR 99hWJ048sg7yww+6OarhLs/8DG8HFuD+o7VdZjyOGKRCObKKkzY4bTyGi2ZOyXnXvt7y glgMGA5eEgym51k/2TCAHR9bIeam0rgB/KiOIFDmIEHgYQsj5caJXrm5Pqz9HoRdzrBp yfM1621giF9wqUe0pTHDYMHSlKjyASgjJWNwo92Dxp4eSAENRwv6dyxUjsyjUXALgdox P/BDrGM5/mYd4b7JJ9flWOAdt5bF5xYTBseZKI39rCo1ppd2u+JW6VHDdE/LTqnXTAzi mBhQ== X-Gm-Message-State: AOAM53133lZHL0myF6YprUYxao0j1Uh9Br16QZPWaBttU9J8Gt7z9wpZ 1l3r10j48H3/YSCiJx0KF36e0c+YW3NDq9zedaQ4wg== X-Google-Smtp-Source: ABdhPJy2gg0pwgufffOlXPR//NYKBRRE7YQswq8GMY6HGu/DSmziIAnClGDP4sEu6YMRCb2AJB3uhS4DQuFcC/jbDPk= X-Received: by 2002:a25:d750:: with SMTP id o77mr7614846ybg.543.1641926810619; Tue, 11 Jan 2022 10:46:50 -0800 (PST) MIME-Version: 1.0 References: <20220104194918.373612-1-rananta@google.com> <20220104194918.373612-2-rananta@google.com> In-Reply-To: From: Raghavendra Rao Ananta Date: Tue, 11 Jan 2022 10:46:40 -0800 Message-ID: Subject: Re: [RFC PATCH v3 01/11] KVM: Capture VM start To: Sean Christopherson Cc: Marc Zyngier , Andrew Jones , James Morse , Alexandru Elisei , Suzuki K Poulose , kvm@vger.kernel.org, Catalin Marinas , Peter Shier , linux-kernel@vger.kernel.org, Paolo Bonzini , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 11, 2022 at 9:36 AM Sean Christopherson wrote: > > On Mon, Jan 10, 2022, Raghavendra Rao Ananta wrote: > > On Fri, Jan 7, 2022 at 5:06 PM Sean Christopherson wrote: > > > > > > On Tue, Jan 04, 2022, Raghavendra Rao Ananta wrote: > > > > +#define kvm_vm_has_started(kvm) (kvm->vm_started) > > > > > > Needs parantheses around (kvm), but why bother with a macro? This is the same > > > header that defines struct kvm. > > > > > No specific reason for creating a macro as such. I can remove it if it > > feels noisy. > > Please do. In the future, don't use a macro unless there's a good reason to do > so. Don't get me wrong, I love abusing macros, but for things like this they are > completely inferior to > > static inline bool kvm_vm_has_started(struct kvm *kvm) > { > return kvm->vm_started; > } > > because a helper function gives us type safety, doesn't suffer from concatenation > of tokens potentially doing weird things, is easier to extend to a multi-line > implementation, etc... > > An example of when it's ok to use a macro is x86's > > #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0) > > which uses a macro instead of a proper function to avoid a circular dependency > due to arch/x86/include/asm/kvm_host.h being included by include/linux/kvm_host.h > and thus x86's implementation of kvm_arch_vcpu_memslots_id() coming before the > definition of struct kvm_vcpu. But that's very much an exception and done only > because the alternatives suck more. > Understood. Thanks for the explanation! Will switch to an inline function. > > > > + */ > > > > + mutex_lock(&kvm->lock); > > > > > > This adds unnecessary lock contention when running vCPUs. The naive solution > > > would be: > > > if (!kvm->vm_started) { > > > ... > > > } > > > > > Not sure if I understood the solution.. > > In your proposed patch, KVM_RUN will take kvm->lock _every_ time. That introduces > unnecessary contention as it will serialize this bit of code if multiple vCPUs > are attempting KVM_RUN. By checking !vm_started, only the "first" KVM_RUN for a > VM will acquire kvm->lock and thus avoid contention once the VM is up and running. > There's still a possibility that multiple vCPUs will contend for kvm->lock on their > first KVM_RUN, hence the quotes. I called it "naive" because it's possible there's > a more elegant solution depending on the use case, e.g. a lockless approach might > work (or it might not). > But is it safe to read kvm->vm_started without grabbing the lock in the first place? use atomic_t maybe for this? > > > > + kvm->vm_started = true; > > > > + mutex_unlock(&kvm->lock); > > > > > > Lastly, why is this in generic KVM? > > > > > The v1 of the series originally had it in the arm specific code. > > However, I was suggested to move it to the generic code since the book > > keeping is not arch specific and could be helpful to others too [1]. > > I'm definitely in favor of moving/adding thing to generic KVM when it makes sense, > but I'm skeptical in this particular case. The code _is_ arch specific in that > arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g. > versus a hypothetical x86 use case that might be completely ok with a lockless > implementation. And it's not obvious that there's a plausible, safe use case > outside of arm64, e.g. on x86, there is very, very little that is truly shared > across the entire VM/system, most things are per-thread/core/package in some way, > shape, or form. In other words, I'm a wary of providing something like this for > x86 because odds are good that any use will be functionally incorrect. I've been going back and forth on this. I've seen a couple of variables declared in the generic struct and used only in the arch code. vcpu->valid_wakeup for instance, which is used only by s390 arch. Maybe I'm looking at it the wrong way as to what can and can't go in the generic kvm code. Thanks, Raghavendra From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mm01.cs.columbia.edu (mm01.cs.columbia.edu [128.59.11.253]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9D85DC433EF for ; Tue, 11 Jan 2022 18:46:55 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id E9B204B0F7; Tue, 11 Jan 2022 13:46:54 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Authentication-Results: mm01.cs.columbia.edu (amavisd-new); dkim=softfail (fail, message has been altered) header.i=@google.com Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id tjxpQJPHXKSI; Tue, 11 Jan 2022 13:46:53 -0500 (EST) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id B48984B1ED; Tue, 11 Jan 2022 13:46:53 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 8BDA94B165 for ; Tue, 11 Jan 2022 13:46:52 -0500 (EST) X-Virus-Scanned: at lists.cs.columbia.edu Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rD0N5joU5u8U for ; Tue, 11 Jan 2022 13:46:51 -0500 (EST) Received: from mail-yb1-f169.google.com (mail-yb1-f169.google.com [209.85.219.169]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 70D6D4B0F7 for ; Tue, 11 Jan 2022 13:46:51 -0500 (EST) Received: by mail-yb1-f169.google.com with SMTP id v186so40666896ybg.1 for ; Tue, 11 Jan 2022 10:46:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=aFmLILnvpHUVz7ssSaFntznd0hUHj6gSC616E6Fkf3U=; b=ePhCIqosOE3NORpI3w54xHfcf6puCuQ+ahHlVC9G+tLO1IGP+Wgo03UffIkLCD29C6 k0CRMWuU19THro9DlEhGP69zZBJFVhAePULjiWgpPP2+R248Tcm165947vP0aZi3s+z3 KJBzevL0NfgzA5F+Uc4oKmj+jYR+wZ55HEE+ti6ILLFigEbDJwP/lrOgXm0M24EPXvYm 4dtTAQF9+cItgcqoCCoSXLydPtnIrMJF1vzL8JO2vTdoooQUT9gSNNHz/Q2miLU8T7Ag MZlAK/xMpFZL9y3rVVJXsoY8rl5Y5BvRXzGKK3g1FVPPKtyr0P1MbZ2vI5P2ZX9v2Gz3 y+SQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=aFmLILnvpHUVz7ssSaFntznd0hUHj6gSC616E6Fkf3U=; b=f2xPYsIV9HEuGGZpH6K8jJlzOYdyeB2cw++yh5WihTTQRxol97WpsormbayMiqNiig cHe+xBtQ9DDX0SGJyMsOD+0Lxo3axMPwJjCOPTQ2+zGMTiUG/udtqvnrUu3VRIMuqq+7 Tz84r9lQHFYeTX0lF4bF60I5ZKpZs5rjkSnHfRia8IQGmxHOWqolFyzbHMALxAsF1Bcn yBqzv9r+pCjTJq+jk3eXO1dJY1BzXmqAvsyUBQSXPy2Tbl61/qp9lnYksXxviKyjh0Aa kmX06wY6MywGrKjrNZ3i2KHYaDvkV0/k0ZnyLHgL6kFg3fOsR1JRNtQtoLlraLsB7ZJ8 Sklg== X-Gm-Message-State: AOAM532JwpX4KjtEm8xzKbyxZGZ2FtUsU92j7i47d2yVN2mvhJx6D1ER d4MB3E3jm1YvXzgAn2mrt7tNJZPeFG8QuGAiQeFDGw== X-Google-Smtp-Source: ABdhPJy2gg0pwgufffOlXPR//NYKBRRE7YQswq8GMY6HGu/DSmziIAnClGDP4sEu6YMRCb2AJB3uhS4DQuFcC/jbDPk= X-Received: by 2002:a25:d750:: with SMTP id o77mr7614846ybg.543.1641926810619; Tue, 11 Jan 2022 10:46:50 -0800 (PST) MIME-Version: 1.0 References: <20220104194918.373612-1-rananta@google.com> <20220104194918.373612-2-rananta@google.com> In-Reply-To: From: Raghavendra Rao Ananta Date: Tue, 11 Jan 2022 10:46:40 -0800 Message-ID: Subject: Re: [RFC PATCH v3 01/11] KVM: Capture VM start To: Sean Christopherson Cc: kvm@vger.kernel.org, Marc Zyngier , Peter Shier , linux-kernel@vger.kernel.org, Will Deacon , Catalin Marinas , Paolo Bonzini , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-BeenThere: kvmarm@lists.cs.columbia.edu X-Mailman-Version: 2.1.14 Precedence: list List-Id: Where KVM/ARM decisions are made List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu On Tue, Jan 11, 2022 at 9:36 AM Sean Christopherson wrote: > > On Mon, Jan 10, 2022, Raghavendra Rao Ananta wrote: > > On Fri, Jan 7, 2022 at 5:06 PM Sean Christopherson wrote: > > > > > > On Tue, Jan 04, 2022, Raghavendra Rao Ananta wrote: > > > > +#define kvm_vm_has_started(kvm) (kvm->vm_started) > > > > > > Needs parantheses around (kvm), but why bother with a macro? This is the same > > > header that defines struct kvm. > > > > > No specific reason for creating a macro as such. I can remove it if it > > feels noisy. > > Please do. In the future, don't use a macro unless there's a good reason to do > so. Don't get me wrong, I love abusing macros, but for things like this they are > completely inferior to > > static inline bool kvm_vm_has_started(struct kvm *kvm) > { > return kvm->vm_started; > } > > because a helper function gives us type safety, doesn't suffer from concatenation > of tokens potentially doing weird things, is easier to extend to a multi-line > implementation, etc... > > An example of when it's ok to use a macro is x86's > > #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0) > > which uses a macro instead of a proper function to avoid a circular dependency > due to arch/x86/include/asm/kvm_host.h being included by include/linux/kvm_host.h > and thus x86's implementation of kvm_arch_vcpu_memslots_id() coming before the > definition of struct kvm_vcpu. But that's very much an exception and done only > because the alternatives suck more. > Understood. Thanks for the explanation! Will switch to an inline function. > > > > + */ > > > > + mutex_lock(&kvm->lock); > > > > > > This adds unnecessary lock contention when running vCPUs. The naive solution > > > would be: > > > if (!kvm->vm_started) { > > > ... > > > } > > > > > Not sure if I understood the solution.. > > In your proposed patch, KVM_RUN will take kvm->lock _every_ time. That introduces > unnecessary contention as it will serialize this bit of code if multiple vCPUs > are attempting KVM_RUN. By checking !vm_started, only the "first" KVM_RUN for a > VM will acquire kvm->lock and thus avoid contention once the VM is up and running. > There's still a possibility that multiple vCPUs will contend for kvm->lock on their > first KVM_RUN, hence the quotes. I called it "naive" because it's possible there's > a more elegant solution depending on the use case, e.g. a lockless approach might > work (or it might not). > But is it safe to read kvm->vm_started without grabbing the lock in the first place? use atomic_t maybe for this? > > > > + kvm->vm_started = true; > > > > + mutex_unlock(&kvm->lock); > > > > > > Lastly, why is this in generic KVM? > > > > > The v1 of the series originally had it in the arm specific code. > > However, I was suggested to move it to the generic code since the book > > keeping is not arch specific and could be helpful to others too [1]. > > I'm definitely in favor of moving/adding thing to generic KVM when it makes sense, > but I'm skeptical in this particular case. The code _is_ arch specific in that > arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g. > versus a hypothetical x86 use case that might be completely ok with a lockless > implementation. And it's not obvious that there's a plausible, safe use case > outside of arm64, e.g. on x86, there is very, very little that is truly shared > across the entire VM/system, most things are per-thread/core/package in some way, > shape, or form. In other words, I'm a wary of providing something like this for > x86 because odds are good that any use will be functionally incorrect. I've been going back and forth on this. I've seen a couple of variables declared in the generic struct and used only in the arch code. vcpu->valid_wakeup for instance, which is used only by s390 arch. Maybe I'm looking at it the wrong way as to what can and can't go in the generic kvm code. Thanks, Raghavendra _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 18C7FC433F5 for ; Tue, 11 Jan 2022 18:48:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=XjVkFaJ5xXr5UmgpPMm8JtatXKuwuKt+F7yi8VIQaIE=; b=snjf7/I94f1EvC O/jImU0FIo8sqQ4yv4aAnFDoCbEAW6Q8B0jyHac5/6BEFjcdg7e9wpqSaRjfjodMkv7Sx3Y0Z/LEt R2GChBbGkCtpTuy7nyb+aGaZ0a2j9sk45+OL0+tKAVxD6pTjAXTfntFvuF121X+z1baVO1antfmCC IsFGb8VF4XCDfQyIjyDxRmpSDA9/JDI4/+660IfpogHlrP17Y+sfQ/RGo32WR4ECCLmutumVRdY1A 8PlJ4/1GRrxqMe4ZhJ8MUqbWvMIhk1rgGWzh3DbGDO6nAZU/4Bv3YGu4f8KznmwH/3GcVlcyAASCm AFa5p7gwU+45hgR6Epgg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n7MAZ-00HI5L-7n; Tue, 11 Jan 2022 18:46:55 +0000 Received: from mail-yb1-xb2c.google.com ([2607:f8b0:4864:20::b2c]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n7MAV-00HI4G-Tq for linux-arm-kernel@lists.infradead.org; Tue, 11 Jan 2022 18:46:53 +0000 Received: by mail-yb1-xb2c.google.com with SMTP id p187so7471371ybc.0 for ; Tue, 11 Jan 2022 10:46:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=aFmLILnvpHUVz7ssSaFntznd0hUHj6gSC616E6Fkf3U=; b=ePhCIqosOE3NORpI3w54xHfcf6puCuQ+ahHlVC9G+tLO1IGP+Wgo03UffIkLCD29C6 k0CRMWuU19THro9DlEhGP69zZBJFVhAePULjiWgpPP2+R248Tcm165947vP0aZi3s+z3 KJBzevL0NfgzA5F+Uc4oKmj+jYR+wZ55HEE+ti6ILLFigEbDJwP/lrOgXm0M24EPXvYm 4dtTAQF9+cItgcqoCCoSXLydPtnIrMJF1vzL8JO2vTdoooQUT9gSNNHz/Q2miLU8T7Ag MZlAK/xMpFZL9y3rVVJXsoY8rl5Y5BvRXzGKK3g1FVPPKtyr0P1MbZ2vI5P2ZX9v2Gz3 y+SQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=aFmLILnvpHUVz7ssSaFntznd0hUHj6gSC616E6Fkf3U=; b=DPY3jCtN04mBQaTQ4DPs2atVi5PiQi+wIdJVGNPGGzEgprXZ11SzOpsbu7xdJ9O8q2 zBmnbvz6Dc7YKMr+gShVWqA1SWIYCWhHQSjD5SMTZWzQPL/D0f9btf1kIHIRQ4RQhS+H chhWWUc3Ws3L0BAaI0GbucF6Ts+xxI9Ty16+JksNTULX8g7U9TxZ3RFEWbFVtyDou+oF oXkVIeza2VKK2/CfW4YJ0DSW0WaxbAIjmi23Npk/s7087EEu2M2ToBK84R5QpAeKrM0v cj3bVIwQKl1f9RsB/r8xFSMAc2a6rtypEFHffjFiScJizo5+MdINHvNL5d3/wQb1QlZB aaMw== X-Gm-Message-State: AOAM533pCMJBLmnCyXnB7ASTnJmMsETeiVYFBfLcGL337hogpumuYtkm 4hM2BKowt/gMGARLLpG7mnFUccgGnFkQyhtamYLpBA== X-Google-Smtp-Source: ABdhPJy2gg0pwgufffOlXPR//NYKBRRE7YQswq8GMY6HGu/DSmziIAnClGDP4sEu6YMRCb2AJB3uhS4DQuFcC/jbDPk= X-Received: by 2002:a25:d750:: with SMTP id o77mr7614846ybg.543.1641926810619; Tue, 11 Jan 2022 10:46:50 -0800 (PST) MIME-Version: 1.0 References: <20220104194918.373612-1-rananta@google.com> <20220104194918.373612-2-rananta@google.com> In-Reply-To: From: Raghavendra Rao Ananta Date: Tue, 11 Jan 2022 10:46:40 -0800 Message-ID: Subject: Re: [RFC PATCH v3 01/11] KVM: Capture VM start To: Sean Christopherson Cc: Marc Zyngier , Andrew Jones , James Morse , Alexandru Elisei , Suzuki K Poulose , kvm@vger.kernel.org, Catalin Marinas , Peter Shier , linux-kernel@vger.kernel.org, Paolo Bonzini , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220111_104651_994025_2D444128 X-CRM114-Status: GOOD ( 38.11 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jan 11, 2022 at 9:36 AM Sean Christopherson wrote: > > On Mon, Jan 10, 2022, Raghavendra Rao Ananta wrote: > > On Fri, Jan 7, 2022 at 5:06 PM Sean Christopherson wrote: > > > > > > On Tue, Jan 04, 2022, Raghavendra Rao Ananta wrote: > > > > +#define kvm_vm_has_started(kvm) (kvm->vm_started) > > > > > > Needs parantheses around (kvm), but why bother with a macro? This is the same > > > header that defines struct kvm. > > > > > No specific reason for creating a macro as such. I can remove it if it > > feels noisy. > > Please do. In the future, don't use a macro unless there's a good reason to do > so. Don't get me wrong, I love abusing macros, but for things like this they are > completely inferior to > > static inline bool kvm_vm_has_started(struct kvm *kvm) > { > return kvm->vm_started; > } > > because a helper function gives us type safety, doesn't suffer from concatenation > of tokens potentially doing weird things, is easier to extend to a multi-line > implementation, etc... > > An example of when it's ok to use a macro is x86's > > #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0) > > which uses a macro instead of a proper function to avoid a circular dependency > due to arch/x86/include/asm/kvm_host.h being included by include/linux/kvm_host.h > and thus x86's implementation of kvm_arch_vcpu_memslots_id() coming before the > definition of struct kvm_vcpu. But that's very much an exception and done only > because the alternatives suck more. > Understood. Thanks for the explanation! Will switch to an inline function. > > > > + */ > > > > + mutex_lock(&kvm->lock); > > > > > > This adds unnecessary lock contention when running vCPUs. The naive solution > > > would be: > > > if (!kvm->vm_started) { > > > ... > > > } > > > > > Not sure if I understood the solution.. > > In your proposed patch, KVM_RUN will take kvm->lock _every_ time. That introduces > unnecessary contention as it will serialize this bit of code if multiple vCPUs > are attempting KVM_RUN. By checking !vm_started, only the "first" KVM_RUN for a > VM will acquire kvm->lock and thus avoid contention once the VM is up and running. > There's still a possibility that multiple vCPUs will contend for kvm->lock on their > first KVM_RUN, hence the quotes. I called it "naive" because it's possible there's > a more elegant solution depending on the use case, e.g. a lockless approach might > work (or it might not). > But is it safe to read kvm->vm_started without grabbing the lock in the first place? use atomic_t maybe for this? > > > > + kvm->vm_started = true; > > > > + mutex_unlock(&kvm->lock); > > > > > > Lastly, why is this in generic KVM? > > > > > The v1 of the series originally had it in the arm specific code. > > However, I was suggested to move it to the generic code since the book > > keeping is not arch specific and could be helpful to others too [1]. > > I'm definitely in favor of moving/adding thing to generic KVM when it makes sense, > but I'm skeptical in this particular case. The code _is_ arch specific in that > arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g. > versus a hypothetical x86 use case that might be completely ok with a lockless > implementation. And it's not obvious that there's a plausible, safe use case > outside of arm64, e.g. on x86, there is very, very little that is truly shared > across the entire VM/system, most things are per-thread/core/package in some way, > shape, or form. In other words, I'm a wary of providing something like this for > x86 because odds are good that any use will be functionally incorrect. I've been going back and forth on this. I've seen a couple of variables declared in the generic struct and used only in the arch code. vcpu->valid_wakeup for instance, which is used only by s390 arch. Maybe I'm looking at it the wrong way as to what can and can't go in the generic kvm code. Thanks, Raghavendra _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel