Message-ID: <CAC_TJveSgd-=t49hqR3LN7G8PMPwdgN+qkQct3zS-N6=CW7uug@mail.gmail.com>
Date:   Sat, 3 Oct 2020 16:01:29 -0400
From:   Kalesh Singh <kaleshsingh@...gle.com>
To:     John Hubbard <jhubbard@...dia.com>
Cc:     Suren Baghdasaryan <surenb@...gle.com>,
        Minchan Kim <minchan@...gle.com>,
        Joel Fernandes <joelaf@...gle.com>,
        Lokesh Gidra <lokeshgidra@...gle.com>,
        "Cc: Android Kernel" <kernel-team@...roid.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Shuah Khan <shuah@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Arnd Bergmann <arnd@...db.de>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Hassan Naveed <hnaveed@...ecomp.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Mark Rutland <mark.rutland@....com>,
        Mark Brown <broonie@...nel.org>,
        Mike Rapoport <rppt@...nel.org>, Gavin Shan <gshan@...hat.com>,
        Chris von Recklinghausen <crecklin@...hat.com>,
        Jia He <justin.he@....com>, Zhenyu Ye <yezhenyu2@...wei.com>,
        Thiago Jung Bauermann <bauerman@...ux.ibm.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Mina Almasry <almasrymina@...gle.com>,
        Ram Pai <linuxram@...ibm.com>,
        Ralph Campbell <rcampbell@...dia.com>,
        Sandipan Das <sandipan@...ux.ibm.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Colin Ian King <colin.king@...onical.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Kamalesh Babulal <kamalesh@...ux.vnet.ibm.com>,
        Ira Weiny <ira.weiny@...el.com>,
        SeongJae Park <sjpark@...zon.de>,
        Brian Geffon <bgeffon@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:MEMORY MANAGEMENT" <linux-mm@...ck.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v2 1/6] kselftests: vm: Add mremap tests

On Sat, Oct 3, 2020 at 3:13 AM John Hubbard <jhubbard@...dia.com> 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 <kaleshsingh@...gle.com>
> > ---
> > 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.camel@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 <jhubbard@...dia.com>
>
> 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. Addresses
> > +      * which have alignments that are multiples of that specified are not considered
> > +      * valid. For instance, 1GB address is 2MB-aligned, however it will not be
> > +      * considered valid for a requested alignment of 2MB. This is done to
> > +      * reduce coincidental alignment in the tests.
> > +      */
> > +     if (((unsigned long long) src_addr & (c.src_alignment - 1)) ||
> > +                     !((unsigned long long) src_addr & c.src_alignment))
> > +             goto retry;
> > +
> > +     if (!src_addr)
> > +             goto error;
> > +
> > +     return src_addr;
> > +error:
> > +     ksft_print_msg("Failed to map source region: %s\n",
> > +                     strerror(errno));
> > +     return NULL;
> > +}
> > +
> > +/* Returns the time taken for the remap on success else returns -1. */
> > +static long long remap_region(struct config c, unsigned int threshold_mb,
> > +                           char pattern_seed)
> > +{
> > +     void *addr, *src_addr, *dest_addr;
> > +     unsigned long long i;
> > +     struct timespec t_start = {0, 0}, t_end = {0, 0};
> > +     long long  start_ns, end_ns, align_mask, ret, offset;
> > +     unsigned long long threshold;
> > +
> > +     if (threshold_mb == VALIDATION_NO_THRESHOLD)
> > +             threshold = c.region_size;
> > +     else
> > +             threshold = MIN(threshold_mb * _1MB, c.region_size);
> > +
> > +     src_addr = get_source_mapping(c);
> > +     if (!src_addr) {
> > +             ret = -1;
> > +             goto out;
> > +     }
> > +
> > +     /* Set byte pattern */
> > +     srand(pattern_seed);
> > +     for (i = 0; i < threshold; i++)
> > +             memset((char *) src_addr + i, (char) rand(), 1);
> > +
> > +     /* Mask to zero out lower bits of address for alignment */
> > +     align_mask = ~(c.dest_alignment - 1);
> > +     /* Offset of destination address from the end of the source region */
> > +     offset = (c.overlapping) ? -c.dest_alignment : c.dest_alignment;
> > +     addr = (void *) (((unsigned long long) src_addr + c.region_size + offset)
> > +                     & align_mask);
> > +
> > +     /* See comment in get_source_mapping() */
> > +     if (!((unsigned long long) addr & c.dest_alignment))
> > +             addr = (void *) ((unsigned long long) addr | c.dest_alignment);
> > +
> > +     clock_gettime(CLOCK_MONOTONIC, &t_start);
> > +     dest_addr = mremap(src_addr, c.region_size, c.region_size,
> > +                     MREMAP_MAYMOVE|MREMAP_FIXED, (char *) addr);
> > +     clock_gettime(CLOCK_MONOTONIC, &t_end);
> > +
> > +     if (dest_addr == MAP_FAILED) {
> > +             ksft_print_msg("mremap failed: %s\n", strerror(errno));
> > +             ret = -1;
> > +             goto clean_up_src;
> > +     }
> > +
> > +     /* Verify byte pattern after remapping */
> > +     srand(pattern_seed);
> > +     for (i = 0; i < threshold; i++) {
> > +             char c = (char) rand();
> > +
> > +             if (((char *) dest_addr)[i] != c) {
> > +                     ksft_print_msg("Data after remap doesn't match at offset %d\n",
> > +                                     i);
> > +                     ksft_print_msg("Expected: %#x\t Got: %#x\n", c & 0xff,
> > +                                     ((char *) dest_addr)[i] & 0xff);
> > +                     ret = -1;
> > +                     goto clean_up_dest;
> > +             }
> > +     }
> > +
> > +     start_ns = t_start.tv_sec * NS_PER_SEC + t_start.tv_nsec;
> > +     end_ns = t_end.tv_sec * NS_PER_SEC + t_end.tv_nsec;
> > +     ret = end_ns - start_ns;
> > +
> > +/*
> > + * Since the destination address is specified using MREMAP_FIXED, subsequent mremap will unmap any
> > + * previous mapping at the address range specified by dest_addr and region_size. This significantly
> > + * affects the remap time of subsequent tests. So we clean up mappings after each test.
> > + */
> > +clean_up_dest:
> > +     munmap(dest_addr, c.region_size);
> > +clean_up_src:
> > +     munmap(src_addr, c.region_size);
> > +out:
> > +     return ret;
> > +}
> > +
> > +static void run_mremap_test_case(struct test test_case, int *failures, unsigned int threshold_mb,
> > +                              unsigned int pattern_seed)
> > +{
> > +     long long remap_time = remap_region(test_case.config, threshold_mb, pattern_seed);
> > +
> > +     if (remap_time < 0) {
> > +             if (test_case.expect_failure)
> > +                     ksft_test_result_pass("%s\n\tExpected mremap failure\n", test_case.name);
> > +             else {
> > +                     ksft_test_result_fail("%s\n", test_case.name);
> > +                     *failures += 1;
> > +             }
> > +     } else {
> > +             /* Comparing mremap time is only applicable if entire region was faulted in */
> > +             if (threshold_mb == VALIDATION_NO_THRESHOLD ||
> > +                 test_case.config.region_size <= threshold_mb * _1MB)
> > +                     ksft_test_result_pass("%s\n\tmremap time: %12lldns\n",
> > +                                           test_case.name, remap_time);
> > +             else
> > +                     ksft_test_result_pass("%s\n", test_case.name);
> > +     }
> > +}
> > +
> > +static void usage(const char *cmd)
> > +{
> > +     fprintf(stderr,
> > +             "Usage: %s [[-t <threshold_mb>] [-p <pattern_seed>]]\n"
> > +             "-t\t only validate threshold_mb of the remapped region\n"
> > +             "  \t if 0 is supplied no threshold is used; all tests\n"
> > +             "  \t are run and remapped regions validated fully.\n"
> > +             "  \t The default threshold used is 4MB.\n"
> > +             "-p\t provide a seed to generate the random pattern for\n"
> > +             "  \t validating the remapped region.\n", cmd);
> > +}
> > +
> > +static int parse_args(int argc, char **argv, unsigned int *threshold_mb,
> > +                   unsigned int *pattern_seed)
> > +{
> > +     const char *optstr = "t:p:";
> > +     int opt;
> > +
> > +     while ((opt = getopt(argc, argv, optstr)) != -1) {
> > +             switch (opt) {
> > +             case 't':
> > +                     *threshold_mb = atoi(optarg);
> > +                     break;
> > +             case 'p':
> > +                     *pattern_seed = atoi(optarg);
> > +                     break;
> > +             default:
> > +                     usage(argv[0]);
> > +                     return -1;
> > +             }
> > +     }
> > +
> > +     if (optind < argc) {
> > +             usage(argv[0]);
> > +             return -1;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +     int failures = 0;
> > +     int i, run_perf_tests;
> > +     unsigned int threshold_mb = VALIDATION_DEFAULT_THRESHOLD;
> > +     unsigned int pattern_seed;
> > +     time_t t;
> > +
> > +     pattern_seed = (unsigned int) time(&t);
> > +
> > +     if (parse_args(argc, argv, &threshold_mb, &pattern_seed) < 0)
> > +             exit(EXIT_FAILURE);
> > +
> > +     ksft_print_msg("Test configs:\n\tthreshold_mb=%u\n\tpattern_seed=%u\n\n",
> > +                    threshold_mb, pattern_seed);
> > +
> > +     struct test test_cases[] = {
> > +             /* Expected mremap failures */
> > +             MAKE_TEST(_4KB, _4KB, _4KB, OVERLAPPING, EXPECT_FAILURE,
> > +                       "mremap - Source and Destination Regions Overlapping"),
> > +             MAKE_TEST(_4KB, _1KB, _4KB, NON_OVERLAPPING, EXPECT_FAILURE,
> > +                       "mremap - Destination Address Misaligned (1KB-aligned)"),
> > +             MAKE_TEST(_1KB, _4KB, _4KB, NON_OVERLAPPING, EXPECT_FAILURE,
> > +                       "mremap - Source Address Misaligned (1KB-aligned)"),
> > +
> > +             /* Src addr PTE aligned */
> > +             MAKE_TEST(PTE, PTE, _8KB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "8KB mremap - Source PTE-aligned, Destination PTE-aligned"),
> > +
> > +             /* Src addr 1MB aligned */
> > +             MAKE_TEST(_1MB, PTE, _2MB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "2MB mremap - Source 1MB-aligned, Destination PTE-aligned"),
> > +             MAKE_TEST(_1MB, _1MB, _2MB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "2MB mremap - Source 1MB-aligned, Destination 1MB-aligned"),
> > +
> > +             /* Src addr PMD aligned */
> > +             MAKE_TEST(PMD, PTE, _4MB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "4MB mremap - Source PMD-aligned, Destination PTE-aligned"),
> > +             MAKE_TEST(PMD, _1MB, _4MB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "4MB mremap - Source PMD-aligned, Destination 1MB-aligned"),
> > +             MAKE_TEST(PMD, PMD, _4MB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "4MB mremap - Source PMD-aligned, Destination PMD-aligned"),
> > +
> > +             /* Src addr PUD aligned */
> > +             MAKE_TEST(PUD, PTE, _2GB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "2GB mremap - Source PUD-aligned, Destination PTE-aligned"),
> > +             MAKE_TEST(PUD, _1MB, _2GB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "2GB mremap - Source PUD-aligned, Destination 1MB-aligned"),
> > +             MAKE_TEST(PUD, PMD, _2GB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "2GB mremap - Source PUD-aligned, Destination PMD-aligned"),
> > +             MAKE_TEST(PUD, PUD, _2GB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "2GB mremap - Source PUD-aligned, Destination PUD-aligned"),
> > +     };
> > +
> > +     struct test perf_test_cases[] = {
> > +             /* mremap 1GB region - Page table level aligned time comparison */
> > +             MAKE_TEST(PTE, PTE, _1GB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "1GB mremap - Source PTE-aligned, Destination PTE-aligned"),
> > +             MAKE_TEST(PMD, PMD, _1GB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "1GB mremap - Source PMD-aligned, Destination PMD-aligned"),
> > +             MAKE_TEST(PUD, PUD, _1GB, NON_OVERLAPPING, EXPECT_SUCCESS,
> > +                       "1GB mremap - Source PUD-aligned, Destination PUD-aligned"),
> > +     };
> > +
> > +     run_perf_tests =  (threshold_mb == VALIDATION_NO_THRESHOLD) ||
> > +                             (threshold_mb * _1MB >= _1GB);
> > +
> > +     ksft_set_plan(ARRAY_SIZE(test_cases) + (run_perf_tests ?
> > +                   ARRAY_SIZE(perf_test_cases) : 0));
> > +
> > +     for (i = 0; i < ARRAY_SIZE(test_cases); i++)
> > +             run_mremap_test_case(test_cases[i], &failures, threshold_mb, pattern_seed);
> > +
> > +     if (run_perf_tests) {
> > +             ksft_print_msg("\n%s\n",
> > +              "mremap HAVE_MOVE_PMD/PUD optimization time comparison for 1GB region:");
> > +             for (i = 0; i < ARRAY_SIZE(perf_test_cases); i++)
> > +                     run_mremap_test_case(perf_test_cases[i], &failures, threshold_mb,
> > +                                          pattern_seed);
> > +     }
> > +
> > +     if (failures > 0)
> > +             ksft_exit_fail();
> > +     else
> > +             ksft_exit_pass();
> > +}
> > diff --git a/tools/testing/selftests/vm/run_vmtests b/tools/testing/selftests/vm/run_vmtests
> > index a3f4f30f0a2e..d578ad831813 100755
> > --- a/tools/testing/selftests/vm/run_vmtests
> > +++ b/tools/testing/selftests/vm/run_vmtests
> > @@ -241,6 +241,17 @@ else
> >       echo "[PASS]"
> >   fi
> >
> > +echo "-------------------"
> > +echo "running mremap_test"
> > +echo "-------------------"
> > +./mremap_test
> > +if [ $? -ne 0 ]; then
> > +     echo "[FAIL]"
> > +     exitcode=1
> > +else
> > +     echo "[PASS]"
> > +fi
> > +
> >   echo "-----------------"
> >   echo "running thuge-gen"
> >   echo "-----------------"
> >
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@...roid.com.
>

Powered by blists - more mailing lists