On Sat, Oct 3, 2020 at 3:13 AM John Hubbard wrote:
>
> On 10/2/20 9:20 AM, Kalesh Singh wrote:
> > Test mremap on regions of various sizes and alignments and validate
> > data after remapping. Also provide total time for remapping
> > the region which is useful for performance comparison of the mremap
> > optimizations that move pages at the PMD/PUD levels if HAVE_MOVE_PMD
> > and/or HAVE_MOVE_PUD are enabled.
> >
> > Signed-off-by: Kalesh Singh
> > ---
> > Changes in v2:
> >- Reduce test time by only validating a certain threshold of the
> > remapped region (4MB by default). The -t flag can be used to
> > set a custom threshold in MB or no threshold by passing 0. (-t0).
> > mremap time is not provided in stdout for only partially validated
> > regions. This time is only applicable for comparison if the entire
> > mapped region was faulted in.
> >- Use a random pattern for validating the remapped region. The -p
> > flag can be used to run the tests with a specified seed for the
> > random pattern.
> >- Print test configs (threshold_mb and pattern_seed) to stdout.
> >- Remove MAKE_SIMPLE_TEST macro.
> >- Define named flags instead of 0 / 1.
> >- Add comments for destination address' align_mask and offset.
>
> Thanks for making those changes. This all looks much nicer, both in the
> code and on the screen. Now it's easy to see which code generates which
> output, and the tests are readable. The new comments are also helpful.
>
> I ran it locally and it came in at 0.6 sec, so that looks good.
>
> A remaining nit: there are lots of > 80 col lines here. Only those lines
> that contain printf output strings really need to be extra-long. The
> others can all be easily made to fit.
Thanks for the review John. I can fix the lines that don't need to be
>80 col. FWIW checkpatch now allows 100 col lines:
https://lore.kernel.org/r/9c360bfa43580ce7726dd3d9d247f1216a690ef0.ca...@perches.com
>
> ...
> > +
> > +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > +#define MIN(X, Y) ((X) < (Y) ? (X) : (Y))
>
>
> Not really a comment on this patch, but: at least two other test suites
> in selftests/ have had to recreate MIN() or min(), and probably a few
> other common things as well. It would be nice to gather up some of these
> common things into one of the shared header files in selftests/.
>
> I'm certainly not saying that this patchset has that responsibility,
> though. Just pointing it out in case someone has a moment to clean up a
> few here and there.
Thanks for pointing this out. I'll leave it as a task for a separate
patch if time permits or no one gets around to it before :-)
>
> I'll assume that any changes to make things fit within 80 cols are
> trivial, so you can add:
>
> Reviewed-by: John Hubbard
>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> > +
> > +struct config {
> > + unsigned long long src_alignment;
> > + unsigned long long dest_alignment;
> > + unsigned long long region_size;
> > + int overlapping;
> > +};
> > +
> > +struct test {
> > + const char *name;
> > + struct config config;
> > + int expect_failure;
> > +};
> > +
> > +enum {
> > + _1KB = 1ULL << 10, /* 1KB -> not page aligned */
> > + _4KB = 4ULL << 10,
> > + _8KB = 8ULL << 10,
> > + _1MB = 1ULL << 20,
> > + _2MB = 2ULL << 20,
> > + _4MB = 4ULL << 20,
> > + _1GB = 1ULL << 30,
> > + _2GB = 2ULL << 30,
> > + PTE = _4KB,
> > + PMD = _2MB,
> > + PUD = _1GB,
> > +};
> > +
> > +#define MAKE_TEST(source_align, destination_align, size, \
> > + overlaps, should_fail, test_name) \
> > +{\
> > + .name = test_name, \
> > + .config = { \
> > + .src_alignment = source_align, \
> > + .dest_alignment = destination_align,\
> > + .region_size = size,\
> > + .overlapping = overlaps,\
> > + }, \
> > + .expect_failure = should_fail \
> > +}
> > +
> > +/*
> > + * Returns the start address of the mapping on success, else returns
> > + * NULL on failure.
> > + */
> > +static void *get_source_mapping(struct config c)
> > +{
> > + unsigned long long addr = 0ULL;
> > + void *src_addr = NULL;
> > +retry:
> > + addr += c.src_alignment;
> > + src_addr = mmap((void *) addr, c.region_size, PROT_READ | PROT_WRITE,
> > + MAP_FIXED | MAP_ANONYMOUS | MAP_SHARED, -1, 0);
> > + if (src_addr == MAP_FAILED) {
> > + if (errno == EPERM)
> > + goto retry;
> > + goto error;
> > + }
> > + /*
> > + * Check that the address is aligned to the specified alignment.
> > Add