From: Babu Moger <babu.moger@amd.com>
To: Fenghua Yu <fenghua.yu@intel.com>, Shuah Khan <shuah@kernel.org>,
Tony Luck <tony.luck@intel.com>,
Reinette Chatre <reinette.chatre@intel.com>
Cc: linux-kselftest <linux-kselftest@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v5 04/21] selftests/resctrl: Clean up resctrl features check
Date: Fri, 12 Mar 2021 13:09:50 -0600 [thread overview]
Message-ID: <ec07e30f-9d8b-6e68-e3c6-743c3435a0d1@amd.com> (raw)
In-Reply-To: <20210307145502.2916364-5-fenghua.yu@intel.com>
> -----Original Message-----
> From: Fenghua Yu <fenghua.yu@intel.com>
> Sent: Sunday, March 7, 2021 8:55 AM
> To: Shuah Khan <shuah@kernel.org>; Tony Luck <tony.luck@intel.com>;
> Reinette Chatre <reinette.chatre@intel.com>; Moger, Babu
> <Babu.Moger@amd.com>
> Cc: linux-kselftest <linux-kselftest@vger.kernel.org>; linux-kernel <linux-
> kernel@vger.kernel.org>; Fenghua Yu <fenghua.yu@intel.com>
> Subject: [PATCH v5 04/21] selftests/resctrl: Clean up resctrl features check
>
> Checking resctrl features call strcmp() to compare feature strings (e.g. "mba",
> "cat" etc). The checkings are error prone and don't have good coding style.
> Define the constant strings in macros and call
> strncmp() to solve the potential issues.
>
> Suggested-by: Shuah Khan <shuah@kernel.org>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> Change Log:
> v5:
> - Remove is_cat() etc functions and directly call strncmp() to check
> the features (Shuah).
>
> tools/testing/selftests/resctrl/cache.c | 8 +++----
> tools/testing/selftests/resctrl/cat_test.c | 2 +-
> tools/testing/selftests/resctrl/cqm_test.c | 2 +-
> tools/testing/selftests/resctrl/fill_buf.c | 4 ++--
> tools/testing/selftests/resctrl/mba_test.c | 2 +-
> tools/testing/selftests/resctrl/mbm_test.c | 2 +-
> tools/testing/selftests/resctrl/resctrl.h | 5 +++++
> .../testing/selftests/resctrl/resctrl_tests.c | 12 +++++-----
> tools/testing/selftests/resctrl/resctrl_val.c | 22 +++++++++----------
> tools/testing/selftests/resctrl/resctrlfs.c | 17 +++++++-------
> 10 files changed, 41 insertions(+), 35 deletions(-)
>
> diff --git a/tools/testing/selftests/resctrl/cache.c
> b/tools/testing/selftests/resctrl/cache.c
> index 38dbf4962e33..5922cc1b0386 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -182,7 +182,7 @@ int measure_cache_vals(struct resctrl_val_param
> *param, int bm_pid)
> /*
> * Measure cache miss from perf.
> */
> - if (!strcmp(param->resctrl_val, "cat")) {
> + if (!strncmp(param->resctrl_val, CAT_STR, sizeof(CAT_STR))) {
> ret = get_llc_perf(&llc_perf_miss);
> if (ret < 0)
> return ret;
> @@ -192,7 +192,7 @@ int measure_cache_vals(struct resctrl_val_param
> *param, int bm_pid)
> /*
> * Measure llc occupancy from resctrl.
> */
> - if (!strcmp(param->resctrl_val, "cqm")) {
> + if (!strncmp(param->resctrl_val, CQM_STR, sizeof(CQM_STR))) {
> ret = get_llc_occu_resctrl(&llc_occu_resc);
> if (ret < 0)
> return ret;
> @@ -234,7 +234,7 @@ int cat_val(struct resctrl_val_param *param)
> if (ret)
> return ret;
>
> - if ((strcmp(resctrl_val, "cat") == 0)) {
> + if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
> ret = initialize_llc_perf();
> if (ret)
> return ret;
> @@ -242,7 +242,7 @@ int cat_val(struct resctrl_val_param *param)
>
> /* Test runs until the callback setup() tells the test to stop. */
> while (1) {
> - if (strcmp(resctrl_val, "cat") == 0) {
> + if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR))) {
> ret = param->setup(1, param);
> if (ret) {
> ret = 0;
> diff --git a/tools/testing/selftests/resctrl/cat_test.c
> b/tools/testing/selftests/resctrl/cat_test.c
> index bdeeb5772592..20823725daca 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -164,7 +164,7 @@ int cat_perf_miss_val(int cpu_no, int n, char
> *cache_type)
> return -1;
>
> struct resctrl_val_param param = {
> - .resctrl_val = "cat",
> + .resctrl_val = CAT_STR,
> .cpu_no = cpu_no,
> .mum_resctrlfs = 0,
> .setup = cat_setup,
> diff --git a/tools/testing/selftests/resctrl/cqm_test.c
> b/tools/testing/selftests/resctrl/cqm_test.c
> index de33d1c0466e..271752e9ef5b 100644
> --- a/tools/testing/selftests/resctrl/cqm_test.c
> +++ b/tools/testing/selftests/resctrl/cqm_test.c
> @@ -145,7 +145,7 @@ int cqm_resctrl_val(int cpu_no, int n, char
> **benchmark_cmd)
> }
>
> struct resctrl_val_param param = {
> - .resctrl_val = "cqm",
> + .resctrl_val = CQM_STR,
> .ctrlgrp = "c1",
> .mongrp = "m1",
> .cpu_no = cpu_no,
> diff --git a/tools/testing/selftests/resctrl/fill_buf.c
> b/tools/testing/selftests/resctrl/fill_buf.c
> index 79c611c99a3d..51e5cf22632f 100644
> --- a/tools/testing/selftests/resctrl/fill_buf.c
> +++ b/tools/testing/selftests/resctrl/fill_buf.c
> @@ -115,7 +115,7 @@ static int fill_cache_read(unsigned char *start_ptr,
> unsigned char *end_ptr,
>
> while (1) {
> ret = fill_one_span_read(start_ptr, end_ptr);
> - if (!strcmp(resctrl_val, "cat"))
> + if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
> break;
> }
>
> @@ -134,7 +134,7 @@ static int fill_cache_write(unsigned char *start_ptr,
> unsigned char *end_ptr, {
> while (1) {
> fill_one_span_write(start_ptr, end_ptr);
> - if (!strcmp(resctrl_val, "cat"))
> + if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)))
> break;
> }
>
> diff --git a/tools/testing/selftests/resctrl/mba_test.c
> b/tools/testing/selftests/resctrl/mba_test.c
> index 7bf8eaa6204b..6449fbd96096 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -141,7 +141,7 @@ void mba_test_cleanup(void) int
> mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
> {
> struct resctrl_val_param param = {
> - .resctrl_val = "mba",
> + .resctrl_val = MBA_STR,
> .ctrlgrp = "c1",
> .mongrp = "m1",
> .cpu_no = cpu_no,
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c
> b/tools/testing/selftests/resctrl/mbm_test.c
> index 4700f7453f81..ec6cfe01c9c2 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -114,7 +114,7 @@ void mbm_test_cleanup(void) int mbm_bw_change(int
> span, int cpu_no, char *bw_report, char **benchmark_cmd) {
> struct resctrl_val_param param = {
> - .resctrl_val = "mbm",
> + .resctrl_val = MBM_STR,
> .ctrlgrp = "c1",
> .mongrp = "m1",
> .span = span,
> diff --git a/tools/testing/selftests/resctrl/resctrl.h
> b/tools/testing/selftests/resctrl/resctrl.h
> index 12b77182cb44..36da6136af96 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -62,6 +62,11 @@ struct resctrl_val_param {
> int (*setup)(int num, ...);
> };
>
> +#define MBM_STR "mbm"
> +#define MBA_STR "mba"
> +#define CQM_STR "cqm"
> +#define CAT_STR "cat"
> +
> extern pid_t bm_pid, ppid;
> extern int tests_run;
>
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c
> b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 425cc85ac883..4b109a59f72d 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -85,13 +85,13 @@ int main(int argc, char **argv)
> cqm_test = false;
> cat_test = false;
> while (token) {
> - if (!strcmp(token, "mbm")) {
> + if (!strncmp(token, MBM_STR,
> sizeof(MBM_STR))) {
> mbm_test = true;
> - } else if (!strcmp(token, "mba")) {
> + } else if (!strncmp(token, MBA_STR,
> sizeof(MBA_STR))) {
> mba_test = true;
> - } else if (!strcmp(token, "cqm")) {
> + } else if (!strncmp(token, CQM_STR,
> sizeof(CQM_STR))) {
> cqm_test = true;
> - } else if (!strcmp(token, "cat")) {
> + } else if (!strncmp(token, CAT_STR,
> sizeof(CAT_STR))) {
> cat_test = true;
> } else {
> printf("invalid argument\n");
> @@ -161,7 +161,7 @@ int main(int argc, char **argv)
> if (!is_amd && mbm_test) {
> printf("# Starting MBM BW change ...\n");
> if (!has_ben)
> - sprintf(benchmark_cmd[5], "%s", "mba");
> + sprintf(benchmark_cmd[5], "%s", MBA_STR);
> res = mbm_bw_change(span, cpu_no, bw_report,
> benchmark_cmd);
> printf("%sok MBM: bw change\n", res ? "not " : "");
> mbm_test_cleanup();
> @@ -181,7 +181,7 @@ int main(int argc, char **argv)
> if (cqm_test) {
> printf("# Starting CQM test ...\n");
> if (!has_ben)
> - sprintf(benchmark_cmd[5], "%s", "cqm");
> + sprintf(benchmark_cmd[5], "%s", CQM_STR);
> res = cqm_resctrl_val(cpu_no, no_of_bits, benchmark_cmd);
> printf("%sok CQM: test\n", res ? "not " : "");
> cqm_test_cleanup();
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c
> b/tools/testing/selftests/resctrl/resctrl_val.c
> index 520fea3606d1..aed71fd0713b 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -397,10 +397,10 @@ static void initialize_mem_bw_resctrl(const char
> *ctrlgrp, const char *mongrp,
> return;
> }
>
> - if (strcmp(resctrl_val, "mbm") == 0)
> + if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)))
> set_mbm_path(ctrlgrp, mongrp, resource_id);
>
> - if ((strcmp(resctrl_val, "mba") == 0)) {
> + if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> if (ctrlgrp)
> sprintf(mbm_total_path,
> CON_MBM_LOCAL_BYTES_PATH,
> RESCTRL_PATH, ctrlgrp, resource_id); @@ -
> 524,7 +524,7 @@ static void initialize_llc_occu_resctrl(const char *ctrlgrp,
> const char *mongrp,
> return;
> }
>
> - if (strcmp(resctrl_val, "cqm") == 0)
> + if (!strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
> set_cqm_path(ctrlgrp, mongrp, resource_id); }
>
> @@ -579,8 +579,8 @@ int resctrl_val(char **benchmark_cmd, struct
> resctrl_val_param *param)
> if (strcmp(param->filename, "") == 0)
> sprintf(param->filename, "stdio");
>
> - if ((strcmp(resctrl_val, "mba")) == 0 ||
> - (strcmp(resctrl_val, "mbm")) == 0) {
> + if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) ||
> + !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
> ret = validate_bw_report_request(param->bw_report);
> if (ret)
> return ret;
> @@ -674,15 +674,15 @@ int resctrl_val(char **benchmark_cmd, struct
> resctrl_val_param *param)
> if (ret)
> goto out;
>
> - if ((strcmp(resctrl_val, "mbm") == 0) ||
> - (strcmp(resctrl_val, "mba") == 0)) {
> + if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> + !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> ret = initialize_mem_bw_imc();
> if (ret)
> goto out;
>
> initialize_mem_bw_resctrl(param->ctrlgrp, param->mongrp,
> param->cpu_no, resctrl_val);
> - } else if (strcmp(resctrl_val, "cqm") == 0)
> + } else if (!strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
> initialize_llc_occu_resctrl(param->ctrlgrp, param->mongrp,
> param->cpu_no, resctrl_val);
>
> @@ -710,8 +710,8 @@ int resctrl_val(char **benchmark_cmd, struct
> resctrl_val_param *param)
>
> /* Test runs until the callback setup() tells the test to stop. */
> while (1) {
> - if ((strcmp(resctrl_val, "mbm") == 0) ||
> - (strcmp(resctrl_val, "mba") == 0)) {
> + if (!strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) ||
> + !strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR))) {
> ret = param->setup(1, param);
> if (ret) {
> ret = 0;
> @@ -721,7 +721,7 @@ int resctrl_val(char **benchmark_cmd, struct
> resctrl_val_param *param)
> ret = measure_vals(param, &bw_resc_start);
> if (ret)
> break;
> - } else if (strcmp(resctrl_val, "cqm") == 0) {
> + } else if (!strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR))) {
> ret = param->setup(1, param);
> if (ret) {
> ret = 0;
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
> b/tools/testing/selftests/resctrl/resctrlfs.c
> index 2a16100c9c3f..4174e48e06d1 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -334,7 +334,7 @@ void run_benchmark(int signum, siginfo_t *info, void
> *ucontext)
> operation = atoi(benchmark_cmd[4]);
> sprintf(resctrl_val, "%s", benchmark_cmd[5]);
>
> - if (strcmp(resctrl_val, "cqm") != 0)
> + if (strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
> buffer_span = span * MB;
> else
> buffer_span = span;
> @@ -459,8 +459,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp,
> char *mongrp,
> goto out;
>
> /* Create mon grp and write pid into it for "mbm" and "cqm" test */
> - if ((strcmp(resctrl_val, "cqm") == 0) ||
> - (strcmp(resctrl_val, "mbm") == 0)) {
> + if (!strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)) ||
> + !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) {
> if (strlen(mongrp)) {
> sprintf(monitorgroup_p, "%s/mon_groups",
> controlgroup);
> sprintf(monitorgroup, "%s/%s", monitorgroup_p,
> mongrp); @@ -505,9 +505,9 @@ int write_schemata(char *ctrlgrp, char
> *schemata, int cpu_no, char *resctrl_val)
> int resource_id, ret = 0;
> FILE *fp;
>
> - if ((strcmp(resctrl_val, "mba") != 0) &&
> - (strcmp(resctrl_val, "cat") != 0) &&
> - (strcmp(resctrl_val, "cqm") != 0))
> + if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) &&
> + strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) &&
> + strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
> return -ENOENT;
>
> if (!schemata) {
> @@ -528,9 +528,10 @@ int write_schemata(char *ctrlgrp, char *schemata, int
> cpu_no, char *resctrl_val)
> else
> sprintf(controlgroup, "%s/schemata", RESCTRL_PATH);
>
> - if (!strcmp(resctrl_val, "cat") || !strcmp(resctrl_val, "cqm"))
> + if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) ||
> + !strncmp(resctrl_val, CQM_STR, sizeof(CQM_STR)))
> sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=',
> schemata);
> - if (strcmp(resctrl_val, "mba") == 0)
> + if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)))
> sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=',
> schemata);
>
> fp = fopen(controlgroup, "w");
> --
> 2.30.1
I see there are few other references as well. Like this.
1 cat_test.c cat_perf_miss_val 135 if
(!validate_resctrl_feature_request("cat"))
2 cqm_test.c cqm_resctrl_val 125 if
(!validate_resctrl_feature_request("cqm"))
3 mba_test.c mba_schemata_change 157 if
(!validate_resctrl_feature_request("mba"))
4 mbm_test.c mbm_bw_change 131 if
(!validate_resctrl_feature_request("mbm"))
Should you use CAT_STR and CQM_STR etc.. in here as well?
next prev parent reply other threads:[~2021-03-12 19:10 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-07 14:54 [PATCH v5 00/21] Miscellaneous fixes for resctrl selftests Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 01/21] selftests/resctrl: Enable gcc checks to detect buffer overflows Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 02/21] selftests/resctrl: Fix compilation issues for global variables Fenghua Yu
2021-03-12 19:08 ` Babu Moger
2021-03-12 21:51 ` Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 03/21] selftests/resctrl: Fix compilation issues for other " Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 04/21] selftests/resctrl: Clean up resctrl features check Fenghua Yu
2021-03-12 19:09 ` Babu Moger [this message]
2021-03-12 22:01 ` Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 05/21] selftests/resctrl: Ensure sibling CPU is not same as original CPU Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 06/21] selftests/resctrl: Fix missing options "-n" and "-p" Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 07/21] selftests/resctrl: Rename CQM test as CMT test Fenghua Yu
2021-03-12 19:10 ` Babu Moger
2021-03-07 14:54 ` [PATCH v5 08/21] selftests/resctrl: Call kselftest APIs to log test results Fenghua Yu
2021-03-12 19:12 ` Babu Moger
2021-03-12 22:09 ` Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 09/21] selftests/resctrl: Share show_cache_info() by CAT and CMT tests Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 10/21] selftests/resctrl: Fix a printed message Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 11/21] selftests/resctrl: Add config dependencies Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 12/21] selftests/resctrl: Check for resctrl mount point only if resctrl FS is supported Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 13/21] selftests/resctrl: Use resctrl/info for feature detection Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 14/21] selftests/resctrl: Fix MBA/MBM results reporting format Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 15/21] selftests/resctrl: Don't hard code value of "no_of_bits" variable Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 16/21] selftests/resctrl: Modularize resctrl test suite main() function Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 17/21] selftests/resctrl: Skip the test if requested resctrl feature is not supported Fenghua Yu
2021-03-07 14:54 ` [PATCH v5 18/21] selftests/resctrl: Fix unmount resctrl FS Fenghua Yu
2021-03-07 14:55 ` [PATCH v5 19/21] selftests/resctrl: Fix incorrect parsing of iMC counters Fenghua Yu
2021-03-07 14:55 ` [PATCH v5 20/21] selftests/resctrl: Fix checking for < 0 for unsigned values Fenghua Yu
2021-03-07 14:55 ` [PATCH v5 21/21] selftests/resctrl: Create .gitignore to include resctrl_tests Fenghua Yu
2021-03-12 19:08 ` [PATCH v5 00/21] Miscellaneous fixes for resctrl selftests Babu Moger
2021-03-12 22:11 ` Fenghua Yu
2021-04-02 18:10 ` Shuah Khan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ec07e30f-9d8b-6e68-e3c6-743c3435a0d1@amd.com \
--to=babu.moger@amd.com \
--cc=fenghua.yu@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=reinette.chatre@intel.com \
--cc=shuah@kernel.org \
--cc=tony.luck@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).