Date:   Fri, 25 Feb 2022 17:35:39 -0700
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Muhammad Usama Anjum <usama.anjum@...labora.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Shuah Khan <shuah@...nel.org>
Cc:     kernel@...labora.com, kernelci@...ups.io,
        Will Deacon <will@...nel.org>,
        Gabriel Krisman Bertazi <krisman@...labora.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-kselftest@...r.kernel.org,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH V3] selftests: vm: Add test for Soft-Dirty PTE bit

On 2/24/22 2:23 PM, Muhammad Usama Anjum wrote:
> This introduces three tests:
> 1) Sanity check soft dirty basic semantics: allocate area, clean, dirty,
> check if the SD bit flipped.
> 2) Check VMA reuse: validate the VM_SOFTDIRTY usage
> 3) Check soft-dirty on huge pages
> 
> This was motivated by Will Deacon's fix commit 912efa17e512 ("mm: proc:
> Invalidate TLB after clearing soft-dirty page state"). I was tracking the
> same issue that he fixed, and this test would have caught it.
> 
> Cc: Will Deacon <will@...nel.org>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@...labora.com>
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@...labora.com>
> ---
> Changes in V3:
> Move test to selftests/vm
> Use kselftest macros
> Minor updates to make code more maintainable
> Add configurations in config file
> 
> Tests of soft dirty bit in this patch and in
> selftest/vm/madv_populate.c are non-overlapping.
> 
> V2 of this patch:
> https://lore.kernel.org/lkml/20210603151518.2437813-1-krisman@collabora.com/
> ---
>   tools/testing/selftests/vm/.gitignore   |   1 +
>   tools/testing/selftests/vm/Makefile     |   1 +
>   tools/testing/selftests/vm/config       |   2 +
>   tools/testing/selftests/vm/soft-dirty.c | 238 ++++++++++++++++++++++++
>   4 files changed, 242 insertions(+)
>   create mode 100644 tools/testing/selftests/vm/soft-dirty.c
> 
> diff --git a/tools/testing/selftests/vm/.gitignore b/tools/testing/selftests/vm/.gitignore
> index d7507f3c7c76..3cb4fa771ec2 100644
> --- a/tools/testing/selftests/vm/.gitignore
> +++ b/tools/testing/selftests/vm/.gitignore
> @@ -29,5 +29,6 @@ write_to_hugetlbfs
>   hmm-tests
>   memfd_secret
>   local_config.*
> +soft-dirty
>   split_huge_page_test
>   ksm_tests
> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> index 5e43f072f5b7..de9b13d018c3 100644
> --- a/tools/testing/selftests/vm/Makefile
> +++ b/tools/testing/selftests/vm/Makefile
> @@ -47,6 +47,7 @@ TEST_GEN_FILES += on-fault-limit
>   TEST_GEN_FILES += thuge-gen
>   TEST_GEN_FILES += transhuge-stress
>   TEST_GEN_FILES += userfaultfd
> +TEST_GEN_FILES += soft-dirty
>   TEST_GEN_FILES += split_huge_page_test
>   TEST_GEN_FILES += ksm_tests
>   
> diff --git a/tools/testing/selftests/vm/config b/tools/testing/selftests/vm/config
> index 60e82da0de85..be087c4bc396 100644
> --- a/tools/testing/selftests/vm/config
> +++ b/tools/testing/selftests/vm/config
> @@ -4,3 +4,5 @@ CONFIG_TEST_VMALLOC=m
>   CONFIG_DEVICE_PRIVATE=y
>   CONFIG_TEST_HMM=m
>   CONFIG_GUP_TEST=y
> +CONFIG_TRANSPARENT_HUGEPAGE=y
> +CONFIG_MEM_SOFT_DIRTY=y
> diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c
> new file mode 100644
> index 000000000000..f56c215e0ece
> --- /dev/null
> +++ b/tools/testing/selftests/vm/soft-dirty.c
> @@ -0,0 +1,238 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdbool.h>
> +#include <fcntl.h>
> +#include <stdint.h>
> +#include <malloc.h>
> +#include <sys/mman.h>
> +#include "../kselftest.h"
> +
> +#define PAGEMAP_PATH		"/proc/self/pagemap"

Why is this names PATH - it is the file name right?

> +#define CLEAR_REFS_PATH		"/proc/self/clear_refs"

Same here  - why is this named _PATH?

Let's line these defines up - makes it easier to read

> +#define SMAP_PATH		"/proc/self/smaps"

Same here - why is this named _PATH?

> +#define PMD_SIZE_PATH		"/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"

Same  here  - why is this named _PATH?

> +#define MAX_LINE_LENGTH		512
> +#define TEST_ITERATIONS		10000
> +#define PAGE_NUM_TO_TEST	2
> +
> +int clear_refs;
> +int pagemap;
> +

Get rid of these globals and pass these in - please find name
that clearly indicates them as fds

> +int pagesize;
> +int mmap_size;	/* Size of test region */

Get rid of these globals and pass these in - please find name
that clearly indicates

> +
> +static void clear_all_refs(void)
> +{
> +	const char *ctrl = "4";
> +
> +	if (write(clear_refs, ctrl, strlen(ctrl)) != strlen(ctrl))
> +		ksft_exit_fail_msg("%s: failed to clear references\n", __func__);
> +}
> +
> +static void touch_page(char *map, int n)
> +{
> +	map[(pagesize * n) + 1]++;
> +}
> +
> +static int check_page(char *start, int page_num, int clear)
> +{
> +	unsigned long pfn = (unsigned long)start / pagesize;
> +	uint64_t entry;
> +	int ret;
> +
> +	ret = pread(pagemap, &entry, sizeof(entry), (pfn + page_num) * sizeof(entry));
> +	if (ret != sizeof(entry))
> +		ksft_exit_fail_msg("reading pagemap failed\n");
> +	if (clear)
> +		clear_all_refs();
> +
> +	return ((entry >> 55) & 1);

Add a define for 55 insead of hardcoding with a meaningful name
that describes what this value is.

> +}
> +
> +static void test_simple(void)
> +{
> +	int i;
> +	char *map;
> +
> +	map = aligned_alloc(pagesize, mmap_size);
> +	if (!map)
> +		ksft_exit_fail_msg("mmap failed\n");
> +
> +	clear_all_refs();

If clear_all_refs() fails and exits, when does map get freed?

> +
> +	for (i = 0 ; i < TEST_ITERATIONS; i++) {
> +		if (check_page(map, PAGE_NUM_TO_TEST, 1) == 1) {
> +			ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
> +			break;
> +		}
> +
> +		touch_page(map, 2);
> +
> +		if (check_page(map, PAGE_NUM_TO_TEST, 1) == 0) {
> +			ksft_print_msg("dirty bit was 0, but should be 1 (i=%d)\n", i);
> +			break;
> +		}
> +
> +	}
> +	free(map);
> +
> +	ksft_test_result(i == TEST_ITERATIONS, "Test %s\n", __func__);
> +}
> +
> +static void test_vma_reuse(void)
> +{
> +	char *map, *map2;
> +
> +	map = (char *) 0x900000000000;
> +	map = mmap(map, mmap_size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
> +	if (map == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap failed");
> +
> +	clear_all_refs();
> +	touch_page(map, PAGE_NUM_TO_TEST);
> +
> +	munmap(map, mmap_size);
> +	map2 = mmap(map, mmap_size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
> +	if (map2 == MAP_FAILED)
> +		ksft_exit_fail_msg("mmap failed");
> +
> +	ksft_test_result(map == map2, "Test %s reused memory location\n", __func__);
> +
> +	ksft_test_result(check_page(map, PAGE_NUM_TO_TEST, 1) != 0,
> +			 "Test %s dirty bit of previous page\n", __func__);
> +
> +	munmap(map2, mmap_size);
> +}
> +
> +/*
> + * read_pmd_pagesize(), check_for_pattern() and check_huge() adapted
> + * from 'tools/testing/selftest/vm/split_huge_page_test.c'

Don't use the full path here - just use the file name

> + */
> +static uint64_t read_pmd_pagesize(void)
> +{
> +	int fd;
> +	char buf[20];
> +	ssize_t num_read;
> +
> +	fd = open(PMD_SIZE_PATH, O_RDONLY);
> +	if (fd == -1)
> +		ksft_exit_fail_msg("Open hpage_pmd_size failed\n");
> +
> +	num_read = read(fd, buf, 19);
> +	if (num_read < 1) {
> +		close(fd);
> +		ksft_exit_fail_msg("Read hpage_pmd_size failed\n");
> +	}
> +	buf[num_read] = '\0';
> +	close(fd);
> +
> +	return strtoul(buf, NULL, 10);
> +}
> +
> +static bool check_for_pattern(FILE *fp, const char *pattern, char *buf)
> +{
> +	while (fgets(buf, MAX_LINE_LENGTH, fp) != NULL) {
> +		if (!strncmp(buf, pattern, strlen(pattern)))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static uint64_t check_huge(void *addr)
> +{
> +	uint64_t thp = 0;
> +	int ret;
> +	FILE *fp;
> +	char buffer[MAX_LINE_LENGTH];
> +	char addr_pattern[MAX_LINE_LENGTH];
> +
> +	ret = snprintf(addr_pattern, MAX_LINE_LENGTH, "%08lx-",
> +		       (unsigned long) addr);
> +	if (ret >= MAX_LINE_LENGTH)
> +		ksft_print_msg("%s: Pattern is too long\n", __func__);

Okay. Pattern is too log - does the test continue?

> +
> +	fp = fopen(SMAP_PATH, "r");
> +	if (!fp)
> +		ksft_print_msg("%s: Failed to open file %s\n", __func__, SMAP_PATH);

Same commnet about root user


> +
> +	if (!check_for_pattern(fp, addr_pattern, buffer))
> +		goto err_out;
> +
> +	/*
> +	 * Fetch the AnonHugePages: in the same block and check the number of
> +	 * hugepages.
> +	 */
> +	if (!check_for_pattern(fp, "AnonHugePages:", buffer))
> +		goto err_out;
> +
> +	if (sscanf(buffer, "AnonHugePages:%10ld kB", &thp) != 1)
> +		ksft_print_msg("Reading smap error\n");
> +
> +err_out:
> +	fclose(fp);
> +
> +	return thp;
> +}
> +
> +static void test_hugepage(void)
> +{
> +	char *map;
> +	int i, ret;
> +	size_t hpage_len = read_pmd_pagesize();
> +
> +	map = memalign(hpage_len, hpage_len);
> +	if (!map)
> +		ksft_exit_fail_msg("memalign failed\n");
> +
> +	ret = madvise(map, hpage_len, MADV_HUGEPAGE);
> +	if (ret)
> +		ksft_exit_fail_msg("madvise failed %d\n", ret);
> +
> +	for (i = 0; i < hpage_len; i++)
> +		map[i] = (char)i;
> +
> +	ksft_test_result(check_huge(map), "Test %s huge page allocation\n", __func__);
> +
> +	clear_all_refs();
> +	for (i = 0 ; i < TEST_ITERATIONS ; i++) {
> +		if (check_page(map, PAGE_NUM_TO_TEST, 1) == 1) {
> +			ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
> +			break;
> +		}
> +
> +		touch_page(map, PAGE_NUM_TO_TEST);
> +
> +		if (check_page(map, PAGE_NUM_TO_TEST, 1) == 0) {
> +			ksft_print_msg("dirty bit was 0, but should be 1 (i=%d)\n", i);
> +			break;
> +		}
> +	}
> +
> +	ksft_test_result(i == TEST_ITERATIONS, "Test %s dirty bit\n", __func__);
> +
> +	munmap(map, mmap_size);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	ksft_print_header();
> +	ksft_set_plan(5);
> +
> +	pagemap = open(PAGEMAP_PATH, O_RDONLY);
> +	if (pagemap < 0)
> +		ksft_exit_fail_msg("Failed to open %s\n", PAGEMAP_PATH);

Can non-root user open this file? If not, when non-root user fails to
open, it is a skip not fail

> +
> +	clear_refs = open(CLEAR_REFS_PATH, O_WRONLY);
> +	if (clear_refs < 0)
> +		ksft_exit_fail_msg("Failed to open %s\n", CLEAR_REFS_PATH);

Same comment as above here

Also why would you want to define clear_refs as global? Why not define it
here - make it clear that it is fd

> +
> +	pagesize = getpagesize();

Why is pagesize global?

> +	mmap_size = 10 * pagesize;

Same here?

> +
> +	test_simple();
> +	test_vma_reuse();
> +	test_hugepage();

What happens when these tests fail?

> +
> +	return ksft_exit_pass();
> +}
> 

Where do CLEAR_REFS_PATH etc. get closed. Please take a look
at the error paths carefully. I would like to see the output for
this test. Please include it in the change log.

thanks,
-- Shuah

Powered by blists - more mailing lists