| Message ID | 20210915213034.1613552-4-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Small fixes for dirty_log_perf_test | expand |
On Wed, Sep 15, 2021 at 2:30 PM David Matlack <dmatlack@google.com> wrote: > > The calculation to get the per-slot dirty bitmap was incorrect leading > to a buffer overrun. Fix it by dividing the number of pages by > BITS_PER_LONG, since each element of the bitmap is a long and there is > one bit per page. > > Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") > Signed-off-by: David Matlack <dmatlack@google.com> I was a little confused initially because we're allocating only one dirty bitmap in userspace even when we have multiple slots, but that's not a problem. Reviewed-by: Ben Gardon <bgardon@google.com> > > --- > tools/testing/selftests/kvm/dirty_log_perf_test.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c > index 5ad9f2bc7369..0dd4626571e9 100644 > --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c > +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c > @@ -118,6 +118,12 @@ static inline void disable_dirty_logging(struct kvm_vm *vm, int slots) > toggle_dirty_logging(vm, slots, false); > } > > +static unsigned long *get_slot_bitmap(unsigned long *bitmap, uint64_t page_offset) > +{ > + /* Guaranteed to be evenly divisible by the TEST_ASSERT in run_test. */ > + return &bitmap[page_offset / BITS_PER_LONG]; > +} > + > static void get_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, > uint64_t nr_pages) > { > @@ -126,7 +132,8 @@ static void get_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, > > for (i = 0; i < slots; i++) { > int slot = PERF_TEST_MEM_SLOT_INDEX + i; > - unsigned long *slot_bitmap = bitmap + i * slot_pages; > + uint64_t page_offset = slot_pages * i; > + unsigned long *slot_bitmap = get_slot_bitmap(bitmap, page_offset); > > kvm_vm_get_dirty_log(vm, slot, slot_bitmap); > } > @@ -140,7 +147,8 @@ static void clear_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, > > for (i = 0; i < slots; i++) { > int slot = PERF_TEST_MEM_SLOT_INDEX + i; > - unsigned long *slot_bitmap = bitmap + i * slot_pages; > + uint64_t page_offset = slot_pages * i; > + unsigned long *slot_bitmap = get_slot_bitmap(bitmap, page_offset); > > kvm_vm_clear_dirty_log(vm, slot, slot_bitmap, 0, slot_pages); > } > @@ -172,6 +180,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) > guest_num_pages = vm_adjust_num_guest_pages(mode, guest_num_pages); > host_num_pages = vm_num_host_pages(mode, guest_num_pages); > bmap = bitmap_alloc(host_num_pages); > + TEST_ASSERT((host_num_pages / p->slots) % BITS_PER_LONG == 0, > + "The number of pages per slot must be divisible by %d.", > + BITS_PER_LONG); > > if (dirty_log_manual_caps) { > cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2; > -- > 2.33.0.309.g3052b89438-goog >
On Wed, Sep 15, 2021 at 02:55:03PM -0700, Ben Gardon wrote: > On Wed, Sep 15, 2021 at 2:30 PM David Matlack <dmatlack@google.com> wrote: > > > > The calculation to get the per-slot dirty bitmap was incorrect leading > > to a buffer overrun. Fix it by dividing the number of pages by > > BITS_PER_LONG, since each element of the bitmap is a long and there is > > one bit per page. > > > > Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") > > Signed-off-by: David Matlack <dmatlack@google.com> > > I was a little confused initially because we're allocating only one > dirty bitmap in userspace even when we have multiple slots, but that's > not a problem. It's also confusing to me. Wouldn't it be better to create a bitmap per slot? I think the new constraint that host mem must be a multiple of 64 is unfortunate. Thanks, drew
On Thu, Sep 16, 2021 at 1:49 AM Andrew Jones <drjones@redhat.com> wrote: > > On Wed, Sep 15, 2021 at 02:55:03PM -0700, Ben Gardon wrote: > > On Wed, Sep 15, 2021 at 2:30 PM David Matlack <dmatlack@google.com> wrote: > > > > > > The calculation to get the per-slot dirty bitmap was incorrect leading > > > to a buffer overrun. Fix it by dividing the number of pages by > > > BITS_PER_LONG, since each element of the bitmap is a long and there is > > > one bit per page. > > > > > > Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") > > > Signed-off-by: David Matlack <dmatlack@google.com> > > > > I was a little confused initially because we're allocating only one > > dirty bitmap in userspace even when we have multiple slots, but that's > > not a problem. > > It's also confusing to me. Wouldn't it be better to create a bitmap per > slot? I think the new constraint that host mem must be a multiple of 64 > is unfortunate. I don't think think the multiple-of-64 (256K) constraint will matter much in practice. But it wouldn't be very complicated to switch to a bitmap per slot, and would prevent further confusion. I'll switch it over in v2. > > Thanks, > drew >
On 16/09/21 10:49, Andrew Jones wrote: >> I was a little confused initially because we're allocating only one >> dirty bitmap in userspace even when we have multiple slots, but that's >> not a problem. > It's also confusing to me. Wouldn't it be better to create a bitmap per > slot? I think the new constraint that host mem must be a multiple of 64 > is unfortunate. Yeah, I wouldn't mind if someone took a look at that. Also because anyway this patch doesn't apply to master right now, I've queued 1-2 only. Paolo
On Wed, Sep 22, 2021 at 01:09:57PM +0200, Paolo Bonzini wrote: > On 16/09/21 10:49, Andrew Jones wrote: > > > I was a little confused initially because we're allocating only one > > > dirty bitmap in userspace even when we have multiple slots, but that's > > > not a problem. > > It's also confusing to me. Wouldn't it be better to create a bitmap per > > slot? I think the new constraint that host mem must be a multiple of 64 > > is unfortunate. > > Yeah, I wouldn't mind if someone took a look at that. Also because anyway > this patch doesn't apply to master right now, I've queued 1-2 only. David posted a v2 on Sept. 17 with the bitmap split up per slot for patch 3/3. I think the other patches got some tweaks too. Thanks drew
diff --git a/tools/testing/selftests/kvm/dirty_log_perf_test.c b/tools/testing/selftests/kvm/dirty_log_perf_test.c index 5ad9f2bc7369..0dd4626571e9 100644 --- a/tools/testing/selftests/kvm/dirty_log_perf_test.c +++ b/tools/testing/selftests/kvm/dirty_log_perf_test.c @@ -118,6 +118,12 @@ static inline void disable_dirty_logging(struct kvm_vm *vm, int slots) toggle_dirty_logging(vm, slots, false); } +static unsigned long *get_slot_bitmap(unsigned long *bitmap, uint64_t page_offset) +{ + /* Guaranteed to be evenly divisible by the TEST_ASSERT in run_test. */ + return &bitmap[page_offset / BITS_PER_LONG]; +} + static void get_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, uint64_t nr_pages) { @@ -126,7 +132,8 @@ static void get_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, for (i = 0; i < slots; i++) { int slot = PERF_TEST_MEM_SLOT_INDEX + i; - unsigned long *slot_bitmap = bitmap + i * slot_pages; + uint64_t page_offset = slot_pages * i; + unsigned long *slot_bitmap = get_slot_bitmap(bitmap, page_offset); kvm_vm_get_dirty_log(vm, slot, slot_bitmap); } @@ -140,7 +147,8 @@ static void clear_dirty_log(struct kvm_vm *vm, int slots, unsigned long *bitmap, for (i = 0; i < slots; i++) { int slot = PERF_TEST_MEM_SLOT_INDEX + i; - unsigned long *slot_bitmap = bitmap + i * slot_pages; + uint64_t page_offset = slot_pages * i; + unsigned long *slot_bitmap = get_slot_bitmap(bitmap, page_offset); kvm_vm_clear_dirty_log(vm, slot, slot_bitmap, 0, slot_pages); } @@ -172,6 +180,9 @@ static void run_test(enum vm_guest_mode mode, void *arg) guest_num_pages = vm_adjust_num_guest_pages(mode, guest_num_pages); host_num_pages = vm_num_host_pages(mode, guest_num_pages); bmap = bitmap_alloc(host_num_pages); + TEST_ASSERT((host_num_pages / p->slots) % BITS_PER_LONG == 0, + "The number of pages per slot must be divisible by %d.", + BITS_PER_LONG); if (dirty_log_manual_caps) { cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2;
The calculation to get the per-slot dirty bitmap was incorrect leading to a buffer overrun. Fix it by dividing the number of pages by BITS_PER_LONG, since each element of the bitmap is a long and there is one bit per page. Fixes: 609e6202ea5f ("KVM: selftests: Support multiple slots in dirty_log_perf_test") Signed-off-by: David Matlack <dmatlack@google.com> --- tools/testing/selftests/kvm/dirty_log_perf_test.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)