From: Paolo Bonzini <pbonzini@redhat.com>
To: Peter Xu <peterx@redhat.com>, Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>,
Gavin Shan <gshan@redhat.com>,
kvmarm@lists.cs.columbia.edu,
linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kselftest@vger.kernel.org, corbet@lwn.net,
james.morse@arm.com, alexandru.elisei@arm.com,
suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org,
shuah@kernel.org, seanjc@google.com, drjones@redhat.com,
dmatlack@google.com, bgardon@google.com, ricarkol@google.com,
zhenyzha@redhat.com, shan.gavin@gmail.com
Subject: Re: [PATCH v1 1/5] KVM: arm64: Enable ring-based dirty memory tracking
Date: Fri, 2 Sep 2022 02:19:46 +0200 [thread overview]
Message-ID: <44a42d03-4dd1-3f1c-3a60-7c2a6a7d417a@redhat.com> (raw)
In-Reply-To: <Yw4hyEAyivKT35vQ@xz-m1.local>
On 8/30/22 16:42, Peter Xu wrote:
> Marc,
>
> I thought we won't hit this as long as we properly take care of other
> orderings of (a) gfn push, and (b) gfn collect, but after a second thought
> I think it's indeed logically possible that with a reversed ordering here
> we can be reading some garbage gfn before (a) happens butt also read the
> valid flag after (b).
>
> It seems we must have all the barriers correctly applied always. If that's
> correct, do you perhaps mean something like this to just add the last piece
> of barrier?
Okay, so I thought about it some more and it's quite tricky.
Strictly speaking, the synchronization is just between userspace and
kernel. The fact that the actual producer of dirty pages is in another
CPU is a red herring, because reset only cares about harvested pages.
In other words, the dirty page ring is essentially two ring buffers in
one and we only care about the "harvested ring", not the "produced ring".
On the other hand, it may happen that userspace has set more RESET flags
while the ioctl is ongoing:
CPU0 CPU1 CPU2
fill gfn0
store-rel flags for gfn0
fill gfn1
store-rel flags for gfn1
load-acq flags for gfn0
set RESET for gfn0
load-acq flags for gfn1
set RESET for gfn1
do ioctl! ----------->
ioctl(RESET_RINGS)
fill gfn2
store-rel flags for gfn2
load-acq flags for gfn2
set RESET for gfn2
process gfn0
process gfn1
process gfn2
do ioctl!
etc.
The three load-acquire in CPU0 synchronize with the three store-release
in CPU2, but CPU0 and CPU1 are only synchronized up to gfn1 and CPU1 may
miss gfn2's fields other than flags.
The kernel must be able to cope with invalid values of the fields, and
userspace will invoke the ioctl once more. However, once the RESET flag
is cleared on gfn2, it is lost forever, therefore in the above scenario
CPU1 must read the correct value of gfn2's fields.
Therefore RESET must be set with a store-release, that will synchronize
with a load-acquire in CPU1 as you suggested.
Paolo
> diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
> index f4c2a6eb1666..ea620bfb012d 100644
> --- a/virt/kvm/dirty_ring.c
> +++ b/virt/kvm/dirty_ring.c
> @@ -84,7 +84,7 @@ static inline void kvm_dirty_gfn_set_dirtied(struct kvm_dirty_gfn *gfn)
>
> static inline bool kvm_dirty_gfn_harvested(struct kvm_dirty_gfn *gfn)
> {
> - return gfn->flags & KVM_DIRTY_GFN_F_RESET;
> + return smp_load_acquire(&gfn->flags) & KVM_DIRTY_GFN_F_RESET;
> }
>
> int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring)
> ===8<===
>
> Thanks,
>
> --
> Peter Xu
>
WARNING: multiple messages have this Message-ID (diff)
From: Paolo Bonzini <pbonzini@redhat.com>
To: Peter Xu <peterx@redhat.com>, Marc Zyngier <maz@kernel.org>
Cc: kvm@vger.kernel.org, linux-doc@vger.kernel.org,
catalin.marinas@arm.com, linux-kselftest@vger.kernel.org,
bgardon@google.com, shuah@kernel.org,
kvmarm@lists.cs.columbia.edu, corbet@lwn.net, will@kernel.org,
shan.gavin@gmail.com, drjones@redhat.com, zhenyzha@redhat.com,
dmatlack@google.com, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/5] KVM: arm64: Enable ring-based dirty memory tracking
Date: Fri, 2 Sep 2022 02:19:46 +0200 [thread overview]
Message-ID: <44a42d03-4dd1-3f1c-3a60-7c2a6a7d417a@redhat.com> (raw)
In-Reply-To: <Yw4hyEAyivKT35vQ@xz-m1.local>
On 8/30/22 16:42, Peter Xu wrote:
> Marc,
>
> I thought we won't hit this as long as we properly take care of other
> orderings of (a) gfn push, and (b) gfn collect, but after a second thought
> I think it's indeed logically possible that with a reversed ordering here
> we can be reading some garbage gfn before (a) happens butt also read the
> valid flag after (b).
>
> It seems we must have all the barriers correctly applied always. If that's
> correct, do you perhaps mean something like this to just add the last piece
> of barrier?
Okay, so I thought about it some more and it's quite tricky.
Strictly speaking, the synchronization is just between userspace and
kernel. The fact that the actual producer of dirty pages is in another
CPU is a red herring, because reset only cares about harvested pages.
In other words, the dirty page ring is essentially two ring buffers in
one and we only care about the "harvested ring", not the "produced ring".
On the other hand, it may happen that userspace has set more RESET flags
while the ioctl is ongoing:
CPU0 CPU1 CPU2
fill gfn0
store-rel flags for gfn0
fill gfn1
store-rel flags for gfn1
load-acq flags for gfn0
set RESET for gfn0
load-acq flags for gfn1
set RESET for gfn1
do ioctl! ----------->
ioctl(RESET_RINGS)
fill gfn2
store-rel flags for gfn2
load-acq flags for gfn2
set RESET for gfn2
process gfn0
process gfn1
process gfn2
do ioctl!
etc.
The three load-acquire in CPU0 synchronize with the three store-release
in CPU2, but CPU0 and CPU1 are only synchronized up to gfn1 and CPU1 may
miss gfn2's fields other than flags.
The kernel must be able to cope with invalid values of the fields, and
userspace will invoke the ioctl once more. However, once the RESET flag
is cleared on gfn2, it is lost forever, therefore in the above scenario
CPU1 must read the correct value of gfn2's fields.
Therefore RESET must be set with a store-release, that will synchronize
with a load-acquire in CPU1 as you suggested.
Paolo
> diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
> index f4c2a6eb1666..ea620bfb012d 100644
> --- a/virt/kvm/dirty_ring.c
> +++ b/virt/kvm/dirty_ring.c
> @@ -84,7 +84,7 @@ static inline void kvm_dirty_gfn_set_dirtied(struct kvm_dirty_gfn *gfn)
>
> static inline bool kvm_dirty_gfn_harvested(struct kvm_dirty_gfn *gfn)
> {
> - return gfn->flags & KVM_DIRTY_GFN_F_RESET;
> + return smp_load_acquire(&gfn->flags) & KVM_DIRTY_GFN_F_RESET;
> }
>
> int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring)
> ===8<===
>
> Thanks,
>
> --
> Peter Xu
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
WARNING: multiple messages have this Message-ID (diff)
From: Paolo Bonzini <pbonzini@redhat.com>
To: Peter Xu <peterx@redhat.com>, Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>,
Gavin Shan <gshan@redhat.com>,
kvmarm@lists.cs.columbia.edu,
linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kselftest@vger.kernel.org, corbet@lwn.net,
james.morse@arm.com, alexandru.elisei@arm.com,
suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org,
shuah@kernel.org, seanjc@google.com, drjones@redhat.com,
dmatlack@google.com, bgardon@google.com, ricarkol@google.com,
zhenyzha@redhat.com, shan.gavin@gmail.com
Subject: Re: [PATCH v1 1/5] KVM: arm64: Enable ring-based dirty memory tracking
Date: Fri, 2 Sep 2022 02:19:46 +0200 [thread overview]
Message-ID: <44a42d03-4dd1-3f1c-3a60-7c2a6a7d417a@redhat.com> (raw)
In-Reply-To: <Yw4hyEAyivKT35vQ@xz-m1.local>
On 8/30/22 16:42, Peter Xu wrote:
> Marc,
>
> I thought we won't hit this as long as we properly take care of other
> orderings of (a) gfn push, and (b) gfn collect, but after a second thought
> I think it's indeed logically possible that with a reversed ordering here
> we can be reading some garbage gfn before (a) happens butt also read the
> valid flag after (b).
>
> It seems we must have all the barriers correctly applied always. If that's
> correct, do you perhaps mean something like this to just add the last piece
> of barrier?
Okay, so I thought about it some more and it's quite tricky.
Strictly speaking, the synchronization is just between userspace and
kernel. The fact that the actual producer of dirty pages is in another
CPU is a red herring, because reset only cares about harvested pages.
In other words, the dirty page ring is essentially two ring buffers in
one and we only care about the "harvested ring", not the "produced ring".
On the other hand, it may happen that userspace has set more RESET flags
while the ioctl is ongoing:
CPU0 CPU1 CPU2
fill gfn0
store-rel flags for gfn0
fill gfn1
store-rel flags for gfn1
load-acq flags for gfn0
set RESET for gfn0
load-acq flags for gfn1
set RESET for gfn1
do ioctl! ----------->
ioctl(RESET_RINGS)
fill gfn2
store-rel flags for gfn2
load-acq flags for gfn2
set RESET for gfn2
process gfn0
process gfn1
process gfn2
do ioctl!
etc.
The three load-acquire in CPU0 synchronize with the three store-release
in CPU2, but CPU0 and CPU1 are only synchronized up to gfn1 and CPU1 may
miss gfn2's fields other than flags.
The kernel must be able to cope with invalid values of the fields, and
userspace will invoke the ioctl once more. However, once the RESET flag
is cleared on gfn2, it is lost forever, therefore in the above scenario
CPU1 must read the correct value of gfn2's fields.
Therefore RESET must be set with a store-release, that will synchronize
with a load-acquire in CPU1 as you suggested.
Paolo
> diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
> index f4c2a6eb1666..ea620bfb012d 100644
> --- a/virt/kvm/dirty_ring.c
> +++ b/virt/kvm/dirty_ring.c
> @@ -84,7 +84,7 @@ static inline void kvm_dirty_gfn_set_dirtied(struct kvm_dirty_gfn *gfn)
>
> static inline bool kvm_dirty_gfn_harvested(struct kvm_dirty_gfn *gfn)
> {
> - return gfn->flags & KVM_DIRTY_GFN_F_RESET;
> + return smp_load_acquire(&gfn->flags) & KVM_DIRTY_GFN_F_RESET;
> }
>
> int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring)
> ===8<===
>
> Thanks,
>
> --
> Peter Xu
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-09-02 0:20 UTC|newest]
Thread overview: 98+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-19 0:55 [PATCH v1 0/5] KVM: arm64: Enable ring-based dirty memory tracking Gavin Shan
2022-08-19 0:55 ` Gavin Shan
2022-08-19 0:55 ` Gavin Shan
2022-08-19 0:55 ` [PATCH v1 1/5] " Gavin Shan
2022-08-19 0:55 ` Gavin Shan
2022-08-19 0:55 ` Gavin Shan
2022-08-19 8:00 ` Marc Zyngier
2022-08-19 8:00 ` Marc Zyngier
2022-08-19 8:00 ` Marc Zyngier
2022-08-22 1:58 ` Gavin Shan
2022-08-22 1:58 ` Gavin Shan
2022-08-22 18:55 ` Peter Xu
2022-08-22 18:55 ` Peter Xu
2022-08-22 18:55 ` Peter Xu
2022-08-23 3:19 ` Gavin Shan
2022-08-23 3:19 ` Gavin Shan
2022-08-23 3:19 ` Gavin Shan
2022-08-22 21:42 ` Marc Zyngier
2022-08-22 21:42 ` Marc Zyngier
2022-08-22 21:42 ` Marc Zyngier
2022-08-23 5:22 ` Gavin Shan
2022-08-23 5:22 ` Gavin Shan
2022-08-23 5:22 ` Gavin Shan
2022-08-23 13:58 ` Peter Xu
2022-08-23 13:58 ` Peter Xu
2022-08-23 13:58 ` Peter Xu
2022-08-23 19:17 ` Marc Zyngier
2022-08-23 19:17 ` Marc Zyngier
2022-08-23 19:17 ` Marc Zyngier
2022-08-23 21:20 ` Peter Xu
2022-08-23 21:20 ` Peter Xu
2022-08-23 21:20 ` Peter Xu
2022-08-23 22:47 ` Marc Zyngier
2022-08-23 22:47 ` Marc Zyngier
2022-08-23 22:47 ` Marc Zyngier
2022-08-23 23:19 ` Peter Xu
2022-08-23 23:19 ` Peter Xu
2022-08-23 23:19 ` Peter Xu
2022-08-24 14:45 ` Marc Zyngier
2022-08-24 14:45 ` Marc Zyngier
2022-08-24 14:45 ` Marc Zyngier
2022-08-24 16:21 ` Peter Xu
2022-08-24 16:21 ` Peter Xu
2022-08-24 16:21 ` Peter Xu
2022-08-24 20:57 ` Marc Zyngier
2022-08-24 20:57 ` Marc Zyngier
2022-08-24 20:57 ` Marc Zyngier
2022-08-26 6:05 ` Gavin Shan
2022-08-26 6:05 ` Gavin Shan
2022-08-26 6:05 ` Gavin Shan
2022-08-26 10:50 ` Paolo Bonzini
2022-08-26 10:50 ` Paolo Bonzini
2022-08-26 10:50 ` Paolo Bonzini
2022-08-26 15:49 ` Marc Zyngier
2022-08-26 15:49 ` Marc Zyngier
2022-08-26 15:49 ` Marc Zyngier
2022-08-27 8:27 ` Paolo Bonzini
2022-08-27 8:27 ` Paolo Bonzini
2022-08-27 8:27 ` Paolo Bonzini
2022-08-29 10:27 ` Paolo Bonzini
2022-08-23 14:44 ` Oliver Upton
2022-08-23 14:44 ` Oliver Upton
2022-08-23 14:44 ` Oliver Upton
2022-08-23 20:35 ` Marc Zyngier
2022-08-23 20:35 ` Marc Zyngier
2022-08-23 20:35 ` Marc Zyngier
2022-08-26 10:58 ` Paolo Bonzini
2022-08-26 10:58 ` Paolo Bonzini
2022-08-26 10:58 ` Paolo Bonzini
2022-08-26 15:28 ` Marc Zyngier
2022-08-26 15:28 ` Marc Zyngier
2022-08-26 15:28 ` Marc Zyngier
2022-08-30 14:42 ` Peter Xu
2022-08-30 14:42 ` Peter Xu
2022-08-30 14:42 ` Peter Xu
2022-09-02 0:19 ` Paolo Bonzini [this message]
2022-09-02 0:19 ` Paolo Bonzini
2022-09-02 0:19 ` Paolo Bonzini
2022-08-19 0:55 ` [PATCH v1 2/5] KVM: selftests: Use host page size to map ring buffer in dirty_log_test Gavin Shan
2022-08-19 0:55 ` Gavin Shan
2022-08-19 0:55 ` Gavin Shan
2022-08-19 0:55 ` [PATCH v1 3/5] KVM: selftests: Dirty host pages " Gavin Shan
2022-08-19 0:55 ` Gavin Shan
2022-08-19 0:55 ` Gavin Shan
2022-08-19 5:28 ` Andrew Jones
2022-08-19 5:28 ` Andrew Jones
2022-08-19 5:28 ` Andrew Jones
2022-08-22 6:29 ` Gavin Shan
2022-08-22 6:29 ` Gavin Shan
2022-08-23 3:09 ` Gavin Shan
2022-08-23 3:09 ` Gavin Shan
2022-08-23 3:09 ` Gavin Shan
2022-08-19 0:56 ` [PATCH v1 4/5] KVM: selftests: Clear dirty ring states between two modes " Gavin Shan
2022-08-19 0:56 ` Gavin Shan
2022-08-19 0:56 ` Gavin Shan
2022-08-19 0:56 ` [PATCH v1 5/5] KVM: selftests: Automate choosing dirty ring size " Gavin Shan
2022-08-19 0:56 ` Gavin Shan
2022-08-19 0:56 ` Gavin Shan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=44a42d03-4dd1-3f1c-3a60-7c2a6a7d417a@redhat.com \
--to=pbonzini@redhat.com \
--cc=alexandru.elisei@arm.com \
--cc=bgardon@google.com \
--cc=catalin.marinas@arm.com \
--cc=corbet@lwn.net \
--cc=dmatlack@google.com \
--cc=drjones@redhat.com \
--cc=gshan@redhat.com \
--cc=james.morse@arm.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=maz@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=peterx@redhat.com \
--cc=ricarkol@google.com \
--cc=seanjc@google.com \
--cc=shan.gavin@gmail.com \
--cc=shuah@kernel.org \
--cc=suzuki.poulose@arm.com \
--cc=will@kernel.org \
--cc=zhenyzha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.