| Message ID | 20241214010721.2356923-10-seanjc@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/20] KVM: selftests: Support multiple write retires in dirty_log_test | expand |
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. > > 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
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);
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. 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(-)