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

[Andrew Jones]

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 <address@hidden>
> 
> ---
> 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