From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id DEA6CC48BC3 for ; Wed, 14 Feb 2024 18:00:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5C96B8D0013; Wed, 14 Feb 2024 13:00:08 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 576DA8D000E; Wed, 14 Feb 2024 13:00:08 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3CA0A8D0013; Wed, 14 Feb 2024 13:00:08 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 2AE298D000E for ; Wed, 14 Feb 2024 13:00:08 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id A1F24A0D59 for ; Wed, 14 Feb 2024 18:00:07 +0000 (UTC) X-FDA: 81791173254.21.82FC1C0 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf12.hostedemail.com (Postfix) with ESMTP id AE37A40011 for ; Wed, 14 Feb 2024 18:00:04 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf12.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707933604; a=rsa-sha256; cv=none; b=X+Ou9azD3lr2wIkJogZ855Cp1nTseiKNSTbHP0/uQW1lvcsw+kwN4Pg5thpJ9VZ50Kdtej zXMzqsdbzJdSkEX+r96oVaA6l7JjQJeQAWBwb91v2i39xnsjCiCItDy/sWHgeyzpbX9iC4 MIt/rpaYd0ADn1G7NZxZwyepz/NFztM= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf12.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707933604; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Nn9F5exwFqymwwAwZSDVrW2ajmQ2zMav6dbaZlIcye0=; b=DIAljfoSKGGvB+Qk9tAUwGeekZO9OekGzvZzy3Lh6CJZEN39yOodnTyFuTag292+FXgQLG MYTTL/JaZEWyY4XcU69qKnFyJtEFWX3w6WuHwKmi+cHupmUti9rWc7EufvGpFfrU7IVJLY GUlVGXPHohp3KYf4tEvktjwdhAv+KQY= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6E0031FB; Wed, 14 Feb 2024 10:00:44 -0800 (PST) Received: from [10.57.64.120] (unknown [10.57.64.120]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3F36F3F766; Wed, 14 Feb 2024 10:00:02 -0800 (PST) Message-ID: <032098e3-38f7-4127-b420-b579a97ce47e@arm.com> Date: Wed, 14 Feb 2024 18:00:00 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 09/12] selftests/mm: thp_settings: conform to TAP format output Content-Language: en-GB From: Ryan Roberts To: Muhammad Usama Anjum , Andrew Morton , Shuah Khan Cc: kernel@collabora.com, linux-mm@kvack.org, linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, Aishwarya TCV References: <20240202113119.2047740-1-usama.anjum@collabora.com> <20240202113119.2047740-10-usama.anjum@collabora.com> <0c3182ae-885c-4156-980b-e35d825fe72e@arm.com> In-Reply-To: <0c3182ae-885c-4156-980b-e35d825fe72e@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: AE37A40011 X-Stat-Signature: sodsebxya6w4snp97c1qsfx1zxtac7h7 X-Rspam-User: X-HE-Tag: 1707933604-147242 X-HE-Meta: U2FsdGVkX18aKNIwKK2A3cYXyAtrOD0PC8MZi0kATttS7Ecz7U1arPDd8DnjOAu1Ckjjnj0sRJw3oADTn8gzh/OM/OPU2mOnLPSdKGqfak4pBVJKSEMoFbnSFvS3KMkiozTRxFbesVl1f/UIifPBo0RFsVWdII8xk6HdOAdx90Sh0JRNSCfZ5O2mCVq2d0KKxO346CP2FIkBqgjpkEYKEVW0QPtQ+O2WoGovxcn5GL5vhB9WXprNnMpvlUeNl7KFtYALfWcgreoeDZq/VeGHKrgMJdIhB6cKoosIP5Q+vIF88iIlBDmEbnkxe7gQPOggT3PexCMa96UUBgP0vB1c5g/Xz+te7ub6+9FOeGYlVq2wICWWmIhb3HinUlaf3aGvxixa8btzfMduvKnv1L2Aokicfr2nGMXddJbgisrPOfteecp7ihfdy+2KUtokXLnrOVi4Rb7S3kSrOopygGL0Nc+VuajHQTMIkwE6oSRST12Ix3VNU624GcKLzy15pATtdpI3YYD6TOwY/n80MWbsdA8npwpaQ67sCtw73hyZet494dYaJAK7m8IWHa0Xdn9+BrPXS4fm1qD0yrrZxZPUf1fwZJVyxe4v2M682F1RMQ/Zn3XvhPsVkztxd+W51bdWTMC4lh64ygd985zXngrCO0je4idY52rQKKaLgZGpKRh7W7p9e6ML2KmPiyNEAt1fjTs3aiNfou0sD15RWDoF2CvzlKuUyWrgiKnlSHOcoMkOk0d50JYxdgZxFEKetUaRzU9DehQNaHqQyRdRCiEcX8T+y43qfwUXUDuH78OysRVmsilIbookoxlKRBILyoB1ZBP5U5hpDRbzsGquu3LszkQWt3HZx6goEawoWWGuQzT+KywtVd62wLLSAjFcmxtBAlN4MCnqPx6puLYXdN7zGck8EUi6Ky9rUywbvh7kHQRAvFzwlrBOMgGK0hRicr9rrjhRJiXehjMVh9zp0so IZUDJJEt wtXz1QRzhBw+eaZnq8EanMbL+RKMODjJXGt1c3skR6B152VTrjsTHrhRKFln3vmNs0fyFFFeE1cHP3/wXYeiz11ZtHHVamhbw3+HE9xVqGKh53fIj3n35JI4qFi0Sg3XCcNnUJ/Whd8SQXE9Iqcq0QcLKSy0WLILFsXnqQK9NTWonxcBe0k9fh2XUKabtvYVzfpQg9NJLV/myf6nsVE6YwdDnjZgbDDCkCznQHa3V3aqrXz8rETYMEiLXv2lr0qYJvX4Z X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: 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 >> --- >> 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 >> #include >> #include >> +#include >> >> +#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); >> >