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@xxxxxxxxxxxxx>
>> ---
>> 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.
>
>
>
--
Muhammad Usama Anjum
