Messages in this thread | | | Date | Fri, 17 Nov 2023 11:19:32 +0800 | Subject | Re: [PATCH v1] KVM: selftests: Initalize sem_vcpu_[cont|stop] before each test in dirty_log_test | From | Shaoqin Huang <> |
| |
Hi Oliver,
On 11/17/23 08:18, Oliver Upton wrote: > Hi Shaoqin, > > On Thu, Nov 16, 2023 at 04:35:36AM -0500, Shaoqin Huang wrote: >> When execute the dirty_log_test on some aarch64 machine, it sometimes >> trigger the ASSERT: >> >> ==== Test Assertion Failure ==== >> dirty_log_test.c:384: dirty_ring_vcpu_ring_full >> pid=14854 tid=14854 errno=22 - Invalid argument >> 1 0x00000000004033eb: dirty_ring_collect_dirty_pages at dirty_log_test.c:384 >> 2 0x0000000000402d27: log_mode_collect_dirty_pages at dirty_log_test.c:505 >> 3 (inlined by) run_test at dirty_log_test.c:802 >> 4 0x0000000000403dc7: for_each_guest_mode at guest_modes.c:100 >> 5 0x0000000000401dff: main at dirty_log_test.c:941 (discriminator 3) >> 6 0x0000ffff9be173c7: ?? ??:0 >> 7 0x0000ffff9be1749f: ?? ??:0 >> 8 0x000000000040206f: _start at ??:? >> Didn't continue vcpu even without ring full >> >> The dirty_log_test fails when execute the dirty-ring test, this is >> because the sem_vcpu_cont and the sem_vcpu_stop is non-zero value when >> execute the dirty_ring_collect_dirty_pages() function. When those two >> sem_t variables are non-zero, the dirty_ring_wait_vcpu() at the >> beginning of the dirty_ring_collect_dirty_pages() will not wait for the >> vcpu to stop, but continue to execute the following code. In this case, >> before vcpu stop, if the dirty_ring_vcpu_ring_full is true, and the >> dirty_ring_collect_dirty_pages() has passed the check for the >> dirty_ring_vcpu_ring_full but hasn't execute the check for the >> continued_vcpu, the vcpu stop, and set the dirty_ring_vcpu_ring_full to >> false. Then dirty_ring_collect_dirty_pages() will trigger the ASSERT. >> >> Why sem_vcpu_cont and sem_vcpu_stop can be non-zero value? It's because >> the dirty_ring_before_vcpu_join() execute the sem_post(&sem_vcpu_cont) >> at the end of each dirty-ring test. It can cause two cases: >> >> 1. sem_vcpu_cont be non-zero. When we set the host_quit to be true, >> the vcpu_worker directly see the host_quit to be true, it quit. So >> the log_mode_before_vcpu_join() function will set the sem_vcpu_cont >> to 1, since the vcpu_worker has quit, it won't consume it. >> 2. sem_vcpu_stop be non-zero. When we set the host_quit to be true, >> the vcpu_worker has entered the guest state, the next time it exit >> from guest state, it will set the sem_vcpu_stop to 1, and then see >> the host_quit, no one will consume the sem_vcpu_stop. >> >> When execute more and more dirty-ring tests, the sem_vcpu_cont and >> sem_vcpu_stop can be larger and larger, which makes many code paths >> don't wait for the sem_t. Thus finally cause the problem. >> >> Fix this problem is easy, simply initialize the sem_t before every test. >> Thus whatever the state previous test left, it won't interfere the next >> test. > > In your changelog you describe what sounds like a semaphore imbalance at > the time of test completion, yet your proposed fix is to just clobber > the error and start fresh. >
Yes. It's a semaphore imbalance problem.
> Why not nip it at the bud and fix the logic bug instead?
I have another patch which fix the logic bug, I will send it out later.
>
-- Shaoqin
| |