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

[Muhammad Usama Anjum <usama.anjum@collabora.com>]

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.
> 
> 
> 

-- 
Muhammad Usama Anjum