Re: [kvm-unit-tests PATCH v7 07/35] common: add memory dirtying vs migration test

["Nicholas Piggin" <npiggin@gmail.com>]

On Fri Mar 29, 2024 at 3:37 AM AEST, Thomas Huth wrote:
> On 19/03/2024 08.58, Nicholas Piggin wrote:
> > This test stores to a bunch of pages and verifies previous stores,
> > while being continually migrated. Default runtime is 5 seconds.
> > 
> > Add this test to ppc64 and s390x builds. This can fail due to a QEMU
> > TCG physical memory dirty bitmap bug, so it is not enabled in unittests
> > for TCG yet.
> > 
> > The selftest-migration test time is reduced significantly because
> > this test
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   common/memory-verify.c      | 67 +++++++++++++++++++++++++++++++++++++
> >   common/selftest-migration.c |  8 ++---
> >   powerpc/Makefile.common     |  1 +
> >   powerpc/memory-verify.c     |  1 +
> >   powerpc/unittests.cfg       |  7 ++++
> >   s390x/Makefile              |  1 +
> >   s390x/memory-verify.c       |  1 +
> >   s390x/unittests.cfg         |  6 ++++
> >   8 files changed, 88 insertions(+), 4 deletions(-)
> >   create mode 100644 common/memory-verify.c
> >   create mode 120000 powerpc/memory-verify.c
> >   create mode 120000 s390x/memory-verify.c
> > 
> > diff --git a/common/memory-verify.c b/common/memory-verify.c
> > new file mode 100644
> > index 000000000..e78fb4338
> > --- /dev/null
> > +++ b/common/memory-verify.c
> > @@ -0,0 +1,67 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Simple memory verification test, used to exercise dirty memory migration.
> > + */
> > +#include <libcflat.h>
> > +#include <migrate.h>
> > +#include <alloc.h>
> > +#include <asm/page.h>
> > +#include <asm/time.h>
> > +
> > +#define NR_PAGES 32
> > +
> > +static unsigned time_sec = 5;
> > +
> > +static void do_getopts(int argc, char **argv)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < argc; ++i) {
> > +		if (strcmp(argv[i], "-t") == 0) {
> > +			i++;
> > +			if (i == argc)
> > +				break;
> > +			time_sec = atol(argv[i]);
> > +		}
> > +	}
> > +
> > +	printf("running for %d secs\n", time_sec);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	void *mem = malloc(NR_PAGES*PAGE_SIZE);
>
> Use alloc_pages(5) instead ? Or add at least some white spaces around "*".

Hmm, alloc_pages is physical? Maybe I should use memalign instead (and
I'll fix the space). Even though it's not using VM, we might change
that.

> Apart from that this patch looks sane to me, so with that line fixed:
> Reviewed-by: Thomas Huth <thuth@redhat.com>

I'll keep your R-B with the memalign change if that's okay.

Thanks,
Nick