| Message ID | 20220912195849.3989707-4-coltonlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: randomize memory access of dirty_log_perf_test | expand |
On Mon, Sep 12, 2022, Colton Lewis wrote: > @@ -57,7 +58,17 @@ void perf_test_guest_code(uint32_t vcpu_id) > > while (true) { > for (i = 0; i < pages; i++) { > - uint64_t addr = gva + (i * pta->guest_page_size); > + guest_random(&rand); > + > + if (pta->random_access) > + addr = gva + ((rand % pages) * pta->guest_page_size); Shouldn't this use a 64-bit random number since "pages" is a 64-bit value? Ha! And another case where the RNG APIs can help, e.g. uint64_t __random_u64(struct ksft_pseudo_rng *rng, uint64_t max); or maybe avoid naming pain and go straight to: uint64_t __random_u64(struct ksft_pseudo_rng *rng, uint64_t min, uint64_t max); > + else > + addr = gva + (i * pta->guest_page_size); Since the calculation is the same, only the page index changes, I think it makes sense to write this as: uint64_t idx = i; if (pta->random_access) idx = __random_u64(rng, 0, pages); addr = gva + (idx * pta->guest_page_size); That way it's easy to introduce other access patterns. > + > + /* > + * Use a new random number here so read/write > + * is not tied to the address used. > + */ > guest_random(&rand); Ya, I'm trippling (quadrupling?) down on my suggestion to improve the APIs. Users should not be able to screw up like this, i.e. shouldn't need comments to warn readers, and adding another call to get a random number shouldn't affect unrelated code.
On Tue, Oct 11, 2022, Colton Lewis wrote: > Sean Christopherson <seanjc@google.com> writes: > > Ya, I'm trippling (quadrupling?) down on my suggestion to improve the > > APIs. Users > > should not be able to screw up like this, i.e. shouldn't need comments > > to warn > > readers, and adding another call to get a random number shouldn't affect > > unrelated > > code. > > Previous calls to PRNGs always affect later calls to PRNGs. That's how > they work. Ya, that's not the type of bugs I'm worried about. > This "screw up" would be equally likely with any API because the caller > always needs to decide if they should reuse the same random number or need a > new one. I disagree, the in/out parameter _requires_ the calling code to store the random number in a variable. Returning the random number allows consuming the number without needing an intermediate variable, e.g. if (random_bool()) <do stuff> which makes it easy to avoid an entire class of bugs.
On Tue, Oct 11, 2022, Colton Lewis wrote: > Sean Christopherson <seanjc@google.com> writes: > > > On Tue, Oct 11, 2022, Colton Lewis wrote: > > > Sean Christopherson <seanjc@google.com> writes: > > > > Ya, I'm trippling (quadrupling?) down on my suggestion to improve the > > > > APIs. Users > > > > should not be able to screw up like this, i.e. shouldn't need comments > > > > to warn > > > > readers, and adding another call to get a random number shouldn't > > > affect > > > > unrelated > > > > code. > > > > Previous calls to PRNGs always affect later calls to PRNGs. That's how > > > they work. > > > Ya, that's not the type of bugs I'm worried about. > > > > This "screw up" would be equally likely with any API because the caller > > > always needs to decide if they should reuse the same random number > > > or need a > > > new one. > > > I disagree, the in/out parameter _requires_ the calling code to store > > the random > > number in a variable. Returning the random number allows consuming the > > number > > without needing an intermediate variable, e.g. > > > if (random_bool()) > > <do stuff> > > > which makes it easy to avoid an entire class of bugs. > > Yes, but it's impossible to do this without hidden global state at the > implementation level. That sacrifices reentrancy and thread-safety. The above is super quick pseudocode that wasn't intended to be taken verbatim. From my original suggestion in patch one[*], throw the seed/metadata in a opaque struct, e.g. ksft_pseudo_rng (or kvm_pseudo_rng if the code ends up being KVM-only). The intent is to hide the details of the rng, both so that the caller doesn't have to worry about those details, but also so that the guts can be changed at will without having to update callers. [*] https://lore.kernel.org/all/20220912195849.3989707-2-coltonlewis@google.com
diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index dfa5957332b1..ccc1f571645a 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -127,6 +127,7 @@ struct test_params { int slots; uint32_t write_percent; uint32_t random_seed; + bool random_access; }; static void toggle_dirty_logging(struct kvm_vm *vm, int slots, bool enable) @@ -256,6 +257,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) * would pollute the performance results. */ perf_test_set_write_percent(vm, 100); + perf_test_set_random_access(vm, false); perf_test_start_vcpu_threads(nr_vcpus, vcpu_worker); /* Allow the vCPUs to populate memory */ @@ -278,6 +280,7 @@ static void run_test(enum vm_guest_mode mode, void *arg) ts_diff.tv_sec, ts_diff.tv_nsec); perf_test_set_write_percent(vm, p->write_percent); + perf_test_set_random_access(vm, p->random_access); while (iteration < p->iterations) { /* @@ -349,10 +352,11 @@ static void run_test(enum vm_guest_mode mode, void *arg) static void help(char *name) { puts(""); - printf("usage: %s [-h] [-i iterations] [-p offset] [-g] " + printf("usage: %s [-h] [-a] [-i iterations] [-p offset] [-g] " "[-m mode] [-n] [-b vcpu bytes] [-v vcpus] [-o] [-r random seed ] [-s mem type]" "[-x memslots] [-w percentage]\n", name); puts(""); + printf(" -a: access memory randomly rather than in order.\n"); printf(" -i: specify iteration counts (default: %"PRIu64")\n", TEST_HOST_LOOP_N); printf(" -g: Do not enable KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2. This\n" @@ -403,8 +407,11 @@ int main(int argc, char *argv[]) guest_modes_append_default(); - while ((opt = getopt(argc, argv, "ghi:p:m:nb:v:or:s:x:w:")) != -1) { + while ((opt = getopt(argc, argv, "aghi:p:m:nb:v:or:s:x:w:")) != -1) { switch (opt) { + case 'a': + p.random_access = true; + break; case 'g': dirty_log_manual_caps = 0; break; diff --git a/tools/testing/selftests/kvm/include/perf_test_util.h b/tools/testing/selftests/kvm/include/perf_test_util.h index f93f2ea7c6a3..d9664a31e01c 100644 --- a/tools/testing/selftests/kvm/include/perf_test_util.h +++ b/tools/testing/selftests/kvm/include/perf_test_util.h @@ -39,6 +39,7 @@ struct perf_test_args { /* Run vCPUs in L2 instead of L1, if the architecture supports it. */ bool nested; + bool random_access; struct perf_test_vcpu_args vcpu_args[KVM_MAX_VCPUS]; }; @@ -53,6 +54,7 @@ void perf_test_destroy_vm(struct kvm_vm *vm); void perf_test_set_write_percent(struct kvm_vm *vm, uint32_t write_percent); void perf_test_set_random_seed(struct kvm_vm *vm, uint32_t random_seed); +void perf_test_set_random_access(struct kvm_vm *vm, bool random_access); void perf_test_start_vcpu_threads(int vcpus, void (*vcpu_fn)(struct perf_test_vcpu_args *)); void perf_test_join_vcpu_threads(int vcpus); diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/selftests/kvm/lib/perf_test_util.c index 9effd229b75d..6b196d003491 100644 --- a/tools/testing/selftests/kvm/lib/perf_test_util.c +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c @@ -46,6 +46,7 @@ void perf_test_guest_code(uint32_t vcpu_id) struct perf_test_vcpu_args *vcpu_args = &pta->vcpu_args[vcpu_id]; uint64_t gva; uint64_t pages; + uint64_t addr; int i; uint32_t rand = pta->random_seed + vcpu_id; @@ -57,7 +58,17 @@ void perf_test_guest_code(uint32_t vcpu_id) while (true) { for (i = 0; i < pages; i++) { - uint64_t addr = gva + (i * pta->guest_page_size); + guest_random(&rand); + + if (pta->random_access) + addr = gva + ((rand % pages) * pta->guest_page_size); + else + addr = gva + (i * pta->guest_page_size); + + /* + * Use a new random number here so read/write + * is not tied to the address used. + */ guest_random(&rand); if (rand % 100 < pta->write_percent) @@ -232,6 +243,12 @@ void perf_test_set_random_seed(struct kvm_vm *vm, uint32_t random_seed) sync_global_to_guest(vm, perf_test_args.random_seed); } +void perf_test_set_random_access(struct kvm_vm *vm, bool random_access) +{ + perf_test_args.random_access = random_access; + sync_global_to_guest(vm, perf_test_args.random_access); +} + uint64_t __weak perf_test_nested_pages(int nr_vcpus) { return 0;