On Fri, Feb 02, 2024, Sean Christopherson wrote:
On Fri, Feb 02, 2024, Shaoqin Huang wrote:
v2->v3:
- Rebase to v6.8-rc2.
- Use TEST_ASSERT().
Patch says otherwise.
@@ -726,6 +728,11 @@ static void run_test(enum vm_guest_mode mode, void *arg) return; }
- sem_getvalue(&sem_vcpu_stop, &sem_val);
- assert(sem_val == 0);
- sem_getvalue(&sem_vcpu_cont, &sem_val);
- assert(sem_val == 0);
- /*
- We reserve page table for 2 times of extra dirty mem which
- will definitely cover the original (1G+) test range. Here
@@ -825,6 +832,13 @@ static void run_test(enum vm_guest_mode mode, void *arg) sync_global_to_guest(vm, iteration); }
- /*
** Before we set the host_quit, let the vcpu has time to run, to make* sure we consume the sem_vcpu_stop and the vcpu consume the* sem_vcpu_cont, to keep the semaphore balance.*/- usleep(p->interval * 1000);
Sorry, I wasn't as explicit as I should have been. When I said I don't have a better solution, I did not mean to imply that I am ok with busy waiting as a hack-a-around.
Against my better judgment, I spent half an hour slogging through this test to figure out what's going wrong. IIUC, the problem is essentially that the test instructs the vCPU worker to continue _after_ the last iteration, and _then_ sets host_quit, which results in the vCPU running one extra (unvalidated) iteration.
For the other modes, which stop if and only if vcpu_sync_stop_requested is set, the extra iteration is a non-issue. But because the dirty ring variant stops after every exit (to purge the ring), it hangs without an extra "continue".
So rather than blindly fire off an extra sem_vcpu_cont that may or may not be consumed, I believe the test can simply set host_quit _before_ the final "continue" so that the vCPU worker doesn't run an extra iteration.
I ran the below with 1000 loops of "for (i = 0; i < LOG_MODE_NUM; i++)" and so no issues. I didn't see the assert you hit, but without the fix, I did see this fire within a few loops (less than 5 I think)l
assert(host_log_mode == LOG_MODE_DIRTY_RING || atomic_read(&vcpu_sync_stop_requested) == false);
I'll post this as two patches: one to fix the bug, and a second to have the LOG_MODE_DIRTY_RING variant clear vcpu_sync_stop_requested so that the above assert() can be modified as below.
Scratch patch 2, I'm pretty sure the vCPU worker can race with the main task and clear vcpu_sync_stop_requested just before the main task sets it, which would result in a false positive. I didn't see any failures, but I'm 99% certain the race exists. I suspect there are some other warts in the test that would complicate attempts to clean things up, so for now I'l just post the fix for the imbalance bug.