[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <901f1117-e1ba-d5f3-f711-b3b581bce6cb@linuxfoundation.org>
Date: Fri, 28 Jan 2022 16:46:03 -0700
From: Shuah Khan <skhan@...uxfoundation.org>
To: Mike Kravetz <mike.kravetz@...cle.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Cc: Naoya Horiguchi <naoya.horiguchi@...ux.dev>,
David Hildenbrand <david@...hat.com>,
Axel Rasmussen <axelrasmussen@...gle.com>,
Mina Almasry <almasrymina@...gle.com>,
Michal Hocko <mhocko@...e.com>, Peter Xu <peterx@...hat.com>,
Andrea Arcangeli <aarcange@...hat.com>,
Shuah Khan <shuah@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 2/3] selftests/vm: add hugetlb madvise MADV_DONTNEED
MADV_REMOVE test
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@...cle.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.
> 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.
> + if (addr == MAP_FAILED) {
> + perror("mmap");
> + exit(1);
> + }
> + /* unmap first and last page so we know nothing is mapped there */
> + if (munmap(addr, huge_page_size) ||
> + munmap(addr + 11 * huge_page_size, huge_page_size)) {
> + perror("munmap");
> + exit(1);
> + }
> + addr = addr + huge_page_size;
> +
> + write_fault_pages(addr, 10);
What is 10 here? Any significance to this value? Add a define for it
so it it clear why it is used.
> + validate_free_pages(free_hugepages - 10);
> +
> + /* addr before mapping should fail */
> + ret = madvise(addr - base_page_size, 10 * huge_page_size,
> + MADV_DONTNEED);
> + if (!ret) {
> + printf("Unexpected success of madvise call with invalid addr line %d\n",
> + __LINE__);
> + exit(1);
> + }
> +
> + /* addr + length after mapping should fail */
> + ret = madvise(addr, (10 * huge_page_size) + base_page_size,
> + MADV_DONTNEED);
> + if (!ret) {
> + printf("Unexpected success of madvise call with invalid length line %d\n",
> + __LINE__);
> + exit(1);
> + }
> +
> + (void)munmap(addr, 10 * huge_page_size);
Same comment on use of 10.
> +
> + /*
> + * Test alignment of MADV_DONTNEED addr and length arguments
> + */
> + addr = mmap(NULL, 10 * huge_page_size, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
> + -1, 0);
> + if (addr == MAP_FAILED) {
> + perror("mmap");
> + exit(1);
> + }
> + write_fault_pages(addr, 10);
Ditto
> + validate_free_pages(free_hugepages - 10);
Ditto
> +
> + /* addr should be aligned down to huge page size */
> + if (madvise(addr + base_page_size,
> + 10 * huge_page_size - base_page_size, MADV_DONTNEED)) {
> + perror("madvise");
> + exit(1);
> + }
> +
> + /* should free all pages in mapping */
> + validate_free_pages(free_hugepages);
> +
> + write_fault_pages(addr, 10);
> + validate_free_pages(free_hugepages - 10);
> +
> + /* addr + length should be aligned up to huge page size */
> + if (madvise(addr, (10 * huge_page_size) - base_page_size,
> + MADV_DONTNEED)) {
> + perror("madvise");
> + exit(1);
> + }
> +
> + /* should free all pages in mapping */
> + validate_free_pages(free_hugepages);
> +
> + (void)munmap(addr, 10 * huge_page_size);
> +
> + /*
> + * Test MADV_DONTNEED on anonymous private mapping
> + */
> + addr = mmap(NULL, 10 * huge_page_size, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
> + -1, 0);
> + if (addr == MAP_FAILED) {
> + perror("mmap");
> + exit(1);
> + }
> + write_fault_pages(addr, 10);
> + validate_free_pages(free_hugepages - 10);
> +
> + if (madvise(addr, 10 * huge_page_size, MADV_DONTNEED)) {
> + perror("madvise");
> + exit(1);
> + }
> +
> + /* should free all pages in mapping */
> + validate_free_pages(free_hugepages);
> +
> + (void)munmap(addr, 10 * huge_page_size);
> +
> + /*
> + * Test MADV_DONTNEED on private mapping of hugetlb file
> + */
> + if (fallocate(fd, 0, 0, 10 * huge_page_size)) {
> + perror("fallocate");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages - 10);
> +
> + addr = mmap(NULL, 10 * huge_page_size, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE, fd, 0);
> + if (addr == MAP_FAILED) {
> + perror("mmap");
> + exit(1);
> + }
> +
> + /* read should not consume any pages */
> + read_fault_pages(addr, 10);
> + validate_free_pages(free_hugepages - 10);
> +
> + /* madvise should not free any pages */
> + if (madvise(addr, 10 * huge_page_size, MADV_DONTNEED)) {
> + perror("madvise");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages - 10);
> +
> + /* writes should allocate private pages */
> + write_fault_pages(addr, 10);
> + validate_free_pages(free_hugepages - 20);
> +
> + /* madvise should free private pages */
> + if (madvise(addr, 10 * huge_page_size, MADV_DONTNEED)) {
> + perror("madvise");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages - 10);
> +
> + /* writes should allocate private pages */
> + write_fault_pages(addr, 10);
> + validate_free_pages(free_hugepages - 20);
> +
> + /*
> + * The fallocate below certainly should free the pages associated
> + * with the file. However, pages in the private mapping are also
> + * freed. This is not the 'correct' behavior, but is expected
> + * because this is how it has worked since the initial hugetlb
> + * implementation.
> + */
> + if (fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> + 0, 10 * huge_page_size)) {
> + perror("fallocate");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages);
> +
> + (void)munmap(addr, 10 * huge_page_size);
> +
> + /*
> + * Test MADV_DONTNEED on shared mapping of hugetlb file
> + */
> + if (fallocate(fd, 0, 0, 10 * huge_page_size)) {
> + perror("fallocate");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages - 10);
> +
> + addr = mmap(NULL, 10 * huge_page_size, PROT_READ | PROT_WRITE,
> + MAP_SHARED, fd, 0);
> + if (addr == MAP_FAILED) {
> + perror("mmap");
> + exit(1);
> + }
> +
> + /* write should not consume any pages */
> + write_fault_pages(addr, 10);
> + validate_free_pages(free_hugepages - 10);
> +
> + /* madvise should not free any pages */
> + if (madvise(addr, 10 * huge_page_size, MADV_DONTNEED)) {
> + perror("madvise");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages - 10);
> +
> + /*
> + * Test MADV_REMOVE on shared mapping of hugetlb file
> + *
> + * madvise is same as hole punch and should free all pages.
> + */
> + if (madvise(addr, 10 * huge_page_size, MADV_REMOVE)) {
> + perror("madvise");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages);
> + (void)munmap(addr, 10 * huge_page_size);
> +
> + /*
> + * Test MADV_REMOVE on shared and private mapping of hugetlb file
> + */
> + if (fallocate(fd, 0, 0, 10 * huge_page_size)) {
> + perror("fallocate");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages - 10);
> +
> + addr = mmap(NULL, 10 * huge_page_size, PROT_READ | PROT_WRITE,
> + MAP_SHARED, fd, 0);
> + if (addr == MAP_FAILED) {
> + perror("mmap");
> + exit(1);
> + }
> +
> + /* shared write should not consume any additional pages */
> + write_fault_pages(addr, 10);
> + validate_free_pages(free_hugepages - 10);
> +
> + addr2 = mmap(NULL, 10 * huge_page_size, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE, fd, 0);
> + if (addr2 == MAP_FAILED) {
> + perror("mmap");
> + exit(1);
> + }
> +
> + /* private read should not consume any pages */
> + read_fault_pages(addr2, 10);
> + validate_free_pages(free_hugepages - 10);
> +
> + /* private write should consume additional pages */
> + write_fault_pages(addr2, 10);
> + validate_free_pages(free_hugepages - 20);
> +
> + /* madvise of shared mapping should not free any pages */
> + if (madvise(addr, 10 * huge_page_size, MADV_DONTNEED)) {
> + perror("madvise");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages - 20);
> +
> + /* madvise of private mapping should free private pages */
> + if (madvise(addr2, 10 * huge_page_size, MADV_DONTNEED)) {
> + perror("madvise");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages - 10);
> +
> + /* private write should consume additional pages again */
> + write_fault_pages(addr2, 10);
> + validate_free_pages(free_hugepages - 20);
> +
> + /*
> + * madvise should free both file and private pages although this is
> + * not correct. private pages should not be freed, but this is
> + * expected. See comment associated with FALLOC_FL_PUNCH_HOLE call.
> + */
> + if (madvise(addr, 10 * huge_page_size, MADV_REMOVE)) {
> + perror("madvise");
> + exit(1);
> + }
> + validate_free_pages(free_hugepages);
> +
> + (void)munmap(addr, 10 * huge_page_size);
> + (void)munmap(addr2, 10 * huge_page_size);
> +
> + close(fd);
> + unlink(argv[1]);
> + return 0;
> +}
Same comment on all usages of value 10
> diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/vm/run_vmtests.sh
> index 75d401741394..e0daf9ff0cbe 100755
> --- a/tools/testing/selftests/vm/run_vmtests.sh
> +++ b/tools/testing/selftests/vm/run_vmtests.sh
> @@ -119,6 +119,18 @@ else
> echo "[PASS]"
> fi
>
> +echo "-----------------------"
> +echo "running hugetlb-madvise"
> +echo "-----------------------"
> +./hugetlb-madvise $mnt/madvise-test
> +if [ $? -ne 0 ]; then
> + echo "[FAIL]"
> + exitcode=1
> +else
> + echo "[PASS]"
> +fi
> +rm -f $mnt/madvise-test
> +
> echo "NOTE: The above hugetlb tests provide minimal coverage. Use"
> echo " https://github.com/libhugetlbfs/libhugetlbfs.git for"
> echo " hugetlb regression testing."
>
With these comments addressed/explained
Reviewed-by: Shuah Khan <skhan@...uxfoundation.org>
thanks,
-- Shuah
Powered by blists - more mailing lists