| Message ID | 20241214010721.2356923-3-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: > 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
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);
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(-)