Re: [Qemu-devel] [kvm-unit-tests PATCH v5 07/11] new arm/tlbflush-test: TLB torture test

Andrew Jones
2015-07-31T18:51:15Z

On Fri, Jul 31, 2015 at 04:53:57PM +0100, Alex Bennée wrote:
> This adds a fairly brain dead torture test for TLB flushes intended for
> stressing the MTTCG QEMU build. It takes the usual -smp option for
> multiple CPUs.
>
> By default it CPU0 will do a TLBIALL flush after each cycle. You can
> pass options via -append to control additional aspects of the test:
>
> - "page" flush each page in turn (one per function)
> - "self" do the flush after each computation cycle
> - "verbose" report progress on each computation cycle
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
> - rename to tlbflush-test
> - made makefile changes cleaner
> - added self/other flush mode
> - create specific prefix
> - whitespace fixes
> ---
> arm/tlbflush-test.c | 194 +++++++++++++++++++++++++++++++++++++++++++
> config/config-arm-common.mak | 7 +-
> 2 files changed, 198 insertions(+), 3 deletions(-)
> create mode 100644 arm/tlbflush-test.c
>
> diff --git a/arm/tlbflush-test.c b/arm/tlbflush-test.c
> new file mode 100644
> index 0000000..0375ad9
> --- /dev/null
> +++ b/arm/tlbflush-test.c

missing GPL header

> @@ -0,0 +1,194 @@
> +#include <libcflat.h>
> +#include <asm/smp.h>
> +#include <asm/cpumask.h>
> +#include <asm/barrier.h>
> +#include <asm/mmu.h>
> +
> +#define SEQ_LENGTH 10
> +#define SEQ_HASH 0x7cd707fe
> +
> +static cpumask_t smp_test_complete;
> +static int flush_count = 1000000;
> +static int flush_self = 0;
> +static int flush_page = 0;
> +static int flush_verbose = 0;

nit: explicit = 0 not necessary and could use type bool

> +
> +/* Work functions
> + *
> + * These work functions need to be:
> + *
> + * - page aligned, so we can flush one function at a time
> + * - have branches, so QEMU TCG generates multiple basic blocks
> + * - call across pages, so we exercise the TCG basic block slow path
> + */

nit: qemu comment style, please use kernel style (also other comments below)

> +
> +/* Adler32 */
> +__attribute__((aligned(PAGE_SIZE))) uint32_t hash_array(const void *buf,
> + size_t buflen)
> +{
> + const uint8_t *data = (uint8_t *) buf;
> + uint32_t s1 = 1;
> + uint32_t s2 = 0;
> +
> + for (size_t n = 0; n < buflen; n++) {
> + s1 = (s1 + data[n]) % 65521;
> + s2 = (s2 + s1) % 65521;
> + }
> + return (s2 << 16) | s1;
> +}
> +
> +__attribute__((aligned(PAGE_SIZE))) void create_fib_sequence(int length,
> + unsigned int *array)
> +{
> + int i;
> +
> + /* first two values */
> + array[0] = 0;
> + array[1] = 1;
> + for (i=2; i<length; i++) {
> + array[i] = array[i-2] + array[i-1];
> + }
> +}
> +
> +__attribute__((aligned(PAGE_SIZE))) unsigned long long factorial(unsigned int n)
> +{
> + unsigned int i;
> + unsigned long long fac = 1;
> + for (i=1; i<=n; i++)
> + {
> + fac = fac * i;
> + }
nit: brace style
> + return fac;
> +}
> +
> +__attribute__((aligned(PAGE_SIZE))) void factorial_array
> +(unsigned int n, unsigned int *input, unsigned long long *output)
> +{
> + unsigned int i;
> + for (i=0; i<n; i++) {
> + output[i] = factorial(input[i]);
> + }
> +}
> +
> +__attribute__((aligned(PAGE_SIZE))) unsigned int do_computation(void)
> +{
> + unsigned int fib_array[SEQ_LENGTH];
> + unsigned long long facfib_array[SEQ_LENGTH];
> + uint32_t fib_hash, facfib_hash;
> +
> + create_fib_sequence(SEQ_LENGTH, &fib_array[0]);
> + fib_hash = hash_array(&fib_array[0], sizeof(fib_array));
> + factorial_array(SEQ_LENGTH, &fib_array[0], &facfib_array[0]);
> + facfib_hash = hash_array(&facfib_array[0], sizeof(facfib_array));
> +
> + return (fib_hash ^ facfib_hash);
> +}

I still find the complex hash distracting. But if you believe it's
necessary for your mttcg test, then please explain why in a comment.

> +
> +/* This provides a table of the work functions so we can flush each
> + * page individually
> + */
> +static void * pages[] = {&hash_array, &create_fib_sequence, &factorial,
> + &factorial_array, &do_computation};
> +
> +static void do_flush(int i)
> +{
> + if (flush_page) {
> + flush_tlb_page((unsigned long)pages[i % ARRAY_SIZE(pages)]);
> + } else {
> + flush_tlb_all();
> + }
nit: no need for braces
> +}
> +
> +
> +static void just_compute(void)
name it do_compute? (it's not just_compute when flush_self is true)
> +{
> + int i, errors = 0;
> + int cpu = smp_processor_id();
> +
> + uint32_t result;
> +
> + printf("CPU%d online\n", cpu);
> +
> + for (i=0; i < flush_count; i++) {
> + result = do_computation();
> +
> + if (result != SEQ_HASH) {
> + errors++;
> + printf("CPU%d: seq%d 0x%x!=0x%x\n",
> + cpu, i, result, SEQ_HASH);
> + }
> +
> + if (flush_verbose && (i % 1000) == 0) {
> + printf("CPU%d: seq%d\n", cpu, i);
> + }
> +
> + if (flush_self) {
> + do_flush(i);
> + }
nit: braces again (I feel like I'm reading qemu code, but with tabs :-)
> + }
> +
> + report("CPU%d: Done - Errors: %d\n", errors == 0, cpu, errors);
> +
> + cpumask_set_cpu(cpu, &smp_test_complete);
> + if (cpu != 0)
> + halt();
> +}
> +
> +static void just_flush(void)
> +{
> + int cpu = smp_processor_id();
> + int i = 0;
> +
> + /* set our CPU as done, keep flushing until everyone else
> + finished */
> + cpumask_set_cpu(cpu, &smp_test_complete);
> +
> + while (!cpumask_full(&smp_test_complete)) {
> + do_flush(i++);
> + }
braces
> +
> + report("CPU%d: Done - Triggered %d flushes\n", true, cpu, i);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int cpu, i;
> + char prefix[100];
> +
> + for (i=0; i<argc; i++) {
> + char *arg = argv[i];
> +
> + if (strcmp(arg, "page") == 0) {
> + flush_page = 1;
> + }
strange spaces after tab before brace here
> +
> + if (strcmp(arg, "self") == 0) {
> + flush_self = 1;
> + }
what happened to tabs here?
> +
> + if (strcmp(arg, "verbose") == 0) {
> + flush_verbose = 1;
> + }
> + }
> +
> + snprintf(prefix, sizeof(prefix), "tlbflush_%s_%s",
> + flush_page?"page":"all",
> + flush_self?"self":"other");
> + report_prefix_push(prefix);
> +
> + for_each_present_cpu(cpu) {
> + if (cpu == 0)
> + continue;
> + smp_boot_secondary(cpu, just_compute);
> + }
> +
> + if (flush_self)
> + just_compute();
> + else
> + just_flush();
> +
> + while (!cpumask_full(&smp_test_complete))
> + cpu_relax();
> +
> + return report_summary();
> +}
> diff --git a/config/config-arm-common.mak b/config/config-arm-common.mak
> index 0674daa..164199b 100644
> --- a/config/config-arm-common.mak
> +++ b/config/config-arm-common.mak
> @@ -9,9 +9,9 @@ ifeq ($(LOADADDR),)
> LOADADDR = 0x40000000
> endif
>
> -tests-common = \
> - $(TEST_DIR)/selftest.flat \
> - $(TEST_DIR)/spinlock-test.flat
> +tests-common = $(TEST_DIR)/selftest.flat
> +tests-common += $(TEST_DIR)/spinlock-test.flat
> +tests-common += $(TEST_DIR)/tlbflush-test.flat
>
> all: test_cases
>
> @@ -72,3 +72,4 @@ test_cases: $(generated_files) $(tests-common) $(tests)
>
> $(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o
> $(TEST_DIR)/spinlock-test.elf: $(cstart.o) $(TEST_DIR)/spinlock-test.o
> +$(TEST_DIR)/tlbflush-test.elf: $(cstart.o) $(TEST_DIR)/tlbflush-test.o
> --
> 2.5.0
>
>

Please run $KERNEL/scripts/checkpatch.pl -f on this file like I suggested
before.

Thanks,
drew