include/linux/kvm_host.h | 2 ++ virt/kvm/kvm_main.c | 5 +++++ 2 files changed, 7 insertions(+)
Fix a variety of flaws and false failures/passes in dirty_log_test, and
drop code/behavior that adds complexity while adding little-to-no benefit.
Lots of details in the changelogs, and a partial list of complaints[1] in
Maxim's original thread[2].
E.g. while not a particular interesting bug, hacking KVM like so doesn't
elicit a test failure.
---
include/linux/kvm_host.h | 2 ++
virt/kvm/kvm_main.c | 5 +++++
2 files changed, 7 insertions(+)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 401439bb21e3..bf7797ae2cdc 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -389,6 +389,8 @@ struct kvm_vcpu {
*/
struct kvm_memory_slot *last_used_slot;
u64 last_used_slot_gen;
+
+ bool extra_dirty;
};
/*
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index de2c11dae231..9981f1cc2780 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3444,6 +3444,11 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
unsigned long rel_gfn = gfn - memslot->base_gfn;
u32 slot = (memslot->as_id << 16) | memslot->id;
+ if (!vcpu->extra_dirty &&
+ gfn_to_memslot(kvm, gfn + 1) == gfn) {
+ vcpu->extra_dirty = true;
+ mark_page_dirty_in_slot(kvm, memslot, gfn + 1);
+ }
if (kvm->dirty_ring_size && vcpu)
kvm_dirty_ring_push(vcpu, slot, rel_gfn);
else if (memslot->dirty_bitmap)
--
[1] https://lore.kernel.org/all/Z1vR25ylN5m_DRSy@google.com
[2] https://lore.kernel.org/all/20241211193706.469817-1-mlevitsk@redhat.com
Maxim Levitsky (2):
KVM: selftests: Support multiple write retires in dirty_log_test
KVM: selftests: Limit dirty_log_test's s390x workaround to s390x
Sean Christopherson (18):
KVM: selftests: Sync dirty_log_test iteration to guest *before*
resuming
KVM: selftests: Drop signal/kick from dirty ring testcase
KVM: selftests: Drop stale srandom() initialization from
dirty_log_test
KVM: selftests: Precisely track number of dirty/clear pages for each
iteration
KVM: selftests: Read per-page value into local var when verifying
dirty_log_test
KVM: selftests: Continuously reap dirty ring while vCPU is running
KVM: selftests: Honor "stop" request in dirty ring test
KVM: selftests: Keep dirty_log_test vCPU in guest until it needs to
stop
KVM: selftests: Post to sem_vcpu_stop if and only if vcpu_stop is true
KVM: selftests: Use continue to handle all "pass" scenarios in
dirty_log_test
KVM: selftests: Print (previous) last_page on dirty page value
mismatch
KVM: selftests: Collect *all* dirty entries in each dirty_log_test
iteration
KVM: sefltests: Verify value of dirty_log_test last page isn't bogus
KVM: selftests: Ensure guest writes min number of pages in
dirty_log_test
KVM: selftests: Tighten checks around prev iter's last dirty page in
ring
KVM: selftests: Set per-iteration variables at the start of each
iteration
KVM: selftests: Fix an off-by-one in the number of dirty_log_test
iterations
KVM: selftests: Allow running a single iteration of dirty_log_test
tools/testing/selftests/kvm/dirty_log_test.c | 515 +++++++++----------
1 file changed, 240 insertions(+), 275 deletions(-)
base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
--
2.47.1.613.gc27f4b7a9f-googFrom: Maxim Levitsky <mlevitsk@redhat.com>
If dirty_log_test is run nested, it is possible for entries in the emulated
PML log to appear before the actual memory write is committed to the RAM,
due to the way KVM retries memory writes as a response to a MMU fault.
In addition to that in some very rare cases retry can happen more than
once, which will lead to the test failure because once the write is
finally committed it may have a very outdated iteration value.
Detect and avoid this case.
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 52 +++++++++++++++++++-
1 file changed, 50 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index aacf80f57439..cdae103314fc 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -152,6 +152,7 @@ static atomic_t vcpu_sync_stop_requested;
* sem_vcpu_stop and before vcpu continues to run.
*/
static bool dirty_ring_vcpu_ring_full;
+
/*
* This is only used for verifying the dirty pages. Dirty ring has a very
* tricky case when the ring just got full, kvm will do userspace exit due to
@@ -166,7 +167,51 @@ static bool dirty_ring_vcpu_ring_full;
* dirty gfn we've collected, so that if a mismatch of data found later in the
* verifying process, we let it pass.
*/
-static uint64_t dirty_ring_last_page;
+static uint64_t dirty_ring_last_page = -1ULL;
+
+/*
+ * In addition to the above, it is possible (especially if this
+ * test is run nested) for the above scenario to repeat multiple times:
+ *
+ * The following can happen:
+ *
+ * - L1 vCPU: Memory write is logged to PML but not committed.
+ *
+ * - L1 test thread: Ignores the write because its last dirty ring entry
+ * Resets the dirty ring which:
+ * - Resets the A/D bits in EPT
+ * - Issues tlb flush (invept), which is intercepted by L0
+ *
+ * - L0: frees the whole nested ept mmu root as the response to invept,
+ * and thus ensures that when memory write is retried, it will fault again
+ *
+ * - L1 vCPU: Same memory write is logged to the PML but not committed again.
+ *
+ * - L1 test thread: Ignores the write because its last dirty ring entry (again)
+ * Resets the dirty ring which:
+ * - Resets the A/D bits in EPT (again)
+ * - Issues tlb flush (again) which is intercepted by L0
+ *
+ * ...
+ *
+ * N times
+ *
+ * - L1 vCPU: Memory write is logged in the PML and then committed.
+ * Lots of other memory writes are logged and committed.
+ * ...
+ *
+ * - L1 test thread: Sees the memory write along with other memory writes
+ * in the dirty ring, and since the write is usually not
+ * the last entry in the dirty-ring and has a very outdated
+ * iteration, the test fails.
+ *
+ *
+ * Note that this is only possible when the write was the last log entry
+ * write during iteration N-1, thus remember last iteration last log entry
+ * and also don't fail when it is reported in the next iteration, together with
+ * an outdated iteration count.
+ */
+static uint64_t dirty_ring_prev_iteration_last_page;
enum log_mode_t {
/* Only use KVM_GET_DIRTY_LOG for logging */
@@ -316,6 +361,8 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns,
struct kvm_dirty_gfn *cur;
uint32_t count = 0;
+ dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
+
while (true) {
cur = &dirty_gfns[*fetch_index % test_dirty_ring_count];
if (!dirty_gfn_is_dirtied(cur))
@@ -613,7 +660,8 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
*/
min_iter = iteration - 1;
continue;
- } else if (page == dirty_ring_last_page) {
+ } else if (page == dirty_ring_last_page ||
+ page == dirty_ring_prev_iteration_last_page) {
/*
* Please refer to comments in
* dirty_ring_last_page.
--
2.47.1.613.gc27f4b7a9f-googSync the new iteration to the guest prior to restarting the vCPU, otherwise
it's possible for the vCPU to dirty memory for the next iteration using the
current iteration's value.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index cdae103314fc..41c158cf5444 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -859,9 +859,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
*/
if (++iteration == p->iterations)
WRITE_ONCE(host_quit, true);
-
- sem_post(&sem_vcpu_cont);
sync_global_to_guest(vm, iteration);
+
+ sem_post(&sem_vcpu_cont);
}
pthread_join(vcpu_thread, NULL);
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Sync the new iteration to the guest prior to restarting the vCPU, otherwise
> it's possible for the vCPU to dirty memory for the next iteration using the
> current iteration's value.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index cdae103314fc..41c158cf5444 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -859,9 +859,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> */
> if (++iteration == p->iterations)
> WRITE_ONCE(host_quit, true);
> -
> - sem_post(&sem_vcpu_cont);
> sync_global_to_guest(vm, iteration);
> +
> + sem_post(&sem_vcpu_cont);
> }
>
> pthread_join(vcpu_thread, NULL);
AFAIK, this patch doesn't 100% gurantee that this won't happen:
The READ_ONCE that guest uses only guarntees no wierd compiler optimizations are used.
The guest can still read the iteration value to a register, get #vmexit, after which the iteration
will be increased and then write the old value.
Is this worth to reorder this to decrease the chances of this happening? I am not sure, as this will
just make this problem rarer and thus harder to debug.
Currently the test just assumes that this can happen and deals with this.
Best regards,
Maxim Levitsky
On Tue, Dec 17, 2024, Maxim Levitsky wrote: > On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote: > > Sync the new iteration to the guest prior to restarting the vCPU, otherwise > > it's possible for the vCPU to dirty memory for the next iteration using the > > current iteration's value. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > tools/testing/selftests/kvm/dirty_log_test.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c > > index cdae103314fc..41c158cf5444 100644 > > --- a/tools/testing/selftests/kvm/dirty_log_test.c > > +++ b/tools/testing/selftests/kvm/dirty_log_test.c > > @@ -859,9 +859,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) > > */ > > if (++iteration == p->iterations) > > WRITE_ONCE(host_quit, true); > > - > > - sem_post(&sem_vcpu_cont); > > sync_global_to_guest(vm, iteration); > > + > > + sem_post(&sem_vcpu_cont); > > } > > > > pthread_join(vcpu_thread, NULL); > > > AFAIK, this patch doesn't 100% gurantee that this won't happen: > > The READ_ONCE that guest uses only guarntees no wierd compiler optimizations > are used. The guest can still read the iteration value to a register, get > #vmexit, after which the iteration will be increased and then write the old > value. Hmm, right, it's not 100% guaranteed because of the register caching angle. But it does guarantee that at most only write can retire with the previous iteration, and patch 1 from you addresses that issue, so I think this is solid? Assuming we end up going with the "collect everything for the current iteration", I'll expand the changelog to call out the dependency along with exactly what protection this does and does not provide > Is this worth to reorder this to decrease the chances of this happening? I am > not sure, as this will just make this problem rarer and thus harder to debug. > Currently the test just assumes that this can happen and deals with this. The test deals with it by effectively disabling verification. IMO, that's just hacking around a bug.
On Wed, 2024-12-18 at 13:36 -0800, Sean Christopherson wrote: > On Tue, Dec 17, 2024, Maxim Levitsky wrote: > > On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote: > > > Sync the new iteration to the guest prior to restarting the vCPU, otherwise > > > it's possible for the vCPU to dirty memory for the next iteration using the > > > current iteration's value. > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > tools/testing/selftests/kvm/dirty_log_test.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c > > > index cdae103314fc..41c158cf5444 100644 > > > --- a/tools/testing/selftests/kvm/dirty_log_test.c > > > +++ b/tools/testing/selftests/kvm/dirty_log_test.c > > > @@ -859,9 +859,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) > > > */ > > > if (++iteration == p->iterations) > > > WRITE_ONCE(host_quit, true); > > > - > > > - sem_post(&sem_vcpu_cont); > > > sync_global_to_guest(vm, iteration); > > > + > > > + sem_post(&sem_vcpu_cont); > > > } > > > > > > pthread_join(vcpu_thread, NULL); > > > > AFAIK, this patch doesn't 100% gurantee that this won't happen: > > > > The READ_ONCE that guest uses only guarntees no wierd compiler optimizations > > are used. The guest can still read the iteration value to a register, get > > #vmexit, after which the iteration will be increased and then write the old > > value. > > Hmm, right, it's not 100% guaranteed because of the register caching angle. But > it does guarantee that at most only write can retire with the previous iteration, > and patch 1 from you addresses that issue, so I think this is solid? > > Assuming we end up going with the "collect everything for the current iteration", > I'll expand the changelog to call out the dependency along with exactly what > protection this does and does not provide > > > Is this worth to reorder this to decrease the chances of this happening? I am > > not sure, as this will just make this problem rarer and thus harder to debug. > > Currently the test just assumes that this can happen and deals with this. > > The test deals with it by effectively disabling verification. IMO, that's just > hacking around a bug. > OK, let it be, but the changelog needs to be updated to state that the race is still possible. Best regards, Maxim Levitsky
Drop the signal/kick from dirty_log_test's dirty ring handling, as kicking
the vCPU adds marginal value, at the cost of adding significant complexity
to the test.
Asynchronously interrupting the vCPU isn't novel; unless the kernel is
fully tickless, the vCPU will be interrupted by IRQs for any decently
large interval.
And exiting to userspace mode in the middle of a sequence isn't novel
either, as the vCPU will do so every time the ring becomes full.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 106 +++----------------
1 file changed, 15 insertions(+), 91 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 41c158cf5444..d9911e20337f 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -236,24 +236,6 @@ static enum log_mode_t host_log_mode;
static pthread_t vcpu_thread;
static uint32_t test_dirty_ring_count = TEST_DIRTY_RING_COUNT;
-static void vcpu_kick(void)
-{
- pthread_kill(vcpu_thread, SIG_IPI);
-}
-
-/*
- * In our test we do signal tricks, let's use a better version of
- * sem_wait to avoid signal interrupts
- */
-static void sem_wait_until(sem_t *sem)
-{
- int ret;
-
- do
- ret = sem_wait(sem);
- while (ret == -1 && errno == EINTR);
-}
-
static bool clear_log_supported(void)
{
return kvm_has_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2);
@@ -292,17 +274,14 @@ static void vcpu_handle_sync_stop(void)
/* It means main thread is sleeping waiting */
atomic_set(&vcpu_sync_stop_requested, false);
sem_post(&sem_vcpu_stop);
- sem_wait_until(&sem_vcpu_cont);
+ sem_wait(&sem_vcpu_cont);
}
}
-static void default_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err)
+static void default_after_vcpu_run(struct kvm_vcpu *vcpu)
{
struct kvm_run *run = vcpu->run;
- TEST_ASSERT(ret == 0 || (ret == -1 && err == EINTR),
- "vcpu run failed: errno=%d", err);
-
TEST_ASSERT(get_ucall(vcpu, NULL) == UCALL_SYNC,
"Invalid guest sync status: exit_reason=%s",
exit_reason_str(run->exit_reason));
@@ -371,7 +350,6 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns,
"%u != %u", cur->slot, slot);
TEST_ASSERT(cur->offset < num_pages, "Offset overflow: "
"0x%llx >= 0x%x", cur->offset, num_pages);
- //pr_info("fetch 0x%x page %llu\n", *fetch_index, cur->offset);
__set_bit_le(cur->offset, bitmap);
dirty_ring_last_page = cur->offset;
dirty_gfn_set_collected(cur);
@@ -382,13 +360,6 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns,
return count;
}
-static void dirty_ring_wait_vcpu(void)
-{
- /* This makes sure that hardware PML cache flushed */
- vcpu_kick();
- sem_wait_until(&sem_vcpu_stop);
-}
-
static void dirty_ring_continue_vcpu(void)
{
pr_info("Notifying vcpu to continue\n");
@@ -400,18 +371,6 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
uint32_t *ring_buf_idx)
{
uint32_t count = 0, cleared;
- bool continued_vcpu = false;
-
- dirty_ring_wait_vcpu();
-
- if (!dirty_ring_vcpu_ring_full) {
- /*
- * This is not a ring-full event, it's safe to allow
- * vcpu to continue
- */
- dirty_ring_continue_vcpu();
- continued_vcpu = true;
- }
/* Only have one vcpu */
count = dirty_ring_collect_one(vcpu_map_dirty_ring(vcpu),
@@ -427,16 +386,13 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
TEST_ASSERT(cleared == count, "Reset dirty pages (%u) mismatch "
"with collected (%u)", cleared, count);
- if (!continued_vcpu) {
- TEST_ASSERT(dirty_ring_vcpu_ring_full,
- "Didn't continue vcpu even without ring full");
+ if (READ_ONCE(dirty_ring_vcpu_ring_full))
dirty_ring_continue_vcpu();
- }
pr_info("Iteration %ld collected %u pages\n", iteration, count);
}
-static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err)
+static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
{
struct kvm_run *run = vcpu->run;
@@ -444,17 +400,14 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err)
if (get_ucall(vcpu, NULL) == UCALL_SYNC) {
/* We should allow this to continue */
;
- } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL ||
- (ret == -1 && err == EINTR)) {
+ } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) {
/* Update the flag first before pause */
- WRITE_ONCE(dirty_ring_vcpu_ring_full,
- run->exit_reason == KVM_EXIT_DIRTY_RING_FULL);
+ WRITE_ONCE(dirty_ring_vcpu_ring_full, true);
sem_post(&sem_vcpu_stop);
- pr_info("vcpu stops because %s...\n",
- dirty_ring_vcpu_ring_full ?
- "dirty ring is full" : "vcpu is kicked out");
- sem_wait_until(&sem_vcpu_cont);
+ pr_info("Dirty ring full, waiting for it to be collected\n");
+ sem_wait(&sem_vcpu_cont);
pr_info("vcpu continues now.\n");
+ WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
} else {
TEST_ASSERT(false, "Invalid guest sync status: "
"exit_reason=%s",
@@ -473,7 +426,7 @@ struct log_mode {
void *bitmap, uint32_t num_pages,
uint32_t *ring_buf_idx);
/* Hook to call when after each vcpu run */
- void (*after_vcpu_run)(struct kvm_vcpu *vcpu, int ret, int err);
+ void (*after_vcpu_run)(struct kvm_vcpu *vcpu);
} log_modes[LOG_MODE_NUM] = {
{
.name = "dirty-log",
@@ -544,47 +497,24 @@ static void log_mode_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
mode->collect_dirty_pages(vcpu, slot, bitmap, num_pages, ring_buf_idx);
}
-static void log_mode_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err)
+static void log_mode_after_vcpu_run(struct kvm_vcpu *vcpu)
{
struct log_mode *mode = &log_modes[host_log_mode];
if (mode->after_vcpu_run)
- mode->after_vcpu_run(vcpu, ret, err);
+ mode->after_vcpu_run(vcpu);
}
static void *vcpu_worker(void *data)
{
- int ret;
struct kvm_vcpu *vcpu = data;
uint64_t pages_count = 0;
- struct kvm_signal_mask *sigmask = alloca(offsetof(struct kvm_signal_mask, sigset)
- + sizeof(sigset_t));
- sigset_t *sigset = (sigset_t *) &sigmask->sigset;
-
- /*
- * SIG_IPI is unblocked atomically while in KVM_RUN. It causes the
- * ioctl to return with -EINTR, but it is still pending and we need
- * to accept it with the sigwait.
- */
- sigmask->len = 8;
- pthread_sigmask(0, NULL, sigset);
- sigdelset(sigset, SIG_IPI);
- vcpu_ioctl(vcpu, KVM_SET_SIGNAL_MASK, sigmask);
-
- sigemptyset(sigset);
- sigaddset(sigset, SIG_IPI);
while (!READ_ONCE(host_quit)) {
- /* Clear any existing kick signals */
pages_count += TEST_PAGES_PER_LOOP;
/* Let the guest dirty the random pages */
- ret = __vcpu_run(vcpu);
- if (ret == -1 && errno == EINTR) {
- int sig = -1;
- sigwait(sigset, &sig);
- assert(sig == SIG_IPI);
- }
- log_mode_after_vcpu_run(vcpu, ret, errno);
+ vcpu_run(vcpu);
+ log_mode_after_vcpu_run(vcpu);
}
pr_info("Dirtied %"PRIu64" pages\n", pages_count);
@@ -838,7 +768,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
* we need to stop vcpu when verify data.
*/
atomic_set(&vcpu_sync_stop_requested, true);
- sem_wait_until(&sem_vcpu_stop);
+ sem_wait(&sem_vcpu_stop);
/*
* NOTE: for dirty ring, it's possible that we didn't stop at
* GUEST_SYNC but instead we stopped because ring is full;
@@ -905,7 +835,6 @@ int main(int argc, char *argv[])
.interval = TEST_HOST_LOOP_INTERVAL,
};
int opt, i;
- sigset_t sigset;
sem_init(&sem_vcpu_stop, 0, 0);
sem_init(&sem_vcpu_cont, 0, 0);
@@ -964,11 +893,6 @@ int main(int argc, char *argv[])
srandom(time(0));
- /* Ensure that vCPU threads start with SIG_IPI blocked. */
- sigemptyset(&sigset);
- sigaddset(&sigset, SIG_IPI);
- pthread_sigmask(SIG_BLOCK, &sigset, NULL);
-
if (host_log_mode_option == LOG_MODE_ALL) {
/* Run each log mode */
for (i = 0; i < LOG_MODE_NUM; i++) {
--
2.47.1.613.gc27f4b7a9f-googDrop an srandom() initialization that was leftover from the conversion to
use selftests' guest_random_xxx() APIs.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index d9911e20337f..55a744373c80 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -891,8 +891,6 @@ int main(int argc, char *argv[])
pr_info("Test iterations: %"PRIu64", interval: %"PRIu64" (ms)\n",
p.iterations, p.interval);
- srandom(time(0));
-
if (host_log_mode_option == LOG_MODE_ALL) {
/* Run each log mode */
for (i = 0; i < LOG_MODE_NUM; i++) {
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Drop an srandom() initialization that was leftover from the conversion to
> use selftests' guest_random_xxx() APIs.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index d9911e20337f..55a744373c80 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -891,8 +891,6 @@ int main(int argc, char *argv[])
> pr_info("Test iterations: %"PRIu64", interval: %"PRIu64" (ms)\n",
> p.iterations, p.interval);
>
> - srandom(time(0));
> -
> if (host_log_mode_option == LOG_MODE_ALL) {
> /* Run each log mode */
> for (i = 0; i < LOG_MODE_NUM; i++) {
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
Track and print the number of dirty and clear pages for each iteration.
This provides parity between all log modes, and will allow collecting the
dirty ring multiple times per iteration without spamming the console.
Opportunistically drop the "Dirtied N pages" print, which is redundant
and wrong. For the dirty ring testcase, the vCPU isn't guaranteed to
complete a loop. And when the vCPU does complete a loot, there are no
guarantees that it has *dirtied* that many pages; because the writes are
to random address, the vCPU may have written the same page over and over,
i.e. only dirtied one page.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 55a744373c80..08cbecd1a135 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -388,8 +388,6 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
if (READ_ONCE(dirty_ring_vcpu_ring_full))
dirty_ring_continue_vcpu();
-
- pr_info("Iteration %ld collected %u pages\n", iteration, count);
}
static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
@@ -508,24 +506,20 @@ static void log_mode_after_vcpu_run(struct kvm_vcpu *vcpu)
static void *vcpu_worker(void *data)
{
struct kvm_vcpu *vcpu = data;
- uint64_t pages_count = 0;
while (!READ_ONCE(host_quit)) {
- pages_count += TEST_PAGES_PER_LOOP;
/* Let the guest dirty the random pages */
vcpu_run(vcpu);
log_mode_after_vcpu_run(vcpu);
}
- pr_info("Dirtied %"PRIu64" pages\n", pages_count);
-
return NULL;
}
static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
{
+ uint64_t page, nr_dirty_pages = 0, nr_clean_pages = 0;
uint64_t step = vm_num_host_pages(mode, 1);
- uint64_t page;
uint64_t *value_ptr;
uint64_t min_iter = 0;
@@ -544,7 +538,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
if (__test_and_clear_bit_le(page, bmap)) {
bool matched;
- host_dirty_count++;
+ nr_dirty_pages++;
/*
* If the bit is set, the value written onto
@@ -605,7 +599,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
" incorrect (iteration=%"PRIu64")",
page, *value_ptr, iteration);
} else {
- host_clear_count++;
+ nr_clean_pages++;
/*
* If cleared, the value written can be any
* value smaller or equals to the iteration
@@ -639,6 +633,12 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
}
}
}
+
+ pr_info("Iteration %2ld: dirty: %-6lu clean: %-6lu\n",
+ iteration, nr_dirty_pages, nr_clean_pages);
+
+ host_dirty_count += nr_dirty_pages;
+ host_clear_count += nr_clean_pages;
}
static struct kvm_vm *create_vm(enum vm_guest_mode mode, struct kvm_vcpu **vcpu,
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Track and print the number of dirty and clear pages for each iteration.
> This provides parity between all log modes, and will allow collecting the
> dirty ring multiple times per iteration without spamming the console.
>
> Opportunistically drop the "Dirtied N pages" print, which is redundant
> and wrong. For the dirty ring testcase, the vCPU isn't guaranteed to
> complete a loop. And when the vCPU does complete a loot, there are no
Typo
> guarantees that it has *dirtied* that many pages; because the writes are
> to random address, the vCPU may have written the same page over and over,
> i.e. only dirtied one page.
Counting how many times a vCPU wrote is also a valid statistic
I think it would be the best to include it as well (e.g call it number of
loops that vCPU did).
Best regards,
Maxim Levitsky
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 55a744373c80..08cbecd1a135 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -388,8 +388,6 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
>
> if (READ_ONCE(dirty_ring_vcpu_ring_full))
> dirty_ring_continue_vcpu();
> -
> - pr_info("Iteration %ld collected %u pages\n", iteration, count);
> }
>
> static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
> @@ -508,24 +506,20 @@ static void log_mode_after_vcpu_run(struct kvm_vcpu *vcpu)
> static void *vcpu_worker(void *data)
> {
> struct kvm_vcpu *vcpu = data;
> - uint64_t pages_count = 0;
>
> while (!READ_ONCE(host_quit)) {
> - pages_count += TEST_PAGES_PER_LOOP;
> /* Let the guest dirty the random pages */
> vcpu_run(vcpu);
> log_mode_after_vcpu_run(vcpu);
> }
>
> - pr_info("Dirtied %"PRIu64" pages\n", pages_count);
> -
> return NULL;
> }
>
> static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> {
> + uint64_t page, nr_dirty_pages = 0, nr_clean_pages = 0;
> uint64_t step = vm_num_host_pages(mode, 1);
> - uint64_t page;
> uint64_t *value_ptr;
> uint64_t min_iter = 0;
>
> @@ -544,7 +538,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> if (__test_and_clear_bit_le(page, bmap)) {
> bool matched;
>
> - host_dirty_count++;
> + nr_dirty_pages++;
>
> /*
> * If the bit is set, the value written onto
> @@ -605,7 +599,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> " incorrect (iteration=%"PRIu64")",
> page, *value_ptr, iteration);
> } else {
> - host_clear_count++;
> + nr_clean_pages++;
> /*
> * If cleared, the value written can be any
> * value smaller or equals to the iteration
> @@ -639,6 +633,12 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> }
> }
> }
> +
> + pr_info("Iteration %2ld: dirty: %-6lu clean: %-6lu\n",
> + iteration, nr_dirty_pages, nr_clean_pages);
> +
> + host_dirty_count += nr_dirty_pages;
> + host_clear_count += nr_clean_pages;
> }
>
> static struct kvm_vm *create_vm(enum vm_guest_mode mode, struct kvm_vcpu **vcpu,
On Tue, Dec 17, 2024, Maxim Levitsky wrote: > On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote: > > Track and print the number of dirty and clear pages for each iteration. > > This provides parity between all log modes, and will allow collecting the > > dirty ring multiple times per iteration without spamming the console. > > > > Opportunistically drop the "Dirtied N pages" print, which is redundant > > and wrong. For the dirty ring testcase, the vCPU isn't guaranteed to > > complete a loop. And when the vCPU does complete a loot, there are no > Typo > > guarantees that it has *dirtied* that many pages; because the writes are > > to random address, the vCPU may have written the same page over and over, > > i.e. only dirtied one page. > > Counting how many times a vCPU wrote is also a valid statistic > > I think it would be the best to include it as well (e.g call it number of > loops that vCPU did). Heh, I originally had it that way, but dropped it because it didn't seem all that interesting. I'll add it back.
Cache the page's value during verification in a local variable, re-reading
from the pointer is ugly and error prone, e.g. allows for bugs like
checking the pointer itself instead of the value.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 08cbecd1a135..5a04a7bd73e0 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -520,11 +520,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
{
uint64_t page, nr_dirty_pages = 0, nr_clean_pages = 0;
uint64_t step = vm_num_host_pages(mode, 1);
- uint64_t *value_ptr;
uint64_t min_iter = 0;
for (page = 0; page < host_num_pages; page += step) {
- value_ptr = host_test_mem + page * host_page_size;
+ uint64_t val = *(uint64_t *)(host_test_mem + page * host_page_size);
/* If this is a special page that we were tracking... */
if (__test_and_clear_bit_le(page, host_bmap_track)) {
@@ -545,11 +544,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
* the corresponding page should be either the
* previous iteration number or the current one.
*/
- matched = (*value_ptr == iteration ||
- *value_ptr == iteration - 1);
+ matched = (val == iteration || val == iteration - 1);
if (host_log_mode == LOG_MODE_DIRTY_RING && !matched) {
- if (*value_ptr == iteration - 2 && min_iter <= iteration - 2) {
+ if (val == iteration - 2 && min_iter <= iteration - 2) {
/*
* Short answer: this case is special
* only for dirty ring test where the
@@ -597,7 +595,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
TEST_ASSERT(matched,
"Set page %"PRIu64" value %"PRIu64
" incorrect (iteration=%"PRIu64")",
- page, *value_ptr, iteration);
+ page, val, iteration);
} else {
nr_clean_pages++;
/*
@@ -619,11 +617,11 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
* we'll see that page P is cleared, with
* value "iteration-1".
*/
- TEST_ASSERT(*value_ptr <= iteration,
+ TEST_ASSERT(val <= iteration,
"Clear page %"PRIu64" value %"PRIu64
" incorrect (iteration=%"PRIu64")",
- page, *value_ptr, iteration);
- if (*value_ptr == iteration) {
+ page, val, iteration);
+ if (val == iteration) {
/*
* This page is _just_ modified; it
* should report its dirtyness in the
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Cache the page's value during verification in a local variable, re-reading
> from the pointer is ugly and error prone, e.g. allows for bugs like
> checking the pointer itself instead of the value.
You should note that there are no functional changes in this patch.
Besides this,
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 08cbecd1a135..5a04a7bd73e0 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -520,11 +520,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> {
> uint64_t page, nr_dirty_pages = 0, nr_clean_pages = 0;
> uint64_t step = vm_num_host_pages(mode, 1);
> - uint64_t *value_ptr;
> uint64_t min_iter = 0;
>
> for (page = 0; page < host_num_pages; page += step) {
> - value_ptr = host_test_mem + page * host_page_size;
> + uint64_t val = *(uint64_t *)(host_test_mem + page * host_page_size);
>
> /* If this is a special page that we were tracking... */
> if (__test_and_clear_bit_le(page, host_bmap_track)) {
> @@ -545,11 +544,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> * the corresponding page should be either the
> * previous iteration number or the current one.
> */
> - matched = (*value_ptr == iteration ||
> - *value_ptr == iteration - 1);
> + matched = (val == iteration || val == iteration - 1);
>
> if (host_log_mode == LOG_MODE_DIRTY_RING && !matched) {
> - if (*value_ptr == iteration - 2 && min_iter <= iteration - 2) {
> + if (val == iteration - 2 && min_iter <= iteration - 2) {
> /*
> * Short answer: this case is special
> * only for dirty ring test where the
> @@ -597,7 +595,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> TEST_ASSERT(matched,
> "Set page %"PRIu64" value %"PRIu64
> " incorrect (iteration=%"PRIu64")",
> - page, *value_ptr, iteration);
> + page, val, iteration);
> } else {
> nr_clean_pages++;
> /*
> @@ -619,11 +617,11 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> * we'll see that page P is cleared, with
> * value "iteration-1".
> */
> - TEST_ASSERT(*value_ptr <= iteration,
> + TEST_ASSERT(val <= iteration,
> "Clear page %"PRIu64" value %"PRIu64
> " incorrect (iteration=%"PRIu64")",
> - page, *value_ptr, iteration);
> - if (*value_ptr == iteration) {
> + page, val, iteration);
> + if (val == iteration) {
> /*
> * This page is _just_ modified; it
> * should report its dirtyness in the
Continue collecting entries from the dirty ring for the entire time the
vCPU is running. Collecting exactly once all but guarantees the vCPU will
encounter a "ring full" event and stop. While testing ring full is
interesting, stopping and doing nothing is not, especially for larger
intervals as the test effectively does nothing for a much longer time.
To balance continuous collection with letting the guest make forward
progress, chunk the interval waiting into 1ms loops (which also makes
the math dead simple).
To maintain coverage for "ring full", collect entries on subsequent
iterations if and only if the ring has been filled at least once. I.e.
let the ring fill up (if the interval allows), but after that contiuously
empty it so that the vCPU can keep running.
Opportunistically drop unnecessary zero-initialization of "count".
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 63 ++++++++++++++------
1 file changed, 46 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 5a04a7bd73e0..2aee047b8b1c 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -340,8 +340,6 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns,
struct kvm_dirty_gfn *cur;
uint32_t count = 0;
- dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
-
while (true) {
cur = &dirty_gfns[*fetch_index % test_dirty_ring_count];
if (!dirty_gfn_is_dirtied(cur))
@@ -360,17 +358,11 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns,
return count;
}
-static void dirty_ring_continue_vcpu(void)
-{
- pr_info("Notifying vcpu to continue\n");
- sem_post(&sem_vcpu_cont);
-}
-
static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
void *bitmap, uint32_t num_pages,
uint32_t *ring_buf_idx)
{
- uint32_t count = 0, cleared;
+ uint32_t count, cleared;
/* Only have one vcpu */
count = dirty_ring_collect_one(vcpu_map_dirty_ring(vcpu),
@@ -385,9 +377,6 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
*/
TEST_ASSERT(cleared == count, "Reset dirty pages (%u) mismatch "
"with collected (%u)", cleared, count);
-
- if (READ_ONCE(dirty_ring_vcpu_ring_full))
- dirty_ring_continue_vcpu();
}
static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
@@ -404,7 +393,6 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
sem_post(&sem_vcpu_stop);
pr_info("Dirty ring full, waiting for it to be collected\n");
sem_wait(&sem_vcpu_cont);
- pr_info("vcpu continues now.\n");
WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
} else {
TEST_ASSERT(false, "Invalid guest sync status: "
@@ -755,11 +743,52 @@ static void run_test(enum vm_guest_mode mode, void *arg)
pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu);
while (iteration < p->iterations) {
+ bool saw_dirty_ring_full = false;
+ unsigned long i;
+
+ dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
+
/* Give the vcpu thread some time to dirty some pages */
- usleep(p->interval * 1000);
- log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
- bmap, host_num_pages,
- &ring_buf_idx);
+ for (i = 0; i < p->interval; i++) {
+ usleep(1000);
+
+ /*
+ * Reap dirty pages while the guest is running so that
+ * dirty ring full events are resolved, i.e. so that a
+ * larger interval doesn't always end up with a vCPU
+ * that's effectively blocked. Collecting while the
+ * guest is running also verifies KVM doesn't lose any
+ * state.
+ *
+ * For bitmap modes, KVM overwrites the entire bitmap,
+ * i.e. collecting the bitmaps is destructive. Collect
+ * the bitmap only on the first pass, otherwise this
+ * test would lose track of dirty pages.
+ */
+ if (i && host_log_mode != LOG_MODE_DIRTY_RING)
+ continue;
+
+ /*
+ * For the dirty ring, empty the ring on subsequent
+ * passes only if the ring was filled at least once,
+ * to verify KVM's handling of a full ring (emptying
+ * the ring on every pass would make it unlikely the
+ * vCPU would ever fill the fing).
+ */
+ if (READ_ONCE(dirty_ring_vcpu_ring_full))
+ saw_dirty_ring_full = true;
+ if (i && !saw_dirty_ring_full)
+ continue;
+
+ log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
+ bmap, host_num_pages,
+ &ring_buf_idx);
+
+ if (READ_ONCE(dirty_ring_vcpu_ring_full)) {
+ pr_info("Dirty ring emptied, restarting vCPU\n");
+ sem_post(&sem_vcpu_cont);
+ }
+ }
/*
* See vcpu_sync_stop_requested definition for details on why
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Continue collecting entries from the dirty ring for the entire time the
> vCPU is running. Collecting exactly once all but guarantees the vCPU will
> encounter a "ring full" event and stop. While testing ring full is
> interesting, stopping and doing nothing is not, especially for larger
> intervals as the test effectively does nothing for a much longer time.
>
> To balance continuous collection with letting the guest make forward
> progress, chunk the interval waiting into 1ms loops (which also makes
> the math dead simple).
>
> To maintain coverage for "ring full", collect entries on subsequent
> iterations if and only if the ring has been filled at least once. I.e.
> let the ring fill up (if the interval allows), but after that contiuously
> empty it so that the vCPU can keep running.
>
> Opportunistically drop unnecessary zero-initialization of "count".
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 63 ++++++++++++++------
> 1 file changed, 46 insertions(+), 17 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 5a04a7bd73e0..2aee047b8b1c 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -340,8 +340,6 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns,
> struct kvm_dirty_gfn *cur;
> uint32_t count = 0;
>
> - dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
> -
> while (true) {
> cur = &dirty_gfns[*fetch_index % test_dirty_ring_count];
> if (!dirty_gfn_is_dirtied(cur))
> @@ -360,17 +358,11 @@ static uint32_t dirty_ring_collect_one(struct kvm_dirty_gfn *dirty_gfns,
> return count;
> }
>
> -static void dirty_ring_continue_vcpu(void)
> -{
> - pr_info("Notifying vcpu to continue\n");
> - sem_post(&sem_vcpu_cont);
> -}
> -
> static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
> void *bitmap, uint32_t num_pages,
> uint32_t *ring_buf_idx)
> {
> - uint32_t count = 0, cleared;
> + uint32_t count, cleared;
>
> /* Only have one vcpu */
> count = dirty_ring_collect_one(vcpu_map_dirty_ring(vcpu),
> @@ -385,9 +377,6 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
> */
> TEST_ASSERT(cleared == count, "Reset dirty pages (%u) mismatch "
> "with collected (%u)", cleared, count);
> -
> - if (READ_ONCE(dirty_ring_vcpu_ring_full))
> - dirty_ring_continue_vcpu();
> }
>
> static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
> @@ -404,7 +393,6 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
> sem_post(&sem_vcpu_stop);
> pr_info("Dirty ring full, waiting for it to be collected\n");
> sem_wait(&sem_vcpu_cont);
> - pr_info("vcpu continues now.\n");
> WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
> } else {
> TEST_ASSERT(false, "Invalid guest sync status: "
> @@ -755,11 +743,52 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu);
>
> while (iteration < p->iterations) {
> + bool saw_dirty_ring_full = false;
> + unsigned long i;
> +
> + dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
> +
> /* Give the vcpu thread some time to dirty some pages */
> - usleep(p->interval * 1000);
> - log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
> - bmap, host_num_pages,
> - &ring_buf_idx);
> + for (i = 0; i < p->interval; i++) {
> + usleep(1000);
> +
> + /*
> + * Reap dirty pages while the guest is running so that
> + * dirty ring full events are resolved, i.e. so that a
> + * larger interval doesn't always end up with a vCPU
> + * that's effectively blocked. Collecting while the
> + * guest is running also verifies KVM doesn't lose any
> + * state.
> + *
> + * For bitmap modes, KVM overwrites the entire bitmap,
> + * i.e. collecting the bitmaps is destructive. Collect
> + * the bitmap only on the first pass, otherwise this
> + * test would lose track of dirty pages.
> + */
> + if (i && host_log_mode != LOG_MODE_DIRTY_RING)
> + continue;
> +
> + /*
> + * For the dirty ring, empty the ring on subsequent
> + * passes only if the ring was filled at least once,
> + * to verify KVM's handling of a full ring (emptying
> + * the ring on every pass would make it unlikely the
> + * vCPU would ever fill the fing).
> + */
> + if (READ_ONCE(dirty_ring_vcpu_ring_full))
> + saw_dirty_ring_full = true;
> + if (i && !saw_dirty_ring_full)
> + continue;
> +
> + log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
> + bmap, host_num_pages,
> + &ring_buf_idx);
> +
> + if (READ_ONCE(dirty_ring_vcpu_ring_full)) {
> + pr_info("Dirty ring emptied, restarting vCPU\n");
> + sem_post(&sem_vcpu_cont);
> + }
> + }
>
> /*
> * See vcpu_sync_stop_requested definition for details on why
This looks reasonable.
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
On Tue, Dec 17, 2024, Maxim Levitsky wrote: > On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote: > > When running dirty_log_test using the dirty ring, post to sem_vcpu_stop > > only when the main thread has explicitly requested that the vCPU stop. > > Synchronizing the vCPU and main thread whenever the dirty ring happens to > > be full is unnecessary, as KVM's ABI is to actively prevent the vCPU from > > running until the ring is no longer full. I.e. attempting to run the vCPU > > will simply result in KVM_EXIT_DIRTY_RING_FULL without ever entering the > > guest. And if KVM doesn't exit, e.g. let's the vCPU dirty more pages, > > then that's a KVM bug worth finding. > > This is probably a good idea to do sometimes, but this can also reduce > coverage because now the vCPU will pointlessly enter and exit when dirty log > is full. But the alternative is simply waiting in host userspace. I agree that doing KVM_RUN when it's guaranteed to get hairpinned back to userspace isn't all that interesting, but it's arguably better than having that task scheduled out while waiting for the main thread, and I definitely don't think it's any worse.
From: Maxim Levitsky <mlevitsk@redhat.com>
s390 specific workaround causes the dirty-log mode of the test to dirty
all guest memory on the first iteration, which is very slow when the test
is run in a nested VM.
Limit this workaround to s390x.
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 2aee047b8b1c..55a385499434 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -98,6 +98,7 @@ static void guest_code(void)
uint64_t addr;
int i;
+#ifdef __s390x__
/*
* On s390x, all pages of a 1M segment are initially marked as dirty
* when a page of the segment is written to for the very first time.
@@ -108,6 +109,7 @@ static void guest_code(void)
addr = guest_test_virt_mem + i * guest_page_size;
vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration));
}
+#endif
while (true) {
for (i = 0; i < TEST_PAGES_PER_LOOP; i++) {
--
2.47.1.613.gc27f4b7a9f-googNow that the vCPU doesn't dirty every page on the first iteration for
architectures that support the dirty ring, honor vcpu_stop in the dirty
ring's vCPU worker, i.e. stop when the main thread says "stop". This will
allow plumbing vcpu_stop into the guest so that the vCPU doesn't need to
periodically exit to userspace just to see if it should stop.
Add a comment explaining that marking all pages as dirty is problematic
for the dirty ring, as it results in the guest getting stuck on "ring
full". This could be addressed by adding a GUEST_SYNC() in that initial
loop, but it's not clear how that would interact with s390's behavior.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 55a385499434..8d31e275a23d 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -387,8 +387,7 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
/* A ucall-sync or ring-full event is allowed */
if (get_ucall(vcpu, NULL) == UCALL_SYNC) {
- /* We should allow this to continue */
- ;
+ vcpu_handle_sync_stop();
} else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) {
/* Update the flag first before pause */
WRITE_ONCE(dirty_ring_vcpu_ring_full, true);
@@ -697,6 +696,15 @@ static void run_test(enum vm_guest_mode mode, void *arg)
#ifdef __s390x__
/* Align to 1M (segment size) */
guest_test_phys_mem = align_down(guest_test_phys_mem, 1 << 20);
+
+ /*
+ * The workaround in guest_code() to write all pages prior to the first
+ * iteration isn't compatible with the dirty ring, as the dirty ring
+ * support relies on the vCPU to actually stop when vcpu_stop is set so
+ * that the vCPU doesn't hang waiting for the dirty ring to be emptied.
+ */
+ TEST_ASSERT(host_log_mode != LOG_MODE_DIRTY_RING,
+ "Test needs to be updated to support s390 dirty ring");
#endif
pr_info("guest physical test memory offset: 0x%lx\n", guest_test_phys_mem);
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Now that the vCPU doesn't dirty every page on the first iteration for
> architectures that support the dirty ring, honor vcpu_stop in the dirty
> ring's vCPU worker, i.e. stop when the main thread says "stop". This will
> allow plumbing vcpu_stop into the guest so that the vCPU doesn't need to
> periodically exit to userspace just to see if it should stop.
This is very misleading - by the very nature of this test it all runs in userspace,
so every time KVM_RUN ioctl exits, it is by definition an userspace VM exit.
>
> Add a comment explaining that marking all pages as dirty is problematic
> for the dirty ring, as it results in the guest getting stuck on "ring
> full". This could be addressed by adding a GUEST_SYNC() in that initial
> loop, but it's not clear how that would interact with s390's behavior.
I think that this commit description should be reworked to state that s390
doesn't support dirty ring currently so the test doesn't introduce a regression.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 55a385499434..8d31e275a23d 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -387,8 +387,7 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
>
> /* A ucall-sync or ring-full event is allowed */
> if (get_ucall(vcpu, NULL) == UCALL_SYNC) {
> - /* We should allow this to continue */
> - ;
> + vcpu_handle_sync_stop();
> } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) {
> /* Update the flag first before pause */
> WRITE_ONCE(dirty_ring_vcpu_ring_full, true);
> @@ -697,6 +696,15 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> #ifdef __s390x__
> /* Align to 1M (segment size) */
> guest_test_phys_mem = align_down(guest_test_phys_mem, 1 << 20);
> +
> + /*
> + * The workaround in guest_code() to write all pages prior to the first
> + * iteration isn't compatible with the dirty ring, as the dirty ring
> + * support relies on the vCPU to actually stop when vcpu_stop is set so
> + * that the vCPU doesn't hang waiting for the dirty ring to be emptied.
> + */
> + TEST_ASSERT(host_log_mode != LOG_MODE_DIRTY_RING,
> + "Test needs to be updated to support s390 dirty ring");
This not clear either, the message makes me think that s390 does support dirty ring.
The comment above should state stat since s390 doesn't support dirty ring,
this is fine, and when/if the support is added,then the test will need to be updated.
> #endif
>
> pr_info("guest physical test memory offset: 0x%lx\n", guest_test_phys_mem);
Best regards,
Maxim Levitsky
On Tue, Dec 17, 2024, Maxim Levitsky wrote:
> On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> > Now that the vCPU doesn't dirty every page on the first iteration for
> > architectures that support the dirty ring, honor vcpu_stop in the dirty
> > ring's vCPU worker, i.e. stop when the main thread says "stop". This will
> > allow plumbing vcpu_stop into the guest so that the vCPU doesn't need to
> > periodically exit to userspace just to see if it should stop.
>
> This is very misleading - by the very nature of this test it all runs in
> userspace, so every time KVM_RUN ioctl exits, it is by definition an
> userspace VM exit.
I honestly don't see how being more precise is misleading. I'm happy to reword
the changelog, but IMO just saying "exit" doesn't make it clear that the goal is
to avoid the deliberate exit to the selftest's userspace side of things. The
vCPU is constantly exiting to KVM for dirty logging, so to me, "periodically exit
just to see if it should stop" is confusing and ambiguous.
> > Add a comment explaining that marking all pages as dirty is problematic
> > for the dirty ring, as it results in the guest getting stuck on "ring
> > full". This could be addressed by adding a GUEST_SYNC() in that initial
> > loop, but it's not clear how that would interact with s390's behavior.
>
> I think that this commit description should be reworked to state that s390
> doesn't support dirty ring currently so the test doesn't introduce a regression.
Can do.
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> > tools/testing/selftests/kvm/dirty_log_test.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> > index 55a385499434..8d31e275a23d 100644
> > --- a/tools/testing/selftests/kvm/dirty_log_test.c
> > +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> > @@ -387,8 +387,7 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
> >
> > /* A ucall-sync or ring-full event is allowed */
> > if (get_ucall(vcpu, NULL) == UCALL_SYNC) {
> > - /* We should allow this to continue */
> > - ;
> > + vcpu_handle_sync_stop();
> > } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) {
> > /* Update the flag first before pause */
> > WRITE_ONCE(dirty_ring_vcpu_ring_full, true);
> > @@ -697,6 +696,15 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> > #ifdef __s390x__
> > /* Align to 1M (segment size) */
> > guest_test_phys_mem = align_down(guest_test_phys_mem, 1 << 20);
> > +
> > + /*
> > + * The workaround in guest_code() to write all pages prior to the first
> > + * iteration isn't compatible with the dirty ring, as the dirty ring
> > + * support relies on the vCPU to actually stop when vcpu_stop is set so
> > + * that the vCPU doesn't hang waiting for the dirty ring to be emptied.
> > + */
> > + TEST_ASSERT(host_log_mode != LOG_MODE_DIRTY_RING,
> > + "Test needs to be updated to support s390 dirty ring");
>
> This not clear either, the message makes me think that s390 does support dirty ring.
> The comment above should state stat since s390 doesn't support dirty ring,
> this is fine, and when/if the support is added,then the test will need to be updated.
How about this?
/*
* The s390 workaround in guest_code() to write all pages prior to the
* first iteration isn't compatible with the dirty ring test, as dirty
* ring testing relies on the vCPU to actually stop when vcpu_stop is
* set. If the vCPU doesn't stop, it will hang waiting for the dirty
* ring to be emptied. s390 doesn't currently support the dirty ring,
* and it's not clear how best to resolve the situation, so punt the
* problem to the future.
*/
TEST_ASSERT(host_log_mode != LOG_MODE_DIRTY_RING,
"Test needs to be updated to support dirty ring on s390; see comment for details");
On Wed, 2024-12-18 at 18:00 -0800, Sean Christopherson wrote:
> On Tue, Dec 17, 2024, Maxim Levitsky wrote:
> > On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> > > Now that the vCPU doesn't dirty every page on the first iteration for
> > > architectures that support the dirty ring, honor vcpu_stop in the dirty
> > > ring's vCPU worker, i.e. stop when the main thread says "stop". This will
> > > allow plumbing vcpu_stop into the guest so that the vCPU doesn't need to
> > > periodically exit to userspace just to see if it should stop.
> >
> > This is very misleading - by the very nature of this test it all runs in
> > userspace, so every time KVM_RUN ioctl exits, it is by definition an
> > userspace VM exit.
>
> I honestly don't see how being more precise is misleading.
"Exit to userspace" is misleading - the *whole test* is userspace.
You treat vCPU worker thread as if it not userspace, but it is *userspace* by the
definition of how KVM works.
Right way to say it is something like 'don't pause the vCPU worker thread when its not needed'
or something like that.
Best regards,
Maxim Levitsky
> I'm happy to reword
> the changelog, but IMO just saying "exit" doesn't make it clear that the goal is
> to avoid the deliberate exit to the selftest's userspace side of things. The
> vCPU is constantly exiting to KVM for dirty logging, so to me, "periodically exit
> just to see if it should stop" is confusing and ambiguous.
>
> > > Add a comment explaining that marking all pages as dirty is problematic
> > > for the dirty ring, as it results in the guest getting stuck on "ring
> > > full". This could be addressed by adding a GUEST_SYNC() in that initial
> > > loop, but it's not clear how that would interact with s390's behavior.
> >
> > I think that this commit description should be reworked to state that s390
> > doesn't support dirty ring currently so the test doesn't introduce a regression.
>
> Can do.
>
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > > tools/testing/selftests/kvm/dirty_log_test.c | 12 ++++++++++--
> > > 1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> > > index 55a385499434..8d31e275a23d 100644
> > > --- a/tools/testing/selftests/kvm/dirty_log_test.c
> > > +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> > > @@ -387,8 +387,7 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
> > >
> > > /* A ucall-sync or ring-full event is allowed */
> > > if (get_ucall(vcpu, NULL) == UCALL_SYNC) {
> > > - /* We should allow this to continue */
> > > - ;
> > > + vcpu_handle_sync_stop();
> > > } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) {
> > > /* Update the flag first before pause */
> > > WRITE_ONCE(dirty_ring_vcpu_ring_full, true);
> > > @@ -697,6 +696,15 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> > > #ifdef __s390x__
> > > /* Align to 1M (segment size) */
> > > guest_test_phys_mem = align_down(guest_test_phys_mem, 1 << 20);
> > > +
> > > + /*
> > > + * The workaround in guest_code() to write all pages prior to the first
> > > + * iteration isn't compatible with the dirty ring, as the dirty ring
> > > + * support relies on the vCPU to actually stop when vcpu_stop is set so
> > > + * that the vCPU doesn't hang waiting for the dirty ring to be emptied.
> > > + */
> > > + TEST_ASSERT(host_log_mode != LOG_MODE_DIRTY_RING,
> > > + "Test needs to be updated to support s390 dirty ring");
> >
> > This not clear either, the message makes me think that s390 does support dirty ring.
> > The comment above should state stat since s390 doesn't support dirty ring,
> > this is fine, and when/if the support is added,then the test will need to be updated.
>
> How about this?
>
> /*
> * The s390 workaround in guest_code() to write all pages prior to the
> * first iteration isn't compatible with the dirty ring test, as dirty
> * ring testing relies on the vCPU to actually stop when vcpu_stop is
> * set. If the vCPU doesn't stop, it will hang waiting for the dirty
> * ring to be emptied. s390 doesn't currently support the dirty ring,
> * and it's not clear how best to resolve the situation, so punt the
> * problem to the future.
> */
> TEST_ASSERT(host_log_mode != LOG_MODE_DIRTY_RING,
> "Test needs to be updated to support dirty ring on s390; see comment for details");
>
On Thu, Dec 19, 2024, Maxim Levitsky wrote:
> On Wed, 2024-12-18 at 18:00 -0800, Sean Christopherson wrote:
> > On Tue, Dec 17, 2024, Maxim Levitsky wrote:
> > > On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> > > > Now that the vCPU doesn't dirty every page on the first iteration for
> > > > architectures that support the dirty ring, honor vcpu_stop in the dirty
> > > > ring's vCPU worker, i.e. stop when the main thread says "stop". This will
> > > > allow plumbing vcpu_stop into the guest so that the vCPU doesn't need to
> > > > periodically exit to userspace just to see if it should stop.
> > >
> > > This is very misleading - by the very nature of this test it all runs in
> > > userspace, so every time KVM_RUN ioctl exits, it is by definition an
> > > userspace VM exit.
> >
> > I honestly don't see how being more precise is misleading.
>
> "Exit to userspace" is misleading - the *whole test* is userspace.
No, the test has a guest component. Just because the host portion of the test
only runs in userspace doesn't make KVM go away. If this were pure emulation,
then I would completely agree, but there multiple distinct components here, one
of which is host userspace.
> You treat vCPU worker thread as if it not userspace, but it is *userspace* by
> the definition of how KVM works.
By simply "vCPU" I am strictly referring to the guest entity. I refered to the
host side worker as "vCPU woker" to try to distinguish between the two.
> Right way to say it is something like 'don't pause the vCPU worker thread
> when its not needed' or something like that.
That's inaccurate though. GUEST_SYNC() doesn't pause the vCPU, it forces it to
exit to userspace. The test forces the vCPU to exit to check to see if it needs
to pause/stop, which I'm contending is wasteful and unnecessarily complex. The
vCPU can instead check to see if it needs to stop simply by reading the global
variable.
If vcpu_sync_stop_requested is false, the worker thread immediated resumes the
vCPU.
/* Should only be called after a GUEST_SYNC */
static void vcpu_handle_sync_stop(void)
{
if (atomic_read(&vcpu_sync_stop_requested)) {
/* It means main thread is sleeping waiting */
atomic_set(&vcpu_sync_stop_requested, false);
sem_post(&sem_vcpu_stop);
sem_wait_until(&sem_vcpu_cont);
}
}
The future cleanup is to change the guest loop to keep running _in the guest_
until a stop is requested. Whereas the current code exits to userspace every
4096 writes to see if it should stop. But as above, the vCPU doesn't actually
stop on each exit.
@@ -112,7 +111,7 @@ static void guest_code(void)
#endif
while (true) {
- for (i = 0; i < TEST_PAGES_PER_LOOP; i++) {
+ while (!READ_ONCE(vcpu_stop)) {
addr = guest_test_virt_mem;
addr += (guest_random_u64(&guest_rng) % guest_num_pages)
* guest_page_size;
On Thu, 2024-12-19 at 07:23 -0800, Sean Christopherson wrote:
> On Thu, Dec 19, 2024, Maxim Levitsky wrote:
> > On Wed, 2024-12-18 at 18:00 -0800, Sean Christopherson wrote:
> > > On Tue, Dec 17, 2024, Maxim Levitsky wrote:
> > > > On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> > > > > Now that the vCPU doesn't dirty every page on the first iteration for
> > > > > architectures that support the dirty ring, honor vcpu_stop in the dirty
> > > > > ring's vCPU worker, i.e. stop when the main thread says "stop". This will
> > > > > allow plumbing vcpu_stop into the guest so that the vCPU doesn't need to
> > > > > periodically exit to userspace just to see if it should stop.
> > > >
> > > > This is very misleading - by the very nature of this test it all runs in
> > > > userspace, so every time KVM_RUN ioctl exits, it is by definition an
> > > > userspace VM exit.
> > >
> > > I honestly don't see how being more precise is misleading.
> >
> > "Exit to userspace" is misleading - the *whole test* is userspace.
>
> No, the test has a guest component. Just because the host portion of the test
> only runs in userspace doesn't make KVM go away. If this were pure emulation,
> then I would completely agree, but there multiple distinct components here, one
> of which is host userspace.
>
> > You treat vCPU worker thread as if it not userspace, but it is *userspace* by
> > the definition of how KVM works.
>
> By simply "vCPU" I am strictly referring to the guest entity. I refered to the
> host side worker as "vCPU woker" to try to distinguish between the two.
>
> > Right way to say it is something like 'don't pause the vCPU worker thread
> > when its not needed' or something like that.
>
> That's inaccurate though. GUEST_SYNC() doesn't pause the vCPU, it forces it to
> exit to userspace. The test forces the vCPU to exit to check to see if it needs
> to pause/stop, which I'm contending is wasteful and unnecessarily complex. The
> vCPU can instead check to see if it needs to stop simply by reading the global
> variable.
>
> If vcpu_sync_stop_requested is false, the worker thread immediated resumes the
> vCPU.
>
> /* Should only be called after a GUEST_SYNC */
> static void vcpu_handle_sync_stop(void)
> {
> if (atomic_read(&vcpu_sync_stop_requested)) {
> /* It means main thread is sleeping waiting */
> atomic_set(&vcpu_sync_stop_requested, false);
> sem_post(&sem_vcpu_stop);
> sem_wait_until(&sem_vcpu_cont);
> }
> }
>
> The future cleanup is to change the guest loop to keep running _in the guest_
> until a stop is requested. Whereas the current code exits to userspace every
> 4096 writes to see if it should stop. But as above, the vCPU doesn't actually
> stop on each exit.
>
> @@ -112,7 +111,7 @@ static void guest_code(void)
> #endif
>
> while (true) {
> - for (i = 0; i < TEST_PAGES_PER_LOOP; i++) {
> + while (!READ_ONCE(vcpu_stop)) {
> addr = guest_test_virt_mem;
> addr += (guest_random_u64(&guest_rng) % guest_num_pages)
> * guest_page_size;
>
Ah OK, I missed the "This *will* allow plumbing", that is the fact this this patch
is only a preparation for this.
Best regards,
Maxim levitsky
In the dirty_log_test guest code, exit to userspace only when the vCPU is
explicitly told to stop. Periodically exiting just to check if a flag has
been set is unnecessary, weirdly complex, and wastes time handling exits
that could be used to dirty memory.
Opportunistically convert 'i' to a uint64_t to guard against the unlikely
scenario that guest_num_pages exceeds the storage of an int.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 43 ++++++++++----------
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 8d31e275a23d..40c8f5551c8e 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -31,9 +31,6 @@
/* Default guest test virtual memory offset */
#define DEFAULT_GUEST_TEST_MEM 0xc0000000
-/* How many pages to dirty for each guest loop */
-#define TEST_PAGES_PER_LOOP 1024
-
/* How many host loops to run (one KVM_GET_DIRTY_LOG for each loop) */
#define TEST_HOST_LOOP_N 32UL
@@ -75,6 +72,7 @@ static uint64_t host_page_size;
static uint64_t guest_page_size;
static uint64_t guest_num_pages;
static uint64_t iteration;
+static bool vcpu_stop;
/*
* Guest physical memory offset of the testing memory slot.
@@ -96,9 +94,10 @@ static uint64_t guest_test_virt_mem = DEFAULT_GUEST_TEST_MEM;
static void guest_code(void)
{
uint64_t addr;
- int i;
#ifdef __s390x__
+ uint64_t i;
+
/*
* On s390x, all pages of a 1M segment are initially marked as dirty
* when a page of the segment is written to for the very first time.
@@ -112,7 +111,7 @@ static void guest_code(void)
#endif
while (true) {
- for (i = 0; i < TEST_PAGES_PER_LOOP; i++) {
+ while (!READ_ONCE(vcpu_stop)) {
addr = guest_test_virt_mem;
addr += (guest_random_u64(&guest_rng) % guest_num_pages)
* guest_page_size;
@@ -140,14 +139,7 @@ static uint64_t host_track_next_count;
/* Whether dirty ring reset is requested, or finished */
static sem_t sem_vcpu_stop;
static sem_t sem_vcpu_cont;
-/*
- * This is only set by main thread, and only cleared by vcpu thread. It is
- * used to request vcpu thread to stop at the next GUEST_SYNC, since GUEST_SYNC
- * is the only place that we'll guarantee both "dirty bit" and "dirty data"
- * will match. E.g., SIG_IPI won't guarantee that if the vcpu is interrupted
- * after setting dirty bit but before the data is written.
- */
-static atomic_t vcpu_sync_stop_requested;
+
/*
* This is updated by the vcpu thread to tell the host whether it's a
* ring-full event. It should only be read until a sem_wait() of
@@ -272,9 +264,7 @@ static void clear_log_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
/* Should only be called after a GUEST_SYNC */
static void vcpu_handle_sync_stop(void)
{
- if (atomic_read(&vcpu_sync_stop_requested)) {
- /* It means main thread is sleeping waiting */
- atomic_set(&vcpu_sync_stop_requested, false);
+ if (READ_ONCE(vcpu_stop)) {
sem_post(&sem_vcpu_stop);
sem_wait(&sem_vcpu_cont);
}
@@ -801,11 +791,24 @@ static void run_test(enum vm_guest_mode mode, void *arg)
}
/*
- * See vcpu_sync_stop_requested definition for details on why
- * we need to stop vcpu when verify data.
+ * Stop the vCPU prior to collecting and verifying the dirty
+ * log. If the vCPU is allowed to run during collection, then
+ * pages that are written during this iteration may be missed,
+ * i.e. collected in the next iteration. And if the vCPU is
+ * writing memory during verification, pages that this thread
+ * sees as clean may be written with this iteration's value.
*/
- atomic_set(&vcpu_sync_stop_requested, true);
+ WRITE_ONCE(vcpu_stop, true);
+ sync_global_to_guest(vm, vcpu_stop);
sem_wait(&sem_vcpu_stop);
+
+ /*
+ * Clear vcpu_stop after the vCPU thread has acknowledge the
+ * stop request and is waiting, i.e. is definitely not running!
+ */
+ WRITE_ONCE(vcpu_stop, false);
+ sync_global_to_guest(vm, vcpu_stop);
+
/*
* NOTE: for dirty ring, it's possible that we didn't stop at
* GUEST_SYNC but instead we stopped because ring is full;
@@ -813,8 +816,6 @@ static void run_test(enum vm_guest_mode mode, void *arg)
* the flush of the last page, and since we handle the last
* page specially verification will succeed anyway.
*/
- assert(host_log_mode == LOG_MODE_DIRTY_RING ||
- atomic_read(&vcpu_sync_stop_requested) == false);
vm_dirty_log_verify(mode, bmap);
/*
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> In the dirty_log_test guest code, exit to userspace
Once again, "exit to userspace" is misleading.
> only when the vCPU is
> explicitly told to stop. Periodically exiting just to check if a flag has
> been set is unnecessary, weirdly complex, and wastes time handling exits
> that could be used to dirty memory.
>
> Opportunistically convert 'i' to a uint64_t to guard against the unlikely
> scenario that guest_num_pages exceeds the storage of an int.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 43 ++++++++++----------
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 8d31e275a23d..40c8f5551c8e 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -31,9 +31,6 @@
> /* Default guest test virtual memory offset */
> #define DEFAULT_GUEST_TEST_MEM 0xc0000000
>
> -/* How many pages to dirty for each guest loop */
> -#define TEST_PAGES_PER_LOOP 1024
> -
> /* How many host loops to run (one KVM_GET_DIRTY_LOG for each loop) */
> #define TEST_HOST_LOOP_N 32UL
>
> @@ -75,6 +72,7 @@ static uint64_t host_page_size;
> static uint64_t guest_page_size;
> static uint64_t guest_num_pages;
> static uint64_t iteration;
> +static bool vcpu_stop;
>
> /*
> * Guest physical memory offset of the testing memory slot.
> @@ -96,9 +94,10 @@ static uint64_t guest_test_virt_mem = DEFAULT_GUEST_TEST_MEM;
> static void guest_code(void)
> {
> uint64_t addr;
> - int i;
>
> #ifdef __s390x__
> + uint64_t i;
> +
> /*
> * On s390x, all pages of a 1M segment are initially marked as dirty
> * when a page of the segment is written to for the very first time.
> @@ -112,7 +111,7 @@ static void guest_code(void)
> #endif
>
> while (true) {
> - for (i = 0; i < TEST_PAGES_PER_LOOP; i++) {
> + while (!READ_ONCE(vcpu_stop)) {
> addr = guest_test_virt_mem;
> addr += (guest_random_u64(&guest_rng) % guest_num_pages)
> * guest_page_size;
> @@ -140,14 +139,7 @@ static uint64_t host_track_next_count;
> /* Whether dirty ring reset is requested, or finished */
> static sem_t sem_vcpu_stop;
> static sem_t sem_vcpu_cont;
> -/*
> - * This is only set by main thread, and only cleared by vcpu thread. It is
> - * used to request vcpu thread to stop at the next GUEST_SYNC, since GUEST_SYNC
> - * is the only place that we'll guarantee both "dirty bit" and "dirty data"
> - * will match. E.g., SIG_IPI won't guarantee that if the vcpu is interrupted
> - * after setting dirty bit but before the data is written.
> - */
> -static atomic_t vcpu_sync_stop_requested;
> +
> /*
> * This is updated by the vcpu thread to tell the host whether it's a
> * ring-full event. It should only be read until a sem_wait() of
> @@ -272,9 +264,7 @@ static void clear_log_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
> /* Should only be called after a GUEST_SYNC */
> static void vcpu_handle_sync_stop(void)
> {
> - if (atomic_read(&vcpu_sync_stop_requested)) {
> - /* It means main thread is sleeping waiting */
> - atomic_set(&vcpu_sync_stop_requested, false);
> + if (READ_ONCE(vcpu_stop)) {
> sem_post(&sem_vcpu_stop);
> sem_wait(&sem_vcpu_cont);
> }
> @@ -801,11 +791,24 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> }
>
> /*
> - * See vcpu_sync_stop_requested definition for details on why
> - * we need to stop vcpu when verify data.
> + * Stop the vCPU prior to collecting and verifying the dirty
> + * log. If the vCPU is allowed to run during collection, then
> + * pages that are written during this iteration may be missed,
> + * i.e. collected in the next iteration. And if the vCPU is
> + * writing memory during verification, pages that this thread
> + * sees as clean may be written with this iteration's value.
> */
> - atomic_set(&vcpu_sync_stop_requested, true);
> + WRITE_ONCE(vcpu_stop, true);
> + sync_global_to_guest(vm, vcpu_stop);
> sem_wait(&sem_vcpu_stop);
> +
> + /*
> + * Clear vcpu_stop after the vCPU thread has acknowledge the
> + * stop request and is waiting, i.e. is definitely not running!
> + */
> + WRITE_ONCE(vcpu_stop, false);
> + sync_global_to_guest(vm, vcpu_stop);
> +
> /*
> * NOTE: for dirty ring, it's possible that we didn't stop at
> * GUEST_SYNC but instead we stopped because ring is full;
> @@ -813,8 +816,6 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> * the flush of the last page, and since we handle the last
> * page specially verification will succeed anyway.
> */
> - assert(host_log_mode == LOG_MODE_DIRTY_RING ||
> - atomic_read(&vcpu_sync_stop_requested) == false);
> vm_dirty_log_verify(mode, bmap);
>
> /*
On Tue, 2024-12-17 at 19:01 -0500, Maxim Levitsky wrote:
> On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> > In the dirty_log_test guest code, exit to userspace
>
> Once again, "exit to userspace" is misleading.
OK, I understand now, this patch does make sense.
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
>
> > only when the vCPU is
> > explicitly told to stop. Periodically exiting just to check if a flag has
> > been set is unnecessary, weirdly complex, and wastes time handling exits
> > that could be used to dirty memory.
> > Opportunistically convert 'i' to a uint64_t to guard against the unlikely
> > scenario that guest_num_pages exceeds the storage of an int.
> >
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> > tools/testing/selftests/kvm/dirty_log_test.c | 43 ++++++++++----------
> > 1 file changed, 22 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> > index 8d31e275a23d..40c8f5551c8e 100644
> > --- a/tools/testing/selftests/kvm/dirty_log_test.c
> > +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> > @@ -31,9 +31,6 @@
> > /* Default guest test virtual memory offset */
> > #define DEFAULT_GUEST_TEST_MEM 0xc0000000
> >
> > -/* How many pages to dirty for each guest loop */
> > -#define TEST_PAGES_PER_LOOP 1024
> > -
> > /* How many host loops to run (one KVM_GET_DIRTY_LOG for each loop) */
> > #define TEST_HOST_LOOP_N 32UL
> >
> > @@ -75,6 +72,7 @@ static uint64_t host_page_size;
> > static uint64_t guest_page_size;
> > static uint64_t guest_num_pages;
> > static uint64_t iteration;
> > +static bool vcpu_stop;
> >
> > /*
> > * Guest physical memory offset of the testing memory slot.
> > @@ -96,9 +94,10 @@ static uint64_t guest_test_virt_mem = DEFAULT_GUEST_TEST_MEM;
> > static void guest_code(void)
> > {
> > uint64_t addr;
> > - int i;
> >
> > #ifdef __s390x__
> > + uint64_t i;
> > +
> > /*
> > * On s390x, all pages of a 1M segment are initially marked as dirty
> > * when a page of the segment is written to for the very first time.
> > @@ -112,7 +111,7 @@ static void guest_code(void)
> > #endif
> >
> > while (true) {
> > - for (i = 0; i < TEST_PAGES_PER_LOOP; i++) {
> > + while (!READ_ONCE(vcpu_stop)) {
> > addr = guest_test_virt_mem;
> > addr += (guest_random_u64(&guest_rng) % guest_num_pages)
> > * guest_page_size;
> > @@ -140,14 +139,7 @@ static uint64_t host_track_next_count;
> > /* Whether dirty ring reset is requested, or finished */
> > static sem_t sem_vcpu_stop;
> > static sem_t sem_vcpu_cont;
> > -/*
> > - * This is only set by main thread, and only cleared by vcpu thread. It is
> > - * used to request vcpu thread to stop at the next GUEST_SYNC, since GUEST_SYNC
> > - * is the only place that we'll guarantee both "dirty bit" and "dirty data"
> > - * will match. E.g., SIG_IPI won't guarantee that if the vcpu is interrupted
> > - * after setting dirty bit but before the data is written.
> > - */
> > -static atomic_t vcpu_sync_stop_requested;
> > +
> > /*
> > * This is updated by the vcpu thread to tell the host whether it's a
> > * ring-full event. It should only be read until a sem_wait() of
> > @@ -272,9 +264,7 @@ static void clear_log_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
> > /* Should only be called after a GUEST_SYNC */
> > static void vcpu_handle_sync_stop(void)
> > {
> > - if (atomic_read(&vcpu_sync_stop_requested)) {
> > - /* It means main thread is sleeping waiting */
> > - atomic_set(&vcpu_sync_stop_requested, false);
> > + if (READ_ONCE(vcpu_stop)) {
> > sem_post(&sem_vcpu_stop);
> > sem_wait(&sem_vcpu_cont);
> > }
> > @@ -801,11 +791,24 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> > }
> >
> > /*
> > - * See vcpu_sync_stop_requested definition for details on why
> > - * we need to stop vcpu when verify data.
> > + * Stop the vCPU prior to collecting and verifying the dirty
> > + * log. If the vCPU is allowed to run during collection, then
> > + * pages that are written during this iteration may be missed,
> > + * i.e. collected in the next iteration. And if the vCPU is
> > + * writing memory during verification, pages that this thread
> > + * sees as clean may be written with this iteration's value.
> > */
> > - atomic_set(&vcpu_sync_stop_requested, true);
> > + WRITE_ONCE(vcpu_stop, true);
> > + sync_global_to_guest(vm, vcpu_stop);
> > sem_wait(&sem_vcpu_stop);
> > +
> > + /*
> > + * Clear vcpu_stop after the vCPU thread has acknowledge the
> > + * stop request and is waiting, i.e. is definitely not running!
> > + */
> > + WRITE_ONCE(vcpu_stop, false);
> > + sync_global_to_guest(vm, vcpu_stop);
> > +
> > /*
> > * NOTE: for dirty ring, it's possible that we didn't stop at
> > * GUEST_SYNC but instead we stopped because ring is full;
> > @@ -813,8 +816,6 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> > * the flush of the last page, and since we handle the last
> > * page specially verification will succeed anyway.
> > */
> > - assert(host_log_mode == LOG_MODE_DIRTY_RING ||
> > - atomic_read(&vcpu_sync_stop_requested) == false);
> > vm_dirty_log_verify(mode, bmap);
> >
> > /*
When running dirty_log_test using the dirty ring, post to sem_vcpu_stop
only when the main thread has explicitly requested that the vCPU stop.
Synchronizing the vCPU and main thread whenever the dirty ring happens to
be full is unnecessary, as KVM's ABI is to actively prevent the vCPU from
running until the ring is no longer full. I.e. attempting to run the vCPU
will simply result in KVM_EXIT_DIRTY_RING_FULL without ever entering the
guest. And if KVM doesn't exit, e.g. let's the vCPU dirty more pages,
then that's a KVM bug worth finding.
Posting to sem_vcpu_stop on ring full also makes it difficult to get the
test logic right, e.g. it's easy to let the vCPU keep running when it
shouldn't, as a ring full can essentially happen at any given time.
Opportunistically rework the handling of dirty_ring_vcpu_ring_full to
leave it set for the remainder of the iteration in order to simplify the
surrounding logic.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 40c8f5551c8e..8544e8425f9c 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -379,12 +379,8 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
if (get_ucall(vcpu, NULL) == UCALL_SYNC) {
vcpu_handle_sync_stop();
} else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) {
- /* Update the flag first before pause */
WRITE_ONCE(dirty_ring_vcpu_ring_full, true);
- sem_post(&sem_vcpu_stop);
- pr_info("Dirty ring full, waiting for it to be collected\n");
- sem_wait(&sem_vcpu_cont);
- WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
+ vcpu_handle_sync_stop();
} else {
TEST_ASSERT(false, "Invalid guest sync status: "
"exit_reason=%s",
@@ -743,7 +739,6 @@ static void run_test(enum vm_guest_mode mode, void *arg)
pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu);
while (iteration < p->iterations) {
- bool saw_dirty_ring_full = false;
unsigned long i;
dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
@@ -775,19 +770,12 @@ static void run_test(enum vm_guest_mode mode, void *arg)
* the ring on every pass would make it unlikely the
* vCPU would ever fill the fing).
*/
- if (READ_ONCE(dirty_ring_vcpu_ring_full))
- saw_dirty_ring_full = true;
- if (i && !saw_dirty_ring_full)
+ if (i && !READ_ONCE(dirty_ring_vcpu_ring_full))
continue;
log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
bmap, host_num_pages,
&ring_buf_idx);
-
- if (READ_ONCE(dirty_ring_vcpu_ring_full)) {
- pr_info("Dirty ring emptied, restarting vCPU\n");
- sem_post(&sem_vcpu_cont);
- }
}
/*
@@ -829,6 +817,8 @@ static void run_test(enum vm_guest_mode mode, void *arg)
WRITE_ONCE(host_quit, true);
sync_global_to_guest(vm, iteration);
+ WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
+
sem_post(&sem_vcpu_cont);
}
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> When running dirty_log_test using the dirty ring, post to sem_vcpu_stop
> only when the main thread has explicitly requested that the vCPU stop.
> Synchronizing the vCPU and main thread whenever the dirty ring happens to
> be full is unnecessary, as KVM's ABI is to actively prevent the vCPU from
> running until the ring is no longer full. I.e. attempting to run the vCPU
> will simply result in KVM_EXIT_DIRTY_RING_FULL without ever entering the
> guest. And if KVM doesn't exit, e.g. let's the vCPU dirty more pages,
> then that's a KVM bug worth finding.
This is probably a good idea to do sometimes, but this can also reduce coverage because
now the vCPU will pointlessly enter and exit when dirty log is full.
Best regards,
Maxim Levitsky
>
> Posting to sem_vcpu_stop on ring full also makes it difficult to get the
> test logic right, e.g. it's easy to let the vCPU keep running when it
> shouldn't, as a ring full can essentially happen at any given time.
>
> Opportunistically rework the handling of dirty_ring_vcpu_ring_full to
> leave it set for the remainder of the iteration in order to simplify the
> surrounding logic.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 18 ++++--------------
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 40c8f5551c8e..8544e8425f9c 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -379,12 +379,8 @@ static void dirty_ring_after_vcpu_run(struct kvm_vcpu *vcpu)
> if (get_ucall(vcpu, NULL) == UCALL_SYNC) {
> vcpu_handle_sync_stop();
> } else if (run->exit_reason == KVM_EXIT_DIRTY_RING_FULL) {
> - /* Update the flag first before pause */
> WRITE_ONCE(dirty_ring_vcpu_ring_full, true);
> - sem_post(&sem_vcpu_stop);
> - pr_info("Dirty ring full, waiting for it to be collected\n");
> - sem_wait(&sem_vcpu_cont);
> - WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
> + vcpu_handle_sync_stop();
> } else {
> TEST_ASSERT(false, "Invalid guest sync status: "
> "exit_reason=%s",
> @@ -743,7 +739,6 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu);
>
> while (iteration < p->iterations) {
> - bool saw_dirty_ring_full = false;
> unsigned long i;
>
> dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
> @@ -775,19 +770,12 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> * the ring on every pass would make it unlikely the
> * vCPU would ever fill the fing).
> */
> - if (READ_ONCE(dirty_ring_vcpu_ring_full))
> - saw_dirty_ring_full = true;
> - if (i && !saw_dirty_ring_full)
> + if (i && !READ_ONCE(dirty_ring_vcpu_ring_full))
> continue;
>
> log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
> bmap, host_num_pages,
> &ring_buf_idx);
> -
> - if (READ_ONCE(dirty_ring_vcpu_ring_full)) {
> - pr_info("Dirty ring emptied, restarting vCPU\n");
> - sem_post(&sem_vcpu_cont);
> - }
> }
>
> /*
> @@ -829,6 +817,8 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> WRITE_ONCE(host_quit, true);
> sync_global_to_guest(vm, iteration);
>
> + WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
> +
> sem_post(&sem_vcpu_cont);
> }
>
When verifying pages in dirty_log_test, immediately continue on all "pass"
scenarios to make the logic consistent in how it handles pass vs. fail.
No functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 8544e8425f9c..d7cf1840bd80 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -510,8 +510,6 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
}
if (__test_and_clear_bit_le(page, bmap)) {
- bool matched;
-
nr_dirty_pages++;
/*
@@ -519,9 +517,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
* the corresponding page should be either the
* previous iteration number or the current one.
*/
- matched = (val == iteration || val == iteration - 1);
+ if (val == iteration || val == iteration - 1)
+ continue;
- if (host_log_mode == LOG_MODE_DIRTY_RING && !matched) {
+ if (host_log_mode == LOG_MODE_DIRTY_RING) {
if (val == iteration - 2 && min_iter <= iteration - 2) {
/*
* Short answer: this case is special
@@ -567,10 +566,8 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
}
}
- TEST_ASSERT(matched,
- "Set page %"PRIu64" value %"PRIu64
- " incorrect (iteration=%"PRIu64")",
- page, val, iteration);
+ TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu)",
+ page, val, iteration);
} else {
nr_clean_pages++;
/*
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> When verifying pages in dirty_log_test, immediately continue on all "pass"
> scenarios to make the logic consistent in how it handles pass vs. fail.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 8544e8425f9c..d7cf1840bd80 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -510,8 +510,6 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> }
>
> if (__test_and_clear_bit_le(page, bmap)) {
> - bool matched;
> -
> nr_dirty_pages++;
>
> /*
> @@ -519,9 +517,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> * the corresponding page should be either the
> * previous iteration number or the current one.
> */
> - matched = (val == iteration || val == iteration - 1);
> + if (val == iteration || val == iteration - 1)
> + continue;
>
> - if (host_log_mode == LOG_MODE_DIRTY_RING && !matched) {
> + if (host_log_mode == LOG_MODE_DIRTY_RING) {
> if (val == iteration - 2 && min_iter <= iteration - 2) {
> /*
> * Short answer: this case is special
> @@ -567,10 +566,8 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> }
> }
>
> - TEST_ASSERT(matched,
> - "Set page %"PRIu64" value %"PRIu64
> - " incorrect (iteration=%"PRIu64")",
> - page, val, iteration);
> + TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu)",
> + page, val, iteration);
> } else {
> nr_clean_pages++;
> /*
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
Print out the last dirty pages from the current and previous iteration on
verification failures. In many cases, bugs (especially test bugs) occur
on the edges, i.e. on or near the last pages, and being able to correlate
failures with the last pages can aid in debug.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index d7cf1840bd80..fe8cc7f77e22 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -566,8 +566,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
}
}
- TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu)",
- page, val, iteration);
+ TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu) "
+ "(last = %lu, prev_last = %lu)",
+ page, val, iteration, dirty_ring_last_page,
+ dirty_ring_prev_iteration_last_page);
} else {
nr_clean_pages++;
/*
@@ -590,9 +592,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
* value "iteration-1".
*/
TEST_ASSERT(val <= iteration,
- "Clear page %"PRIu64" value %"PRIu64
- " incorrect (iteration=%"PRIu64")",
- page, val, iteration);
+ "Clear page %lu value (%lu) > iteration (%lu) "
+ "(last = %lu, prev_last = %lu)",
+ page, val, iteration, dirty_ring_last_page,
+ dirty_ring_prev_iteration_last_page);
if (val == iteration) {
/*
* This page is _just_ modified; it
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Print out the last dirty pages from the current and previous iteration on
> verification failures. In many cases, bugs (especially test bugs) occur
> on the edges, i.e. on or near the last pages, and being able to correlate
> failures with the last pages can aid in debug.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index d7cf1840bd80..fe8cc7f77e22 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -566,8 +566,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> }
> }
>
> - TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu)",
> - page, val, iteration);
> + TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu) "
> + "(last = %lu, prev_last = %lu)",
> + page, val, iteration, dirty_ring_last_page,
> + dirty_ring_prev_iteration_last_page);
> } else {
> nr_clean_pages++;
> /*
> @@ -590,9 +592,10 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> * value "iteration-1".
> */
> TEST_ASSERT(val <= iteration,
> - "Clear page %"PRIu64" value %"PRIu64
> - " incorrect (iteration=%"PRIu64")",
> - page, val, iteration);
> + "Clear page %lu value (%lu) > iteration (%lu) "
> + "(last = %lu, prev_last = %lu)",
> + page, val, iteration, dirty_ring_last_page,
> + dirty_ring_prev_iteration_last_page);
> if (val == iteration) {
> /*
> * This page is _just_ modified; it
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Collect all dirty entries during each iteration of dirty_log_test by
doing a final collection after the vCPU has been stopped. To deal with
KVM's destructive approach to getting the dirty bitmaps, use a second
bitmap for the post-stop collection.
Collecting all entries that were dirtied during an iteration simplifies
the verification logic *and* improves test coverage.
- If a page is written during iteration X, but not seen as dirty until
X+1, the test can get a false pass if the page is also written during
X+1.
- If a dirty page used a stale value from a previous iteration, the test
would grant a false pass.
- If a missed dirty log occurs in the last iteration, the test would fail
to detect the issue.
E.g. modifying mark_page_dirty_in_slot() to dirty an unwritten gfn:
if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
unsigned long rel_gfn = gfn - memslot->base_gfn;
u32 slot = (memslot->as_id << 16) | memslot->id;
if (!vcpu->extra_dirty &&
gfn_to_memslot(kvm, gfn + 1) == memslot) {
vcpu->extra_dirty = true;
mark_page_dirty_in_slot(kvm, memslot, gfn + 1);
}
if (kvm->dirty_ring_size && vcpu)
kvm_dirty_ring_push(vcpu, slot, rel_gfn);
else if (memslot->dirty_bitmap)
set_bit_le(rel_gfn, memslot->dirty_bitmap);
}
isn't detected with the current approach, even with an interval of 1ms
(when running nested in a VM; bare metal would be even *less* likely to
detect the bug due to the vCPU being able to dirty more memory). Whereas
collecting all dirty entries consistently detects failures with an
interval of 700ms or more (the longer interval means a higher probability
of an actual write to the prematurely-dirtied page).
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 149 ++++++-------------
1 file changed, 45 insertions(+), 104 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index fe8cc7f77e22..3a4e411353d7 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -134,7 +134,6 @@ static uint64_t host_num_pages;
/* For statistics only */
static uint64_t host_dirty_count;
static uint64_t host_clear_count;
-static uint64_t host_track_next_count;
/* Whether dirty ring reset is requested, or finished */
static sem_t sem_vcpu_stop;
@@ -422,15 +421,6 @@ struct log_mode {
},
};
-/*
- * We use this bitmap to track some pages that should have its dirty
- * bit set in the _next_ iteration. For example, if we detected the
- * page value changed to current iteration but at the same time the
- * page bit is cleared in the latest bitmap, then the system must
- * report that write in the next get dirty log call.
- */
-static unsigned long *host_bmap_track;
-
static void log_modes_dump(void)
{
int i;
@@ -491,79 +481,52 @@ static void *vcpu_worker(void *data)
return NULL;
}
-static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
+static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap)
{
uint64_t page, nr_dirty_pages = 0, nr_clean_pages = 0;
uint64_t step = vm_num_host_pages(mode, 1);
- uint64_t min_iter = 0;
for (page = 0; page < host_num_pages; page += step) {
uint64_t val = *(uint64_t *)(host_test_mem + page * host_page_size);
+ bool bmap0_dirty = __test_and_clear_bit_le(page, bmap[0]);
- /* If this is a special page that we were tracking... */
- if (__test_and_clear_bit_le(page, host_bmap_track)) {
- host_track_next_count++;
- TEST_ASSERT(test_bit_le(page, bmap),
- "Page %"PRIu64" should have its dirty bit "
- "set in this iteration but it is missing",
- page);
- }
-
- if (__test_and_clear_bit_le(page, bmap)) {
+ /*
+ * Ensure both bitmaps are cleared, as a page can be written
+ * multiple times per iteration, i.e. can show up in both
+ * bitmaps, and the dirty ring is additive, i.e. doesn't purge
+ * bitmap entries from previous collections.
+ */
+ if (__test_and_clear_bit_le(page, bmap[1]) || bmap0_dirty) {
nr_dirty_pages++;
/*
- * If the bit is set, the value written onto
- * the corresponding page should be either the
- * previous iteration number or the current one.
+ * If the page is dirty, the value written to memory
+ * should be the current iteration number.
*/
- if (val == iteration || val == iteration - 1)
+ if (val == iteration)
continue;
if (host_log_mode == LOG_MODE_DIRTY_RING) {
- if (val == iteration - 2 && min_iter <= iteration - 2) {
- /*
- * Short answer: this case is special
- * only for dirty ring test where the
- * page is the last page before a kvm
- * dirty ring full in iteration N-2.
- *
- * Long answer: Assuming ring size R,
- * one possible condition is:
- *
- * main thr vcpu thr
- * -------- --------
- * iter=1
- * write 1 to page 0~(R-1)
- * full, vmexit
- * collect 0~(R-1)
- * kick vcpu
- * write 1 to (R-1)~(2R-2)
- * full, vmexit
- * iter=2
- * collect (R-1)~(2R-2)
- * kick vcpu
- * write 1 to (2R-2)
- * (NOTE!!! "1" cached in cpu reg)
- * write 2 to (2R-1)~(3R-3)
- * full, vmexit
- * iter=3
- * collect (2R-2)~(3R-3)
- * (here if we read value on page
- * "2R-2" is 1, while iter=3!!!)
- *
- * This however can only happen once per iteration.
- */
- min_iter = iteration - 1;
+ /*
+ * The last page in the ring from this iteration
+ * or the previous can be written with the value
+ * from the previous iteration (relative to the
+ * last page's iteration), as the value to be
+ * written may be cached in a CPU register.
+ */
+ if (page == dirty_ring_last_page ||
+ page == dirty_ring_prev_iteration_last_page)
continue;
- } else if (page == dirty_ring_last_page ||
- page == dirty_ring_prev_iteration_last_page) {
- /*
- * Please refer to comments in
- * dirty_ring_last_page.
- */
- continue;
- }
+ } else if (!val && iteration == 1 && bmap0_dirty) {
+ /*
+ * When testing get+clear, the dirty bitmap
+ * starts with all bits set, and so the first
+ * iteration can observe a "dirty" page that
+ * was never written, but only in the first
+ * bitmap (collecting the bitmap also clears
+ * all dirty pages).
+ */
+ continue;
}
TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu) "
@@ -574,36 +537,13 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
nr_clean_pages++;
/*
* If cleared, the value written can be any
- * value smaller or equals to the iteration
- * number. Note that the value can be exactly
- * (iteration-1) if that write can happen
- * like this:
- *
- * (1) increase loop count to "iteration-1"
- * (2) write to page P happens (with value
- * "iteration-1")
- * (3) get dirty log for "iteration-1"; we'll
- * see that page P bit is set (dirtied),
- * and not set the bit in host_bmap_track
- * (4) increase loop count to "iteration"
- * (which is current iteration)
- * (5) get dirty log for current iteration,
- * we'll see that page P is cleared, with
- * value "iteration-1".
+ * value smaller than the iteration number.
*/
- TEST_ASSERT(val <= iteration,
- "Clear page %lu value (%lu) > iteration (%lu) "
+ TEST_ASSERT(val < iteration,
+ "Clear page %lu value (%lu) >= iteration (%lu) "
"(last = %lu, prev_last = %lu)",
page, val, iteration, dirty_ring_last_page,
dirty_ring_prev_iteration_last_page);
- if (val == iteration) {
- /*
- * This page is _just_ modified; it
- * should report its dirtyness in the
- * next run
- */
- __set_bit_le(page, host_bmap_track);
- }
}
}
@@ -639,7 +579,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
struct test_params *p = arg;
struct kvm_vcpu *vcpu;
struct kvm_vm *vm;
- unsigned long *bmap;
+ unsigned long *bmap[2];
uint32_t ring_buf_idx = 0;
int sem_val;
@@ -695,8 +635,8 @@ static void run_test(enum vm_guest_mode mode, void *arg)
pr_info("guest physical test memory offset: 0x%lx\n", guest_test_phys_mem);
- bmap = bitmap_zalloc(host_num_pages);
- host_bmap_track = bitmap_zalloc(host_num_pages);
+ bmap[0] = bitmap_zalloc(host_num_pages);
+ bmap[1] = bitmap_zalloc(host_num_pages);
/* Add an extra memory slot for testing dirty logging */
vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
@@ -723,7 +663,6 @@ static void run_test(enum vm_guest_mode mode, void *arg)
WRITE_ONCE(host_quit, false);
host_dirty_count = 0;
host_clear_count = 0;
- host_track_next_count = 0;
WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
/*
@@ -774,7 +713,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
continue;
log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
- bmap, host_num_pages,
+ bmap[0], host_num_pages,
&ring_buf_idx);
}
@@ -804,6 +743,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
* the flush of the last page, and since we handle the last
* page specially verification will succeed anyway.
*/
+ log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
+ bmap[1], host_num_pages,
+ &ring_buf_idx);
vm_dirty_log_verify(mode, bmap);
/*
@@ -824,12 +766,11 @@ static void run_test(enum vm_guest_mode mode, void *arg)
pthread_join(vcpu_thread, NULL);
- pr_info("Total bits checked: dirty (%"PRIu64"), clear (%"PRIu64"), "
- "track_next (%"PRIu64")\n", host_dirty_count, host_clear_count,
- host_track_next_count);
+ pr_info("Total bits checked: dirty (%lu), clear (%lu)\n",
+ host_dirty_count, host_clear_count);
- free(bmap);
- free(host_bmap_track);
+ free(bmap[0]);
+ free(bmap[1]);
kvm_vm_free(vm);
}
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Collect all dirty entries during each iteration of dirty_log_test by
> doing a final collection after the vCPU has been stopped. To deal with
> KVM's destructive approach to getting the dirty bitmaps, use a second
> bitmap for the post-stop collection.
>
> Collecting all entries that were dirtied during an iteration simplifies
> the verification logic *and* improves test coverage.
>
> - If a page is written during iteration X, but not seen as dirty until
> X+1, the test can get a false pass if the page is also written during
> X+1.
>
> - If a dirty page used a stale value from a previous iteration, the test
> would grant a false pass.
>
> - If a missed dirty log occurs in the last iteration, the test would fail
> to detect the issue.
>
> E.g. modifying mark_page_dirty_in_slot() to dirty an unwritten gfn:
>
> if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
> unsigned long rel_gfn = gfn - memslot->base_gfn;
> u32 slot = (memslot->as_id << 16) | memslot->id;
>
> if (!vcpu->extra_dirty &&
> gfn_to_memslot(kvm, gfn + 1) == memslot) {
> vcpu->extra_dirty = true;
> mark_page_dirty_in_slot(kvm, memslot, gfn + 1);
> }
> if (kvm->dirty_ring_size && vcpu)
> kvm_dirty_ring_push(vcpu, slot, rel_gfn);
> else if (memslot->dirty_bitmap)
> set_bit_le(rel_gfn, memslot->dirty_bitmap);
> }
>
> isn't detected with the current approach, even with an interval of 1ms
> (when running nested in a VM; bare metal would be even *less* likely to
> detect the bug due to the vCPU being able to dirty more memory). Whereas
> collecting all dirty entries consistently detects failures with an
> interval of 700ms or more (the longer interval means a higher probability
> of an actual write to the prematurely-dirtied page).
While this patch might improve coverage for this particular case,
I think that this patch will make the test to be much more deterministic,
and thus have less chance of catching various races in the kernel that can happen.
In fact in my option I prefer moving this test in
other direction by verifying dirty ring while the *vCPU runs* as well,
in other words, not stopping the vCPU at all unless its dirty ring is full.
Best regards,
Maxim Levitsky
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 149 ++++++-------------
> 1 file changed, 45 insertions(+), 104 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index fe8cc7f77e22..3a4e411353d7 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -134,7 +134,6 @@ static uint64_t host_num_pages;
> /* For statistics only */
> static uint64_t host_dirty_count;
> static uint64_t host_clear_count;
> -static uint64_t host_track_next_count;
>
> /* Whether dirty ring reset is requested, or finished */
> static sem_t sem_vcpu_stop;
> @@ -422,15 +421,6 @@ struct log_mode {
> },
> };
>
> -/*
> - * We use this bitmap to track some pages that should have its dirty
> - * bit set in the _next_ iteration. For example, if we detected the
> - * page value changed to current iteration but at the same time the
> - * page bit is cleared in the latest bitmap, then the system must
> - * report that write in the next get dirty log call.
> - */
> -static unsigned long *host_bmap_track;
> -
> static void log_modes_dump(void)
> {
> int i;
> @@ -491,79 +481,52 @@ static void *vcpu_worker(void *data)
> return NULL;
> }
>
> -static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> +static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap)
> {
> uint64_t page, nr_dirty_pages = 0, nr_clean_pages = 0;
> uint64_t step = vm_num_host_pages(mode, 1);
> - uint64_t min_iter = 0;
>
> for (page = 0; page < host_num_pages; page += step) {
> uint64_t val = *(uint64_t *)(host_test_mem + page * host_page_size);
> + bool bmap0_dirty = __test_and_clear_bit_le(page, bmap[0]);
>
> - /* If this is a special page that we were tracking... */
> - if (__test_and_clear_bit_le(page, host_bmap_track)) {
> - host_track_next_count++;
> - TEST_ASSERT(test_bit_le(page, bmap),
> - "Page %"PRIu64" should have its dirty bit "
> - "set in this iteration but it is missing",
> - page);
> - }
> -
> - if (__test_and_clear_bit_le(page, bmap)) {
> + /*
> + * Ensure both bitmaps are cleared, as a page can be written
> + * multiple times per iteration, i.e. can show up in both
> + * bitmaps, and the dirty ring is additive, i.e. doesn't purge
> + * bitmap entries from previous collections.
> + */
> + if (__test_and_clear_bit_le(page, bmap[1]) || bmap0_dirty) {
> nr_dirty_pages++;
>
> /*
> - * If the bit is set, the value written onto
> - * the corresponding page should be either the
> - * previous iteration number or the current one.
> + * If the page is dirty, the value written to memory
> + * should be the current iteration number.
> */
> - if (val == iteration || val == iteration - 1)
> + if (val == iteration)
> continue;
>
> if (host_log_mode == LOG_MODE_DIRTY_RING) {
> - if (val == iteration - 2 && min_iter <= iteration - 2) {
> - /*
> - * Short answer: this case is special
> - * only for dirty ring test where the
> - * page is the last page before a kvm
> - * dirty ring full in iteration N-2.
> - *
> - * Long answer: Assuming ring size R,
> - * one possible condition is:
> - *
> - * main thr vcpu thr
> - * -------- --------
> - * iter=1
> - * write 1 to page 0~(R-1)
> - * full, vmexit
> - * collect 0~(R-1)
> - * kick vcpu
> - * write 1 to (R-1)~(2R-2)
> - * full, vmexit
> - * iter=2
> - * collect (R-1)~(2R-2)
> - * kick vcpu
> - * write 1 to (2R-2)
> - * (NOTE!!! "1" cached in cpu reg)
> - * write 2 to (2R-1)~(3R-3)
> - * full, vmexit
> - * iter=3
> - * collect (2R-2)~(3R-3)
> - * (here if we read value on page
> - * "2R-2" is 1, while iter=3!!!)
> - *
> - * This however can only happen once per iteration.
> - */
> - min_iter = iteration - 1;
> + /*
> + * The last page in the ring from this iteration
> + * or the previous can be written with the value
> + * from the previous iteration (relative to the
> + * last page's iteration), as the value to be
> + * written may be cached in a CPU register.
> + */
> + if (page == dirty_ring_last_page ||
> + page == dirty_ring_prev_iteration_last_page)
> continue;
> - } else if (page == dirty_ring_last_page ||
> - page == dirty_ring_prev_iteration_last_page) {
> - /*
> - * Please refer to comments in
> - * dirty_ring_last_page.
> - */
> - continue;
> - }
> + } else if (!val && iteration == 1 && bmap0_dirty) {
> + /*
> + * When testing get+clear, the dirty bitmap
> + * starts with all bits set, and so the first
> + * iteration can observe a "dirty" page that
> + * was never written, but only in the first
> + * bitmap (collecting the bitmap also clears
> + * all dirty pages).
> + */
> + continue;
> }
>
> TEST_FAIL("Dirty page %lu value (%lu) != iteration (%lu) "
> @@ -574,36 +537,13 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
> nr_clean_pages++;
> /*
> * If cleared, the value written can be any
> - * value smaller or equals to the iteration
> - * number. Note that the value can be exactly
> - * (iteration-1) if that write can happen
> - * like this:
> - *
> - * (1) increase loop count to "iteration-1"
> - * (2) write to page P happens (with value
> - * "iteration-1")
> - * (3) get dirty log for "iteration-1"; we'll
> - * see that page P bit is set (dirtied),
> - * and not set the bit in host_bmap_track
> - * (4) increase loop count to "iteration"
> - * (which is current iteration)
> - * (5) get dirty log for current iteration,
> - * we'll see that page P is cleared, with
> - * value "iteration-1".
> + * value smaller than the iteration number.
> */
> - TEST_ASSERT(val <= iteration,
> - "Clear page %lu value (%lu) > iteration (%lu) "
> + TEST_ASSERT(val < iteration,
> + "Clear page %lu value (%lu) >= iteration (%lu) "
> "(last = %lu, prev_last = %lu)",
> page, val, iteration, dirty_ring_last_page,
> dirty_ring_prev_iteration_last_page);
> - if (val == iteration) {
> - /*
> - * This page is _just_ modified; it
> - * should report its dirtyness in the
> - * next run
> - */
> - __set_bit_le(page, host_bmap_track);
> - }
> }
> }
>
> @@ -639,7 +579,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> struct test_params *p = arg;
> struct kvm_vcpu *vcpu;
> struct kvm_vm *vm;
> - unsigned long *bmap;
> + unsigned long *bmap[2];
> uint32_t ring_buf_idx = 0;
> int sem_val;
>
> @@ -695,8 +635,8 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>
> pr_info("guest physical test memory offset: 0x%lx\n", guest_test_phys_mem);
>
> - bmap = bitmap_zalloc(host_num_pages);
> - host_bmap_track = bitmap_zalloc(host_num_pages);
> + bmap[0] = bitmap_zalloc(host_num_pages);
> + bmap[1] = bitmap_zalloc(host_num_pages);
>
> /* Add an extra memory slot for testing dirty logging */
> vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
> @@ -723,7 +663,6 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> WRITE_ONCE(host_quit, false);
> host_dirty_count = 0;
> host_clear_count = 0;
> - host_track_next_count = 0;
> WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
>
> /*
> @@ -774,7 +713,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> continue;
>
> log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
> - bmap, host_num_pages,
> + bmap[0], host_num_pages,
> &ring_buf_idx);
> }
>
> @@ -804,6 +743,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> * the flush of the last page, and since we handle the last
> * page specially verification will succeed anyway.
> */
> + log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
> + bmap[1], host_num_pages,
> + &ring_buf_idx);
> vm_dirty_log_verify(mode, bmap);
>
> /*
> @@ -824,12 +766,11 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>
> pthread_join(vcpu_thread, NULL);
>
> - pr_info("Total bits checked: dirty (%"PRIu64"), clear (%"PRIu64"), "
> - "track_next (%"PRIu64")\n", host_dirty_count, host_clear_count,
> - host_track_next_count);
> + pr_info("Total bits checked: dirty (%lu), clear (%lu)\n",
> + host_dirty_count, host_clear_count);
>
> - free(bmap);
> - free(host_bmap_track);
> + free(bmap[0]);
> + free(bmap[1]);
> kvm_vm_free(vm);
> }
>
On Tue, Dec 17, 2024, Maxim Levitsky wrote:
> On Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> > Collect all dirty entries during each iteration of dirty_log_test by
> > doing a final collection after the vCPU has been stopped. To deal with
> > KVM's destructive approach to getting the dirty bitmaps, use a second
> > bitmap for the post-stop collection.
> >
> > Collecting all entries that were dirtied during an iteration simplifies
> > the verification logic *and* improves test coverage.
> >
> > - If a page is written during iteration X, but not seen as dirty until
> > X+1, the test can get a false pass if the page is also written during
> > X+1.
> >
> > - If a dirty page used a stale value from a previous iteration, the test
> > would grant a false pass.
> >
> > - If a missed dirty log occurs in the last iteration, the test would fail
> > to detect the issue.
> >
> > E.g. modifying mark_page_dirty_in_slot() to dirty an unwritten gfn:
> >
> > if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
> > unsigned long rel_gfn = gfn - memslot->base_gfn;
> > u32 slot = (memslot->as_id << 16) | memslot->id;
> >
> > if (!vcpu->extra_dirty &&
> > gfn_to_memslot(kvm, gfn + 1) == memslot) {
> > vcpu->extra_dirty = true;
> > mark_page_dirty_in_slot(kvm, memslot, gfn + 1);
> > }
> > if (kvm->dirty_ring_size && vcpu)
> > kvm_dirty_ring_push(vcpu, slot, rel_gfn);
> > else if (memslot->dirty_bitmap)
> > set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > }
> >
> > isn't detected with the current approach, even with an interval of 1ms
> > (when running nested in a VM; bare metal would be even *less* likely to
> > detect the bug due to the vCPU being able to dirty more memory). Whereas
> > collecting all dirty entries consistently detects failures with an
> > interval of 700ms or more (the longer interval means a higher probability
> > of an actual write to the prematurely-dirtied page).
>
> While this patch might improve coverage for this particular case,
> I think that this patch will make the test to be much more deterministic,
The verification will be more deterministic, but the actual testcase itself is
just as random as it was before.
> and thus have less chance of catching various races in the kernel that can happen.
>
> In fact in my option I prefer moving this test in other direction by
> verifying dirty ring while the *vCPU runs* as well, in other words, not
> stopping the vCPU at all unless its dirty ring is full.
I don't see how letting verification be coincident with the vCPU running is at
all interesting for a dirty logging. Host userspace reading guest memory while
it's being written by the guest doesn't stress KVM's dirty logging in any meaningful
way. E.g. it exercises hardware far more than anything else. If we want to stress
that boundary, then we should spin up another vCPU or host thread to randomly read
while the test is in-progress, and also to write to bytes 4095:8 (assuming a 4KiB
page size), e.g. to ensure that dueling writes to a cacheline that trigger false
sharing are handled correct.
But letting the vCPU-under-test keep changing the memory while it's being validated
would add significant complexity, without any benefit insofar as I can see. As
evidenced by the bug the current approach can't detect, heavily stressing the
system is meaningless if it's impossible to separate the signal from the noise.
On 12/19/24 03:13, Sean Christopherson wrote: > On Tue, Dec 17, 2024, Maxim Levitsky wrote: >> While this patch might improve coverage for this particular case, >> I think that this patch will make the test to be much more deterministic, > > The verification will be more deterministic, but the actual testcase itself is > just as random as it was before. Based on my recollection of designing this thing with Peter, I can "confirm" that there was no particular intention of making the verification more random. >> and thus have less chance of catching various races in the kernel that can happen. >> >> In fact in my option I prefer moving this test in other direction by >> verifying dirty ring while the *vCPU runs* as well, in other words, not >> stopping the vCPU at all unless its dirty ring is full. > > But letting the vCPU-under-test keep changing the memory while it's being validated > would add significant complexity, without any benefit insofar as I can see. As > evidenced by the bug the current approach can't detect, heavily stressing the > system is meaningless if it's impossible to separate the signal from the noise. Yes, I agree. Paolo
On Thu, 2024-12-19 at 13:55 +0100, Paolo Bonzini wrote: > On 12/19/24 03:13, Sean Christopherson wrote: > > On Tue, Dec 17, 2024, Maxim Levitsky wrote: > > > While this patch might improve coverage for this particular case, > > > I think that this patch will make the test to be much more deterministic, > > > > The verification will be more deterministic, but the actual testcase itself is > > just as random as it was before. > > Based on my recollection of designing this thing with Peter, I can > "confirm" that there was no particular intention of making the > verification more random. > > > > and thus have less chance of catching various races in the kernel that can happen. > > > > > > In fact in my option I prefer moving this test in other direction by > > > verifying dirty ring while the *vCPU runs* as well, in other words, not > > > stopping the vCPU at all unless its dirty ring is full. > > > > But letting the vCPU-under-test keep changing the memory while it's being validated > > would add significant complexity, without any benefit insofar as I can see. As > > evidenced by the bug the current approach can't detect, heavily stressing the > > system is meaningless if it's impossible to separate the signal from the noise. > > Yes, I agree. > > Paolo > In this case I don't have any objections. Best regards, Maxim Levitsky
Add a sanity check that a completely garbage value wasn't written to
the last dirty page in the ring, e.g. that it doesn't contain the *next*
iteration's value.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 3a4e411353d7..500257b712e3 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -514,8 +514,9 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap)
* last page's iteration), as the value to be
* written may be cached in a CPU register.
*/
- if (page == dirty_ring_last_page ||
- page == dirty_ring_prev_iteration_last_page)
+ if ((page == dirty_ring_last_page ||
+ page == dirty_ring_prev_iteration_last_page) &&
+ val < iteration)
continue;
} else if (!val && iteration == 1 && bmap0_dirty) {
/*
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Add a sanity check that a completely garbage value wasn't written to
> the last dirty page in the ring, e.g. that it doesn't contain the *next*
> iteration's value.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 3a4e411353d7..500257b712e3 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -514,8 +514,9 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap)
> * last page's iteration), as the value to be
> * written may be cached in a CPU register.
> */
> - if (page == dirty_ring_last_page ||
> - page == dirty_ring_prev_iteration_last_page)
> + if ((page == dirty_ring_last_page ||
> + page == dirty_ring_prev_iteration_last_page) &&
> + val < iteration)
> continue;
> } else if (!val && iteration == 1 && bmap0_dirty) {
> /*
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Ensure the vCPU fully completes at least one write in each dirty_log_test
iteration, as failure to dirty any pages complicates verification and
forces the test to be overly conservative about possible values. E.g.
verification needs to allow the last dirty page from a previous iteration
to have *any* value, because the vCPU could get stuck for multiple
iterations, which is unlikely but can happen in heavily overloaded and/or
nested virtualization setups.
Somewhat arbitrarily set the minimum to 0x100/256; high enough to be
interesting, but not so high as to lead to pointlessly long runtimes.
Reported-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 30 ++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 500257b712e3..8eb51597f762 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -37,6 +37,12 @@
/* Interval for each host loop (ms) */
#define TEST_HOST_LOOP_INTERVAL 10UL
+/*
+ * Ensure the vCPU is able to perform a reasonable number of writes in each
+ * iteration to provide a lower bound on coverage.
+ */
+#define TEST_MIN_WRITES_PER_ITERATION 0x100
+
/* Dirty bitmaps are always little endian, so we need to swap on big endian */
#if defined(__s390x__)
# define BITOP_LE_SWIZZLE ((BITS_PER_LONG-1) & ~0x7)
@@ -72,6 +78,7 @@ static uint64_t host_page_size;
static uint64_t guest_page_size;
static uint64_t guest_num_pages;
static uint64_t iteration;
+static uint64_t nr_writes;
static bool vcpu_stop;
/*
@@ -107,6 +114,7 @@ static void guest_code(void)
for (i = 0; i < guest_num_pages; i++) {
addr = guest_test_virt_mem + i * guest_page_size;
vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration));
+ nr_writes++;
}
#endif
@@ -118,6 +126,7 @@ static void guest_code(void)
addr = align_down(addr, host_page_size);
vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration));
+ nr_writes++;
}
GUEST_SYNC(1);
@@ -665,6 +674,8 @@ static void run_test(enum vm_guest_mode mode, void *arg)
host_dirty_count = 0;
host_clear_count = 0;
WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
+ WRITE_ONCE(nr_writes, 0);
+ sync_global_to_guest(vm, nr_writes);
/*
* Ensure the previous iteration didn't leave a dangling semaphore, i.e.
@@ -683,10 +694,22 @@ static void run_test(enum vm_guest_mode mode, void *arg)
dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
- /* Give the vcpu thread some time to dirty some pages */
- for (i = 0; i < p->interval; i++) {
+ /*
+ * Let the vCPU run beyond the configured interval until it has
+ * performed the minimum number of writes. This verifies the
+ * guest is making forward progress, e.g. isn't stuck because
+ * of a KVM bug, and puts a firm floor on test coverage.
+ */
+ for (i = 0; i < p->interval || nr_writes < TEST_MIN_WRITES_PER_ITERATION; i++) {
+ /*
+ * Sleep in 1ms chunks to keep the interval math simple
+ * and so that the test doesn't run too far beyond the
+ * specified interval.
+ */
usleep(1000);
+ sync_global_from_guest(vm, nr_writes);
+
/*
* Reap dirty pages while the guest is running so that
* dirty ring full events are resolved, i.e. so that a
@@ -760,6 +783,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
WRITE_ONCE(host_quit, true);
sync_global_to_guest(vm, iteration);
+ WRITE_ONCE(nr_writes, 0);
+ sync_global_to_guest(vm, nr_writes);
+
WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
sem_post(&sem_vcpu_cont);
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Ensure the vCPU fully completes at least one write in each dirty_log_test
> iteration, as failure to dirty any pages complicates verification and
> forces the test to be overly conservative about possible values. E.g.
> verification needs to allow the last dirty page from a previous iteration
> to have *any* value, because the vCPU could get stuck for multiple
> iterations, which is unlikely but can happen in heavily overloaded and/or
> nested virtualization setups.
>
> Somewhat arbitrarily set the minimum to 0x100/256; high enough to be
> interesting, but not so high as to lead to pointlessly long runtimes.
>
> Reported-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 30 ++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 500257b712e3..8eb51597f762 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -37,6 +37,12 @@
> /* Interval for each host loop (ms) */
> #define TEST_HOST_LOOP_INTERVAL 10UL
>
> +/*
> + * Ensure the vCPU is able to perform a reasonable number of writes in each
> + * iteration to provide a lower bound on coverage.
> + */
> +#define TEST_MIN_WRITES_PER_ITERATION 0x100
> +
> /* Dirty bitmaps are always little endian, so we need to swap on big endian */
> #if defined(__s390x__)
> # define BITOP_LE_SWIZZLE ((BITS_PER_LONG-1) & ~0x7)
> @@ -72,6 +78,7 @@ static uint64_t host_page_size;
> static uint64_t guest_page_size;
> static uint64_t guest_num_pages;
> static uint64_t iteration;
> +static uint64_t nr_writes;
> static bool vcpu_stop;
>
> /*
> @@ -107,6 +114,7 @@ static void guest_code(void)
> for (i = 0; i < guest_num_pages; i++) {
> addr = guest_test_virt_mem + i * guest_page_size;
> vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration));
> + nr_writes++;
> }
> #endif
>
> @@ -118,6 +126,7 @@ static void guest_code(void)
> addr = align_down(addr, host_page_size);
>
> vcpu_arch_put_guest(*(uint64_t *)addr, READ_ONCE(iteration));
> + nr_writes++;
> }
>
> GUEST_SYNC(1);
> @@ -665,6 +674,8 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> host_dirty_count = 0;
> host_clear_count = 0;
> WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
> + WRITE_ONCE(nr_writes, 0);
> + sync_global_to_guest(vm, nr_writes);
>
> /*
> * Ensure the previous iteration didn't leave a dangling semaphore, i.e.
> @@ -683,10 +694,22 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>
> dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
>
> - /* Give the vcpu thread some time to dirty some pages */
> - for (i = 0; i < p->interval; i++) {
> + /*
> + * Let the vCPU run beyond the configured interval until it has
> + * performed the minimum number of writes. This verifies the
> + * guest is making forward progress, e.g. isn't stuck because
> + * of a KVM bug, and puts a firm floor on test coverage.
> + */
> + for (i = 0; i < p->interval || nr_writes < TEST_MIN_WRITES_PER_ITERATION; i++) {
> + /*
> + * Sleep in 1ms chunks to keep the interval math simple
> + * and so that the test doesn't run too far beyond the
> + * specified interval.
> + */
> usleep(1000);
>
> + sync_global_from_guest(vm, nr_writes);
> +
> /*
> * Reap dirty pages while the guest is running so that
> * dirty ring full events are resolved, i.e. so that a
> @@ -760,6 +783,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> WRITE_ONCE(host_quit, true);
> sync_global_to_guest(vm, iteration);
>
> + WRITE_ONCE(nr_writes, 0);
> + sync_global_to_guest(vm, nr_writes);
> +
> WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
>
> sem_post(&sem_vcpu_cont);
This makes sense.
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
Now that each iteration collects all dirty entries and ensures the guest
*completes* at least one write, tighten the exemptions for the last dirty
page of the previous iteration. Specifically, the only legal value (other
than the current iteration) is N-1.
Unlike the last page for the current iteration, the in-progress write from
the previous iteration is guaranteed to have completed, otherwise the test
would have hung.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 22 +++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 8eb51597f762..18d41537e737 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -517,14 +517,22 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap)
if (host_log_mode == LOG_MODE_DIRTY_RING) {
/*
- * The last page in the ring from this iteration
- * or the previous can be written with the value
- * from the previous iteration (relative to the
- * last page's iteration), as the value to be
- * written may be cached in a CPU register.
+ * The last page in the ring from previous
+ * iteration can be written with the value
+ * from the previous iteration, as the value to
+ * be written may be cached in a CPU register.
*/
- if ((page == dirty_ring_last_page ||
- page == dirty_ring_prev_iteration_last_page) &&
+ if (page == dirty_ring_prev_iteration_last_page &&
+ val == iteration - 1)
+ continue;
+
+ /*
+ * Any value from a previous iteration is legal
+ * for the last entry, as the write may not yet
+ * have retired, i.e. the page may hold whatever
+ * it had before this iteration started.
+ */
+ if (page == dirty_ring_last_page &&
val < iteration)
continue;
} else if (!val && iteration == 1 && bmap0_dirty) {
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Now that each iteration collects all dirty entries and ensures the guest
> *completes* at least one write, tighten the exemptions for the last dirty
> page of the previous iteration. Specifically, the only legal value (other
> than the current iteration) is N-1.
>
> Unlike the last page for the current iteration, the in-progress write from
> the previous iteration is guaranteed to have completed, otherwise the test
> would have hung.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 22 +++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 8eb51597f762..18d41537e737 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -517,14 +517,22 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long **bmap)
>
> if (host_log_mode == LOG_MODE_DIRTY_RING) {
> /*
> - * The last page in the ring from this iteration
> - * or the previous can be written with the value
> - * from the previous iteration (relative to the
> - * last page's iteration), as the value to be
> - * written may be cached in a CPU register.
> + * The last page in the ring from previous
> + * iteration can be written with the value
> + * from the previous iteration, as the value to
> + * be written may be cached in a CPU register.
> */
> - if ((page == dirty_ring_last_page ||
> - page == dirty_ring_prev_iteration_last_page) &&
> + if (page == dirty_ring_prev_iteration_last_page &&
> + val == iteration - 1)
> + continue;
> +
> + /*
> + * Any value from a previous iteration is legal
> + * for the last entry, as the write may not yet
> + * have retired, i.e. the page may hold whatever
> + * it had before this iteration started.
> + */
> + if (page == dirty_ring_last_page &&
> val < iteration)
> continue;
> } else if (!val && iteration == 1 && bmap0_dirty) {
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Set the per-iteration variables at the start of each iteration instead of
setting them before the loop, and at the end of each iteration. To ensure
the vCPU doesn't race ahead before the first iteration, simply have the
vCPU worker want for sem_vcpu_cont, which conveniently avoids the need to
special case posting sem_vcpu_cont from the loop.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 43 ++++++++------------
1 file changed, 17 insertions(+), 26 deletions(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 18d41537e737..f156459bf1ae 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -481,6 +481,8 @@ static void *vcpu_worker(void *data)
{
struct kvm_vcpu *vcpu = data;
+ sem_wait(&sem_vcpu_cont);
+
while (!READ_ONCE(host_quit)) {
/* Let the guest dirty the random pages */
vcpu_run(vcpu);
@@ -675,15 +677,9 @@ static void run_test(enum vm_guest_mode mode, void *arg)
sync_global_to_guest(vm, guest_test_virt_mem);
sync_global_to_guest(vm, guest_num_pages);
- /* Start the iterations */
- iteration = 1;
- sync_global_to_guest(vm, iteration);
- WRITE_ONCE(host_quit, false);
host_dirty_count = 0;
host_clear_count = 0;
- WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
- WRITE_ONCE(nr_writes, 0);
- sync_global_to_guest(vm, nr_writes);
+ WRITE_ONCE(host_quit, false);
/*
* Ensure the previous iteration didn't leave a dangling semaphore, i.e.
@@ -695,12 +691,22 @@ static void run_test(enum vm_guest_mode mode, void *arg)
sem_getvalue(&sem_vcpu_cont, &sem_val);
TEST_ASSERT_EQ(sem_val, 0);
+ TEST_ASSERT_EQ(vcpu_stop, false);
+
pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu);
- while (iteration < p->iterations) {
+ for (iteration = 1; iteration < p->iterations; iteration++) {
unsigned long i;
+ sync_global_to_guest(vm, iteration);
+
+ WRITE_ONCE(nr_writes, 0);
+ sync_global_to_guest(vm, nr_writes);
+
dirty_ring_prev_iteration_last_page = dirty_ring_last_page;
+ WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
+
+ sem_post(&sem_vcpu_cont);
/*
* Let the vCPU run beyond the configured interval until it has
@@ -779,26 +785,11 @@ static void run_test(enum vm_guest_mode mode, void *arg)
bmap[1], host_num_pages,
&ring_buf_idx);
vm_dirty_log_verify(mode, bmap);
-
- /*
- * Set host_quit before sem_vcpu_cont in the final iteration to
- * ensure that the vCPU worker doesn't resume the guest. As
- * above, the dirty ring test may stop and wait even when not
- * explicitly request to do so, i.e. would hang waiting for a
- * "continue" if it's allowed to resume the guest.
- */
- if (++iteration == p->iterations)
- WRITE_ONCE(host_quit, true);
- sync_global_to_guest(vm, iteration);
-
- WRITE_ONCE(nr_writes, 0);
- sync_global_to_guest(vm, nr_writes);
-
- WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
-
- sem_post(&sem_vcpu_cont);
}
+ WRITE_ONCE(host_quit, true);
+ sem_post(&sem_vcpu_cont);
+
pthread_join(vcpu_thread, NULL);
pr_info("Total bits checked: dirty (%lu), clear (%lu)\n",
--
2.47.1.613.gc27f4b7a9f-googActually run all requested iterations, instead of iterations-1 (the count
starts at '1' due to the need to avoid '0' as an in-memory value for a
dirty page).
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index f156459bf1ae..ccc5d9800bbf 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -695,7 +695,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu);
- for (iteration = 1; iteration < p->iterations; iteration++) {
+ for (iteration = 1; iteration <= p->iterations; iteration++) {
unsigned long i;
sync_global_to_guest(vm, iteration);
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Actually run all requested iterations, instead of iterations-1 (the count
> starts at '1' due to the need to avoid '0' as an in-memory value for a
> dirty page).
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index f156459bf1ae..ccc5d9800bbf 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -695,7 +695,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
>
> pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu);
>
> - for (iteration = 1; iteration < p->iterations; iteration++) {
> + for (iteration = 1; iteration <= p->iterations; iteration++) {
> unsigned long i;
>
> sync_global_to_guest(vm, iteration);
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Now that dirty_log_test doesn't require running multiple iterations to
verify dirty pages, and actually runs the requested number of iterations,
drop the requirement that the test run at least "3" (which was really "2"
at the time the test was written) iterations.
Opportunistically use atoi_positive() to do the heavy lifting.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
tools/testing/selftests/kvm/dirty_log_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index ccc5d9800bbf..05b06476bea4 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -880,7 +880,7 @@ int main(int argc, char *argv[])
}
}
- TEST_ASSERT(p.iterations > 2, "Iterations must be greater than two");
+ TEST_ASSERT(p.iterations > 0, "Iterations must be greater than two");
TEST_ASSERT(p.interval > 0, "Interval must be greater than zero");
pr_info("Test iterations: %"PRIu64", interval: %"PRIu64" (ms)\n",
--
2.47.1.613.gc27f4b7a9f-googOn Fri, 2024-12-13 at 17:07 -0800, Sean Christopherson wrote:
> Now that dirty_log_test doesn't require running multiple iterations to
> verify dirty pages, and actually runs the requested number of iterations,
> drop the requirement that the test run at least "3" (which was really "2"
> at the time the test was written) iterations.
>
> Opportunistically use atoi_positive() to do the heavy lifting.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> tools/testing/selftests/kvm/dirty_log_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index ccc5d9800bbf..05b06476bea4 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -880,7 +880,7 @@ int main(int argc, char *argv[])
> }
> }
>
> - TEST_ASSERT(p.iterations > 2, "Iterations must be greater than two");
> + TEST_ASSERT(p.iterations > 0, "Iterations must be greater than two");
You didn't update the assert message.
> TEST_ASSERT(p.interval > 0, "Interval must be greater than zero");
>
> pr_info("Test iterations: %"PRIu64", interval: %"PRIu64" (ms)\n",
© 2016 - 2025 Red Hat, Inc.