Message-ID: <032098e3-38f7-4127-b420-b579a97ce47e@arm.com>
Date: Wed, 14 Feb 2024 18:00:00 +0000
From: Ryan Roberts <ryan.roberts@....com>
To: Muhammad Usama Anjum <usama.anjum@...labora.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Shuah Khan <shuah@...nel.org>
Cc: kernel@...labora.com, linux-mm@...ck.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 Aishwarya TCV <Aishwarya.TCV@....com>
Subject: 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@...labora.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);
>>  
> 

Powered by blists - more mailing lists