Hi David,
Thank you for the review. I'll correct everything mentioned here in the next iteration.
+[Gabriel]
On 3/16/22 1:55 PM, David Hildenbrand wrote:
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.