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 A6DFDC48BC3 for ; Wed, 14 Feb 2024 17:19:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 340268D000E; Wed, 14 Feb 2024 12:19:32 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2F1656B008A; Wed, 14 Feb 2024 12:19:32 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 16A008D000E; Wed, 14 Feb 2024 12:19:32 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 057A26B0088 for ; Wed, 14 Feb 2024 12:19:32 -0500 (EST) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id BFF91160E5D for ; Wed, 14 Feb 2024 17:19:31 +0000 (UTC) X-FDA: 81791070942.10.8C5C0FF Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf14.hostedemail.com (Postfix) with ESMTP id BE713100024 for ; Wed, 14 Feb 2024 17:19:29 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=none; spf=pass (imf14.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707931170; 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=Y3Rk4n6mvAEI/osp1D/cQ2B265hvZ5L3+BZn5cE3FK4=; b=mUvBbcS2NlBecT2f3dNuTjQX1f/9EgRJhbp4XZ1nyvObfi0FnxpLSYWBiopK7BO8c8jV93 4re7mSNY6LL6p6XCRC7WzNbV8Tp/q812l/5z8B0LpKexM23VMT7iVo5XJp87N41ZtXExLu uEXgs1+1Y2BhcJjOIetiqUSyGsQ92XM= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707931170; a=rsa-sha256; cv=none; b=5DPaB2aV4uvsxnpbBkdn7AQgRcx4CyAaHrfjI8jLAH+zxlPD5ct0ZqXHZtACobsSNzq/Bf 3LZy8XF5FrQV91Mv5kwkdT0H8P/6yaxuUFG9ux9WcY3jHHhFhb3dnxVSuCgESMvFlGIUZJ Wm7puxOvwRSN/W71MH2hPaFWd2GvPqU= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=none; spf=pass (imf14.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com 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 B77B31FB; Wed, 14 Feb 2024 09:20:09 -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 83F643F766; Wed, 14 Feb 2024 09:19:27 -0800 (PST) Message-ID: <0c3182ae-885c-4156-980b-e35d825fe72e@arm.com> Date: Wed, 14 Feb 2024 17:19:25 +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 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> From: Ryan Roberts In-Reply-To: <20240202113119.2047740-10-usama.anjum@collabora.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: BE713100024 X-Rspam-User: X-Stat-Signature: 5f1qx18hpqky5on87mognb3ne4yj7brw X-Rspamd-Server: rspam03 X-HE-Tag: 1707931169-43402 X-HE-Meta: U2FsdGVkX19tS6DX4WExAgHP1SC7QXKo8XgQPDM9GhMTG39mTsBm+fdu5HDmY36HGRR2JG66AcpyrB6sm1AdNSnHudRbKE5WcIKhAP4gBDZWGqkyxLO1s/TGVBodU2+/hdC+zhYbWcNjYOkbaeUv4JenUXJB8GiItJbz67c3/COheBLU9av8pKKJvZ7bMw1gqM6M4SQB+0ogOHp5FVSOeIexKUTefVl3bTx4OaG/a41s5YtKlnVFB2ux3mDVPgXJpaqs1dymINbxyPV6M6bWYt8jD1CvXrFLsRglNUGg7rUMyCQalSR9N4+louTAawg8GPy+G4o6QoSs99Dyh+1+6Hibx8rg2jukbonHtGCBNVvhO69Ll2ind/9z7JD4y3qzkGfXcEL1n+jSMFPs0bTapsrn/ijQYrx2E52lt3+pNaST0z8VINUi9RwSnXv0HDOHYyWXxgKJe5mQjMNyMI/hIb2jpNq9poMFfAJ/KhbgaynPXtTqnKkpV9fyotFJjPCfQ8u+0lxDPOWo/+B2nuRtyDtQ0qI/4LvvsWQTeuzs8j2PzLbTqwdRksuvn/xvnDfMtA8Jf3JDwyXX07wuDnLfjAkhtN6Q7kZ4hfhhronHdY7tD9ugY5iBaaZEdsh9B/YwGtneE7ZtD2/M2mVELWPulQ8DiRNxML0aJIJ/Qv8Cb2wR+MWCTs6pHY74g9ig3JSCWmnc2DU3TX8TTuX0J61WGakpyLfS/4R1usP9UkKiSuPwVV3wSDr5pY216WKrE7Th81jIiCl2Hcj3Vr6LMSW1W9J/nw9EGTbz20AoMATdVb1ZpkcgE+B2MxDUNSP3CWge3r3/AYtFak8u3lXDFo0kpZIGeBxOEgNGo0mUdE/jbVwc1SJyvu2ULprCGxolkIHvqYzVhrs336itPH336lanbQywc+vpKW1fya6Wv4RwrdOM7+06en/3KWUIqiPZgK2PFyT7LhwVe0HhB4UTpw6 f8oZNRwW mzOVM3uSbgKdM6CE+CcfpbViKUsnlS7JDCyypEBmDU6bs3suWlK5TwRW4t8pdMVDof0lnqbOAAfnZT4efAM9yKZqLxPVGnIafJT66nx41sVSY7cuEfu80QsqW8RWjPTpNxefI1tLaYqnAfkvkXtZlZey3wFujCXOq+4LgvIMHGp+7CTiBQ5S9QOc15oIdBBtPVnry+Wo7rIKpy5QK5pyM/KK/MJLuBGuGyhquKw8WvYRAzNVtFpC5bYAr9yE3zvpzyJXw 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 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. 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); >