| Message ID | c3ff2c7df3d10931087e25e5488eb1ab2f5fe13c.1726164080.git.reinette.chatre@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selftests/resctrl: Support diverse platforms with MBM and MBA tests | expand |
On Thu, 12 Sep 2024, Reinette Chatre wrote: > The MBM and MBA tests need to discover the event and umask with which to > configure the performance event used to measure read memory bandwidth. > This is done by parsing the > /sys/bus/event_source/devices/uncore_imc_<imc instance>/events/cas_count_read > file for each iMC instance that contains the formatted > output: "event=<event>,umask=<umask>" > > Parsing of cas_count_read contents is done by initializing an array of > MAX_TOKENS elements with tokens (deliminated by "=,") from this file. > Start by removing the unnecessary append of a delimiter to the string Start what? (It sounds odd given the lack of any context, my guess is you're trying to refer to start/first one of the changes you make in the patch but the textual context does not support that conclusion.) I suggest you just rephrase it and avoid using "start" word altogether. > needing to be parsed. Per the strtok() man page: "delimiter bytes at > the start or end of the string are ignored". This has no impact on > the token placement within the array. > > After initialization, the actual event and umask is determined by > parsing the tokens directly following the "event" and "umask" tokens > respectively. > > Iterating through the array up to index "i < MAX_TOKENS" but then > accessing index "i + 1" risks array overrun during the final iteration. > Avoid array overrun by ensuring that the index used within for > loop will always be valid. > > Fixes: 1d3f08687d76 ("selftests/resctrl: Read memory bandwidth from perf IMC counter and from resctrl file system") > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> > --- > Changes since V1: > - New patch. > --- > tools/testing/selftests/resctrl/resctrl_val.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 70e8e31f5d1a..e88d5ca30517 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -83,13 +83,12 @@ static void get_event_and_umask(char *cas_count_cfg, int count, bool op) > char *token[MAX_TOKENS]; > int i = 0; > > - strcat(cas_count_cfg, ","); > token[0] = strtok(cas_count_cfg, "=,"); > > for (i = 1; i < MAX_TOKENS; i++) > token[i] = strtok(NULL, "=,"); > > - for (i = 0; i < MAX_TOKENS; i++) { > + for (i = 0; i < MAX_TOKENS - 1; i++) { > if (!token[i]) > break; > if (strcmp(token[i], "event") == 0) { > The code change seems fine so after improving the commit message, please add: Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Hi Ilpo, On 9/30/24 6:35 AM, Ilpo Järvinen wrote: > On Thu, 12 Sep 2024, Reinette Chatre wrote: > >> The MBM and MBA tests need to discover the event and umask with which to >> configure the performance event used to measure read memory bandwidth. >> This is done by parsing the >> /sys/bus/event_source/devices/uncore_imc_<imc instance>/events/cas_count_read >> file for each iMC instance that contains the formatted >> output: "event=<event>,umask=<umask>" >> >> Parsing of cas_count_read contents is done by initializing an array of >> MAX_TOKENS elements with tokens (deliminated by "=,") from this file. >> Start by removing the unnecessary append of a delimiter to the string > > Start what? (It sounds odd given the lack of any context, my guess is > you're trying to refer to start/first one of the changes you make in the > patch but the textual context does not support that conclusion.) I suggest > you just rephrase it and avoid using "start" word altogether. Indeed, I'll just drop the "Start by" and have the sentence be: "Remove the unnecessary append of a delimiter ..." > >> needing to be parsed. Per the strtok() man page: "delimiter bytes at >> the start or end of the string are ignored". This has no impact on >> the token placement within the array. >> >> After initialization, the actual event and umask is determined by >> parsing the tokens directly following the "event" and "umask" tokens >> respectively. >> >> Iterating through the array up to index "i < MAX_TOKENS" but then >> accessing index "i + 1" risks array overrun during the final iteration. >> Avoid array overrun by ensuring that the index used within for >> loop will always be valid. >> >> Fixes: 1d3f08687d76 ("selftests/resctrl: Read memory bandwidth from perf IMC counter and from resctrl file system") >> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> >> --- >> Changes since V1: >> - New patch. >> --- >> tools/testing/selftests/resctrl/resctrl_val.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c >> index 70e8e31f5d1a..e88d5ca30517 100644 >> --- a/tools/testing/selftests/resctrl/resctrl_val.c >> +++ b/tools/testing/selftests/resctrl/resctrl_val.c >> @@ -83,13 +83,12 @@ static void get_event_and_umask(char *cas_count_cfg, int count, bool op) >> char *token[MAX_TOKENS]; >> int i = 0; >> >> - strcat(cas_count_cfg, ","); >> token[0] = strtok(cas_count_cfg, "=,"); >> >> for (i = 1; i < MAX_TOKENS; i++) >> token[i] = strtok(NULL, "=,"); >> >> - for (i = 0; i < MAX_TOKENS; i++) { >> + for (i = 0; i < MAX_TOKENS - 1; i++) { >> if (!token[i]) >> break; >> if (strcmp(token[i], "event") == 0) { >> > > The code change seems fine so after improving the commit message, please > add: > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Thank you very much. Reinette
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 70e8e31f5d1a..e88d5ca30517 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -83,13 +83,12 @@ static void get_event_and_umask(char *cas_count_cfg, int count, bool op) char *token[MAX_TOKENS]; int i = 0; - strcat(cas_count_cfg, ","); token[0] = strtok(cas_count_cfg, "=,"); for (i = 1; i < MAX_TOKENS; i++) token[i] = strtok(NULL, "=,"); - for (i = 0; i < MAX_TOKENS; i++) { + for (i = 0; i < MAX_TOKENS - 1; i++) { if (!token[i]) break; if (strcmp(token[i], "event") == 0) {
The MBM and MBA tests need to discover the event and umask with which to configure the performance event used to measure read memory bandwidth. This is done by parsing the /sys/bus/event_source/devices/uncore_imc_<imc instance>/events/cas_count_read file for each iMC instance that contains the formatted output: "event=<event>,umask=<umask>" Parsing of cas_count_read contents is done by initializing an array of MAX_TOKENS elements with tokens (deliminated by "=,") from this file. Start by removing the unnecessary append of a delimiter to the string needing to be parsed. Per the strtok() man page: "delimiter bytes at the start or end of the string are ignored". This has no impact on the token placement within the array. After initialization, the actual event and umask is determined by parsing the tokens directly following the "event" and "umask" tokens respectively. Iterating through the array up to index "i < MAX_TOKENS" but then accessing index "i + 1" risks array overrun during the final iteration. Avoid array overrun by ensuring that the index used within for loop will always be valid. Fixes: 1d3f08687d76 ("selftests/resctrl: Read memory bandwidth from perf IMC counter and from resctrl file system") Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- Changes since V1: - New patch. --- tools/testing/selftests/resctrl/resctrl_val.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)