On Mon, Nov 16, 2020 at 01:40:11PM -0500, Peter Xu wrote: > On Mon, Nov 16, 2020 at 07:16:50PM +0100, Paolo Bonzini wrote: > > On 16/11/20 13:19, Andrew Jones wrote: > > > This series attempts to clean up demand_paging_test, dirty_log_perf_test, > > > and dirty_log_test by factoring out common code, creating some new API > > > along the way. It also splits include/perf_test_util.h into a more > > > conventional header and source pair. > > > > > > I've tested on x86 and AArch64 (one config each), but not s390x. > > > > > > v3: > > > - Rebased remaining four patches from v2 onto kvm/queue > > > - Picked up r-b's from Peter and Ben > > > > > > v2: https://www.spinics.net/lists/kvm/msg228711.html > > > > Unfortunately patch 2 is still broken: > > > > $ ./dirty_log_test -M dirty-ring > > Setting log mode to: 'dirty-ring' > > Test iterations: 32, interval: 10 (ms) > > Testing guest mode: PA-bits:ANY, VA-bits:48, 4K pages > > ==== Test Assertion Failure ==== > > lib/kvm_util.c:85: ret == 0 > > pid=2010122 tid=2010122 - Invalid argument > > 1 0x0000000000402ee7: vm_enable_cap at kvm_util.c:84 > > 2 0x0000000000403004: vm_enable_dirty_ring at kvm_util.c:124 > > 3 0x00000000004021a5: log_mode_create_vm_done at dirty_log_test.c:453 > > 4 (inlined by) run_test at dirty_log_test.c:683 > > 5 0x000000000040b643: for_each_guest_mode at guest_modes.c:37 > > 6 0x00000000004019c2: main at dirty_log_test.c:864 > > 7 0x00007fe3f48207b2: ?? ??:0 > > 8 0x0000000000401aad: _start at ??:? > > KVM_ENABLE_CAP IOCTL failed, > > rc: -1 errno: 22 > > > > (Also fails without -M). > > It should be because of the ordering of creating vcpu and enabling dirty rings, > since currently for simplicity when enabling dirty ring we must have not > created any vcpus: > > + if (kvm->created_vcpus) { > + /* We don't allow to change this value after vcpu created */ > + r = -EINVAL; > + } else { > + kvm->dirty_ring_size = size; > + r = 0; > + } > > We may need to call log_mode_create_vm_done() before creating any vcpus > somehow. Sorry to not have noticed that when reviewing it. > And sorry for having not tested with '-M dirty-ring'. I thought we were trying to ensure each unique test type had its own test file (even if we have to do the weird inclusion of C files). Doing that, the command line options are then only used to change stuff like verbosity or to experiment with tweaked configurations. If we're not doing that, then I think we should. We don't want to try and explain to all the CI people how each test should be run. It's much easier to say "run all the binaries, no parameters necessary". Each binary with no parameters should run the test(s) using a good default or by executing all possible configurations. Thanks, drew
