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

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.