Re: [PATCH v3 5/7] selftests/mm: hugetlb-read-hwpoison: conform test to TAP format output

On 1/16/24 10:07 PM, Jiaqi Yan wrote:

On Sun, Jan 14, 2024 at 11:33 PM Muhammad Usama Anjum usama.anjum@collabora.com wrote:

...

Conform the layout, informational and status messages to TAP. No functional change is intended other than the layout of output messages.

Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com

Changes in v3:

• Use ksft_perror as short hand instead of missing strerror(errno) at one place

Minor thing: I think we should preserve previous changelogs, right?

Definately. There was no changes in v2 for this patch. Hence there isn't any changelog before this series.

...

Tested this by reverting the patch a08c7193e4f18dc8508f2d07d0de2c5b94cb39a3 ("mm/filemap: remove hugetlb special casing in filemap.c") as it has broken the test. The bug report can be found at [1].

Tested with proposed fix as well [2].

[1] https://lore.kernel.org/all/079335ab-190f-41f7-b832-6ffe7528fd8b@collabora.c... [2] https://lore.kernel.org/all/a20e7bdb-7344-306d-e8f5-5ee69af7d5ea@oracle.com

---

.../selftests/mm/hugetlb-read-hwpoison.c | 116 ++++++++---------- 1 file changed, 54 insertions(+), 62 deletions(-)

diff --git a/tools/testing/selftests/mm/hugetlb-read-hwpoison.c b/tools/testing/selftests/mm/hugetlb-read-hwpoison.c index ba6cc6f9cabc..23b41b88c6af 100644 --- a/tools/testing/selftests/mm/hugetlb-read-hwpoison.c +++ b/tools/testing/selftests/mm/hugetlb-read-hwpoison.c @@ -58,8 +58,8 @@ static bool verify_chunk(char *buf, size_t len, char val)

    for (i = 0; i < len; ++i) {
            if (buf[i] != val) {

•                   printf(PREFIX ERROR_PREFIX "check fail: buf[%lu] = %u != %u\n",
  

•                           i, buf[i], val);
  

•                   ksft_print_msg(PREFIX ERROR_PREFIX "check fail: buf[%lu] = %u != %u\n",
  

•                                  i, buf[i], val);
                    return false;
            }
    }
  

@@ -75,21 +75,21 @@ static bool seek_read_hugepage_filemap(int fd, size_t len, size_t wr_chunk_size, ssize_t total_ret_count = 0; char val = offset / wr_chunk_size + offset % wr_chunk_size;

•   printf(PREFIX PREFIX "init val=%u with offset=0x%lx\n", val, offset);
  

•   printf(PREFIX PREFIX "expect to read 0x%lx bytes of data in total\n",
  

•          expected);
  

•   ksft_print_msg(PREFIX PREFIX "init val=%u with offset=0x%lx\n", val, offset);
  

•   ksft_print_msg(PREFIX PREFIX "expect to read 0x%lx bytes of data in total\n",
  

•                  expected);
    if (lseek(fd, offset, SEEK_SET) < 0) {
  

•           perror(PREFIX ERROR_PREFIX "seek failed");
  

•           ksft_perror(PREFIX ERROR_PREFIX "seek failed");
            return false;
    }
  
    while (offset + total_ret_count < len) {
            ret_count = read(fd, buf, wr_chunk_size);
            if (ret_count == 0) {
  

•                   printf(PREFIX PREFIX "read reach end of the file\n");
  

•                   ksft_print_msg(PREFIX PREFIX "read reach end of the file\n");
                    break;
            } else if (ret_count < 0) {
  

•                   perror(PREFIX ERROR_PREFIX "read failed");
  

•                   ksft_perror(PREFIX ERROR_PREFIX "read failed");
                    break;
            }
            ++val;
  

@@ -98,8 +98,8 @@ static bool seek_read_hugepage_filemap(int fd, size_t len, size_t wr_chunk_size,

            total_ret_count += ret_count;
    }

•   printf(PREFIX PREFIX "actually read 0x%lx bytes of data in total\n",
  

•          total_ret_count);
  

•   ksft_print_msg(PREFIX PREFIX "actually read 0x%lx bytes of data in total\n",
  

•                  total_ret_count);
  
    return total_ret_count == expected;
  

} @@ -112,15 +112,15 @@ static bool read_hugepage_filemap(int fd, size_t len, ssize_t total_ret_count = 0; char val = 0;

•   printf(PREFIX PREFIX "expect to read 0x%lx bytes of data in total\n",
  

•          expected);
  

•   ksft_print_msg(PREFIX PREFIX "expect to read 0x%lx bytes of data in total\n",
  

•                  expected);
    while (total_ret_count < len) {
            ret_count = read(fd, buf, wr_chunk_size);
            if (ret_count == 0) {
  

•                   printf(PREFIX PREFIX "read reach end of the file\n");
  

•                   ksft_print_msg(PREFIX PREFIX "read reach end of the file\n");
                    break;
            } else if (ret_count < 0) {
  

•                   perror(PREFIX ERROR_PREFIX "read failed");
  

•                   ksft_perror(PREFIX ERROR_PREFIX "read failed");
                    break;
            }
            ++val;
  

@@ -129,8 +129,8 @@ static bool read_hugepage_filemap(int fd, size_t len,

            total_ret_count += ret_count;
    }

•   printf(PREFIX PREFIX "actually read 0x%lx bytes of data in total\n",
  

•          total_ret_count);
  

•   ksft_print_msg(PREFIX PREFIX "actually read 0x%lx bytes of data in total\n",
  

•                  total_ret_count);
  
    return total_ret_count == expected;
  

} @@ -142,14 +142,14 @@ test_hugetlb_read(int fd, size_t len, size_t wr_chunk_size) char *filemap = NULL;

    if (ftruncate(fd, len) < 0) {

•           perror(PREFIX ERROR_PREFIX "ftruncate failed");
  

•           ksft_perror(PREFIX ERROR_PREFIX "ftruncate failed");
            return status;
    }
  
    filemap = mmap(NULL, len, PROT_READ | PROT_WRITE,
                   MAP_SHARED | MAP_POPULATE, fd, 0);
    if (filemap == MAP_FAILED) {
  

•           perror(PREFIX ERROR_PREFIX "mmap for primary mapping failed");
  

•           ksft_perror(PREFIX ERROR_PREFIX "mmap for primary mapping failed");
            goto done;
    }
  
  

@@ -162,7 +162,7 @@ test_hugetlb_read(int fd, size_t len, size_t wr_chunk_size) munmap(filemap, len); done: if (ftruncate(fd, 0) < 0) {

•           perror(PREFIX ERROR_PREFIX "ftruncate back to 0 failed");
  

•           ksft_perror(PREFIX ERROR_PREFIX "ftruncate back to 0 failed");
            status = TEST_FAILED;
    }
  
  

@@ -179,14 +179,14 @@ test_hugetlb_read_hwpoison(int fd, size_t len, size_t wr_chunk_size, const unsigned long pagesize = getpagesize();

    if (ftruncate(fd, len) < 0) {

•           perror(PREFIX ERROR_PREFIX "ftruncate failed");
  

•           ksft_perror(PREFIX ERROR_PREFIX "ftruncate failed");
            return status;
    }
  
    filemap = mmap(NULL, len, PROT_READ | PROT_WRITE,
                   MAP_SHARED | MAP_POPULATE, fd, 0);
    if (filemap == MAP_FAILED) {
  

•           perror(PREFIX ERROR_PREFIX "mmap for primary mapping failed");
  

•           ksft_perror(PREFIX ERROR_PREFIX "mmap for primary mapping failed");
            goto done;
    }
  
  

@@ -201,7 +201,7 @@ test_hugetlb_read_hwpoison(int fd, size_t len, size_t wr_chunk_size, */ hwp_addr = filemap + len / 2 + pagesize; if (madvise(hwp_addr, pagesize, MADV_HWPOISON) < 0) {

•           perror(PREFIX ERROR_PREFIX "MADV_HWPOISON failed");
  

•           ksft_perror(PREFIX ERROR_PREFIX "MADV_HWPOISON failed");
            goto unmap;
    }
  
  

@@ -228,7 +228,7 @@ test_hugetlb_read_hwpoison(int fd, size_t len, size_t wr_chunk_size, munmap(filemap, len); done: if (ftruncate(fd, 0) < 0) {

•           perror(PREFIX ERROR_PREFIX "ftruncate back to 0 failed");
  

•           ksft_perror(PREFIX ERROR_PREFIX "ftruncate back to 0 failed");
            status = TEST_FAILED;
    }
  
  

@@ -240,27 +240,32 @@ static int create_hugetlbfs_file(struct statfs *file_stat) int fd;

    fd = memfd_create("hugetlb_tmp", MFD_HUGETLB);

•   if (fd < 0) {
  

•           perror(PREFIX ERROR_PREFIX "could not open hugetlbfs file");
  

•           return -1;
  

•   }
  

•   if (fd < 0)
  

•           ksft_exit_fail_msg(PREFIX ERROR_PREFIX "could not open hugetlbfs file: %s\n",
  

•                              strerror(errno));
  
    memset(file_stat, 0, sizeof(*file_stat));
  

•   if (fstatfs(fd, file_stat)) {
  

•           perror(PREFIX ERROR_PREFIX "fstatfs failed");
  

•           goto close;
  

•           close(fd);
  

•           ksft_exit_fail_msg(PREFIX ERROR_PREFIX "fstatfs failed: %s\n", strerror(errno));
    }
    if (file_stat->f_type != HUGETLBFS_MAGIC) {
  

•           printf(PREFIX ERROR_PREFIX "not hugetlbfs file\n");
  

•           goto close;
  

•           close(fd);
  

•           ksft_exit_fail_msg(PREFIX ERROR_PREFIX "not hugetlbfs file\n");
    }
  
    return fd;
  

-close:

•   close(fd);
  

•   return -1;
  

}

+#define KSFT_PRINT_MSG(status, fmt, ...) \

•   do {                                                                    \
  

•           if (status == TEST_SKIPPED)                                     \
  

•                   ksft_test_result_skip(fmt, __VA_ARGS__);                \
  

•           else                                                            \
  

•                   ksft_test_result(status == TEST_PASSED, fmt, __VA_ARGS__); \
  

•   } while (0)
  

int main(void) { int fd; @@ -273,50 +278,37 @@ int main(void) }; size_t i;

•   ksft_print_header();
  

•   ksft_set_plan(12);
  

Minor: can this number be calculated, or at least defined as a macro with documents? That would make it easier for reading.

The number can be calculated to some extent via macro. As all the tests don't define a macro for this, I'll prepare one patch for all the tests later on.

...

•   for (i = 0; i < ARRAY_SIZE(wr_chunk_sizes); ++i) {
  

•           printf("Write/read chunk size=0x%lx\n",
  

•                  wr_chunk_sizes[i]);
  

•           ksft_print_msg("Write/read chunk size=0x%lx\n",
  

•                          wr_chunk_sizes[i]);
  
            fd = create_hugetlbfs_file(&file_stat);
  

•           if (fd < 0)
  

•                   goto create_failure;
  

•           printf(PREFIX "HugeTLB read regression test...\n");
  

•           ksft_print_msg(PREFIX "HugeTLB read regression test...\n");
            status = test_hugetlb_read(fd, file_stat.f_bsize,
                                       wr_chunk_sizes[i]);
  

•           printf(PREFIX "HugeTLB read regression test...%s\n",
  

•                  status_to_str(status));
  

•           KSFT_PRINT_MSG(status, PREFIX "HugeTLB read regression test...%s\n",
  

•                          status_to_str(status));
            close(fd);
  

•           if (status == TEST_FAILED)
  

•                   return -1;
  
            fd = create_hugetlbfs_file(&file_stat);
  

•           if (fd < 0)
  

•                   goto create_failure;
  

•           printf(PREFIX "HugeTLB read HWPOISON test...\n");
  

•           ksft_print_msg(PREFIX "HugeTLB read HWPOISON test...\n");
            status = test_hugetlb_read_hwpoison(fd, file_stat.f_bsize,
                                                wr_chunk_sizes[i], false);
  

•           printf(PREFIX "HugeTLB read HWPOISON test...%s\n",
  

•                  status_to_str(status));
  

•           KSFT_PRINT_MSG(status, PREFIX "HugeTLB read HWPOISON test..%s\n",
  

•                          status_to_str(status));
            close(fd);
  

•           if (status == TEST_FAILED)
  

•                   return -1;
  
            fd = create_hugetlbfs_file(&file_stat);
  

•           if (fd < 0)
  

•                   goto create_failure;
  

•           printf(PREFIX "HugeTLB seek then read HWPOISON test...\n");
  

•           ksft_print_msg(PREFIX "HugeTLB seek then read HWPOISON test...\n");
            status = test_hugetlb_read_hwpoison(fd, file_stat.f_bsize,
                                                wr_chunk_sizes[i], true);
  

•           printf(PREFIX "HugeTLB seek then read HWPOISON test...%s\n",
  

•                  status_to_str(status));
  

•           KSFT_PRINT_MSG(status, PREFIX "HugeTLB seek then read HWPOISON test...%s\n",
  

•                          status_to_str(status));
            close(fd);
  

•           if (status == TEST_FAILED)
  

•                   return -1;
    }
  
  

•   return 0;
  

-create_failure:

•   printf(ERROR_PREFIX "Abort test: failed to create hugetlbfs file\n");
  

•   return -1;
  

•   ksft_finished();
  

}

2.42.0

This version looks good to me. Maybe someone else need to take another look, just add mine:

Reviewed-by: Jiaqi Yan jiaqiyan@google.com

Thanks