Re: [PATCH 2/3] selftests/vm: add hugetlb madvise MADV_DONTNEED MADV_REMOVE test

[Mike Kravetz <mike.kravetz@oracle.com>]

On 1/28/22 15:46, Shuah Khan wrote:
> On 1/28/22 3:26 PM, Mike Kravetz wrote:
>> Now that MADV_DONTNEED support for hugetlb is enabled, add corresponding
>> tests.  MADV_REMOVE has been enabled for some time, but no tests exist
>> so add them as well.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>   tools/testing/selftests/vm/Makefile          |   1 +
>>   tools/testing/selftests/vm/hugetlb-madvise.c | 401 +++++++++++++++++++
>>   tools/testing/selftests/vm/run_vmtests.sh    |  12 +
>>   3 files changed, 414 insertions(+)
>>   create mode 100644 tools/testing/selftests/vm/hugetlb-madvise.c
>>
>> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
>> index 1607322a112c..f60cf43bbf61 100644
>> --- a/tools/testing/selftests/vm/Makefile
>> +++ b/tools/testing/selftests/vm/Makefile
>> @@ -28,6 +28,7 @@ LDLIBS = -lrt -lpthread
>>   TEST_GEN_FILES = compaction_test
>>   TEST_GEN_FILES += gup_test
>>   TEST_GEN_FILES += hmm-tests
>> +TEST_GEN_FILES += hugetlb-madvise
> 
> Please update .gitignore with the new binary.
> 

Will do.

>>   TEST_GEN_FILES += hugepage-mmap
>>   TEST_GEN_FILES += hugepage-mremap
>>   TEST_GEN_FILES += hugepage-shm
>> diff --git a/tools/testing/selftests/vm/hugetlb-madvise.c b/tools/testing/selftests/vm/hugetlb-madvise.c
>> new file mode 100644
>> index 000000000000..31c302528f2c
>> --- /dev/null
>> +++ b/tools/testing/selftests/vm/hugetlb-madvise.c
>> @@ -0,0 +1,401 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * hugepage-madvise:
>> + *
>> + * Basic functional testing of madvise MADV_DONTNEED and MADV_REMOVE
>> + * on hugetlb mappings.
>> + *
>> + * Before running this test, make sure the administrator has pre-allocated
>> + * at least MIN_FREE_PAGES hugetlb pages and they are free.  In addition,
>> + * the test takes an argument that is the path to a file in a hugetlbfs
>> + * filesystem.  Therefore, a hugetlbfs filesystem must be mounted on some
>> + * directory.
>> + */
>> +
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <sys/mman.h>
>> +#define __USE_GNU
>> +#include <fcntl.h>
>> +
>> +#define USAGE    "USAGE: %s <hugepagefile_name>\n"
>> +#define MIN_FREE_PAGES    20
>> +
>> +#define validate_free_pages(exp_free)                    \
>> +    do {                                \
>> +        int fhp = get_free_hugepages();                \
>> +        if (fhp != (exp_free)) {                \
>> +            printf("Unexpected number of free huge "    \
>> +                "pages line %d\n", __LINE__);        \
>> +            exit(1);                    \
>> +        }                            \
>> +    } while (0)
>> +
>> +unsigned long huge_page_size;
>> +unsigned long base_page_size;
>> +
>> +/*
>> + * default_huge_page_size copied from mlock2-tests.c
>> + */
>> +unsigned long default_huge_page_size(void)
>> +{
>> +    unsigned long hps = 0;
>> +    char *line = NULL;
>> +    size_t linelen = 0;
>> +    FILE *f = fopen("/proc/meminfo", "r");
>> +
>> +    if (!f)
>> +        return 0;
>> +    while (getline(&line, &linelen, f) > 0) {
>> +        if (sscanf(line, "Hugepagesize:       %lu kB", &hps) == 1) {
>> +            hps <<= 10;
>> +            break;
>> +        }
>> +    }
>> +
>> +    free(line);
>> +    fclose(f);
>> +    return hps;
>> +}
>> +
>> +unsigned long get_free_hugepages(void)
>> +{
>> +    unsigned long fhp = 0;
>> +    char *line = NULL;
>> +    size_t linelen = 0;
>> +    FILE *f = fopen("/proc/meminfo", "r");
>> +
>> +    if (!f)
>> +        return fhp;
>> +    while (getline(&line, &linelen, f) > 0) {
>> +        if (sscanf(line, "HugePages_Free:      %lu", &fhp) == 1)
>> +            break;
>> +    }
>> +
>> +    free(line);
>> +    fclose(f);
>> +    return fhp;
>> +}
>> +
>> +void write_fault_pages(void *addr, unsigned long nr_pages)
>> +{
>> +    unsigned long i;
>> +
>> +    for (i = 0; i < nr_pages; i++)
>> +        *((unsigned long *)(addr + (i * huge_page_size))) = i;
>> +}
>> +
>> +void read_fault_pages(void *addr, unsigned long nr_pages)
>> +{
>> +    unsigned long i, tmp;
>> +
>> +    for (i = 0; i < nr_pages; i++)
>> +        tmp += *((unsigned long *)(addr + (i * huge_page_size)));
>> +}
>> +
>> +int main(int argc, char **argv)
>> +{
>> +    unsigned long free_hugepages;
>> +    void *addr, *addr2;
>> +    int fd;
>> +    int ret;
>> +
>> +    if (argc != 2) {
>> +        printf(USAGE, argv[0]);
>> +        exit(1);
>> +    }
>> +
>> +    huge_page_size = default_huge_page_size();
>> +    if (!huge_page_size) {
>> +        printf("Unable to determine huge page size, exiting!\n");
>> +        exit(1);
>> +    }
>> +    base_page_size = sysconf(_SC_PAGE_SIZE);
>> +    if (!huge_page_size) {
>> +        printf("Unable to determine base page size, exiting!\n");
>> +        exit(1);
>> +    }
>> +
>> +    free_hugepages = get_free_hugepages();
>> +    if (free_hugepages < MIN_FREE_PAGES) {
>> +        printf("Not enough free huge pages to test, exiting!\n");
>> +        exit(1);
>> +    }
>> +
>> +    fd = open(argv[1], O_CREAT | O_RDWR, 0755);
>> +    if (fd < 0) {
>> +        perror("Open failed");
>> +        exit(1);
>> +    }
>> +
>> +    /*
>> +     * Test validity of MADV_DONTNEED addr and length arguments
>> +     */
>> +    addr = mmap(NULL, 12 * huge_page_size, PROT_READ | PROT_WRITE,
>> +            MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
>> +            -1, 0);
> 
> What is 12 here? Any significance to this value? Add a define for it
> so it it clear why it is used.

I'll add a commented #define for the 10 as that is used throughout the
file.  I'll add a comment here about mapping and then upmapping areas to
make sure nothing is mapped at specific addresses.

Thanks for taking a look!
-- 
Mike Kravetz