| Message ID | 20221014071914.227134-6-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: memslot_perf_test: aarch64 cleanup/fixes | expand |
On 14.10.2022 09:19, Gavin Shan wrote: > The addresses and sizes passed to madvise() and vm_userspace_mem_region_add() > should be aligned to host page size, which can be 64KB on aarch64. So it's > wrong by passing additional fixed 4KB memory area to various tests. > > Fix it by passing additional fixed 64KB memory area to various tests. After > it's applied, the following command works fine on 64KB-page-size-host and > 4KB-page-size-guest. > > # ./memslot_perf_test -v -s 512 > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > .../testing/selftests/kvm/memslot_perf_test.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c > index d587bd952ff9..e6d34744b45d 100644 > --- a/tools/testing/selftests/kvm/memslot_perf_test.c > +++ b/tools/testing/selftests/kvm/memslot_perf_test.c > @@ -25,12 +25,14 @@ > #include <kvm_util.h> > #include <processor.h> > > -#define MEM_SIZE ((512U << 20) + 4096) > -#define MEM_GPA 0x10000000UL > +#define MEM_EXTRA_SIZE 0x10000 So the biggest page size supported right now is 64 KiB - it would be good to have an assert somewhere to explicitly check for this (regardless of implicit checks present in other calculations). Also, an expression like "(64 << 10)" is more readable than a "1" with a tail of zeroes (it's easy to add one zero too many or be one zero short). Thanks, Maciej
On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote: > > +#define MEM_EXTRA_SIZE 0x10000 > > Also, an expression like "(64 << 10)" is more readable than a "1" > with a tail of zeroes (it's easy to add one zero too many or be one > zero short). +1 to not open coding raw numbers. I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB, 16KB, 64K, 2MB, 1GB, etc... Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do math off of those.
On 10/18/22 6:08 AM, Sean Christopherson wrote: > On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote: >>> +#define MEM_EXTRA_SIZE 0x10000 >> >> Also, an expression like "(64 << 10)" is more readable than a "1" >> with a tail of zeroes (it's easy to add one zero too many or be one >> zero short). > > +1 to not open coding raw numbers. > > I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB, > 16KB, 64K, 2MB, 1GB, etc... > > Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do > math off of those. > Ok. I will have one separate patch to define those sizes in kvm_util_base.h, right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know if it looks good to you? #define KB (1UL << 10) #define MB (1UL << 20) #define GB (1UL << 30) #define TB (1UL << 40) /* Base page and huge page size */ #define SIZE_4KB ( 4 * KB) #define SIZE_16KB ( 16 * KB) #define SIZE_64KB ( 64 * KB) #define SIZE_2MB ( 2 * MB) #define SIZE_32MB ( 32 * MB) #define SIZE_512MB (512 * MB) #define SIZE_1GB ( 1 * GB) #define SIZE_16GB ( 16 * GB) Thanks, Gavin
On 18.10.2022 00:51, Gavin Shan wrote: > On 10/18/22 6:08 AM, Sean Christopherson wrote: >> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote: >>>> +#define MEM_EXTRA_SIZE 0x10000 >>> >>> Also, an expression like "(64 << 10)" is more readable than a "1" >>> with a tail of zeroes (it's easy to add one zero too many or be one >>> zero short). >> >> +1 to not open coding raw numbers. >> >> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB, >> 16KB, 64K, 2MB, 1GB, etc... >> >> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do >> math off of those. >> > > Ok. I will have one separate patch to define those sizes in kvm_util_base.h, > right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know > if it looks good to you? > > #define KB (1UL << 10) > #define MB (1UL << 20) > #define GB (1UL << 30) > #define TB (1UL << 40) > > /* Base page and huge page size */ > #define SIZE_4KB ( 4 * KB) > #define SIZE_16KB ( 16 * KB) > #define SIZE_64KB ( 64 * KB) > #define SIZE_2MB ( 2 * MB) > #define SIZE_32MB ( 32 * MB) > #define SIZE_512MB (512 * MB) > #define SIZE_1GB ( 1 * GB) > #define SIZE_16GB ( 16 * GB) FYI, QEMU uses KiB, MiB, GiB, etc., see [1]. > Thanks, > Gavin > Thanks, Maciej [1]: https://git.qemu.org/?p=qemu.git;a=blob;f=include/qemu/units.h;hb=HEAD
On 10/18/22 6:56 AM, Maciej S. Szmigiero wrote: > On 18.10.2022 00:51, Gavin Shan wrote: >> On 10/18/22 6:08 AM, Sean Christopherson wrote: >>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote: >>>>> +#define MEM_EXTRA_SIZE 0x10000 >>>> >>>> Also, an expression like "(64 << 10)" is more readable than a "1" >>>> with a tail of zeroes (it's easy to add one zero too many or be one >>>> zero short). >>> >>> +1 to not open coding raw numbers. >>> >>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB, >>> 16KB, 64K, 2MB, 1GB, etc... >>> >>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do >>> math off of those. >>> >> >> Ok. I will have one separate patch to define those sizes in kvm_util_base.h, >> right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know >> if it looks good to you? >> >> #define KB (1UL << 10) >> #define MB (1UL << 20) >> #define GB (1UL << 30) >> #define TB (1UL << 40) >> >> /* Base page and huge page size */ >> #define SIZE_4KB ( 4 * KB) >> #define SIZE_16KB ( 16 * KB) >> #define SIZE_64KB ( 64 * KB) >> #define SIZE_2MB ( 2 * MB) >> #define SIZE_32MB ( 32 * MB) >> #define SIZE_512MB (512 * MB) >> #define SIZE_1GB ( 1 * GB) >> #define SIZE_16GB ( 16 * GB) > > FYI, QEMU uses KiB, MiB, GiB, etc., see [1]. > Right. I checked QEMU's definitions and it makes sense to use KiB, MiB, GiB, TiB. I don't think we need PiB and EiB because our tests don't use that large memory. > > [1]: https://git.qemu.org/?p=qemu.git;a=blob;f=include/qemu/units.h;hb=HEAD > Thanks, Gavin
On Tue, Oct 18, 2022, Gavin Shan wrote: > On 10/18/22 6:56 AM, Maciej S. Szmigiero wrote: > > On 18.10.2022 00:51, Gavin Shan wrote: > > > On 10/18/22 6:08 AM, Sean Christopherson wrote: > > > > On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote: > > > > > > +#define MEM_EXTRA_SIZE 0x10000 > > > > > > > > > > Also, an expression like "(64 << 10)" is more readable than a "1" > > > > > with a tail of zeroes (it's easy to add one zero too many or be one > > > > > zero short). > > > > > > > > +1 to not open coding raw numbers. > > > > > > > > I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB, > > > > 16KB, 64K, 2MB, 1GB, etc... > > > > > > > > Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do > > > > math off of those. > > > > > > > > > > Ok. I will have one separate patch to define those sizes in kvm_util_base.h, > > > right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know > > > if it looks good to you? > > > > > > #define KB (1UL << 10) > > > #define MB (1UL << 20) > > > #define GB (1UL << 30) > > > #define TB (1UL << 40) Any objection to prefixing these with SIZE_ as well? IMO it's worth burning the extra five characters to make it all but impossible to misinterpret code. > > > /* Base page and huge page size */ > > > #define SIZE_4KB ( 4 * KB) > > > #define SIZE_16KB ( 16 * KB) > > > #define SIZE_64KB ( 64 * KB) > > > #define SIZE_2MB ( 2 * MB) > > > #define SIZE_32MB ( 32 * MB) > > > #define SIZE_512MB (512 * MB) > > > #define SIZE_1GB ( 1 * GB) > > > #define SIZE_16GB ( 16 * GB) > > > > FYI, QEMU uses KiB, MiB, GiB, etc., see [1]. > > > > Right. I checked QEMU's definitions and it makes sense to use > KiB, MiB, GiB, TiB. I don't think we need PiB and EiB because > our tests don't use that large memory. Ha! I had typed out KiB, etc... but then thought, "nah, I'm being silly". KiB and friends work for me.
On 10/18/22 7:32 AM, Sean Christopherson wrote: > On Tue, Oct 18, 2022, Gavin Shan wrote: >> On 10/18/22 6:56 AM, Maciej S. Szmigiero wrote: >>> On 18.10.2022 00:51, Gavin Shan wrote: >>>> On 10/18/22 6:08 AM, Sean Christopherson wrote: >>>>> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote: >>>>>>> +#define MEM_EXTRA_SIZE 0x10000 >>>>>> >>>>>> Also, an expression like "(64 << 10)" is more readable than a "1" >>>>>> with a tail of zeroes (it's easy to add one zero too many or be one >>>>>> zero short). >>>>> >>>>> +1 to not open coding raw numbers. >>>>> >>>>> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB, >>>>> 16KB, 64K, 2MB, 1GB, etc... >>>>> >>>>> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do >>>>> math off of those. >>>>> >>>> >>>> Ok. I will have one separate patch to define those sizes in kvm_util_base.h, >>>> right after '#define NSEC_PER_SEC 1000000000L'. Sean, could you let me know >>>> if it looks good to you? >>>> >>>> #define KB (1UL << 10) >>>> #define MB (1UL << 20) >>>> #define GB (1UL << 30) >>>> #define TB (1UL << 40) > > Any objection to prefixing these with SIZE_ as well? IMO it's worth burning the > extra five characters to make it all but impossible to misinterpret code. > 'SIZE_' prefix works for me either. >>>> /* Base page and huge page size */ >>>> #define SIZE_4KB ( 4 * KB) >>>> #define SIZE_16KB ( 16 * KB) >>>> #define SIZE_64KB ( 64 * KB) >>>> #define SIZE_2MB ( 2 * MB) >>>> #define SIZE_32MB ( 32 * MB) >>>> #define SIZE_512MB (512 * MB) >>>> #define SIZE_1GB ( 1 * GB) >>>> #define SIZE_16GB ( 16 * GB) >>> >>> FYI, QEMU uses KiB, MiB, GiB, etc., see [1]. >>> >> >> Right. I checked QEMU's definitions and it makes sense to use >> KiB, MiB, GiB, TiB. I don't think we need PiB and EiB because >> our tests don't use that large memory. > > Ha! I had typed out KiB, etc... but then thought, "nah, I'm being silly". KiB > and friends work for me. > Thanks for your confirm, Sean. Thanks, Gavin
On 10/18/22 5:36 AM, Maciej S. Szmigiero wrote: > On 14.10.2022 09:19, Gavin Shan wrote: >> The addresses and sizes passed to madvise() and vm_userspace_mem_region_add() >> should be aligned to host page size, which can be 64KB on aarch64. So it's >> wrong by passing additional fixed 4KB memory area to various tests. >> >> Fix it by passing additional fixed 64KB memory area to various tests. After >> it's applied, the following command works fine on 64KB-page-size-host and >> 4KB-page-size-guest. >> >> # ./memslot_perf_test -v -s 512 >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> .../testing/selftests/kvm/memslot_perf_test.c | 18 ++++++++++-------- >> 1 file changed, 10 insertions(+), 8 deletions(-) >> >> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c >> index d587bd952ff9..e6d34744b45d 100644 >> --- a/tools/testing/selftests/kvm/memslot_perf_test.c >> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c >> @@ -25,12 +25,14 @@ >> #include <kvm_util.h> >> #include <processor.h> >> -#define MEM_SIZE ((512U << 20) + 4096) >> -#define MEM_GPA 0x10000000UL >> +#define MEM_EXTRA_SIZE 0x10000 > > So the biggest page size supported right now is 64 KiB - it would be > good to have an assert somewhere to explicitly check for this > (regardless of implicit checks present in other calculations). > > Also, an expression like "(64 << 10)" is more readable than a "1" > with a tail of zeroes (it's easy to add one zero too many or be one > zero short). > Yes, it makes sense to me. Lets add check in check_memory_sizes(), which was added in the previous patch, to fail early if host/guest page size exceeds 64KB. if (host_page_size > SIZE_64KiB || guest_page_size > SIZE_64KiB) { pr_info("Unsupported page size on host (0x%x) or guest (0x%x)\n", host_page_size, guest_page_size); } For the macros, I think all of us agree on KiB, MiB, GiB, TiB and their variants :) Thanks, Gavin
On Mon, Oct 17, 2022 at 10:08:48PM +0000, Sean Christopherson wrote: > On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote: > > > +#define MEM_EXTRA_SIZE 0x10000 > > > > Also, an expression like "(64 << 10)" is more readable than a "1" > > with a tail of zeroes (it's easy to add one zero too many or be one > > zero short). > > +1 to not open coding raw numbers. > > I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB, > 16KB, 64K, 2MB, 1GB, etc... > > Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do > math off of those. I mean I love boilerplate as much as the next guy, but we can just use tools/include/linux/sizes.h -- Thanks, Oliver
On 10/18/22 3:47 PM, Oliver Upton wrote: > On Mon, Oct 17, 2022 at 10:08:48PM +0000, Sean Christopherson wrote: >> On Mon, Oct 17, 2022, Maciej S. Szmigiero wrote: >>>> +#define MEM_EXTRA_SIZE 0x10000 >>> >>> Also, an expression like "(64 << 10)" is more readable than a "1" >>> with a tail of zeroes (it's easy to add one zero too many or be one >>> zero short). >> >> +1 to not open coding raw numbers. >> >> I think it's high time KVM selftests add #defines for the common sizes, e.g. SIZE_4KB, >> 16KB, 64K, 2MB, 1GB, etc... >> >> Alternatively (or in addition), just #define 1KB, 1MB, 1GB, and 1TB, and then do >> math off of those. > > I mean I love boilerplate as much as the next guy, but we can just use > tools/include/linux/sizes.h > Nice point, I didn't realize we already had 'tools/include/linux/sizes.h'. The suggested macros (KiB, MiB, GiB, TiB and their variants) have been added to PATCH[v2 5/6]. I think it's reasonable to use 'tools/include/linux/sizes.h' directly instead of reinventing the wheel. I will go ahead to use 'tools/include/linux/sizes.h' directly in v3 if nobody objects. I would like to receive comments on v2 before I'm going to post v3. Thanks, Gavin
diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c index d587bd952ff9..e6d34744b45d 100644 --- a/tools/testing/selftests/kvm/memslot_perf_test.c +++ b/tools/testing/selftests/kvm/memslot_perf_test.c @@ -25,12 +25,14 @@ #include <kvm_util.h> #include <processor.h> -#define MEM_SIZE ((512U << 20) + 4096) -#define MEM_GPA 0x10000000UL +#define MEM_EXTRA_SIZE 0x10000 + +#define MEM_SIZE ((512U << 20) + MEM_EXTRA_SIZE) +#define MEM_GPA 0x10000000UL #define MEM_AUX_GPA MEM_GPA #define MEM_SYNC_GPA MEM_AUX_GPA -#define MEM_TEST_GPA (MEM_AUX_GPA + 4096) -#define MEM_TEST_SIZE (MEM_SIZE - 4096) +#define MEM_TEST_GPA (MEM_AUX_GPA + MEM_EXTRA_SIZE) +#define MEM_TEST_SIZE (MEM_SIZE - MEM_EXTRA_SIZE) /* * 32 MiB is max size that gets well over 100 iterations on 509 slots. @@ -38,8 +40,8 @@ * 8194 slots in use can then be tested (although with slightly * limited resolution). */ -#define MEM_SIZE_MAP ((32U << 20) + 4096) -#define MEM_TEST_MAP_SIZE (MEM_SIZE_MAP - 4096) +#define MEM_SIZE_MAP ((32U << 20) + MEM_EXTRA_SIZE) +#define MEM_TEST_MAP_SIZE (MEM_SIZE_MAP - MEM_EXTRA_SIZE) /* * 128 MiB is min size that fills 32k slots with at least one page in each @@ -799,13 +801,13 @@ static const struct test_data tests[] = { }, { .name = "unmap", - .mem_size = MEM_TEST_UNMAP_SIZE + 4096, + .mem_size = MEM_TEST_UNMAP_SIZE + MEM_EXTRA_SIZE, .guest_code = guest_code_test_memslot_unmap, .loop = test_memslot_unmap_loop, }, { .name = "unmap chunked", - .mem_size = MEM_TEST_UNMAP_SIZE + 4096, + .mem_size = MEM_TEST_UNMAP_SIZE + MEM_EXTRA_SIZE, .guest_code = guest_code_test_memslot_unmap, .loop = test_memslot_unmap_loop_chunked, },
The addresses and sizes passed to madvise() and vm_userspace_mem_region_add() should be aligned to host page size, which can be 64KB on aarch64. So it's wrong by passing additional fixed 4KB memory area to various tests. Fix it by passing additional fixed 64KB memory area to various tests. After it's applied, the following command works fine on 64KB-page-size-host and 4KB-page-size-guest. # ./memslot_perf_test -v -s 512 Signed-off-by: Gavin Shan <gshan@redhat.com> --- .../testing/selftests/kvm/memslot_perf_test.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)