| Message ID | 20170912141658.8146-1-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/09/2017 16:16, David Hildenbrand wrote: > I want to use this in another file, so instead of replicating, factoring > it out feels like the right thing to do. > > Let's directly provide it for all architectures, they just have > to implement get_time_ms() in asm/time.h to use it. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > > The next step would be allowing to add custom parameters to a test. > (defining them similar to the test cases). > > Any other requests/nice to have? Does this make sense? It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with not exactly overlapping usecases and command-line syntax. Would it make sense to have an API that is not home-grown, e.g. a subset of GTest? The command-line arguments could be globs (like GTest's "-p"). Paolo > lib/s390x/asm/time.h | 24 ++++++++++++ > lib/testrun.c | 89 ++++++++++++++++++++++++++++++++++++++++++ > lib/testrun.h | 41 ++++++++++++++++++++ > s390x/Makefile | 1 + > s390x/intercept.c | 106 ++++++++------------------------------------------- > 5 files changed, 170 insertions(+), 91 deletions(-) > create mode 100644 lib/s390x/asm/time.h > create mode 100644 lib/testrun.c > create mode 100644 lib/testrun.h > > diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h > new file mode 100644 > index 0000000..856facd > --- /dev/null > +++ b/lib/s390x/asm/time.h > @@ -0,0 +1,24 @@ > +/* > + * Copyright (c) 2017 Red Hat Inc > + * > + * Authors: > + * Thomas Huth <thuth@redhat.com> > + * David Hildenbrand <david@redhat.com> > + * > + * This code is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Library General Public License version 2. > + */ > +#ifndef __ASMS390X_TIME_H > +#define __ASMS390X_TIME_H > + > +static inline uint64_t get_clock_ms(void) > +{ > + uint64_t clk; > + > + asm volatile(" stck %0 " : : "Q"(clk) : "memory"); > + > + /* Bit 51 is incrememented each microsecond */ > + return (clk >> (63 - 51)) / 1000; > +} > + > +#endif > diff --git a/lib/testrun.c b/lib/testrun.c > new file mode 100644 > index 0000000..6034767 > --- /dev/null > +++ b/lib/testrun.c > @@ -0,0 +1,89 @@ > +/* > + * Utility functions for running/configuring a set of tests > + * > + * Copyright (c) 2017 Red Hat Inc > + * > + * Authors: > + * Thomas Huth <thuth@redhat.com> > + * David Hildenbrand <david@redhat.com> > + * > + * This code is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Library General Public License version 2. > + */ > + > +#include <testrun.h> > +#include <asm/time.h> > + > +void parse_test_args(const int argc, const char **argv, struct test *test) > +{ > + bool run_all = true; > + int i, ti; > + > + test->iterations = 0; > + test->time_to_run = 0; > + > + for (i = 1; i < argc; i++) { > + if (!strcmp("-i", argv[i])) { > + i++; > + if (i >= argc) > + report_abort("-i needs a parameter"); > + test->iterations = atol(argv[i]); > + } else if (!strcmp("-t", argv[i])) { > + i++; > + if (i >= argc) > + report_abort("-t needs a parameter"); > + test->time_to_run = atol(argv[i]); > + } else for (ti = 0; test->test_cases[ti].name != NULL; ti++) { > + if (!strcmp(test->test_cases[ti].name, argv[i])) { > + run_all = false; > + test->test_cases[ti].run_it = true; > + break; > + } else if (test->test_cases[ti + 1].name == NULL) { > + report_abort("Unsupported parameter '%s'", > + argv[i]); > + } > + } > + } > + > + if (test->iterations == 0 && test->time_to_run == 0) > + test->iterations = 1; > + > + if ((test->iterations > 1 || test->time_to_run) && > + !test->multirun_supported) > + report_abort("Test does not support multiruns."); > + > + if (run_all) { > + for (ti = 0; test->test_cases[ti].name != NULL; ti++) > + test->test_cases[ti].run_it = true; > + } > +} > + > +int run_test(struct test *test) > +{ > + unsigned long iterations = test->iterations; > + unsigned long time_to_run = test->time_to_run; > + uint64_t startclk; > + int ti; > + > + report_prefix_push(test->name); > + startclk = get_clock_ms(); > + for (;;) { > + for (ti = 0; test->test_cases[ti].name != NULL; ti++) { > + report_prefix_push(test->test_cases[ti].name); > + if (test->test_cases[ti].run_it) > + test->test_cases[ti].func(); > + report_prefix_pop(); > + } > + if (iterations) { > + iterations -= 1; > + if (iterations == 0) > + break; > + } > + if (time_to_run) { > + if (get_clock_ms() - startclk > time_to_run) > + break; > + } > + } > + report_prefix_pop(); > + return report_summary(); > +} > diff --git a/lib/testrun.h b/lib/testrun.h > new file mode 100644 > index 0000000..8f9a2d7 > --- /dev/null > +++ b/lib/testrun.h > @@ -0,0 +1,41 @@ > +/* > + * Utility functions for running/configuring a set of test cases > + * > + * Copyright (c) 2017 Red Hat Inc > + * > + * Authors: > + * Thomas Huth <thuth@redhat.com> > + * David Hildenbrand <david@redhat.com> > + * > + * This code is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Library General Public License version 2. > + */ > +#ifndef __TESTRUN_H > +#define __TESTRUN_H > + > +#include <libcflat.h> > + > +struct test_case { > + const char *name; > + void (*func)(void); > + /* initialized by parse_test_args() */ > + bool run_it; > +}; > + > +#define DEFINE_TEST_CASE(_name, _func) { _name, _func, false } > +#define DEFINE_TEST_CASE_END_OF_LIST() { } > + > +struct test { > + /* to be initialized by the test */ > + const char *name; > + bool multirun_supported; > + struct test_case *test_cases; > + /* initialized by parse_test_args() */ > + unsigned long iterations; > + unsigned long time_to_run; > +}; > + > +void parse_test_args(int argc, const char **argv, struct test *test); > +int run_test(struct test *test); > + > +#endif /* __TESTRUN_H */ > diff --git a/s390x/Makefile b/s390x/Makefile > index bc099da..e174895 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -21,6 +21,7 @@ include $(SRCDIR)/scripts/asm-offsets.mak > > cflatobjs += lib/util.o > cflatobjs += lib/alloc.o > +cflatobjs += lib/testrun.o > cflatobjs += lib/s390x/io.o > cflatobjs += lib/s390x/stack.o > cflatobjs += lib/s390x/sclp-ascii.o > diff --git a/s390x/intercept.c b/s390x/intercept.c > index 59e5fca..15e50f6 100644 > --- a/s390x/intercept.c > +++ b/s390x/intercept.c > @@ -10,15 +10,13 @@ > * under the terms of the GNU Library General Public License version 2. > */ > #include <libcflat.h> > +#include <testrun.h> > #include <asm/asm-offsets.h> > #include <asm/interrupt.h> > #include <asm/page.h> > > static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2))); > > -static unsigned long nr_iterations; > -static unsigned long time_to_run; > - > /* Test the STORE PREFIX instruction */ > static void test_stpx(void) > { > @@ -159,95 +157,21 @@ static void test_testblock(void) > check_pgm_int_code(PGM_INT_CODE_ADDRESSING); > } > > -static uint64_t get_clock_ms(void) > -{ > - uint64_t clk; > - > - asm volatile(" stck %0 " : : "Q"(clk) : "memory"); > - > - /* Bit 51 is incrememented each microsecond */ > - return (clk >> (63 - 51)) / 1000; > -} > - > -struct { > - const char *name; > - void (*func)(void); > - bool run_it; > -} tests[] = { > - { "stpx", test_stpx, false }, > - { "spx", test_spx, false }, > - { "stap", test_stap, false }, > - { "stidp", test_stidp, false }, > - { "testblock", test_testblock, false }, > - { NULL, NULL, false } > -}; > - > -static void parse_intercept_test_args(int argc, char **argv) > -{ > - int i, ti; > - bool run_all = true; > - > - for (i = 1; i < argc; i++) { > - if (!strcmp("-i", argv[i])) { > - i++; > - if (i >= argc) > - report_abort("-i needs a parameter"); > - nr_iterations = atol(argv[i]); > - } else if (!strcmp("-t", argv[i])) { > - i++; > - if (i >= argc) > - report_abort("-t needs a parameter"); > - time_to_run = atol(argv[i]); > - } else for (ti = 0; tests[ti].name != NULL; ti++) { > - if (!strcmp(tests[ti].name, argv[i])) { > - run_all = false; > - tests[ti].run_it = true; > - break; > - } else if (tests[ti + 1].name == NULL) { > - report_abort("Unsupported parameter '%s'", > - argv[i]); > - } > - } > - } > - > - if (run_all) { > - for (ti = 0; tests[ti].name != NULL; ti++) > - tests[ti].run_it = true; > +static struct test test = { > + .name = "intercept", > + .multirun_supported = true, > + .test_cases = (struct test_case[]) { > + DEFINE_TEST_CASE("stpx", test_stpx), > + DEFINE_TEST_CASE("spx", test_spx), > + DEFINE_TEST_CASE("stap", test_stap), > + DEFINE_TEST_CASE("stidp", test_stidp), > + DEFINE_TEST_CASE("testblock", test_testblock), > + DEFINE_TEST_CASE_END_OF_LIST() > } > -} > +}; > > -int main(int argc, char **argv) > +int main(int argc, const char **argv) > { > - uint64_t startclk; > - int ti; > - > - parse_intercept_test_args(argc, argv); > - > - if (nr_iterations == 0 && time_to_run == 0) > - nr_iterations = 1; > - > - report_prefix_push("intercept"); > - > - startclk = get_clock_ms(); > - for (;;) { > - for (ti = 0; tests[ti].name != NULL; ti++) { > - report_prefix_push(tests[ti].name); > - if (tests[ti].run_it) > - tests[ti].func(); > - report_prefix_pop(); > - } > - if (nr_iterations) { > - nr_iterations -= 1; > - if (nr_iterations == 0) > - break; > - } > - if (time_to_run) { > - if (get_clock_ms() - startclk > time_to_run) > - break; > - } > - } > - > - report_prefix_pop(); > - > - return report_summary(); > + parse_test_args(argc, argv, &test); > + return run_test(&test); > } >
On 12.09.2017 17:01, Paolo Bonzini wrote: > On 12/09/2017 16:16, David Hildenbrand wrote: >> I want to use this in another file, so instead of replicating, factoring >> it out feels like the right thing to do. >> >> Let's directly provide it for all architectures, they just have >> to implement get_time_ms() in asm/time.h to use it. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> >> The next step would be allowing to add custom parameters to a test. >> (defining them similar to the test cases). >> >> Any other requests/nice to have? Does this make sense? > > It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with > not exactly overlapping usecases and command-line syntax. Would it make Yes, the ultimate goal would be to combine all into one. I only had a look at some ARM tests yet. Will have a look at these. > sense to have an API that is not home-grown, e.g. a subset of GTest? > The command-line arguments could be globs (like GTest's "-p"). I'll have a look. Any experts around? > > Paolo
On 12/09/2017 17:15, David Hildenbrand wrote: > On 12.09.2017 17:01, Paolo Bonzini wrote: >> On 12/09/2017 16:16, David Hildenbrand wrote: >>> I want to use this in another file, so instead of replicating, factoring >>> it out feels like the right thing to do. >>> >>> Let's directly provide it for all architectures, they just have >>> to implement get_time_ms() in asm/time.h to use it. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> >>> The next step would be allowing to add custom parameters to a test. >>> (defining them similar to the test cases). >>> >>> Any other requests/nice to have? Does this make sense? >> >> It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with >> not exactly overlapping usecases and command-line syntax. Would it make > > Yes, the ultimate goal would be to combine all into one. I only had a > look at some ARM tests yet. Will have a look at these. > >> sense to have an API that is not home-grown, e.g. a subset of GTest? >> The command-line arguments could be globs (like GTest's "-p"). > > I'll have a look. Any experts around? Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html. But... one obstacle is probably the lack of malloc in x86, so this would be a prerequisite. Andrew first proposed it last year, and there was some discussion on the design and how to abstract giving more memory to malloc. You can find the result here: https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, but it should be fun to implement. :) The idea is to have four allocator APIs: - "phys" (phys_alloc) provides contiguous physical addresses and is used in early allocations or for tests that do not enable the MMU. It need not be able to free anything. It also provides an arch-dependent interface to register the available memory. - "page" (alloc_page) is initialized after the MMU is started. It picks whatever memory wasn't allocated by phys, and slices it in pages. Like "phys", it provides contiguous physical addresses. - "virt" (alloc_vpages) is also initialized after the MMU is started, and it allocates virtual address space. It also provides an arch-dependent interface to install PTEs. It provides contiguous virtual addresses. - "malloc" is the API we all love :) and it uses a pluggable allocator underneath, called "morecore" in the thread, with the prototype void *(*morecore)(size_t size, size_t align_min); In the beginning "morecore" simply uses "phys", later it is switched (when the MMU is started) to use both "page" and "virt". That is it takes not necessarily consecutive physical memory at page granularity from "page", it assigning consecutive virtual address (from "virt") to it, and if needed it also slices pages into smaller allocations. Of course each could be arbitrarily complicated, but it need not be. "page" could be a buddy system, but really in practice it need not even support reusing freed pages, except for 1-page allocations. This makes it enough to use a simple free list of pages, plus a separately-tracked huge block of consecutive free physical addresses at the top. Likewise, "malloc" could be Doug Lea's malloc, but really in practice it will just be a small veneer over morecore, with a dummy free. Like the testing thing, the important part is avoiding under-engineering. Quoting from the thread, you can choose separately at each layer whether to implement freeing or not. "phys", "virt" and "morecore" do not need it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to transform Drew's v2 series (https://www.spinics.net/lists/kvm/msg139685.html) into the design envisioned above. Paolo
On 12.09.2017 17:46, Paolo Bonzini wrote: > On 12/09/2017 17:15, David Hildenbrand wrote: >> On 12.09.2017 17:01, Paolo Bonzini wrote: >>> On 12/09/2017 16:16, David Hildenbrand wrote: >>>> I want to use this in another file, so instead of replicating, factoring >>>> it out feels like the right thing to do. >>>> >>>> Let's directly provide it for all architectures, they just have >>>> to implement get_time_ms() in asm/time.h to use it. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> >>>> The next step would be allowing to add custom parameters to a test. >>>> (defining them similar to the test cases). >>>> >>>> Any other requests/nice to have? Does this make sense? >>> >>> It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with >>> not exactly overlapping usecases and command-line syntax. Would it make >> >> Yes, the ultimate goal would be to combine all into one. I only had a >> look at some ARM tests yet. Will have a look at these. >> >>> sense to have an API that is not home-grown, e.g. a subset of GTest? >>> The command-line arguments could be globs (like GTest's "-p"). >> >> I'll have a look. Any experts around? > > Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html. > > But... one obstacle is probably the lack of malloc in x86, so this would > be a prerequisite. Andrew first proposed it last year, and there was > some discussion on the design and how to abstract giving more memory to > malloc. You can find the result here: > https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, > but it should be fun to implement. :) > > The idea is to have four allocator APIs: > > - "phys" (phys_alloc) provides contiguous physical addresses and is used > in early allocations or for tests that do not enable the MMU. It need > not be able to free anything. It also provides an arch-dependent > interface to register the available memory. > > - "page" (alloc_page) is initialized after the MMU is started. It picks > whatever memory wasn't allocated by phys, and slices it in pages. Like > "phys", it provides contiguous physical addresses. > > - "virt" (alloc_vpages) is also initialized after the MMU is started, > and it allocates virtual address space. It also provides an > arch-dependent interface to install PTEs. It provides contiguous > virtual addresses. > > - "malloc" is the API we all love :) and it uses a pluggable allocator > underneath, called "morecore" in the thread, with the prototype > > void *(*morecore)(size_t size, size_t align_min); > > In the beginning "morecore" simply uses "phys", later it is switched > (when the MMU is started) to use both "page" and "virt". That is it > takes not necessarily consecutive physical memory at page > granularity from "page", it assigning consecutive virtual address (from > "virt") to it, and if needed it also slices pages into smaller allocations. > > Of course each could be arbitrarily complicated, but it need not be. > > "page" could be a buddy system, but really in practice it need not even > support reusing freed pages, except for 1-page allocations. This makes > it enough to use a simple free list of pages, plus a separately-tracked > huge block of consecutive free physical addresses at the top. > > Likewise, "malloc" could be Doug Lea's malloc, but really in practice it > will just be a small veneer over morecore, with a dummy free. Like the > testing thing, the important part is avoiding under-engineering. > > Quoting from the thread, you can choose separately at each layer whether > to implement freeing or not. "phys", "virt" and "morecore" do not need > it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to > transform Drew's v2 series > (https://www.spinics.net/lists/kvm/msg139685.html) into the design > envisioned above. > > Paolo > Holy cow, that sounds like a lot of work, especially as malloc is also not expected to work on s390x - guess because of which architecture I started to factor out ;) phys_alloc_init() yields only arm and powerpc. ... so while having GTest would be the optimal solution, I wonder if it is worth the trouble right now. (sure, malloc for x86 and s390x would also be nice, but already getting that implemented sounds like it could take quite a while). Hmm.... probably have too look into the details of the malloc discussion...
On 12/09/2017 18:17, David Hildenbrand wrote: > On 12.09.2017 17:46, Paolo Bonzini wrote: >> On 12/09/2017 17:15, David Hildenbrand wrote: >>> On 12.09.2017 17:01, Paolo Bonzini wrote: >>>> On 12/09/2017 16:16, David Hildenbrand wrote: >>>>> I want to use this in another file, so instead of replicating, factoring >>>>> it out feels like the right thing to do. >>>>> >>>>> Let's directly provide it for all architectures, they just have >>>>> to implement get_time_ms() in asm/time.h to use it. >>>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> >>>>> The next step would be allowing to add custom parameters to a test. >>>>> (defining them similar to the test cases). >>>>> >>>>> Any other requests/nice to have? Does this make sense? >>>> >>>> It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with >>>> not exactly overlapping usecases and command-line syntax. Would it make >>> >>> Yes, the ultimate goal would be to combine all into one. I only had a >>> look at some ARM tests yet. Will have a look at these. >>> >>>> sense to have an API that is not home-grown, e.g. a subset of GTest? >>>> The command-line arguments could be globs (like GTest's "-p"). >>> >>> I'll have a look. Any experts around? >> >> Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html. >> >> But... one obstacle is probably the lack of malloc in x86, so this would >> be a prerequisite. Andrew first proposed it last year, and there was >> some discussion on the design and how to abstract giving more memory to >> malloc. You can find the result here: >> https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, >> but it should be fun to implement. :) >> >> The idea is to have four allocator APIs: >> >> - "phys" (phys_alloc) provides contiguous physical addresses and is used >> in early allocations or for tests that do not enable the MMU. It need >> not be able to free anything. It also provides an arch-dependent >> interface to register the available memory. >> >> - "page" (alloc_page) is initialized after the MMU is started. It picks >> whatever memory wasn't allocated by phys, and slices it in pages. Like >> "phys", it provides contiguous physical addresses. >> >> - "virt" (alloc_vpages) is also initialized after the MMU is started, >> and it allocates virtual address space. It also provides an >> arch-dependent interface to install PTEs. It provides contiguous >> virtual addresses. >> >> - "malloc" is the API we all love :) and it uses a pluggable allocator >> underneath, called "morecore" in the thread, with the prototype >> >> void *(*morecore)(size_t size, size_t align_min); >> >> In the beginning "morecore" simply uses "phys", later it is switched >> (when the MMU is started) to use both "page" and "virt". That is it >> takes not necessarily consecutive physical memory at page >> granularity from "page", it assigning consecutive virtual address (from >> "virt") to it, and if needed it also slices pages into smaller allocations. >> >> Of course each could be arbitrarily complicated, but it need not be. >> >> "page" could be a buddy system, but really in practice it need not even >> support reusing freed pages, except for 1-page allocations. This makes >> it enough to use a simple free list of pages, plus a separately-tracked >> huge block of consecutive free physical addresses at the top. >> >> Likewise, "malloc" could be Doug Lea's malloc, but really in practice it >> will just be a small veneer over morecore, with a dummy free. Like the >> testing thing, the important part is avoiding under-engineering. >> >> Quoting from the thread, you can choose separately at each layer whether >> to implement freeing or not. "phys", "virt" and "morecore" do not need >> it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to >> transform Drew's v2 series >> (https://www.spinics.net/lists/kvm/msg139685.html) into the design >> envisioned above. > > Holy cow, that sounds like a lot of work, especially as malloc is also > not expected to work on s390x - guess because of which architecture I > started to factor out ;) > > phys_alloc_init() yields only arm and powerpc. > > ... so while having GTest would be the optimal solution, I wonder if it > is worth the trouble right now. (sure, malloc for x86 and s390x would > also be nice, but already getting that implemented sounds like it could > take quite a while). > > Hmm.... probably have too look into the details of the malloc discussion... It's really less work than it sounds. I'm not wed to GTest, but I think the ability to define test cases programmatically is useful (vmexit.c for example has a case where the testcase list is actually in QEMU!) and it pretty much requires malloc. Paolo
On 12.09.2017 18:24, Paolo Bonzini wrote: > On 12/09/2017 18:17, David Hildenbrand wrote: >> On 12.09.2017 17:46, Paolo Bonzini wrote: >>> On 12/09/2017 17:15, David Hildenbrand wrote: >>>> On 12.09.2017 17:01, Paolo Bonzini wrote: >>>>> On 12/09/2017 16:16, David Hildenbrand wrote: >>>>>> I want to use this in another file, so instead of replicating, factoring >>>>>> it out feels like the right thing to do. >>>>>> >>>>>> Let's directly provide it for all architectures, they just have >>>>>> to implement get_time_ms() in asm/time.h to use it. >>>>>> >>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>>> --- >>>>>> >>>>>> The next step would be allowing to add custom parameters to a test. >>>>>> (defining them similar to the test cases). >>>>>> >>>>>> Any other requests/nice to have? Does this make sense? >>>>> >>>>> It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with >>>>> not exactly overlapping usecases and command-line syntax. Would it make >>>> >>>> Yes, the ultimate goal would be to combine all into one. I only had a >>>> look at some ARM tests yet. Will have a look at these. >>>> >>>>> sense to have an API that is not home-grown, e.g. a subset of GTest? >>>>> The command-line arguments could be globs (like GTest's "-p"). >>>> >>>> I'll have a look. Any experts around? >>> >>> Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html. >>> >>> But... one obstacle is probably the lack of malloc in x86, so this would >>> be a prerequisite. Andrew first proposed it last year, and there was >>> some discussion on the design and how to abstract giving more memory to >>> malloc. You can find the result here: >>> https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, >>> but it should be fun to implement. :) >>> >>> The idea is to have four allocator APIs: >>> >>> - "phys" (phys_alloc) provides contiguous physical addresses and is used >>> in early allocations or for tests that do not enable the MMU. It need >>> not be able to free anything. It also provides an arch-dependent >>> interface to register the available memory. >>> >>> - "page" (alloc_page) is initialized after the MMU is started. It picks >>> whatever memory wasn't allocated by phys, and slices it in pages. Like >>> "phys", it provides contiguous physical addresses. >>> >>> - "virt" (alloc_vpages) is also initialized after the MMU is started, >>> and it allocates virtual address space. It also provides an >>> arch-dependent interface to install PTEs. It provides contiguous >>> virtual addresses. >>> >>> - "malloc" is the API we all love :) and it uses a pluggable allocator >>> underneath, called "morecore" in the thread, with the prototype >>> >>> void *(*morecore)(size_t size, size_t align_min); >>> >>> In the beginning "morecore" simply uses "phys", later it is switched >>> (when the MMU is started) to use both "page" and "virt". That is it >>> takes not necessarily consecutive physical memory at page >>> granularity from "page", it assigning consecutive virtual address (from >>> "virt") to it, and if needed it also slices pages into smaller allocations. >>> >>> Of course each could be arbitrarily complicated, but it need not be. >>> >>> "page" could be a buddy system, but really in practice it need not even >>> support reusing freed pages, except for 1-page allocations. This makes >>> it enough to use a simple free list of pages, plus a separately-tracked >>> huge block of consecutive free physical addresses at the top. >>> >>> Likewise, "malloc" could be Doug Lea's malloc, but really in practice it >>> will just be a small veneer over morecore, with a dummy free. Like the >>> testing thing, the important part is avoiding under-engineering. >>> >>> Quoting from the thread, you can choose separately at each layer whether >>> to implement freeing or not. "phys", "virt" and "morecore" do not need >>> it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to >>> transform Drew's v2 series >>> (https://www.spinics.net/lists/kvm/msg139685.html) into the design >>> envisioned above. >> >> Holy cow, that sounds like a lot of work, especially as malloc is also >> not expected to work on s390x - guess because of which architecture I >> started to factor out ;) >> >> phys_alloc_init() yields only arm and powerpc. >> >> ... so while having GTest would be the optimal solution, I wonder if it >> is worth the trouble right now. (sure, malloc for x86 and s390x would >> also be nice, but already getting that implemented sounds like it could >> take quite a while). >> >> Hmm.... probably have too look into the details of the malloc discussion... > > It's really less work than it sounds. I'm not wed to GTest, but I think > the ability to define test cases programmatically is useful (vmexit.c I agree, as I said I would also like to use that. But at least at the first sight this really looks like a lot of work. > for example has a case where the testcase list is actually in QEMU!) and > it pretty much requires malloc. > > Paolo > gtestutils.c also uses g_timer() ... this will be fun :)
On 12/09/2017 18:27, David Hildenbrand wrote: > On 12.09.2017 18:24, Paolo Bonzini wrote: >> On 12/09/2017 18:17, David Hildenbrand wrote: >>> On 12.09.2017 17:46, Paolo Bonzini wrote: >>>> On 12/09/2017 17:15, David Hildenbrand wrote: >>>>> On 12.09.2017 17:01, Paolo Bonzini wrote: >>>>>> On 12/09/2017 16:16, David Hildenbrand wrote: >>>>>>> I want to use this in another file, so instead of replicating, factoring >>>>>>> it out feels like the right thing to do. >>>>>>> >>>>>>> Let's directly provide it for all architectures, they just have >>>>>>> to implement get_time_ms() in asm/time.h to use it. >>>>>>> >>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>>>> --- >>>>>>> >>>>>>> The next step would be allowing to add custom parameters to a test. >>>>>>> (defining them similar to the test cases). >>>>>>> >>>>>>> Any other requests/nice to have? Does this make sense? >>>>>> >>>>>> It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with >>>>>> not exactly overlapping usecases and command-line syntax. Would it make >>>>> >>>>> Yes, the ultimate goal would be to combine all into one. I only had a >>>>> look at some ARM tests yet. Will have a look at these. >>>>> >>>>>> sense to have an API that is not home-grown, e.g. a subset of GTest? >>>>>> The command-line arguments could be globs (like GTest's "-p"). >>>>> >>>>> I'll have a look. Any experts around? >>>> >>>> Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html. >>>> >>>> But... one obstacle is probably the lack of malloc in x86, so this would >>>> be a prerequisite. Andrew first proposed it last year, and there was >>>> some discussion on the design and how to abstract giving more memory to >>>> malloc. You can find the result here: >>>> https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, >>>> but it should be fun to implement. :) >>>> >>>> The idea is to have four allocator APIs: >>>> >>>> - "phys" (phys_alloc) provides contiguous physical addresses and is used >>>> in early allocations or for tests that do not enable the MMU. It need >>>> not be able to free anything. It also provides an arch-dependent >>>> interface to register the available memory. >>>> >>>> - "page" (alloc_page) is initialized after the MMU is started. It picks >>>> whatever memory wasn't allocated by phys, and slices it in pages. Like >>>> "phys", it provides contiguous physical addresses. >>>> >>>> - "virt" (alloc_vpages) is also initialized after the MMU is started, >>>> and it allocates virtual address space. It also provides an >>>> arch-dependent interface to install PTEs. It provides contiguous >>>> virtual addresses. >>>> >>>> - "malloc" is the API we all love :) and it uses a pluggable allocator >>>> underneath, called "morecore" in the thread, with the prototype >>>> >>>> void *(*morecore)(size_t size, size_t align_min); >>>> >>>> In the beginning "morecore" simply uses "phys", later it is switched >>>> (when the MMU is started) to use both "page" and "virt". That is it >>>> takes not necessarily consecutive physical memory at page >>>> granularity from "page", it assigning consecutive virtual address (from >>>> "virt") to it, and if needed it also slices pages into smaller allocations. >>>> >>>> Of course each could be arbitrarily complicated, but it need not be. >>>> >>>> "page" could be a buddy system, but really in practice it need not even >>>> support reusing freed pages, except for 1-page allocations. This makes >>>> it enough to use a simple free list of pages, plus a separately-tracked >>>> huge block of consecutive free physical addresses at the top. >>>> >>>> Likewise, "malloc" could be Doug Lea's malloc, but really in practice it >>>> will just be a small veneer over morecore, with a dummy free. Like the >>>> testing thing, the important part is avoiding under-engineering. >>>> >>>> Quoting from the thread, you can choose separately at each layer whether >>>> to implement freeing or not. "phys", "virt" and "morecore" do not need >>>> it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to >>>> transform Drew's v2 series >>>> (https://www.spinics.net/lists/kvm/msg139685.html) into the design >>>> envisioned above. >>> >>> Holy cow, that sounds like a lot of work, especially as malloc is also >>> not expected to work on s390x - guess because of which architecture I >>> started to factor out ;) >>> >>> phys_alloc_init() yields only arm and powerpc. >>> >>> ... so while having GTest would be the optimal solution, I wonder if it >>> is worth the trouble right now. (sure, malloc for x86 and s390x would >>> also be nice, but already getting that implemented sounds like it could >>> take quite a while). >>> >>> Hmm.... probably have too look into the details of the malloc discussion... >> >> It's really less work than it sounds. I'm not wed to GTest, but I think >> the ability to define test cases programmatically is useful (vmexit.c > > I agree, as I said I would also like to use that. But at least at the > first sight this really looks like a lot of work. > >> for example has a case where the testcase list is actually in QEMU!) and >> it pretty much requires malloc. > > gtestutils.c also uses g_timer() ... this will be fun :) You don't need to implement the full API. Just g_test_{init,add*,run} would be enough for a start. Paolo
>> Hmm.... probably have too look into the details of the malloc discussion... > > It's really less work than it sounds. I'm not wed to GTest, but I think > the ability to define test cases programmatically is useful (vmexit.c > for example has a case where the testcase list is actually in QEMU!) and > it pretty much requires malloc. Btw, by "has a case where the testcase list is actually in QEMU" you mean simply selecting the list of tests to run via -apend? This is already supported in this current RFC, and the way Thomas implemented it doesn't require malloc - see "run_it". (it's not 100% clean but for our purpose just ok). > > Paolo >
On 12/09/2017 18:39, David Hildenbrand wrote: > >>> Hmm.... probably have too look into the details of the malloc discussion... >> >> It's really less work than it sounds. I'm not wed to GTest, but I think >> the ability to define test cases programmatically is useful (vmexit.c >> for example has a case where the testcase list is actually in QEMU!) and >> it pretty much requires malloc. > > Btw, by "has a case where the testcase list is actually in QEMU" you > mean simply selecting the list of tests to run via -apend? No, QEMU has a test device that can provide the names of several testcases. So you could have a test named "name_of_test1" but name_of_test1 is actually not in kvm-unit-tests, it is in QEMU. Paolo > This is already supported in this current RFC, and the way Thomas > implemented it doesn't require malloc - see "run_it". (it's not 100% > clean but for our purpose just ok). > > >> >> Paolo >> > >
On 12.09.2017 18:43, Paolo Bonzini wrote: > On 12/09/2017 18:39, David Hildenbrand wrote: >> >>>> Hmm.... probably have too look into the details of the malloc discussion... >>> >>> It's really less work than it sounds. I'm not wed to GTest, but I think >>> the ability to define test cases programmatically is useful (vmexit.c >>> for example has a case where the testcase list is actually in QEMU!) and >>> it pretty much requires malloc. >> >> Btw, by "has a case where the testcase list is actually in QEMU" you >> mean simply selecting the list of tests to run via -apend? > > No, QEMU has a test device that can provide the names of several > testcases. So you could have a test named "name_of_test1" but > name_of_test1 is actually not in kvm-unit-tests, it is in QEMU. > > Paolo > >> This is already supported in this current RFC, and the way Thomas >> implemented it doesn't require malloc - see "run_it". (it's not 100% >> clean but for our purpose just ok). >> pci-testdev (I learned something today :)). Got it, basically dynamically adding tests.
On Tue, Sep 12, 2017 at 05:46:48PM +0200, Paolo Bonzini wrote: > On 12/09/2017 17:15, David Hildenbrand wrote: > > On 12.09.2017 17:01, Paolo Bonzini wrote: > >> On 12/09/2017 16:16, David Hildenbrand wrote: > >>> I want to use this in another file, so instead of replicating, factoring > >>> it out feels like the right thing to do. > >>> > >>> Let's directly provide it for all architectures, they just have > >>> to implement get_time_ms() in asm/time.h to use it. > >>> > >>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>> --- > >>> > >>> The next step would be allowing to add custom parameters to a test. > >>> (defining them similar to the test cases). > >>> > >>> Any other requests/nice to have? Does this make sense? > >> > >> It does, but we have at least this, vmx.c (v1 and v2) and vmexit.c, with > >> not exactly overlapping usecases and command-line syntax. Would it make > > > > Yes, the ultimate goal would be to combine all into one. I only had a > > look at some ARM tests yet. Will have a look at these. > > > >> sense to have an API that is not home-grown, e.g. a subset of GTest? > >> The command-line arguments could be globs (like GTest's "-p"). > > > > I'll have a look. Any experts around? > > Take a look at https://developer.gnome.org/glib/stable/glib-Testing.html. > > But... one obstacle is probably the lack of malloc in x86, so this would > be a prerequisite. Andrew first proposed it last year, and there was > some discussion on the design and how to abstract giving more memory to > malloc. You can find the result here: > https://patchwork.kernel.org/patch/9409843/ -- it's a longish thread, > but it should be fun to implement. :) > > The idea is to have four allocator APIs: > > - "phys" (phys_alloc) provides contiguous physical addresses and is used > in early allocations or for tests that do not enable the MMU. It need > not be able to free anything. It also provides an arch-dependent > interface to register the available memory. > > - "page" (alloc_page) is initialized after the MMU is started. It picks > whatever memory wasn't allocated by phys, and slices it in pages. Like > "phys", it provides contiguous physical addresses. > > - "virt" (alloc_vpages) is also initialized after the MMU is started, > and it allocates virtual address space. It also provides an > arch-dependent interface to install PTEs. It provides contiguous > virtual addresses. > > - "malloc" is the API we all love :) and it uses a pluggable allocator > underneath, called "morecore" in the thread, with the prototype > > void *(*morecore)(size_t size, size_t align_min); > > In the beginning "morecore" simply uses "phys", later it is switched > (when the MMU is started) to use both "page" and "virt". That is it > takes not necessarily consecutive physical memory at page > granularity from "page", it assigning consecutive virtual address (from > "virt") to it, and if needed it also slices pages into smaller allocations. > > Of course each could be arbitrarily complicated, but it need not be. > > "page" could be a buddy system, but really in practice it need not even > support reusing freed pages, except for 1-page allocations. This makes > it enough to use a simple free list of pages, plus a separately-tracked > huge block of consecutive free physical addresses at the top. > > Likewise, "malloc" could be Doug Lea's malloc, but really in practice it > will just be a small veneer over morecore, with a dummy free. Like the > testing thing, the important part is avoiding under-engineering. > > Quoting from the thread, you can choose separately at each layer whether > to implement freeing or not. "phys", "virt" and "morecore" do not need > it. See in particular the Nov. 3, 2016, 5:34 p.m. message for a plan to > transform Drew's v2 series > (https://www.spinics.net/lists/kvm/msg139685.html) into the design > envisioned above. > I'd love to see this loop closed, and I like the GTest API idea too. I'm not opposed to picking this back up myself, but it wouldn't be until after KVM Forum. Anybody who wants to start now should feel free to :-) Regarding this series: should we find a way to hack a malloc in order to use the GTest API now? x86 and s390 could just reserve some memory for malloc to use with their flat.lds files, for example. I'll go ahead and give this series a quick review too, as any comments I may have may apply to whatever followup series comes next. Thanks, drew
On Tue, Sep 12, 2017 at 04:16:58PM +0200, David Hildenbrand wrote: > I want to use this in another file, so instead of replicating, factoring > it out feels like the right thing to do. > > Let's directly provide it for all architectures, they just have > to implement get_time_ms() in asm/time.h to use it. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > > The next step would be allowing to add custom parameters to a test. > (defining them similar to the test cases). > > Any other requests/nice to have? Does this make sense? > > lib/s390x/asm/time.h | 24 ++++++++++++ > lib/testrun.c | 89 ++++++++++++++++++++++++++++++++++++++++++ > lib/testrun.h | 41 ++++++++++++++++++++ > s390x/Makefile | 1 + > s390x/intercept.c | 106 ++++++++------------------------------------------- > 5 files changed, 170 insertions(+), 91 deletions(-) > create mode 100644 lib/s390x/asm/time.h > create mode 100644 lib/testrun.c > create mode 100644 lib/testrun.h > > diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h > new file mode 100644 > index 0000000..856facd > --- /dev/null > +++ b/lib/s390x/asm/time.h > @@ -0,0 +1,24 @@ > +/* > + * Copyright (c) 2017 Red Hat Inc > + * > + * Authors: > + * Thomas Huth <thuth@redhat.com> > + * David Hildenbrand <david@redhat.com> > + * > + * This code is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Library General Public License version 2. > + */ > +#ifndef __ASMS390X_TIME_H > +#define __ASMS390X_TIME_H > + > +static inline uint64_t get_clock_ms(void) > +{ > + uint64_t clk; > + > + asm volatile(" stck %0 " : : "Q"(clk) : "memory"); > + > + /* Bit 51 is incrememented each microsecond */ > + return (clk >> (63 - 51)) / 1000; > +} > + > +#endif > diff --git a/lib/testrun.c b/lib/testrun.c > new file mode 100644 > index 0000000..6034767 > --- /dev/null > +++ b/lib/testrun.c We have the under utilized lib/util.[ch] files already. I think either these functions should go there, or the functions there should get merged into whatever we want to call the files for these functions, and util.[ch] should then be removed. Actually, I think Radim wanted to rework the one and only function in util.[ch] too, to improve its interface... > @@ -0,0 +1,89 @@ > +/* > + * Utility functions for running/configuring a set of tests > + * > + * Copyright (c) 2017 Red Hat Inc > + * > + * Authors: > + * Thomas Huth <thuth@redhat.com> > + * David Hildenbrand <david@redhat.com> > + * > + * This code is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Library General Public License version 2. > + */ > + > +#include <testrun.h> > +#include <asm/time.h> > + > +void parse_test_args(const int argc, const char **argv, struct test *test) > +{ > + bool run_all = true; > + int i, ti; > + > + test->iterations = 0; > + test->time_to_run = 0; > + > + for (i = 1; i < argc; i++) { > + if (!strcmp("-i", argv[i])) { > + i++; > + if (i >= argc) > + report_abort("-i needs a parameter"); > + test->iterations = atol(argv[i]); > + } else if (!strcmp("-t", argv[i])) { > + i++; > + if (i >= argc) > + report_abort("-t needs a parameter"); > + test->time_to_run = atol(argv[i]); I'd prefer we use long form args, like --iterations=<num> and --time-to-run=<seconds>. That way unittests.cfg files will be self documenting. > + } else for (ti = 0; test->test_cases[ti].name != NULL; ti++) { Unusual (at least to me) coding style. I'd prefer the more usual (to me) else { for ... { } } > + if (!strcmp(test->test_cases[ti].name, argv[i])) { > + run_all = false; > + test->test_cases[ti].run_it = true; > + break; The break means we can only list a single test to run, right? What about being able to run more than one by listing all you want? > + } else if (test->test_cases[ti + 1].name == NULL) { > + report_abort("Unsupported parameter '%s'", > + argv[i]); Instead of this, we could use a counter to track number of recognized args and then, after running the loop, if the counter doesn't match argc complain/abort. > + } > + } > + } > + > + if (test->iterations == 0 && test->time_to_run == 0) > + test->iterations = 1; > + > + if ((test->iterations > 1 || test->time_to_run) && > + !test->multirun_supported) > + report_abort("Test does not support multiruns."); > + > + if (run_all) { > + for (ti = 0; test->test_cases[ti].name != NULL; ti++) > + test->test_cases[ti].run_it = true; > + } > +} > + > +int run_test(struct test *test) I prefer this be plural, run_tests, and the argument test doesn't seem named quite right either, "testset" maybe? > +{ > + unsigned long iterations = test->iterations; > + unsigned long time_to_run = test->time_to_run; > + uint64_t startclk; > + int ti; > + > + report_prefix_push(test->name); > + startclk = get_clock_ms(); > + for (;;) { > + for (ti = 0; test->test_cases[ti].name != NULL; ti++) { > + report_prefix_push(test->test_cases[ti].name); > + if (test->test_cases[ti].run_it) > + test->test_cases[ti].func(); > + report_prefix_pop(); > + } > + if (iterations) { > + iterations -= 1; > + if (iterations == 0) > + break; > + } > + if (time_to_run) { > + if (get_clock_ms() - startclk > time_to_run) > + break; > + } > + } > + report_prefix_pop(); > + return report_summary(); > +} > diff --git a/lib/testrun.h b/lib/testrun.h > new file mode 100644 > index 0000000..8f9a2d7 > --- /dev/null > +++ b/lib/testrun.h > @@ -0,0 +1,41 @@ > +/* > + * Utility functions for running/configuring a set of test cases > + * > + * Copyright (c) 2017 Red Hat Inc > + * > + * Authors: > + * Thomas Huth <thuth@redhat.com> > + * David Hildenbrand <david@redhat.com> > + * > + * This code is free software; you can redistribute it and/or modify it > + * under the terms of the GNU Library General Public License version 2. > + */ > +#ifndef __TESTRUN_H > +#define __TESTRUN_H > + > +#include <libcflat.h> > + > +struct test_case { > + const char *name; > + void (*func)(void); > + /* initialized by parse_test_args() */ > + bool run_it; > +}; > + > +#define DEFINE_TEST_CASE(_name, _func) { _name, _func, false } > +#define DEFINE_TEST_CASE_END_OF_LIST() { } > + > +struct test { > + /* to be initialized by the test */ > + const char *name; > + bool multirun_supported; > + struct test_case *test_cases; > + /* initialized by parse_test_args() */ > + unsigned long iterations; > + unsigned long time_to_run; > +}; > + > +void parse_test_args(int argc, const char **argv, struct test *test); > +int run_test(struct test *test); > + > +#endif /* __TESTRUN_H */ Thanks, drew
> We have the under utilized lib/util.[ch] files already. I think either > these functions should go there, or the functions there should get merged > into whatever we want to call the files for these functions, and util.[ch] > should then be removed. Actually, I think Radim wanted to rework the one > and only function in util.[ch] too, to improve its interface... > In general, I think I'd prefer to drop util.X and rather give the child a name. :) > >> @@ -0,0 +1,89 @@ >> +/* >> + * Utility functions for running/configuring a set of tests >> + * >> + * Copyright (c) 2017 Red Hat Inc >> + * >> + * Authors: >> + * Thomas Huth <thuth@redhat.com> >> + * David Hildenbrand <david@redhat.com> >> + * >> + * This code is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU Library General Public License version 2. >> + */ >> + >> +#include <testrun.h> >> +#include <asm/time.h> >> + >> +void parse_test_args(const int argc, const char **argv, struct test *test) >> +{ >> + bool run_all = true; >> + int i, ti; >> + >> + test->iterations = 0; >> + test->time_to_run = 0; >> + >> + for (i = 1; i < argc; i++) { >> + if (!strcmp("-i", argv[i])) { >> + i++; >> + if (i >= argc) >> + report_abort("-i needs a parameter"); >> + test->iterations = atol(argv[i]); >> + } else if (!strcmp("-t", argv[i])) { >> + i++; >> + if (i >= argc) >> + report_abort("-t needs a parameter"); >> + test->time_to_run = atol(argv[i]); > > I'd prefer we use long form args, like --iterations=<num> and > --time-to-run=<seconds>. That way unittests.cfg files will be self > documenting. I just moved this code from intercept.c. So this sure can be done. > >> + } else for (ti = 0; test->test_cases[ti].name != NULL; ti++) { > > Unusual (at least to me) coding style. I'd prefer the more usual (to me) > Absolutely, I also just moved it. But this really looks ... special :) > else { > for ... { > } > } > >> + if (!strcmp(test->test_cases[ti].name, argv[i])) { >> + run_all = false; >> + test->test_cases[ti].run_it = true; >> + break; > > The break means we can only list a single test to run, right? What about > being able to run more than one by listing all you want? That should be supported, no? The break will simply start looking for a test named "argv[i]". The next one (argv[i + 1]) would be processed afterwards. > >> + } else if (test->test_cases[ti + 1].name == NULL) { >> + report_abort("Unsupported parameter '%s'", >> + argv[i]); > > Instead of this, we could use a counter to track number of recognized > args and then, after running the loop, if the counter doesn't match argc > complain/abort. Yes, this could be refactored a lot, especially if we want to include parsing test specific parameters (specified in struct test.). > >> + } >> + } >> + } >> + >> + if (test->iterations == 0 && test->time_to_run == 0) >> + test->iterations = 1; >> + >> + if ((test->iterations > 1 || test->time_to_run) && >> + !test->multirun_supported) >> + report_abort("Test does not support multiruns."); >> + >> + if (run_all) { >> + for (ti = 0; test->test_cases[ti].name != NULL; ti++) >> + test->test_cases[ti].run_it = true; >> + } >> +} >> + >> +int run_test(struct test *test) > > I prefer this be plural, run_tests, and the argument test doesn't > seem named quite right either, "testset" maybe? I renamed this at least 10 times :) We have a test that contains test cases. So it should be singular. and struct test handed over is not a list but only one element. But we would have to agree on a common terminology. Main question is, if it makes sense to work on this or directly try to implement at least parts of the glib test API as Paolo suggests. Any thoughts? Thanks!
On Wed, Sep 13, 2017 at 03:07:23PM +0200, David Hildenbrand wrote: > > > We have the under utilized lib/util.[ch] files already. I think either > > these functions should go there, or the functions there should get merged > > into whatever we want to call the files for these functions, and util.[ch] > > should then be removed. Actually, I think Radim wanted to rework the one > > and only function in util.[ch] too, to improve its interface... > > > > In general, I think I'd prefer to drop util.X and rather give the child > a name. :) > > > > >> @@ -0,0 +1,89 @@ > >> +/* > >> + * Utility functions for running/configuring a set of tests > >> + * > >> + * Copyright (c) 2017 Red Hat Inc > >> + * > >> + * Authors: > >> + * Thomas Huth <thuth@redhat.com> > >> + * David Hildenbrand <david@redhat.com> > >> + * > >> + * This code is free software; you can redistribute it and/or modify it > >> + * under the terms of the GNU Library General Public License version 2. > >> + */ > >> + > >> +#include <testrun.h> > >> +#include <asm/time.h> > >> + > >> +void parse_test_args(const int argc, const char **argv, struct test *test) > >> +{ > >> + bool run_all = true; > >> + int i, ti; > >> + > >> + test->iterations = 0; > >> + test->time_to_run = 0; > >> + > >> + for (i = 1; i < argc; i++) { > >> + if (!strcmp("-i", argv[i])) { > >> + i++; > >> + if (i >= argc) > >> + report_abort("-i needs a parameter"); > >> + test->iterations = atol(argv[i]); > >> + } else if (!strcmp("-t", argv[i])) { > >> + i++; > >> + if (i >= argc) > >> + report_abort("-t needs a parameter"); > >> + test->time_to_run = atol(argv[i]); > > > > I'd prefer we use long form args, like --iterations=<num> and > > --time-to-run=<seconds>. That way unittests.cfg files will be self > > documenting. > > I just moved this code from intercept.c. So this sure can be done. > > > > >> + } else for (ti = 0; test->test_cases[ti].name != NULL; ti++) { > > > > Unusual (at least to me) coding style. I'd prefer the more usual (to me) > > > > Absolutely, I also just moved it. But this really looks ... special :) > > > else { > > for ... { > > } > > } > > > >> + if (!strcmp(test->test_cases[ti].name, argv[i])) { > >> + run_all = false; > >> + test->test_cases[ti].run_it = true; > >> + break; > > > > The break means we can only list a single test to run, right? What about > > being able to run more than one by listing all you want? > > That should be supported, no? The break will simply start looking for a > test named "argv[i]". The next one (argv[i + 1]) would be processed > afterwards. I somehow forgot there was an outer for loop... So, yeah, this is fine. > > > > >> + } else if (test->test_cases[ti + 1].name == NULL) { > >> + report_abort("Unsupported parameter '%s'", > >> + argv[i]); > > > > Instead of this, we could use a counter to track number of recognized > > args and then, after running the loop, if the counter doesn't match argc > > complain/abort. > > Yes, this could be refactored a lot, especially if we want to include > parsing test specific parameters (specified in struct test.). > > > > >> + } > >> + } > >> + } > >> + > >> + if (test->iterations == 0 && test->time_to_run == 0) > >> + test->iterations = 1; > >> + > >> + if ((test->iterations > 1 || test->time_to_run) && > >> + !test->multirun_supported) > >> + report_abort("Test does not support multiruns."); > >> + > >> + if (run_all) { > >> + for (ti = 0; test->test_cases[ti].name != NULL; ti++) > >> + test->test_cases[ti].run_it = true; > >> + } > >> +} > >> + > >> +int run_test(struct test *test) > > > > I prefer this be plural, run_tests, and the argument test doesn't > > seem named quite right either, "testset" maybe? > > I renamed this at least 10 times :) > > We have a test that contains test cases. So it should be singular. and > struct test handed over is not a list but only one element. But we would > have to agree on a common terminology. I can retrain my brain to think this way, and then the terminology makes sense. I just wonder if I'm the only one who needs retraining, or if the API naming will always be a source of confusion? Anyway, the API naming can be left to GTest. > > Main question is, if it makes sense to work on this or directly try to > implement at least parts of the glib test API as Paolo suggests. Any > thoughts? Yes, I prefer the GTest idea as well. I just wanted to review this patch too, in case any of the code would still be used when converting it. Thanks, drew
On 12/09/2017 18:27, David Hildenbrand wrote: >> It's really less work than it sounds. I'm not wed to GTest, but I think >> the ability to define test cases programmatically is useful (vmexit.c > > I agree, as I said I would also like to use that. But at least at the > first sight this really looks like a lot of work. I did the first part, splitting the architecture-independent part of lib/x86/vmalloc.c. Paolo
diff --git a/lib/s390x/asm/time.h b/lib/s390x/asm/time.h new file mode 100644 index 0000000..856facd --- /dev/null +++ b/lib/s390x/asm/time.h @@ -0,0 +1,24 @@ +/* + * Copyright (c) 2017 Red Hat Inc + * + * Authors: + * Thomas Huth <thuth@redhat.com> + * David Hildenbrand <david@redhat.com> + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU Library General Public License version 2. + */ +#ifndef __ASMS390X_TIME_H +#define __ASMS390X_TIME_H + +static inline uint64_t get_clock_ms(void) +{ + uint64_t clk; + + asm volatile(" stck %0 " : : "Q"(clk) : "memory"); + + /* Bit 51 is incrememented each microsecond */ + return (clk >> (63 - 51)) / 1000; +} + +#endif diff --git a/lib/testrun.c b/lib/testrun.c new file mode 100644 index 0000000..6034767 --- /dev/null +++ b/lib/testrun.c @@ -0,0 +1,89 @@ +/* + * Utility functions for running/configuring a set of tests + * + * Copyright (c) 2017 Red Hat Inc + * + * Authors: + * Thomas Huth <thuth@redhat.com> + * David Hildenbrand <david@redhat.com> + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU Library General Public License version 2. + */ + +#include <testrun.h> +#include <asm/time.h> + +void parse_test_args(const int argc, const char **argv, struct test *test) +{ + bool run_all = true; + int i, ti; + + test->iterations = 0; + test->time_to_run = 0; + + for (i = 1; i < argc; i++) { + if (!strcmp("-i", argv[i])) { + i++; + if (i >= argc) + report_abort("-i needs a parameter"); + test->iterations = atol(argv[i]); + } else if (!strcmp("-t", argv[i])) { + i++; + if (i >= argc) + report_abort("-t needs a parameter"); + test->time_to_run = atol(argv[i]); + } else for (ti = 0; test->test_cases[ti].name != NULL; ti++) { + if (!strcmp(test->test_cases[ti].name, argv[i])) { + run_all = false; + test->test_cases[ti].run_it = true; + break; + } else if (test->test_cases[ti + 1].name == NULL) { + report_abort("Unsupported parameter '%s'", + argv[i]); + } + } + } + + if (test->iterations == 0 && test->time_to_run == 0) + test->iterations = 1; + + if ((test->iterations > 1 || test->time_to_run) && + !test->multirun_supported) + report_abort("Test does not support multiruns."); + + if (run_all) { + for (ti = 0; test->test_cases[ti].name != NULL; ti++) + test->test_cases[ti].run_it = true; + } +} + +int run_test(struct test *test) +{ + unsigned long iterations = test->iterations; + unsigned long time_to_run = test->time_to_run; + uint64_t startclk; + int ti; + + report_prefix_push(test->name); + startclk = get_clock_ms(); + for (;;) { + for (ti = 0; test->test_cases[ti].name != NULL; ti++) { + report_prefix_push(test->test_cases[ti].name); + if (test->test_cases[ti].run_it) + test->test_cases[ti].func(); + report_prefix_pop(); + } + if (iterations) { + iterations -= 1; + if (iterations == 0) + break; + } + if (time_to_run) { + if (get_clock_ms() - startclk > time_to_run) + break; + } + } + report_prefix_pop(); + return report_summary(); +} diff --git a/lib/testrun.h b/lib/testrun.h new file mode 100644 index 0000000..8f9a2d7 --- /dev/null +++ b/lib/testrun.h @@ -0,0 +1,41 @@ +/* + * Utility functions for running/configuring a set of test cases + * + * Copyright (c) 2017 Red Hat Inc + * + * Authors: + * Thomas Huth <thuth@redhat.com> + * David Hildenbrand <david@redhat.com> + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU Library General Public License version 2. + */ +#ifndef __TESTRUN_H +#define __TESTRUN_H + +#include <libcflat.h> + +struct test_case { + const char *name; + void (*func)(void); + /* initialized by parse_test_args() */ + bool run_it; +}; + +#define DEFINE_TEST_CASE(_name, _func) { _name, _func, false } +#define DEFINE_TEST_CASE_END_OF_LIST() { } + +struct test { + /* to be initialized by the test */ + const char *name; + bool multirun_supported; + struct test_case *test_cases; + /* initialized by parse_test_args() */ + unsigned long iterations; + unsigned long time_to_run; +}; + +void parse_test_args(int argc, const char **argv, struct test *test); +int run_test(struct test *test); + +#endif /* __TESTRUN_H */ diff --git a/s390x/Makefile b/s390x/Makefile index bc099da..e174895 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -21,6 +21,7 @@ include $(SRCDIR)/scripts/asm-offsets.mak cflatobjs += lib/util.o cflatobjs += lib/alloc.o +cflatobjs += lib/testrun.o cflatobjs += lib/s390x/io.o cflatobjs += lib/s390x/stack.o cflatobjs += lib/s390x/sclp-ascii.o diff --git a/s390x/intercept.c b/s390x/intercept.c index 59e5fca..15e50f6 100644 --- a/s390x/intercept.c +++ b/s390x/intercept.c @@ -10,15 +10,13 @@ * under the terms of the GNU Library General Public License version 2. */ #include <libcflat.h> +#include <testrun.h> #include <asm/asm-offsets.h> #include <asm/interrupt.h> #include <asm/page.h> static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2))); -static unsigned long nr_iterations; -static unsigned long time_to_run; - /* Test the STORE PREFIX instruction */ static void test_stpx(void) { @@ -159,95 +157,21 @@ static void test_testblock(void) check_pgm_int_code(PGM_INT_CODE_ADDRESSING); } -static uint64_t get_clock_ms(void) -{ - uint64_t clk; - - asm volatile(" stck %0 " : : "Q"(clk) : "memory"); - - /* Bit 51 is incrememented each microsecond */ - return (clk >> (63 - 51)) / 1000; -} - -struct { - const char *name; - void (*func)(void); - bool run_it; -} tests[] = { - { "stpx", test_stpx, false }, - { "spx", test_spx, false }, - { "stap", test_stap, false }, - { "stidp", test_stidp, false }, - { "testblock", test_testblock, false }, - { NULL, NULL, false } -}; - -static void parse_intercept_test_args(int argc, char **argv) -{ - int i, ti; - bool run_all = true; - - for (i = 1; i < argc; i++) { - if (!strcmp("-i", argv[i])) { - i++; - if (i >= argc) - report_abort("-i needs a parameter"); - nr_iterations = atol(argv[i]); - } else if (!strcmp("-t", argv[i])) { - i++; - if (i >= argc) - report_abort("-t needs a parameter"); - time_to_run = atol(argv[i]); - } else for (ti = 0; tests[ti].name != NULL; ti++) { - if (!strcmp(tests[ti].name, argv[i])) { - run_all = false; - tests[ti].run_it = true; - break; - } else if (tests[ti + 1].name == NULL) { - report_abort("Unsupported parameter '%s'", - argv[i]); - } - } - } - - if (run_all) { - for (ti = 0; tests[ti].name != NULL; ti++) - tests[ti].run_it = true; +static struct test test = { + .name = "intercept", + .multirun_supported = true, + .test_cases = (struct test_case[]) { + DEFINE_TEST_CASE("stpx", test_stpx), + DEFINE_TEST_CASE("spx", test_spx), + DEFINE_TEST_CASE("stap", test_stap), + DEFINE_TEST_CASE("stidp", test_stidp), + DEFINE_TEST_CASE("testblock", test_testblock), + DEFINE_TEST_CASE_END_OF_LIST() } -} +}; -int main(int argc, char **argv) +int main(int argc, const char **argv) { - uint64_t startclk; - int ti; - - parse_intercept_test_args(argc, argv); - - if (nr_iterations == 0 && time_to_run == 0) - nr_iterations = 1; - - report_prefix_push("intercept"); - - startclk = get_clock_ms(); - for (;;) { - for (ti = 0; tests[ti].name != NULL; ti++) { - report_prefix_push(tests[ti].name); - if (tests[ti].run_it) - tests[ti].func(); - report_prefix_pop(); - } - if (nr_iterations) { - nr_iterations -= 1; - if (nr_iterations == 0) - break; - } - if (time_to_run) { - if (get_clock_ms() - startclk > time_to_run) - break; - } - } - - report_prefix_pop(); - - return report_summary(); + parse_test_args(argc, argv, &test); + return run_test(&test); }
I want to use this in another file, so instead of replicating, factoring it out feels like the right thing to do. Let's directly provide it for all architectures, they just have to implement get_time_ms() in asm/time.h to use it. Signed-off-by: David Hildenbrand <david@redhat.com> --- The next step would be allowing to add custom parameters to a test. (defining them similar to the test cases). Any other requests/nice to have? Does this make sense? lib/s390x/asm/time.h | 24 ++++++++++++ lib/testrun.c | 89 ++++++++++++++++++++++++++++++++++++++++++ lib/testrun.h | 41 ++++++++++++++++++++ s390x/Makefile | 1 + s390x/intercept.c | 106 ++++++++------------------------------------------- 5 files changed, 170 insertions(+), 91 deletions(-) create mode 100644 lib/s390x/asm/time.h create mode 100644 lib/testrun.c create mode 100644 lib/testrun.h