Received-SPF: pass (google.com: domain of linux-kernel+bounces-28540-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <d9c99910-364c-4b18-9f26-169c719de29b@collabora.com>
Date: Wed, 17 Jan 2024 10:34:49 +0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>,
 Andrew Morton <akpm@linux-foundation.org>, Shuah Khan <shuah@kernel.org>,
 kernel@collabora.com, linux-mm@kvack.org, linux-kselftest@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 5/7] selftests/mm: hugetlb-read-hwpoison: conform test
 to TAP format output
Content-Language: en-US
To: Jiaqi Yan <jiaqiyan@google.com>
References: <20240115073247.1280266-1-usama.anjum@collabora.com>
 <20240115073247.1280266-5-usama.anjum@collabora.com>
 <CACw3F531DPDtQe5PJmH091n9RhK57FhUYR1L45FdjrYBMSUOtQ@mail.gmail.com>
From: Muhammad Usama Anjum <usama.anjum@collabora.com>
In-Reply-To: <CACw3F531DPDtQe5PJmH091n9RhK57FhUYR1L45FdjrYBMSUOtQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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

> 

-- 
BR,
Muhammad Usama Anjum