From: Muhammad Usama Anjum <usama.anjum@collabora.com>
To: Ryan Roberts <ryan.roberts@arm.com>,
Andrew Morton <akpm@linux-foundation.org>,
Shuah Khan <shuah@kernel.org>
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>,
kernel@collabora.com, linux-mm@kvack.org,
linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
Aishwarya TCV <Aishwarya.TCV@arm.com>
Subject: Re: [PATCH v2 09/12] selftests/mm: thp_settings: conform to TAP format output
Date: Thu, 15 Feb 2024 09:59:58 +0500 [thread overview]
Message-ID: <fa22a33d-6142-48aa-a5b7-48e49bf0acec@collabora.com> (raw)
In-Reply-To: <0c3182ae-885c-4156-980b-e35d825fe72e@arm.com>
On 2/14/24 10:19 PM, Ryan Roberts wrote:
> On 02/02/2024 11:31, Muhammad Usama Anjum 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>
>> ---
>> tools/testing/selftests/mm/khugepaged.c | 3 +-
>> tools/testing/selftests/mm/thp_settings.c | 123 ++++++++--------------
>> tools/testing/selftests/mm/thp_settings.h | 4 +-
>> 3 files changed, 47 insertions(+), 83 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c
>> index d51fdaee7dc6a..3f202da0867c5 100644
>> --- a/tools/testing/selftests/mm/khugepaged.c
>> +++ b/tools/testing/selftests/mm/khugepaged.c
>> @@ -152,8 +152,7 @@ static void get_finfo(const char *dir)
>> major(path_stat.st_dev), minor(path_stat.st_dev)) >= sizeof(path))
>> ksft_exit_fail_msg("%s: Pathname is too long\n", __func__);
>>
>> - if (read_file(path, buf, sizeof(buf)) < 0)
>> - ksft_exit_fail_msg("read_file(read_num): %s\n", strerror(errno));
>> + read_file(path, buf, sizeof(buf));
>>
>> if (strstr(buf, "DEVTYPE=disk")) {
>> /* Found it */
>> diff --git a/tools/testing/selftests/mm/thp_settings.c b/tools/testing/selftests/mm/thp_settings.c
>> index a4163438108ec..273a95d025285 100644
>> --- a/tools/testing/selftests/mm/thp_settings.c
>> +++ b/tools/testing/selftests/mm/thp_settings.c
>> @@ -5,7 +5,9 @@
>> #include <stdlib.h>
>> #include <string.h>
>> #include <unistd.h>
>> +#include <errno.h>
>>
>> +#include "../kselftest.h"
>> #include "thp_settings.h"
>>
>> #define THP_SYSFS "/sys/kernel/mm/transparent_hugepage/"
>> @@ -42,58 +44,45 @@ static const char * const shmem_enabled_strings[] = {
>> NULL
>> };
>>
>> -int read_file(const char *path, char *buf, size_t buflen)
>> +void read_file(const char *path, char *buf, size_t buflen)
>> {
>> int fd;
>> ssize_t numread;
>>
>> fd = open(path, O_RDONLY);
>> if (fd == -1)
>> - return 0;
>> + ksft_exit_fail_msg("%s open failed: %s\n", path, strerror(errno));
>
> Hi,
>
> This change has broken back compat. It's no longer possible to run cow and
> khugepaged tests against older kernels.
>
> This function, as well as others in this file are intended to return 0 to
> indicate the file could not be accessed (e.g. doesn't exist or don't have
> permission, etc). Then higher level code can decide how to handle that. For
> example, thp_supported_orders() determines which THP orders are supported by the
> system based on the existence of certain files in sysfs. Then cow decides which
> test variants to run based on the supported orders. With your change, it all
> goes bang on the first probe and the whole test suite gets failed without
> running any tests.
>
> I've no problem with improving the TAP output from tests, but this must only be
> done at the test level, where it makes sense to do so. You can just call
> ksft_exit_fail_msg() from deep within a utility function.
>
> Please can we remove this from mm-unstable.
Sorry, not sure how I missed this. Let's drop this patch entirely.
>
> Thanks,
> Ryan
>
>
>>
>> numread = read(fd, buf, buflen - 1);
>> if (numread < 1) {
>> close(fd);
>> - return 0;
>> + ksft_exit_fail_msg("No data read\n");
>> }
>>
>> buf[numread] = '\0';
>> close(fd);
>> -
>> - return (unsigned int) numread;
>> }
>>
>> -int write_file(const char *path, const char *buf, size_t buflen)
>> +void write_file(const char *path, const char *buf, size_t buflen)
>> {
>> int fd;
>> ssize_t numwritten;
>>
>> fd = open(path, O_WRONLY);
>> - if (fd == -1) {
>> - printf("open(%s)\n", path);
>> - exit(EXIT_FAILURE);
>> - return 0;
>> - }
>> + if (fd == -1)
>> + ksft_exit_fail_msg("%s open failed\n", path);
>>
>> numwritten = write(fd, buf, buflen - 1);
>> close(fd);
>> - if (numwritten < 1) {
>> - printf("write(%s)\n", buf);
>> - exit(EXIT_FAILURE);
>> - return 0;
>> - }
>> -
>> - return (unsigned int) numwritten;
>> + if (numwritten < 1)
>> + ksft_exit_fail_msg("write failed (%s)\n", buf);
>> }
>>
>> const unsigned long read_num(const char *path)
>> {
>> char buf[21];
>>
>> - if (read_file(path, buf, sizeof(buf)) < 0) {
>> - perror("read_file()");
>> - exit(EXIT_FAILURE);
>> - }
>> + read_file(path, buf, sizeof(buf));
>>
>> return strtoul(buf, NULL, 10);
>> }
>> @@ -103,10 +92,7 @@ void write_num(const char *path, unsigned long num)
>> char buf[21];
>>
>> sprintf(buf, "%ld", num);
>> - if (!write_file(path, buf, strlen(buf) + 1)) {
>> - perror(path);
>> - exit(EXIT_FAILURE);
>> - }
>> + write_file(path, buf, strlen(buf) + 1);
>> }
>>
>> int thp_read_string(const char *name, const char * const strings[])
>> @@ -117,30 +103,22 @@ int thp_read_string(const char *name, const char * const strings[])
>> int ret;
>>
>> ret = snprintf(path, PATH_MAX, THP_SYSFS "%s", name);
>> - if (ret >= PATH_MAX) {
>> - printf("%s: Pathname is too long\n", __func__);
>> - exit(EXIT_FAILURE);
>> - }
>> + if (ret >= PATH_MAX)
>> + ksft_exit_fail_msg("%s: Pathname is too long\n", __func__);
>>
>> - if (!read_file(path, buf, sizeof(buf))) {
>> - perror(path);
>> - exit(EXIT_FAILURE);
>> - }
>> + read_file(path, buf, sizeof(buf));
>>
>> c = strchr(buf, '[');
>> - if (!c) {
>> - printf("%s: Parse failure\n", __func__);
>> - exit(EXIT_FAILURE);
>> - }
>> + if (!c)
>> + ksft_exit_fail_msg("%s: Parse failure\n", __func__);
>>
>> c++;
>> memmove(buf, c, sizeof(buf) - (c - buf));
>>
>> c = strchr(buf, ']');
>> - if (!c) {
>> - printf("%s: Parse failure\n", __func__);
>> - exit(EXIT_FAILURE);
>> - }
>> + if (!c)
>> + ksft_exit_fail_msg("%s: Parse failure\n", __func__);
>> +
>> *c = '\0';
>>
>> ret = 0;
>> @@ -150,8 +128,8 @@ int thp_read_string(const char *name, const char * const strings[])
>> ret++;
>> }
>>
>> - printf("Failed to parse %s\n", name);
>> - exit(EXIT_FAILURE);
>> + ksft_exit_fail_msg("Failed to parse %s\n", name);
>> + return -1;
>> }
>>
>> void thp_write_string(const char *name, const char *val)
>> @@ -160,15 +138,10 @@ void thp_write_string(const char *name, const char *val)
>> int ret;
>>
>> ret = snprintf(path, PATH_MAX, THP_SYSFS "%s", name);
>> - if (ret >= PATH_MAX) {
>> - printf("%s: Pathname is too long\n", __func__);
>> - exit(EXIT_FAILURE);
>> - }
>> + if (ret >= PATH_MAX)
>> + ksft_exit_fail_msg("%s: Pathname is too long\n", __func__);
>>
>> - if (!write_file(path, val, strlen(val) + 1)) {
>> - perror(path);
>> - exit(EXIT_FAILURE);
>> - }
>> + write_file(path, val, strlen(val) + 1);
>> }
>>
>> const unsigned long thp_read_num(const char *name)
>> @@ -177,10 +150,9 @@ const unsigned long thp_read_num(const char *name)
>> int ret;
>>
>> ret = snprintf(path, PATH_MAX, THP_SYSFS "%s", name);
>> - if (ret >= PATH_MAX) {
>> - printf("%s: Pathname is too long\n", __func__);
>> - exit(EXIT_FAILURE);
>> - }
>> + if (ret >= PATH_MAX)
>> + ksft_exit_fail_msg("%s: Pathname is too long\n", __func__);
>> +
>> return read_num(path);
>> }
>>
>> @@ -190,10 +162,9 @@ void thp_write_num(const char *name, unsigned long num)
>> int ret;
>>
>> ret = snprintf(path, PATH_MAX, THP_SYSFS "%s", name);
>> - if (ret >= PATH_MAX) {
>> - printf("%s: Pathname is too long\n", __func__);
>> - exit(EXIT_FAILURE);
>> - }
>> + if (ret >= PATH_MAX)
>> + ksft_exit_fail_msg("%s: Pathname is too long\n", __func__);
>> +
>> write_num(path, num);
>> }
>>
>> @@ -275,29 +246,26 @@ void thp_write_settings(struct thp_settings *settings)
>>
>> struct thp_settings *thp_current_settings(void)
>> {
>> - if (!settings_index) {
>> - printf("Fail: No settings set");
>> - exit(EXIT_FAILURE);
>> - }
>> + if (!settings_index)
>> + ksft_exit_fail_msg("Fail: No settings set\n");
>> +
>> return settings_stack + settings_index - 1;
>> }
>>
>> void thp_push_settings(struct thp_settings *settings)
>> {
>> - if (settings_index >= MAX_SETTINGS_DEPTH) {
>> - printf("Fail: Settings stack exceeded");
>> - exit(EXIT_FAILURE);
>> - }
>> + if (settings_index >= MAX_SETTINGS_DEPTH)
>> + ksft_exit_fail_msg("Fail: Settings stack exceeded\n");
>> +
>> settings_stack[settings_index++] = *settings;
>> thp_write_settings(thp_current_settings());
>> }
>>
>> void thp_pop_settings(void)
>> {
>> - if (settings_index <= 0) {
>> - printf("Fail: Settings stack empty");
>> - exit(EXIT_FAILURE);
>> - }
>> + if (settings_index <= 0)
>> + ksft_exit_fail_msg("Fail: Settings stack empty\n");
>> +
>> --settings_index;
>> thp_write_settings(thp_current_settings());
>> }
>> @@ -335,14 +303,11 @@ unsigned long thp_supported_orders(void)
>> for (i = 0; i < NR_ORDERS; i++) {
>> ret = snprintf(path, PATH_MAX, THP_SYSFS "hugepages-%ukB/enabled",
>> (getpagesize() >> 10) << i);
>> - if (ret >= PATH_MAX) {
>> - printf("%s: Pathname is too long\n", __func__);
>> - exit(EXIT_FAILURE);
>> - }
>> + if (ret >= PATH_MAX)
>> + ksft_exit_fail_msg("%s: Pathname is too long\n", __func__);
>>
>> - ret = read_file(path, buf, sizeof(buf));
>> - if (ret)
>> - orders |= 1UL << i;
>> + read_file(path, buf, sizeof(buf));
>> + orders |= 1UL << i;
>> }
>>
>> return orders;
>> diff --git a/tools/testing/selftests/mm/thp_settings.h b/tools/testing/selftests/mm/thp_settings.h
>> index 71cbff05f4c7f..04a6a7bbd08f8 100644
>> --- a/tools/testing/selftests/mm/thp_settings.h
>> +++ b/tools/testing/selftests/mm/thp_settings.h
>> @@ -56,8 +56,8 @@ struct thp_settings {
>> struct hugepages_settings hugepages[NR_ORDERS];
>> };
>>
>> -int read_file(const char *path, char *buf, size_t buflen);
>> -int write_file(const char *path, const char *buf, size_t buflen);
>> +void read_file(const char *path, char *buf, size_t buflen);
>> +void write_file(const char *path, const char *buf, size_t buflen);
>> const unsigned long read_num(const char *path);
>> void write_num(const char *path, unsigned long num);
>>
>
>
--
BR,
Muhammad Usama Anjum
next prev parent reply other threads:[~2024-02-15 4:59 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-02 11:31 [PATCH v2 00/12] conform tests " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 01/12] selftests/mm: map_fixed_noreplace: conform test " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 02/12] selftests/mm: map_hugetlb: " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 03/12] selftests/mm: map_populate: " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 04/12] selftests/mm: mlock-random-test: " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 05/12] selftests/mm: mlock2-tests: " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 06/12] selftests/mm: mrelease_test: " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 07/12] selftests/mm: mremap_dontunmap: " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 08/12] selftests/mm: split_huge_page_test: " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 09/12] selftests/mm: thp_settings: conform " Muhammad Usama Anjum
2024-02-14 17:19 ` Ryan Roberts
2024-02-14 18:00 ` Ryan Roberts
2024-02-15 4:59 ` Muhammad Usama Anjum [this message]
2024-02-02 11:31 ` [PATCH v2 10/12] selftests/mm: thuge-gen: " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 11/12] selftests/mm: transhuge-stress: " Muhammad Usama Anjum
2024-02-02 11:31 ` [PATCH v2 12/12] selftests/mm: virtual_address_range: " Muhammad Usama Anjum
2024-03-14 5:00 ` Dev Jain
2024-03-14 8:46 ` Muhammad Usama Anjum
2024-03-14 12:22 ` [PATCH] selftests/mm: virtual_address_range: Switch to ksft_exit_fail_msg Dev Jain
2024-03-14 12:33 ` Muhammad Usama Anjum
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fa22a33d-6142-48aa-a5b7-48e49bf0acec@collabora.com \
--to=usama.anjum@collabora.com \
--cc=Aishwarya.TCV@arm.com \
--cc=akpm@linux-foundation.org \
--cc=kernel@collabora.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=ryan.roberts@arm.com \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox