Re: [PATCH v1] KVM: selftests: Initalize sem_vcpu_[cont|stop] before each test in dirty_log_test

[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