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 1695FC43217 for ; Wed, 2 Nov 2022 15:58:51 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 89E7340417; Wed, 2 Nov 2022 11:58:51 -0400 (EDT) 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=@kernel.org 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 7unYzaSnKEQp; Wed, 2 Nov 2022 11:58:50 -0400 (EDT) Received: from mm01.cs.columbia.edu (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 3A5A54B984; Wed, 2 Nov 2022 11:58:50 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 3DC1F4B904 for ; Wed, 2 Nov 2022 11:58:49 -0400 (EDT) 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 V7+tFV92JrPm for ; Wed, 2 Nov 2022 11:58:48 -0400 (EDT) Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 0EC3540417 for ; Wed, 2 Nov 2022 11:58:48 -0400 (EDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id D830261590; Wed, 2 Nov 2022 15:58:46 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A1416C433D6; Wed, 2 Nov 2022 15:58:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1667404726; bh=HX9zj9GM9OWe4YXhMsVjlp4lVDfOB/zbQjFOQfuR2To=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Qh07gmOgCeigjcvSBI719bVcZ7HH0lWvCkCGQatFgY/ual/4f6cDOOhVzsLPxzQvy XvsFO0xWQD+E81EXUtgp+9Jehx8H+b7VIreoffjM+hQ4YNvGfMvzWGoABfro9nCQ67 A3wOvvcpj9am58IZVvd8ByhttmSTO5t38zowdEZ2Y+bGYKAnsTiGKSNjw40Zll0hnV NVg5OQooEBpYS+JexDlD8pwo6NDfvfbxuye3TmaVWlR6JW5x3kpsgkGRbC39jd0ZUL bqHiRhkI8CHNH3PUyCP9CAhqwgoNDzztJTci00YVy+MqO/CEqb3mYu9/N8PEvxden7 8ywr4QILxnE5g== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oqG8Z-003FIG-DC; Wed, 02 Nov 2022 15:58:44 +0000 Date: Wed, 02 Nov 2022 15:58:43 +0000 Message-ID: <867d0de4b0.wl-maz@kernel.org> From: Marc Zyngier To: Peter Xu Subject: Re: [PATCH v7 1/9] KVM: x86: Introduce KVM_REQ_DIRTY_RING_SOFT_FULL In-Reply-To: References: <20221031003621.164306-1-gshan@redhat.com> <20221031003621.164306-2-gshan@redhat.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: peterx@redhat.com, seanjc@google.com, gshan@redhat.com, kvmarm@lists.linux.dev, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, andrew.jones@linux.dev, ajones@ventanamicro.com, bgardon@google.com, catalin.marinas@arm.com, dmatlack@google.com, will@kernel.org, pbonzini@redhat.com, oliver.upton@linux.dev, james.morse@arm.com, shuah@kernel.org, suzuki.poulose@arm.com, alexandru.elisei@arm.com, zhenyzha@redhat.com, shan.gavin@gmail.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Cc: shuah@kernel.org, kvm@vger.kernel.org, andrew.jones@linux.dev, dmatlack@google.com, will@kernel.org, shan.gavin@gmail.com, bgardon@google.com, kvmarm@lists.linux.dev, pbonzini@redhat.com, zhenyzha@redhat.com, catalin.marinas@arm.com, kvmarm@lists.cs.columbia.edu, ajones@ventanamicro.com 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 Wed, 02 Nov 2022 14:29:26 +0000, Peter Xu wrote: > > On Tue, Nov 01, 2022 at 07:39:25PM +0000, Sean Christopherson wrote: > > > @@ -142,13 +144,17 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring) > > > > > > kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask); > > > > > > + if (!kvm_dirty_ring_soft_full(ring)) > > > + kvm_clear_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu); > > > + > > > > Marc, Peter, and/or Paolo, can you confirm that clearing the > > request here won't cause ordering problems? Logically, this makes > > perfect sense (to me, since I suggested it), but I'm mildly > > concerned I'm overlooking an edge case where KVM could end up with > > a soft-full ring but no pending request. > > I don't see an ordering issue here, as long as kvm_clear_request() is using > atomic version of bit clear, afaict that's genuine RMW and should always > imply a full memory barrier (on any arch?) between the soft full check and > the bit clear. At least for x86 the lock prefix was applied. No, clear_bit() is not a full barrier. It only atomic, and thus completely unordered (see Documentation/atomic_bitops.txt). If you want a full barrier, you need to use test_and_clear_bit(). > > However I don't see anything stops a simple "race" to trigger like below: > > recycle thread vcpu thread > -------------- ----------- > if (!dirty_ring_soft_full) <--- not full > dirty_ring_push(); > if (dirty_ring_soft_full) <--- full due to the push > set_request(SOFT_FULL); > clear_request(SOFT_FULL); <--- can wrongly clear the request? > Hmmm, well spotted. That's another ugly effect of the recycle thread playing with someone else's toys. > But I don't think that's a huge matter, as it'll just let the vcpu to have > one more chance to do another round of KVM_RUN. Normally I think it means > there can be one more dirty GFN (perhaps there're cases that it can push >1 > gfns for one KVM_RUN cycle? I never figured out the details here, but > still..) pushed to the ring so closer to the hard limit, but we have had a > buffer zone of KVM_DIRTY_RING_RSVD_ENTRIES (64) entries. So I assume > that's still fine, but maybe worth a short comment here? > > I never know what's the maximum possible GFNs being dirtied for a KVM_RUN > cycle. It would be good if there's an answer to that from anyone. This is dangerous, and I'd rather not go there. It is starting to look like we need the recycle thread to get out of the way. And to be honest: + if (!kvm_dirty_ring_soft_full(ring)) + kvm_clear_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu); seems rather superfluous. Only clearing the flag in the vcpu entry path feels much saner, and I can't see anything that would break. Thoughts? M. -- Without deviation from the norm, progress is not possible. _______________________________________________ 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 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 00DFA6D18 for ; Wed, 2 Nov 2022 15:58:46 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A1416C433D6; Wed, 2 Nov 2022 15:58:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1667404726; bh=HX9zj9GM9OWe4YXhMsVjlp4lVDfOB/zbQjFOQfuR2To=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Qh07gmOgCeigjcvSBI719bVcZ7HH0lWvCkCGQatFgY/ual/4f6cDOOhVzsLPxzQvy XvsFO0xWQD+E81EXUtgp+9Jehx8H+b7VIreoffjM+hQ4YNvGfMvzWGoABfro9nCQ67 A3wOvvcpj9am58IZVvd8ByhttmSTO5t38zowdEZ2Y+bGYKAnsTiGKSNjw40Zll0hnV NVg5OQooEBpYS+JexDlD8pwo6NDfvfbxuye3TmaVWlR6JW5x3kpsgkGRbC39jd0ZUL bqHiRhkI8CHNH3PUyCP9CAhqwgoNDzztJTci00YVy+MqO/CEqb3mYu9/N8PEvxden7 8ywr4QILxnE5g== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oqG8Z-003FIG-DC; Wed, 02 Nov 2022 15:58:44 +0000 Date: Wed, 02 Nov 2022 15:58:43 +0000 Message-ID: <867d0de4b0.wl-maz@kernel.org> From: Marc Zyngier To: Peter Xu Cc: Sean Christopherson , Gavin Shan , kvmarm@lists.linux.dev, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, andrew.jones@linux.dev, ajones@ventanamicro.com, bgardon@google.com, catalin.marinas@arm.com, dmatlack@google.com, will@kernel.org, pbonzini@redhat.com, oliver.upton@linux.dev, james.morse@arm.com, shuah@kernel.org, suzuki.poulose@arm.com, alexandru.elisei@arm.com, zhenyzha@redhat.com, shan.gavin@gmail.com Subject: Re: [PATCH v7 1/9] KVM: x86: Introduce KVM_REQ_DIRTY_RING_SOFT_FULL In-Reply-To: References: <20221031003621.164306-1-gshan@redhat.com> <20221031003621.164306-2-gshan@redhat.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: peterx@redhat.com, seanjc@google.com, gshan@redhat.com, kvmarm@lists.linux.dev, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, andrew.jones@linux.dev, ajones@ventanamicro.com, bgardon@google.com, catalin.marinas@arm.com, dmatlack@google.com, will@kernel.org, pbonzini@redhat.com, oliver.upton@linux.dev, james.morse@arm.com, shuah@kernel.org, suzuki.poulose@arm.com, alexandru.elisei@arm.com, zhenyzha@redhat.com, shan.gavin@gmail.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Message-ID: <20221102155843.gEKk_wF8zBriuFXnt56a1nVTbvuI7cLnS6t53Ql-tG4@z> On Wed, 02 Nov 2022 14:29:26 +0000, Peter Xu wrote: > > On Tue, Nov 01, 2022 at 07:39:25PM +0000, Sean Christopherson wrote: > > > @@ -142,13 +144,17 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring) > > > > > > kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask); > > > > > > + if (!kvm_dirty_ring_soft_full(ring)) > > > + kvm_clear_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu); > > > + > > > > Marc, Peter, and/or Paolo, can you confirm that clearing the > > request here won't cause ordering problems? Logically, this makes > > perfect sense (to me, since I suggested it), but I'm mildly > > concerned I'm overlooking an edge case where KVM could end up with > > a soft-full ring but no pending request. > > I don't see an ordering issue here, as long as kvm_clear_request() is using > atomic version of bit clear, afaict that's genuine RMW and should always > imply a full memory barrier (on any arch?) between the soft full check and > the bit clear. At least for x86 the lock prefix was applied. No, clear_bit() is not a full barrier. It only atomic, and thus completely unordered (see Documentation/atomic_bitops.txt). If you want a full barrier, you need to use test_and_clear_bit(). > > However I don't see anything stops a simple "race" to trigger like below: > > recycle thread vcpu thread > -------------- ----------- > if (!dirty_ring_soft_full) <--- not full > dirty_ring_push(); > if (dirty_ring_soft_full) <--- full due to the push > set_request(SOFT_FULL); > clear_request(SOFT_FULL); <--- can wrongly clear the request? > Hmmm, well spotted. That's another ugly effect of the recycle thread playing with someone else's toys. > But I don't think that's a huge matter, as it'll just let the vcpu to have > one more chance to do another round of KVM_RUN. Normally I think it means > there can be one more dirty GFN (perhaps there're cases that it can push >1 > gfns for one KVM_RUN cycle? I never figured out the details here, but > still..) pushed to the ring so closer to the hard limit, but we have had a > buffer zone of KVM_DIRTY_RING_RSVD_ENTRIES (64) entries. So I assume > that's still fine, but maybe worth a short comment here? > > I never know what's the maximum possible GFNs being dirtied for a KVM_RUN > cycle. It would be good if there's an answer to that from anyone. This is dangerous, and I'd rather not go there. It is starting to look like we need the recycle thread to get out of the way. And to be honest: + if (!kvm_dirty_ring_soft_full(ring)) + kvm_clear_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu); seems rather superfluous. Only clearing the flag in the vcpu entry path feels much saner, and I can't see anything that would break. Thoughts? M. -- Without deviation from the norm, progress is not possible.