From patchwork Tue Oct 10 10:16:10 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Maciej Wieczor-Retman X-Patchwork-Id: 731728 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E55FCCD691F for ; Tue, 10 Oct 2023 10:16:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230438AbjJJKQp (ORCPT ); Tue, 10 Oct 2023 06:16:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40374 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231153AbjJJKQh (ORCPT ); Tue, 10 Oct 2023 06:16:37 -0400 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.151]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6256CE6; Tue, 10 Oct 2023 03:16:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1696932992; x=1728468992; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=13xQlZVS/d3Fnw5Uih4vnyyNxo0vPH4oWAoiObRGY2Y=; b=Kbe9avFIH/UyxCYN4c6cgBGzxw1HNHnlzeNf4LE4zJLPMDFPT8Kejwoh GYHw77mCjHMBhJbMkV7Qk4ZO4++edf2rNa7JLtb21AlT6JMNhfCSnE3ZA 5IfEc104TPoPxBnN35rrPqHnxbuPAVL1RCamGHjGKDOD6jnbYQ99Qp6Hu a8E8R3C2UXBDRC3BWL+k64Y8ymm0xrTn53Mn2jDiMOefRUwNM9/AuLySp eC8EPrxT16OFYfDSa2JEXrtmwiUalK/ePskDmnq7RYuPph1Psgcde4zAw R8GLWZv8ef/GJ+3I0v/QjT0cICCYIA7ZM005CdXYS6G+r5jLcrdfOly6r A==; X-IronPort-AV: E=McAfee;i="6600,9927,10858"; a="364652593" X-IronPort-AV: E=Sophos;i="6.03,212,1694761200"; d="scan'208";a="364652593" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Oct 2023 03:16:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10858"; a="703252901" X-IronPort-AV: E=Sophos;i="6.03,212,1694761200"; d="scan'208";a="703252901" Received: from asalaman-mobl.ger.corp.intel.com (HELO wieczorr-mobl1.intel.com) ([10.213.16.145]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Oct 2023 03:16:27 -0700 From: Maciej Wieczor-Retman To: Fenghua Yu , Reinette Chatre , Shuah Khan Cc: ilpo.jarvinen@linux.intel.com, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org Subject: [PATCH RESEND v7 1/2] selftests/resctrl: Fix schemata write error check Date: Tue, 10 Oct 2023 12:16:10 +0200 Message-ID: <7d463146d208304e61cd97c60e718372fc2c21d6.1696932728.git.maciej.wieczor-retman@intel.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-kselftest@vger.kernel.org Writing bitmasks to the schemata can fail when the bitmask doesn't adhere to constraints defined by what a particular CPU supports. Some example of constraints are max length or having contiguous bits. The driver should properly return errors when any rule concerning bitmask format is broken. Resctrl FS returns error codes from fprintf() only when fclose() is called. Current error checking scheme allows invalid bitmasks to be written into schemata file and the selftest doesn't notice because the fclose() error code isn't checked. Substitute fopen(), flose() and fprintf() with open(), close() and write() to avoid error code buffering between fprintf() and fclose(). Remove newline character from the schema string after writing it to the schemata file so it prints correctly before function return. Pass the string generated with strerror() to the "reason" buffer so the error message is more verbose. Extend "reason" buffer so it can hold longer messages. Signed-off-by: Maciej Wieczor-Retman Reviewed-by: Ilpo Järvinen Reviewed-by: Reinette Chatre --- Changelog v7: - Add label for non-empty schema error case. (Reinette) - Add Reinette's reviewed-by tag. Changelog v6: - Align schema_len error checking with typical snprintf format. (Reinette) - Initialize schema string for early return eventuality. (Reinette) Changelog v5: - Add Ilpo's reviewed-by tag. - Fix wrong open() error checking. (Reinette) - Add error checking to schema_len variable. Changelog v4: - Unify error checking between open() and write(). (Reinette) - Add fcntl.h for glibc backward compatiblitiy. (Reinette) Changelog v3: - Rename fp to fd. (Ilpo) - Remove strlen, strcspn and just use the snprintf value instead. (Ilpo) Changelog v2: - Rewrite patch message. - Double "reason" buffer size to fit longer error explanation. - Redo file interactions with syscalls instead of stdio functions. tools/testing/selftests/resctrl/resctrlfs.c | 41 +++++++++++++-------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 3a8111362d26..05390afd4d6f 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -8,6 +8,7 @@ * Sai Praneeth Prakhya , * Fenghua Yu */ +#include #include #include "resctrl.h" @@ -490,9 +491,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, */ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) { - char controlgroup[1024], schema[1024], reason[64]; - int resource_id, ret = 0; - FILE *fp; + char controlgroup[1024], reason[128], schema[1024] = {}; + int resource_id, fd, schema_len = -1, ret = 0; if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) && strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) && @@ -520,28 +520,39 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) || !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) - sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata); + schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n", + "L3:", resource_id, '=', schemata); if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) || !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) - sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata); - - fp = fopen(controlgroup, "w"); - if (!fp) { - sprintf(reason, "Failed to open control group"); + schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n", + "MB:", resource_id, '=', schemata); + if (schema_len < 0 || schema_len >= sizeof(schema)) { + snprintf(reason, sizeof(reason), + "snprintf() failed with return value : %d", schema_len); ret = -1; - goto out; } - if (fprintf(fp, "%s\n", schema) < 0) { - sprintf(reason, "Failed to write schemata in control group"); - fclose(fp); + fd = open(controlgroup, O_WRONLY); + if (fd < 0) { + snprintf(reason, sizeof(reason), + "open() failed : %s", strerror(errno)); ret = -1; - goto out; + goto err_schema_not_empty; } - fclose(fp); + if (write(fd, schema, schema_len) < 0) { + snprintf(reason, sizeof(reason), + "write() failed : %s", strerror(errno)); + close(fd); + ret = -1; + + goto err_schema_not_empty; + } + close(fd); +err_schema_not_empty: + schema[schema_len - 1] = 0; out: ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n", schema, ret ? " # " : "", From patchwork Tue Oct 10 10:16:11 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Maciej Wieczor-Retman X-Patchwork-Id: 732329 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5B5A7E95A9C for ; Tue, 10 Oct 2023 10:16:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231153AbjJJKQu (ORCPT ); Tue, 10 Oct 2023 06:16:50 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40368 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230473AbjJJKQo (ORCPT ); Tue, 10 Oct 2023 06:16:44 -0400 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.151]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 23CE6101; Tue, 10 Oct 2023 03:16:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1696932999; x=1728468999; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=SBKJbfScbC8O7PragttfsiK5GsA19YlgI7tsRkSnlbg=; b=J4l9ywriSBGMLwGmzJznzkNzBRqN5vpmjhAZ9bElWPBZbcKl02KHZxrL Sn/Y++uyJYXe838VSO2f11LQnNFwWE2HXpLpuXRjqcTuIP0fnWYroUBha b2WcH2GM1VJzQvmJhBRpE2pHjp3UCdZPxWB7OKnNkCScav5VqmK0Rtick RE3jhYlGUAACFoOY9wGVri2ZWGuOI+p4yZb3TgD7zhDZmPWIbpM0tHSqH mZzz0C16067+i183hnitXGBruOU7L6qSsucKvaemRBQzmGiU68I4hFKMA toZ3SlCYWZDiyL1QqpucoCB2Ktfvosy+v7NZCLpcv4NKDc/vbPbm8mg9m Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10858"; a="364652617" X-IronPort-AV: E=Sophos;i="6.03,212,1694761200"; d="scan'208";a="364652617" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Oct 2023 03:16:38 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10858"; a="703252960" X-IronPort-AV: E=Sophos;i="6.03,212,1694761200"; d="scan'208";a="703252960" Received: from asalaman-mobl.ger.corp.intel.com (HELO wieczorr-mobl1.intel.com) ([10.213.16.145]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Oct 2023 03:16:35 -0700 From: Maciej Wieczor-Retman To: Fenghua Yu , Reinette Chatre , Shuah Khan Cc: ilpo.jarvinen@linux.intel.com, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org Subject: [PATCH RESEND v7 2/2] selftests/resctrl: Move run_benchmark() to a more fitting file Date: Tue, 10 Oct 2023 12:16:11 +0200 Message-ID: <6b1def8917a0718a34c38fea4c4e81f3117e7572.1696932728.git.maciej.wieczor-retman@intel.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-kselftest@vger.kernel.org resctrlfs.c contains mostly functions that interact in some way with resctrl FS entries while functions inside resctrl_val.c deal with measurements and benchmarking. run_benchmark() is located in resctrlfs.c even though it's purpose is not interacting with the resctrl FS but to execute cache checking logic. Move run_benchmark() to resctrl_val.c just before resctrl_val() that makes use of run_benchmark(). Make run_benchmark() static since it's not used between multiple files anymore. Remove return comment from kernel-doc since the function is type void. Signed-off-by: Maciej Wieczor-Retman Reviewed-by: Ilpo Järvinen Reviewed-by: Reinette Chatre --- Changelog v4: - Reword patch message very slightly. (Reinette) Changelog v3: - Make run_benchmark() static and remove it from the header. (Reinette) - Remove return void kernel-doc comment. (Ilpo) - Added Ilpo's reviewed-by tag. tools/testing/selftests/resctrl/resctrl.h | 1 - tools/testing/selftests/resctrl/resctrl_val.c | 50 ++++++++++++++++++ tools/testing/selftests/resctrl/resctrlfs.c | 52 ------------------- 3 files changed, 50 insertions(+), 53 deletions(-) diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 8578a8b4e145..a33f414f6019 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -86,7 +86,6 @@ int validate_bw_report_request(char *bw_report); bool validate_resctrl_feature_request(const char *resource, const char *feature); char *fgrep(FILE *inf, const char *str); int taskset_benchmark(pid_t bm_pid, int cpu_no); -void run_benchmark(int signum, siginfo_t *info, void *ucontext); int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val); int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index a9fe61133119..0577e983067a 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -625,6 +625,56 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start) return 0; } +/* + * run_benchmark - Run a specified benchmark or fill_buf (default benchmark) + * in specified signal. Direct benchmark stdio to /dev/null. + * @signum: signal number + * @info: signal info + * @ucontext: user context in signal handling + */ +static void run_benchmark(int signum, siginfo_t *info, void *ucontext) +{ + int operation, ret, memflush; + char **benchmark_cmd; + size_t span; + bool once; + FILE *fp; + + benchmark_cmd = info->si_ptr; + + /* + * Direct stdio of child to /dev/null, so that only parent writes to + * stdio (console) + */ + fp = freopen("/dev/null", "w", stdout); + if (!fp) + PARENT_EXIT("Unable to direct benchmark status to /dev/null"); + + if (strcmp(benchmark_cmd[0], "fill_buf") == 0) { + /* Execute default fill_buf benchmark */ + span = strtoul(benchmark_cmd[1], NULL, 10); + memflush = atoi(benchmark_cmd[2]); + operation = atoi(benchmark_cmd[3]); + if (!strcmp(benchmark_cmd[4], "true")) + once = true; + else if (!strcmp(benchmark_cmd[4], "false")) + once = false; + else + PARENT_EXIT("Invalid once parameter"); + + if (run_fill_buf(span, memflush, operation, once)) + fprintf(stderr, "Error in running fill buffer\n"); + } else { + /* Execute specified benchmark */ + ret = execvp(benchmark_cmd[0], benchmark_cmd); + if (ret) + perror("wrong\n"); + } + + fclose(stdout); + PARENT_EXIT("Unable to run specified benchmark"); +} + /* * resctrl_val: execute benchmark and measure memory bandwidth on * the benchmark diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 05390afd4d6f..5ebd43683876 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -294,58 +294,6 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no) return 0; } -/* - * run_benchmark - Run a specified benchmark or fill_buf (default benchmark) - * in specified signal. Direct benchmark stdio to /dev/null. - * @signum: signal number - * @info: signal info - * @ucontext: user context in signal handling - * - * Return: void - */ -void run_benchmark(int signum, siginfo_t *info, void *ucontext) -{ - int operation, ret, memflush; - char **benchmark_cmd; - size_t span; - bool once; - FILE *fp; - - benchmark_cmd = info->si_ptr; - - /* - * Direct stdio of child to /dev/null, so that only parent writes to - * stdio (console) - */ - fp = freopen("/dev/null", "w", stdout); - if (!fp) - PARENT_EXIT("Unable to direct benchmark status to /dev/null"); - - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) { - /* Execute default fill_buf benchmark */ - span = strtoul(benchmark_cmd[1], NULL, 10); - memflush = atoi(benchmark_cmd[2]); - operation = atoi(benchmark_cmd[3]); - if (!strcmp(benchmark_cmd[4], "true")) - once = true; - else if (!strcmp(benchmark_cmd[4], "false")) - once = false; - else - PARENT_EXIT("Invalid once parameter"); - - if (run_fill_buf(span, memflush, operation, once)) - fprintf(stderr, "Error in running fill buffer\n"); - } else { - /* Execute specified benchmark */ - ret = execvp(benchmark_cmd[0], benchmark_cmd); - if (ret) - perror("wrong\n"); - } - - fclose(stdout); - PARENT_EXIT("Unable to run specified benchmark"); -} - /* * create_grp - Create a group only if one doesn't exist * @grp_name: Name of the group