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.
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@xxxxxxxxxx>
> ---
> 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);
> }
>
