[5/6] KVM: selftests: memslot_perf_test: Consolidate memory sizes

  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(-)
  

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