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@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxx>
> + * David Hildenbrand <david@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>
> + * David Hildenbrand <david@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>
> + * David Hildenbrand <david@xxxxxxxxxx>
> + *
> + * 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
