| Message ID | 20210208090841.333724-2-wangyanan55@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add a test for kvm page table code | expand |
On Mon, Feb 08, 2021, Yanan Wang wrote: > Add a macro to get string of the backing source memory type, so that > application can add choices for source types in the help() function, > and users can specify which type to use for testing. > > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > tools/testing/selftests/kvm/include/kvm_util.h | 3 +++ > tools/testing/selftests/kvm/lib/kvm_util.c | 8 ++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h > index 5cbb861525ed..f5fc29dc9ee6 100644 > --- a/tools/testing/selftests/kvm/include/kvm_util.h > +++ b/tools/testing/selftests/kvm/include/kvm_util.h > @@ -69,7 +69,9 @@ enum vm_guest_mode { > #define PTES_PER_MIN_PAGE ptes_per_page(MIN_PAGE_SIZE) > > #define vm_guest_mode_string(m) vm_guest_mode_string[m] > +#define vm_mem_backing_src_type_string(s) vm_mem_backing_src_type_string[s] Oof, I see this is just following vm_guest_mode_string. IMO, defining the string to look like a function is unnecessary and rather mean. > extern const char * const vm_guest_mode_string[]; > +extern const char * const vm_mem_backing_src_type_string[]; > > struct vm_guest_mode_params { > unsigned int pa_bits; > @@ -83,6 +85,7 @@ enum vm_mem_backing_src_type { > VM_MEM_SRC_ANONYMOUS, > VM_MEM_SRC_ANONYMOUS_THP, > VM_MEM_SRC_ANONYMOUS_HUGETLB, > + NUM_VM_BACKING_SRC_TYPES, > }; > > int kvm_check_cap(long cap); > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index fa5a90e6c6f0..a9b651c7f866 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -165,6 +165,14 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = { > _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES, > "Missing new mode params?"); > > +const char * const vm_mem_backing_src_type_string[] = { A shorter name would be nice, though I don't have a good suggestion. > + "VM_MEM_SRC_ANONYMOUS ", > + "VM_MEM_SRC_ANONYMOUS_THP ", > + "VM_MEM_SRC_ANONYMOUS_HUGETLB", It'd be more robust to explicitly assign indices, that way tweaks to vm_mem_backing_src_type won't cause silent breakage. Ditto for the existing vm_guest_mode_string. E.g. I think something like this would work (completely untested) const char *vm_guest_mode_string(int i) { static const char *const strings[] = { [VM_MODE_P52V48_4K] = "PA-bits:52, VA-bits:48, 4K pages", [VM_MODE_P52V48_64K] = "PA-bits:52, VA-bits:48, 64K pages", [VM_MODE_P48V48_4K] = "PA-bits:48, VA-bits:48, 4K pages", [VM_MODE_P48V48_64K] = "PA-bits:48, VA-bits:48, 64K pages", [VM_MODE_P40V48_4K] = "PA-bits:40, VA-bits:48, 4K pages", [VM_MODE_P40V48_64K] = "PA-bits:40, VA-bits:48, 64K pages", [VM_MODE_PXXV48_4K] = "PA-bits:ANY, VA-bits:48, 4K pages", }; _Static_assert(sizeof(strings)/sizeof(char *) == NUM_VM_MODES, "Missing new mode strings?"); TEST_ASSERT(i < NUM_VM_MODES); return strings[i]; } > +}; > +_Static_assert(sizeof(vm_mem_backing_src_type_string)/sizeof(char *) == NUM_VM_BACKING_SRC_TYPES, > + "Missing new source type strings?"); > + > /* > * VM Create > * > -- > 2.23.0 >
Hi Sean, On 2021/2/9 1:43, Sean Christopherson wrote: > On Mon, Feb 08, 2021, Yanan Wang wrote: >> Add a macro to get string of the backing source memory type, so that >> application can add choices for source types in the help() function, >> and users can specify which type to use for testing. >> >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >> --- >> tools/testing/selftests/kvm/include/kvm_util.h | 3 +++ >> tools/testing/selftests/kvm/lib/kvm_util.c | 8 ++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h >> index 5cbb861525ed..f5fc29dc9ee6 100644 >> --- a/tools/testing/selftests/kvm/include/kvm_util.h >> +++ b/tools/testing/selftests/kvm/include/kvm_util.h >> @@ -69,7 +69,9 @@ enum vm_guest_mode { >> #define PTES_PER_MIN_PAGE ptes_per_page(MIN_PAGE_SIZE) >> >> #define vm_guest_mode_string(m) vm_guest_mode_string[m] >> +#define vm_mem_backing_src_type_string(s) vm_mem_backing_src_type_string[s] > Oof, I see this is just following vm_guest_mode_string. IMO, defining the > string to look like a function is unnecessary and rather mean. > >> extern const char * const vm_guest_mode_string[]; >> +extern const char * const vm_mem_backing_src_type_string[]; >> >> struct vm_guest_mode_params { >> unsigned int pa_bits; >> @@ -83,6 +85,7 @@ enum vm_mem_backing_src_type { >> VM_MEM_SRC_ANONYMOUS, >> VM_MEM_SRC_ANONYMOUS_THP, >> VM_MEM_SRC_ANONYMOUS_HUGETLB, >> + NUM_VM_BACKING_SRC_TYPES, >> }; >> >> int kvm_check_cap(long cap); >> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c >> index fa5a90e6c6f0..a9b651c7f866 100644 >> --- a/tools/testing/selftests/kvm/lib/kvm_util.c >> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c >> @@ -165,6 +165,14 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = { >> _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES, >> "Missing new mode params?"); >> >> +const char * const vm_mem_backing_src_type_string[] = { > A shorter name would be nice, though I don't have a good suggestion. > >> + "VM_MEM_SRC_ANONYMOUS ", >> + "VM_MEM_SRC_ANONYMOUS_THP ", >> + "VM_MEM_SRC_ANONYMOUS_HUGETLB", > It'd be more robust to explicitly assign indices, that way tweaks to > vm_mem_backing_src_type won't cause silent breakage. Ditto for the existing > vm_guest_mode_string. > > E.g. I think something like this would work (completely untested) > > const char *vm_guest_mode_string(int i) > { > static const char *const strings[] = { > [VM_MODE_P52V48_4K] = "PA-bits:52, VA-bits:48, 4K pages", > [VM_MODE_P52V48_64K] = "PA-bits:52, VA-bits:48, 64K pages", > [VM_MODE_P48V48_4K] = "PA-bits:48, VA-bits:48, 4K pages", > [VM_MODE_P48V48_64K] = "PA-bits:48, VA-bits:48, 64K pages", > [VM_MODE_P40V48_4K] = "PA-bits:40, VA-bits:48, 4K pages", > [VM_MODE_P40V48_64K] = "PA-bits:40, VA-bits:48, 64K pages", > [VM_MODE_PXXV48_4K] = "PA-bits:ANY, VA-bits:48, 4K pages", > }; > > _Static_assert(sizeof(strings)/sizeof(char *) == NUM_VM_MODES, > "Missing new mode strings?"); > > TEST_ASSERT(i < NUM_VM_MODES); > > return strings[i]; > } I think this is better. Moving these three staffs together into a single function and check the indices here is more reasonable. Thanks, Yanan. > >> +}; >> +_Static_assert(sizeof(vm_mem_backing_src_type_string)/sizeof(char *) == NUM_VM_BACKING_SRC_TYPES, >> + "Missing new source type strings?"); >> + >> /* >> * VM Create >> * >> -- >> 2.23.0 >> > .
On 2021/2/9 2:13, Ben Gardon wrote: > On Mon, Feb 8, 2021 at 1:08 AM Yanan Wang <wangyanan55@huawei.com> wrote: >> Add a macro to get string of the backing source memory type, so that >> application can add choices for source types in the help() function, >> and users can specify which type to use for testing. > Coincidentally, I sent out a change last week to do the same thing: > "KVM: selftests: Add backing src parameter to dirty_log_perf_test" > (https://lkml.org/lkml/2021/2/2/1430) > Whichever way this ends up being implemented, I'm happy to see others > interested in testing different backing source types too. Thanks Ben! I have a little question here. Can we just present three IDs (0/1/2) but not strings for users to choose which backing_src_type to use like the way of guest modes, which I think can make cmdlines more consise and easier to print. And is it better to make a universal API to get backing_src_strings like Sean have suggested, so that the API can be used elsewhere ? >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> >> --- >> tools/testing/selftests/kvm/include/kvm_util.h | 3 +++ >> tools/testing/selftests/kvm/lib/kvm_util.c | 8 ++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h >> index 5cbb861525ed..f5fc29dc9ee6 100644 >> --- a/tools/testing/selftests/kvm/include/kvm_util.h >> +++ b/tools/testing/selftests/kvm/include/kvm_util.h >> @@ -69,7 +69,9 @@ enum vm_guest_mode { >> #define PTES_PER_MIN_PAGE ptes_per_page(MIN_PAGE_SIZE) >> >> #define vm_guest_mode_string(m) vm_guest_mode_string[m] >> +#define vm_mem_backing_src_type_string(s) vm_mem_backing_src_type_string[s] >> extern const char * const vm_guest_mode_string[]; >> +extern const char * const vm_mem_backing_src_type_string[]; >> >> struct vm_guest_mode_params { >> unsigned int pa_bits; >> @@ -83,6 +85,7 @@ enum vm_mem_backing_src_type { >> VM_MEM_SRC_ANONYMOUS, >> VM_MEM_SRC_ANONYMOUS_THP, >> VM_MEM_SRC_ANONYMOUS_HUGETLB, >> + NUM_VM_BACKING_SRC_TYPES, >> }; >> >> int kvm_check_cap(long cap); >> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c >> index fa5a90e6c6f0..a9b651c7f866 100644 >> --- a/tools/testing/selftests/kvm/lib/kvm_util.c >> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c >> @@ -165,6 +165,14 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = { >> _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES, >> "Missing new mode params?"); >> >> +const char * const vm_mem_backing_src_type_string[] = { >> + "VM_MEM_SRC_ANONYMOUS ", >> + "VM_MEM_SRC_ANONYMOUS_THP ", >> + "VM_MEM_SRC_ANONYMOUS_HUGETLB", >> +}; >> +_Static_assert(sizeof(vm_mem_backing_src_type_string)/sizeof(char *) == NUM_VM_BACKING_SRC_TYPES, >> + "Missing new source type strings?"); >> + >> /* >> * VM Create >> * >> -- >> 2.23.0 >> > .
On Tue, Feb 9, 2021 at 3:21 AM wangyanan (Y) <wangyanan55@huawei.com> wrote: > > > On 2021/2/9 2:13, Ben Gardon wrote: > > On Mon, Feb 8, 2021 at 1:08 AM Yanan Wang <wangyanan55@huawei.com> wrote: > >> Add a macro to get string of the backing source memory type, so that > >> application can add choices for source types in the help() function, > >> and users can specify which type to use for testing. > > Coincidentally, I sent out a change last week to do the same thing: > > "KVM: selftests: Add backing src parameter to dirty_log_perf_test" > > (https://lkml.org/lkml/2021/2/2/1430) > > Whichever way this ends up being implemented, I'm happy to see others > > interested in testing different backing source types too. > > Thanks Ben! I have a little question here. > > Can we just present three IDs (0/1/2) but not strings for users to > choose which backing_src_type to use like the way of guest modes, That would be fine with me. The string names are easier for me to read than an ID number (especially if you were to add additional options e.g. 1G hugetlb or file backed / shared memory) but it's mostly an aesthetic preference, so I don't have strong feelings either way. > > which I think can make cmdlines more consise and easier to print. And is > it better to make a universal API to get backing_src_strings > > like Sean have suggested, so that the API can be used elsewhere ? Definitely. This should be as easy as possible to incorporate into all selftests. > > >> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > >> --- > >> tools/testing/selftests/kvm/include/kvm_util.h | 3 +++ > >> tools/testing/selftests/kvm/lib/kvm_util.c | 8 ++++++++ > >> 2 files changed, 11 insertions(+) > >> > >> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h > >> index 5cbb861525ed..f5fc29dc9ee6 100644 > >> --- a/tools/testing/selftests/kvm/include/kvm_util.h > >> +++ b/tools/testing/selftests/kvm/include/kvm_util.h > >> @@ -69,7 +69,9 @@ enum vm_guest_mode { > >> #define PTES_PER_MIN_PAGE ptes_per_page(MIN_PAGE_SIZE) > >> > >> #define vm_guest_mode_string(m) vm_guest_mode_string[m] > >> +#define vm_mem_backing_src_type_string(s) vm_mem_backing_src_type_string[s] > >> extern const char * const vm_guest_mode_string[]; > >> +extern const char * const vm_mem_backing_src_type_string[]; > >> > >> struct vm_guest_mode_params { > >> unsigned int pa_bits; > >> @@ -83,6 +85,7 @@ enum vm_mem_backing_src_type { > >> VM_MEM_SRC_ANONYMOUS, > >> VM_MEM_SRC_ANONYMOUS_THP, > >> VM_MEM_SRC_ANONYMOUS_HUGETLB, > >> + NUM_VM_BACKING_SRC_TYPES, > >> }; > >> > >> int kvm_check_cap(long cap); > >> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > >> index fa5a90e6c6f0..a9b651c7f866 100644 > >> --- a/tools/testing/selftests/kvm/lib/kvm_util.c > >> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > >> @@ -165,6 +165,14 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = { > >> _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES, > >> "Missing new mode params?"); > >> > >> +const char * const vm_mem_backing_src_type_string[] = { > >> + "VM_MEM_SRC_ANONYMOUS ", > >> + "VM_MEM_SRC_ANONYMOUS_THP ", > >> + "VM_MEM_SRC_ANONYMOUS_HUGETLB", > >> +}; > >> +_Static_assert(sizeof(vm_mem_backing_src_type_string)/sizeof(char *) == NUM_VM_BACKING_SRC_TYPES, > >> + "Missing new source type strings?"); > >> + > >> /* > >> * VM Create > >> * > >> -- > >> 2.23.0 > >> > > .
On Tue, Feb 09, 2021, Ben Gardon wrote: > On Tue, Feb 9, 2021 at 3:21 AM wangyanan (Y) <wangyanan55@huawei.com> wrote: > > > > > > On 2021/2/9 2:13, Ben Gardon wrote: > > > On Mon, Feb 8, 2021 at 1:08 AM Yanan Wang <wangyanan55@huawei.com> wrote: > > >> Add a macro to get string of the backing source memory type, so that > > >> application can add choices for source types in the help() function, > > >> and users can specify which type to use for testing. > > > Coincidentally, I sent out a change last week to do the same thing: > > > "KVM: selftests: Add backing src parameter to dirty_log_perf_test" > > > (https://lkml.org/lkml/2021/2/2/1430) > > > Whichever way this ends up being implemented, I'm happy to see others > > > interested in testing different backing source types too. > > > > Thanks Ben! I have a little question here. > > > > Can we just present three IDs (0/1/2) but not strings for users to > > choose which backing_src_type to use like the way of guest modes, > > That would be fine with me. The string names are easier for me to read > than an ID number (especially if you were to add additional options > e.g. 1G hugetlb or file backed / shared memory) but it's mostly an > aesthetic preference, so I don't have strong feelings either way. I vote to expose/consume strings, being able to do ".dirty_log_perf_test --help" and understand the backing options without having to dig into source was super nice.
On 2021/2/10 1:35, Sean Christopherson wrote: > On Tue, Feb 09, 2021, Ben Gardon wrote: >> On Tue, Feb 9, 2021 at 3:21 AM wangyanan (Y) <wangyanan55@huawei.com> wrote: >>> >>> On 2021/2/9 2:13, Ben Gardon wrote: >>>> On Mon, Feb 8, 2021 at 1:08 AM Yanan Wang <wangyanan55@huawei.com> wrote: >>>>> Add a macro to get string of the backing source memory type, so that >>>>> application can add choices for source types in the help() function, >>>>> and users can specify which type to use for testing. >>>> Coincidentally, I sent out a change last week to do the same thing: >>>> "KVM: selftests: Add backing src parameter to dirty_log_perf_test" >>>> (https://lkml.org/lkml/2021/2/2/1430) >>>> Whichever way this ends up being implemented, I'm happy to see others >>>> interested in testing different backing source types too. >>> Thanks Ben! I have a little question here. >>> >>> Can we just present three IDs (0/1/2) but not strings for users to >>> choose which backing_src_type to use like the way of guest modes, >> That would be fine with me. The string names are easier for me to read >> than an ID number (especially if you were to add additional options >> e.g. 1G hugetlb or file backed / shared memory) but it's mostly an >> aesthetic preference, so I don't have strong feelings either way. > I vote to expose/consume strings, being able to do ".dirty_log_perf_test --help" > and understand the backing options without having to dig into source was super > nice. > . Fine then:), I will make some change based on (https://lkml.org/lkml/2021/2/2/1430), thanks!
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index 5cbb861525ed..f5fc29dc9ee6 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -69,7 +69,9 @@ enum vm_guest_mode { #define PTES_PER_MIN_PAGE ptes_per_page(MIN_PAGE_SIZE) #define vm_guest_mode_string(m) vm_guest_mode_string[m] +#define vm_mem_backing_src_type_string(s) vm_mem_backing_src_type_string[s] extern const char * const vm_guest_mode_string[]; +extern const char * const vm_mem_backing_src_type_string[]; struct vm_guest_mode_params { unsigned int pa_bits; @@ -83,6 +85,7 @@ enum vm_mem_backing_src_type { VM_MEM_SRC_ANONYMOUS, VM_MEM_SRC_ANONYMOUS_THP, VM_MEM_SRC_ANONYMOUS_HUGETLB, + NUM_VM_BACKING_SRC_TYPES, }; int kvm_check_cap(long cap); diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index fa5a90e6c6f0..a9b651c7f866 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -165,6 +165,14 @@ const struct vm_guest_mode_params vm_guest_mode_params[] = { _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES, "Missing new mode params?"); +const char * const vm_mem_backing_src_type_string[] = { + "VM_MEM_SRC_ANONYMOUS ", + "VM_MEM_SRC_ANONYMOUS_THP ", + "VM_MEM_SRC_ANONYMOUS_HUGETLB", +}; +_Static_assert(sizeof(vm_mem_backing_src_type_string)/sizeof(char *) == NUM_VM_BACKING_SRC_TYPES, + "Missing new source type strings?"); + /* * VM Create *
Add a macro to get string of the backing source memory type, so that application can add choices for source types in the help() function, and users can specify which type to use for testing. Signed-off-by: Yanan Wang <wangyanan55@huawei.com> --- tools/testing/selftests/kvm/include/kvm_util.h | 3 +++ tools/testing/selftests/kvm/lib/kvm_util.c | 8 ++++++++ 2 files changed, 11 insertions(+)