Re: [PATCH v2 09/12] selftests/mm: thp_settings: conform to TAP format output

On 14/02/2024 17:19, 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.

Actually it is even worse than I first thought. With this change, the tests fail even when running against a matched kernel.

thp_supported_orders() tries to probe every possible order and relies on the probe succeeding or failing to determine which orders are actually supported. The first order it tries is 0 (4K). That is not THP, so there is no hugepages-4K/enabled file for it. So it blows up immediately.

So I don't think you even ran the tests locally after making the change?

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);