mbox series

[v2,00/12] conform tests to TAP format output

Message ID 20240202113119.2047740-1-usama.anjum@collabora.com
Headers show
Series conform tests to TAP format output | expand

Message

Muhammad Usama Anjum Feb. 2, 2024, 11:31 a.m. UTC
Changes since v1:
- Rebased the series on top of next-20240202

Muhammad Usama Anjum (12):
  selftests/mm: map_fixed_noreplace: conform test to TAP format output
  selftests/mm: map_hugetlb: conform test to TAP format output
  selftests/mm: map_populate: conform test to TAP format output
  selftests/mm: mlock-random-test: conform test to TAP format output
  selftests/mm: mlock2-tests: conform test to TAP format output
  selftests/mm: mrelease_test: conform test to TAP format output
  selftests/mm: mremap_dontunmap: conform test to TAP format output
  selftests/mm: split_huge_page_test: conform test to TAP format output
  selftests/mm: thp_settings: conform to TAP format output
  selftests/mm: thuge-gen: conform to TAP format output
  selftests/mm: transhuge-stress: conform to TAP format output
  selftests/mm: virtual_address_range: conform to TAP format output

 tools/testing/selftests/mm/khugepaged.c       |   3 +-
 .../selftests/mm/map_fixed_noreplace.c        |  96 ++----
 tools/testing/selftests/mm/map_hugetlb.c      |  42 ++-
 tools/testing/selftests/mm/map_populate.c     |  37 ++-
 .../testing/selftests/mm/mlock-random-test.c  | 136 ++++-----
 tools/testing/selftests/mm/mlock2-tests.c     | 282 +++++++-----------
 tools/testing/selftests/mm/mlock2.h           |  11 +-
 tools/testing/selftests/mm/mrelease_test.c    |  80 ++---
 tools/testing/selftests/mm/mremap_dontunmap.c |  32 +-
 .../selftests/mm/split_huge_page_test.c       | 161 +++++-----
 tools/testing/selftests/mm/thp_settings.c     | 123 +++-----
 tools/testing/selftests/mm/thp_settings.h     |   4 +-
 tools/testing/selftests/mm/thuge-gen.c        | 147 ++++-----
 tools/testing/selftests/mm/transhuge-stress.c |  36 ++-
 .../selftests/mm/virtual_address_range.c      |  44 +--
 tools/testing/selftests/mm/vm_util.c          |   6 +-
 16 files changed, 537 insertions(+), 703 deletions(-)

Comments

Ryan Roberts Feb. 14, 2024, 5:19 p.m. UTC | #1
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.

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);
>
Ryan Roberts Feb. 14, 2024, 6 p.m. UTC | #2
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);
>>  
>
Muhammad Usama Anjum Feb. 15, 2024, 4:59 a.m. UTC | #3
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);
>>  
> 
>