Re: [PATCH V4 1/2] selftests: vm: bring common functions to a new file

[David Hildenbrand <david@redhat.com>]

On 15.03.22 09:50, Muhammad Usama Anjum wrote:
> Bring common functions to a new file. These functions can be used in the
> new tests. This helps in code duplication.
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
>  tools/testing/selftests/vm/Makefile           |   7 +-
>  tools/testing/selftests/vm/madv_populate.c    |  34 +-----
>  .../selftests/vm/split_huge_page_test.c       |  77 +------------
>  tools/testing/selftests/vm/vm_util.c          | 103 ++++++++++++++++++
>  tools/testing/selftests/vm/vm_util.h          |  15 +++
>  5 files changed, 125 insertions(+), 111 deletions(-)
>  create mode 100644 tools/testing/selftests/vm/vm_util.c
>  create mode 100644 tools/testing/selftests/vm/vm_util.h
> 
> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> index 5e43f072f5b76..4e68edb26d6b6 100644
> --- a/tools/testing/selftests/vm/Makefile
> +++ b/tools/testing/selftests/vm/Makefile
> @@ -34,7 +34,7 @@ TEST_GEN_FILES += hugepage-mremap
>  TEST_GEN_FILES += hugepage-shm
>  TEST_GEN_FILES += hugepage-vmemmap
>  TEST_GEN_FILES += khugepaged
> -TEST_GEN_FILES += madv_populate
> +TEST_GEN_PROGS = madv_populate
>  TEST_GEN_FILES += map_fixed_noreplace
>  TEST_GEN_FILES += map_hugetlb
>  TEST_GEN_FILES += map_populate
> @@ -47,7 +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 += split_huge_page_test
> +TEST_GEN_PROGS += split_huge_page_test
>  TEST_GEN_FILES += ksm_tests
>  
>  ifeq ($(MACHINE),x86_64)
> @@ -91,6 +91,9 @@ TEST_FILES := test_vmalloc.sh
>  KSFT_KHDR_INSTALL := 1
>  include ../lib.mk
>  
> +$(OUTPUT)/madv_populate: vm_util.c
> +$(OUTPUT)/split_huge_page_test: vm_util.c
> +


[...]

> +// SPDX-License-Identifier: GPL-2.0
> +#include <stdbool.h>
> +#include <string.h>
> +#include "vm_util.h"
> +
> +uint64_t pagemap_get_entry(int fd, char *start)
> +{
> +	const unsigned long pfn = (unsigned long)start / getpagesize();
> +	uint64_t entry;
> +	int ret;
> +
> +	ret = pread(fd, &entry, sizeof(entry), pfn * sizeof(entry));
> +	if (ret != sizeof(entry))
> +		ksft_exit_fail_msg("reading pagemap failed\n");
> +	return entry;
> +}
> +
> +bool pagemap_is_softdirty(int fd, char *start)
> +{
> +	uint64_t entry = pagemap_get_entry(fd, start);
> +
> +	return ((entry >> DIRTY_BIT_LOCATION) & 1);
> +}

Please leave code you're moving around as untouched as possible to avoid
unrelated bugs that happen by mistake and are hard to review.

> +
> +void clear_softdirty(void)
> +{
> +	int ret;
> +	const char *ctrl = "4";
> +	int fd = open("/proc/self/clear_refs", O_WRONLY);
> +
> +	if (fd < 0)
> +		ksft_exit_fail_msg("opening clear_refs failed\n");
> +	ret = write(fd, ctrl, strlen(ctrl));
> +	close(fd);
> +	if (ret != strlen(ctrl))
> +		ksft_exit_fail_msg("writing clear_refs failed\n");
> +}
> +
> +
> +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;
> +}
> +
> +uint64_t read_pmd_pagesize(void)
> +{
> +	int fd;
> +	char buf[20];
> +	ssize_t num_read;
> +
> +	fd = open(PMD_SIZE, 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);
> +}
> +
> +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_exit_fail_msg("%s: Pattern is too long\n", __func__);
> +
> +	fp = fopen(SMAP, "r");
> +	if (!fp)
> +		ksft_exit_fail_msg("%s: Failed to open file %s\n", __func__, SMAP);
> +
> +	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_exit_fail_msg("Reading smap error\n");
> +
> +err_out:
> +	fclose(fp);
> +	return thp;
> +}
> diff --git a/tools/testing/selftests/vm/vm_util.h b/tools/testing/selftests/vm/vm_util.h
> new file mode 100644
> index 0000000000000..7522dbb859f0f
> --- /dev/null
> +++ b/tools/testing/selftests/vm/vm_util.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <stdint.h>
> +#include <fcntl.h>
> +#include "../kselftest.h"
> +
> +#define	PMD_SIZE		"/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"

Ehm no. PMD_SIZE_PATH at best -- just as it used to.

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

SMAPS_PATH

> +#define	DIRTY_BIT_LOCATION	55

Please inline that just as it used to. There is no value in a magic
define without any proper namespace.

> +#define	MAX_LINE_LENGTH		512

This used to be 500. Why the change?


Also: weird indentation and these all look like the should go into
vm_util.c. They are not used outside that file.



-- 
Thanks,

David / dhildenb