| Message ID | 20230808091625.12760-6-ilpo.jarvinen@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Rework benchmark command handling | expand |
Hi Ilpo, On 8/8/2023 2:16 AM, Ilpo Järvinen wrote: > Benchmark parameter uses fixed-size buffers in stack which is slightly > dangerous. As benchmark command is used in multiple tests, it should Could you please be specific with issues with current implementation? The term "slightly dangerous" is vague. > not be mutated by the tests. Due to the order of tests, mutating the > span argument in CMT test does not trigger any real problems currently. > > Mark benchmark_cmd strings as const and setup the benchmark command > using pointers. As span is constant in main(), just provide the default > span also as string to be used in setting up the default fill_buf > argument so no malloc() is required for it. What is wrong with using malloc()? > > CMT test has to create a copy of the benchmark command before altering > the benchmark command. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > tools/testing/selftests/resctrl/cmt_test.c | 23 ++++++++++--- > tools/testing/selftests/resctrl/mba_test.c | 2 +- > tools/testing/selftests/resctrl/mbm_test.c | 2 +- > tools/testing/selftests/resctrl/resctrl.h | 16 ++++++--- > .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++----------- > tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++-- > 6 files changed, 54 insertions(+), 32 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c > index 9d8e38e995ef..a40e12c3b1a7 100644 > --- a/tools/testing/selftests/resctrl/cmt_test.c > +++ b/tools/testing/selftests/resctrl/cmt_test.c > @@ -68,14 +68,16 @@ void cmt_test_cleanup(void) > remove(RESULT_FILE_NAME); > } > > -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd) > { > + const char *cmd[BENCHMARK_ARGS]; > unsigned long cache_size = 0; > unsigned long long_mask; > + char *span_str = NULL; > char cbm_mask[256]; > int count_of_bits; > size_t span; > - int ret; > + int ret, i; > > if (!validate_resctrl_feature_request(CMT_STR)) > return -1; > @@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > }; > > span = cache_size * n / count_of_bits; > - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) > - sprintf(benchmark_cmd[1], "%zu", span); > + /* Duplicate the command to be able to replace span in it */ > + for (i = 0; benchmark_cmd[i]; i++) > + cmd[i] = benchmark_cmd[i]; > + cmd[i] = NULL; > + > + if (strcmp(cmd[0], "fill_buf") == 0) { > + span_str = malloc(SIZE_MAX_DECIMAL_SIZE); > + if (!span_str) > + return -1; > + snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span); Have you considered asprintf()? > + cmd[1] = span_str; > + } It looks to me that array only needs to be duplicated if the default benchmark is used? > > remove(RESULT_FILE_NAME); > > - ret = resctrl_val(benchmark_cmd, ¶m); > + ret = resctrl_val(cmd, ¶m); > if (ret) > goto out; > ... > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index bcd0d2060f81..ddb1e83a3a64 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -6,6 +6,7 @@ > #include <math.h> > #include <errno.h> > #include <sched.h> > +#include <stdint.h> > #include <stdlib.h> > #include <unistd.h> > #include <string.h> > @@ -38,7 +39,14 @@ > > #define END_OF_TESTS 1 > > +#define BENCHMARK_ARGS 64 > + > +/* Approximate %zu max length */ > +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2) > + > +/* Define default span both as integer and string, these should match */ > #define DEFAULT_SPAN (250 * MB) > +#define DEFAULT_SPAN_STR "262144000" I think above hardcoding can be eliminated by using asprintf()? This does allocate memory though so I would like to understand why one goal is to not dynamically allocate memory. > > #define PARENT_EXIT(err_msg) \ > do { \ Reinette
Hi Ilpo, On 8/15/2023 2:42 AM, Ilpo Järvinen wrote: > On Mon, 14 Aug 2023, Reinette Chatre wrote: > >> Hi Ilpo, >> >> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote: >>> Benchmark parameter uses fixed-size buffers in stack which is slightly >>> dangerous. As benchmark command is used in multiple tests, it should >> >> Could you please be specific with issues with current implementation? >> The term "slightly dangerous" is vague. > > I've reworded this so this fragment no longer remains here because the > earlier patch got changes so the dangerous part is no longer there. > >>> not be mutated by the tests. Due to the order of tests, mutating the >>> span argument in CMT test does not trigger any real problems currently. >>> >>> Mark benchmark_cmd strings as const and setup the benchmark command >>> using pointers. As span is constant in main(), just provide the default >>> span also as string to be used in setting up the default fill_buf >>> argument so no malloc() is required for it. >> >> What is wrong with using malloc()? > > Nothing. I think you slightly misunderstood what I meant here. > > The main challenge is not malloc() itself but keeping track of what memory > has been dynamically allocated, which is simple if nothing has been > malloc()ed. With the const benchmark command and default span, there's no > need to malloc(), thus I avoid it to keep things simpler on the free() > side. Keeping things symmetrical helps. > > I've tried to reword the entire changelog, please check the v2 changelog > once I post it. > >>> CMT test has to create a copy of the benchmark command before altering >>> the benchmark command. >>> >>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >>> --- >>> tools/testing/selftests/resctrl/cmt_test.c | 23 ++++++++++--- >>> tools/testing/selftests/resctrl/mba_test.c | 2 +- >>> tools/testing/selftests/resctrl/mbm_test.c | 2 +- >>> tools/testing/selftests/resctrl/resctrl.h | 16 ++++++--- >>> .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++----------- >>> tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++-- >>> 6 files changed, 54 insertions(+), 32 deletions(-) >>> >>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c >>> index 9d8e38e995ef..a40e12c3b1a7 100644 >>> --- a/tools/testing/selftests/resctrl/cmt_test.c >>> +++ b/tools/testing/selftests/resctrl/cmt_test.c >>> @@ -68,14 +68,16 @@ void cmt_test_cleanup(void) >>> remove(RESULT_FILE_NAME); >>> } >>> >>> -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) >>> +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd) >>> { >>> + const char *cmd[BENCHMARK_ARGS]; >>> unsigned long cache_size = 0; >>> unsigned long long_mask; >>> + char *span_str = NULL; >>> char cbm_mask[256]; >>> int count_of_bits; >>> size_t span; >>> - int ret; >>> + int ret, i; >>> >>> if (!validate_resctrl_feature_request(CMT_STR)) >>> return -1; >>> @@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) >>> }; >>> >>> span = cache_size * n / count_of_bits; >>> - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) >>> - sprintf(benchmark_cmd[1], "%zu", span); >>> + /* Duplicate the command to be able to replace span in it */ >>> + for (i = 0; benchmark_cmd[i]; i++) >>> + cmd[i] = benchmark_cmd[i]; >>> + cmd[i] = NULL; >>> + >>> + if (strcmp(cmd[0], "fill_buf") == 0) { >>> + span_str = malloc(SIZE_MAX_DECIMAL_SIZE); >>> + if (!span_str) >>> + return -1; >>> + snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span); >> >> Have you considered asprintf()? > > Changed to asprintf() now. > >>> + cmd[1] = span_str; >>> + } >> >> It looks to me that array only needs to be duplicated if the >> default benchmark is used? > > While it's true, another aspect is how that affects the code flow. If I > make that change, the benchmark command could come from two different > places which is now avoided. IMHO, the current approach is simpler to > understand even if it does the unnecessary copy of a few pointers. cmd provided to resctrl_val() can point to original buffer or modified buffer. What is wrong with a pointer possibly pointing to two different locations? > > But please let me know if you still prefer the other way around so I can > change to that. Your motivation for this approach is not clear to me. > >>> remove(RESULT_FILE_NAME); >>> >>> - ret = resctrl_val(benchmark_cmd, ¶m); >>> + ret = resctrl_val(cmd, ¶m); >>> if (ret) >>> goto out; >>> >> >> ... >> >>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h >>> index bcd0d2060f81..ddb1e83a3a64 100644 >>> --- a/tools/testing/selftests/resctrl/resctrl.h >>> +++ b/tools/testing/selftests/resctrl/resctrl.h >>> @@ -6,6 +6,7 @@ >>> #include <math.h> >>> #include <errno.h> >>> #include <sched.h> >>> +#include <stdint.h> >>> #include <stdlib.h> >>> #include <unistd.h> >>> #include <string.h> >>> @@ -38,7 +39,14 @@ >>> >>> #define END_OF_TESTS 1 >>> >>> +#define BENCHMARK_ARGS 64 >>> + >>> +/* Approximate %zu max length */ >>> +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2) >>> + >>> +/* Define default span both as integer and string, these should match */ >>> #define DEFAULT_SPAN (250 * MB) >>> +#define DEFAULT_SPAN_STR "262144000" >> >> I think above hardcoding can be eliminated by using asprintf()? This >> does allocate memory though so I would like to understand why one >> goal is to not dynamically allocate memory. > > Because it's simpler on the _free() side_. If there's no allocation, no > free() is needed. > > Only challenge that remains is the int -> string conversion for the > default span which can be either done like in the patch or using some > preprocessor trickery to convert the number to string. If you prefer the > latter, I can change to that so it's not hardcoded both as int and string. > This manual int->string sounds like the trickery to me and can be avoided by just using asprintf(). I understand that no free() is needed when no memory is allocated but it looks to me as though these allocations can be symmetrical - allocate the memory before the tests are run and free it after? Reinette
On Tue, 15 Aug 2023, Reinette Chatre wrote: > On 8/15/2023 2:42 AM, Ilpo Järvinen wrote: > > On Mon, 14 Aug 2023, Reinette Chatre wrote: > >> > >> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote: > >>> Benchmark parameter uses fixed-size buffers in stack which is slightly > >>> dangerous. As benchmark command is used in multiple tests, it should > >> > >> Could you please be specific with issues with current implementation? > >> The term "slightly dangerous" is vague. > > > > I've reworded this so this fragment no longer remains here because the > > earlier patch got changes so the dangerous part is no longer there. > > > >>> not be mutated by the tests. Due to the order of tests, mutating the > >>> span argument in CMT test does not trigger any real problems currently. > >>> > >>> Mark benchmark_cmd strings as const and setup the benchmark command > >>> using pointers. As span is constant in main(), just provide the default > >>> span also as string to be used in setting up the default fill_buf > >>> argument so no malloc() is required for it. > >> > >> What is wrong with using malloc()? > > > > Nothing. I think you slightly misunderstood what I meant here. > > > > The main challenge is not malloc() itself but keeping track of what memory > > has been dynamically allocated, which is simple if nothing has been > > malloc()ed. With the const benchmark command and default span, there's no > > need to malloc(), thus I avoid it to keep things simpler on the free() > > side. > > Keeping things symmetrical helps. > > > I've tried to reword the entire changelog, please check the v2 changelog > > once I post it. > > > >>> CMT test has to create a copy of the benchmark command before altering > >>> the benchmark command. > >>> > >>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > >>> --- > >>> tools/testing/selftests/resctrl/cmt_test.c | 23 ++++++++++--- > >>> tools/testing/selftests/resctrl/mba_test.c | 2 +- > >>> tools/testing/selftests/resctrl/mbm_test.c | 2 +- > >>> tools/testing/selftests/resctrl/resctrl.h | 16 ++++++--- > >>> .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++----------- > >>> tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++-- > >>> 6 files changed, 54 insertions(+), 32 deletions(-) > >>> > >>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c > >>> index 9d8e38e995ef..a40e12c3b1a7 100644 > >>> --- a/tools/testing/selftests/resctrl/cmt_test.c > >>> +++ b/tools/testing/selftests/resctrl/cmt_test.c > >>> @@ -68,14 +68,16 @@ void cmt_test_cleanup(void) > >>> remove(RESULT_FILE_NAME); > >>> } > >>> > >>> -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > >>> +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd) > >>> { > >>> + const char *cmd[BENCHMARK_ARGS]; > >>> unsigned long cache_size = 0; > >>> unsigned long long_mask; > >>> + char *span_str = NULL; > >>> char cbm_mask[256]; > >>> int count_of_bits; > >>> size_t span; > >>> - int ret; > >>> + int ret, i; > >>> > >>> if (!validate_resctrl_feature_request(CMT_STR)) > >>> return -1; > >>> @@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > >>> }; > >>> > >>> span = cache_size * n / count_of_bits; > >>> - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) > >>> - sprintf(benchmark_cmd[1], "%zu", span); > >>> + /* Duplicate the command to be able to replace span in it */ > >>> + for (i = 0; benchmark_cmd[i]; i++) > >>> + cmd[i] = benchmark_cmd[i]; > >>> + cmd[i] = NULL; > >>> + > >>> + if (strcmp(cmd[0], "fill_buf") == 0) { > >>> + span_str = malloc(SIZE_MAX_DECIMAL_SIZE); > >>> + if (!span_str) > >>> + return -1; > >>> + snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span); > >> > >> Have you considered asprintf()? > > > > Changed to asprintf() now. > > > >>> + cmd[1] = span_str; > >>> + } > >> > >> It looks to me that array only needs to be duplicated if the > >> default benchmark is used? > > > > While it's true, another aspect is how that affects the code flow. If I > > make that change, the benchmark command could come from two different > > places which is now avoided. IMHO, the current approach is simpler to > > understand even if it does the unnecessary copy of a few pointers. > > cmd provided to resctrl_val() can point to original buffer or modified > buffer. What is wrong with a pointer possibly pointing to two different > locations? I'll change to that. > > But please let me know if you still prefer the other way around so I can > > change to that. > > Your motivation for this approach is not clear to me. > > > > >>> remove(RESULT_FILE_NAME); > >>> > >>> - ret = resctrl_val(benchmark_cmd, ¶m); > >>> + ret = resctrl_val(cmd, ¶m); > >>> if (ret) > >>> goto out; > >>> > >> > >> ... > >> > >>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > >>> index bcd0d2060f81..ddb1e83a3a64 100644 > >>> --- a/tools/testing/selftests/resctrl/resctrl.h > >>> +++ b/tools/testing/selftests/resctrl/resctrl.h > >>> @@ -6,6 +6,7 @@ > >>> #include <math.h> > >>> #include <errno.h> > >>> #include <sched.h> > >>> +#include <stdint.h> > >>> #include <stdlib.h> > >>> #include <unistd.h> > >>> #include <string.h> > >>> @@ -38,7 +39,14 @@ > >>> > >>> #define END_OF_TESTS 1 > >>> > >>> +#define BENCHMARK_ARGS 64 > >>> + > >>> +/* Approximate %zu max length */ > >>> +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2) > >>> + > >>> +/* Define default span both as integer and string, these should match */ > >>> #define DEFAULT_SPAN (250 * MB) > >>> +#define DEFAULT_SPAN_STR "262144000" > >> > >> I think above hardcoding can be eliminated by using asprintf()? This > >> does allocate memory though so I would like to understand why one > >> goal is to not dynamically allocate memory. > > > > Because it's simpler on the _free() side_. If there's no allocation, no > > free() is needed. > > > > Only challenge that remains is the int -> string conversion for the > > default span which can be either done like in the patch or using some > > preprocessor trickery to convert the number to string. If you prefer the > > latter, I can change to that so it's not hardcoded both as int and string. > > > > This manual int->string sounds like the trickery to me and can be avoided > by just using asprintf(). I understand that no free() is needed when no > memory is allocated but it looks to me as though these allocations can > be symmetrical - allocate the memory before the tests are run and free it > after? It could be symmetrical but that means I'll be doing unnecessary alloc if -b is provided which I assume you're against given your comment on always creating copy of cmd in CMT test's case. I think I'll use similar resolution to this as CMT test does, it has an extra variable which is NULL in when -b is provided so free() is no-op on that path. Then I can use asprintf().
Hi Ilpo, On 8/17/2023 1:32 AM, Ilpo Järvinen wrote: > On Wed, 16 Aug 2023, Reinette Chatre wrote: >> On 8/16/2023 12:13 AM, Ilpo Järvinen wrote: >>> On Tue, 15 Aug 2023, Reinette Chatre wrote: >>>> On 8/15/2023 2:42 AM, Ilpo Järvinen wrote: >>>>> On Mon, 14 Aug 2023, Reinette Chatre wrote: >>>>>> >>>>>> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote: >> >> ... >>>>>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h >>>>>>> index bcd0d2060f81..ddb1e83a3a64 100644 >>>>>>> --- a/tools/testing/selftests/resctrl/resctrl.h >>>>>>> +++ b/tools/testing/selftests/resctrl/resctrl.h >>>>>>> @@ -6,6 +6,7 @@ >>>>>>> #include <math.h> >>>>>>> #include <errno.h> >>>>>>> #include <sched.h> >>>>>>> +#include <stdint.h> >>>>>>> #include <stdlib.h> >>>>>>> #include <unistd.h> >>>>>>> #include <string.h> >>>>>>> @@ -38,7 +39,14 @@ >>>>>>> >>>>>>> #define END_OF_TESTS 1 >>>>>>> >>>>>>> +#define BENCHMARK_ARGS 64 >>>>>>> + >>>>>>> +/* Approximate %zu max length */ >>>>>>> +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2) >>>>>>> + >>>>>>> +/* Define default span both as integer and string, these should match */ >>>>>>> #define DEFAULT_SPAN (250 * MB) >>>>>>> +#define DEFAULT_SPAN_STR "262144000" >>>>>> >>>>>> I think above hardcoding can be eliminated by using asprintf()? This >>>>>> does allocate memory though so I would like to understand why one >>>>>> goal is to not dynamically allocate memory. >>>>> >>>>> Because it's simpler on the _free() side_. If there's no allocation, no >>>>> free() is needed. >>>>> >>>>> Only challenge that remains is the int -> string conversion for the >>>>> default span which can be either done like in the patch or using some >>>>> preprocessor trickery to convert the number to string. If you prefer the >>>>> latter, I can change to that so it's not hardcoded both as int and string. >>>>> >>>> >>>> This manual int->string sounds like the trickery to me and can be avoided >>>> by just using asprintf(). I understand that no free() is needed when no >>>> memory is allocated but it looks to me as though these allocations can >>>> be symmetrical - allocate the memory before the tests are run and free it >>>> after? >>> >>> It could be symmetrical but that means I'll be doing unnecessary alloc if >>> -b is provided which I assume you're against given your comment on always >>> creating copy of cmd in CMT test's case. >> >> I seemed to have lost track here ... could you please elaborate where the >> unnecessary alloc will be? > > If there's what you call "symmetry", it implies the code always does > alloc. However, the logic in main() is such that when -b is provided, no No. Symmetry does not mean "always alloc" - what I attempted to covey was that tracking allocations become easier if the memory is freed in code that is symmetrical to where the memory is allocated. For example, if memory is allocated at the beginning of main(), then it is freed on exit of main(), or if there is a "test_resources_alloc()" that is called before a test is run then there could be a "test_resources_free()" that is called after a test is run. > default benchmark command needs to be assigned, so no alloc for span is > necessary. Thus, there either is unnecessary alloc with -b or _no > symmetry_. > > But I've already converted to asprintf() so no need to continue this > discussion. Please note that asprintf() allocates memory that needs to be freed. Reinette
On Thu, 17 Aug 2023, Reinette Chatre wrote: > On 8/17/2023 1:32 AM, Ilpo Järvinen wrote: > > On Wed, 16 Aug 2023, Reinette Chatre wrote: > >> On 8/16/2023 12:13 AM, Ilpo Järvinen wrote: > >>> On Tue, 15 Aug 2023, Reinette Chatre wrote: > >>>> On 8/15/2023 2:42 AM, Ilpo Järvinen wrote: > >>>>> On Mon, 14 Aug 2023, Reinette Chatre wrote: > >>>>>> On 8/8/2023 2:16 AM, Ilpo Järvinen wrote: > >>>>>>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > >>>>>>> index bcd0d2060f81..ddb1e83a3a64 100644 > >>>>>>> --- a/tools/testing/selftests/resctrl/resctrl.h > >>>>>>> +++ b/tools/testing/selftests/resctrl/resctrl.h > >>>>>>> @@ -6,6 +6,7 @@ > >>>>>>> #include <math.h> > >>>>>>> #include <errno.h> > >>>>>>> #include <sched.h> > >>>>>>> +#include <stdint.h> > >>>>>>> #include <stdlib.h> > >>>>>>> #include <unistd.h> > >>>>>>> #include <string.h> > >>>>>>> @@ -38,7 +39,14 @@ > >>>>>>> > >>>>>>> #define END_OF_TESTS 1 > >>>>>>> > >>>>>>> +#define BENCHMARK_ARGS 64 > >>>>>>> + > >>>>>>> +/* Approximate %zu max length */ > >>>>>>> +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2) > >>>>>>> + > >>>>>>> +/* Define default span both as integer and string, these should match */ > >>>>>>> #define DEFAULT_SPAN (250 * MB) > >>>>>>> +#define DEFAULT_SPAN_STR "262144000" > >>>>>> > >>>>>> I think above hardcoding can be eliminated by using asprintf()? This > >>>>>> does allocate memory though so I would like to understand why one > >>>>>> goal is to not dynamically allocate memory. > >>>>> > >>>>> Because it's simpler on the _free() side_. If there's no allocation, no > >>>>> free() is needed. > >>>>> > >>>>> Only challenge that remains is the int -> string conversion for the > >>>>> default span which can be either done like in the patch or using some > >>>>> preprocessor trickery to convert the number to string. If you prefer the > >>>>> latter, I can change to that so it's not hardcoded both as int and string. > >>>>> > >>>> > >>>> This manual int->string sounds like the trickery to me and can be avoided > >>>> by just using asprintf(). I understand that no free() is needed when no > >>>> memory is allocated but it looks to me as though these allocations can > >>>> be symmetrical - allocate the memory before the tests are run and free it > >>>> after? > >>> > >>> It could be symmetrical but that means I'll be doing unnecessary alloc if > >>> -b is provided which I assume you're against given your comment on always > >>> creating copy of cmd in CMT test's case. > >> > >> I seemed to have lost track here ... could you please elaborate where the > >> unnecessary alloc will be? > > > > If there's what you call "symmetry", it implies the code always does > > alloc. However, the logic in main() is such that when -b is provided, no > > No. Symmetry does not mean "always alloc" Oh, so it simply meant code without memory leaks :-). > - what I attempted to covey was > that tracking allocations become easier if the memory is freed in code > that is symmetrical to where the memory is allocated. That's, unfortunately, what I needed to do even if it resulted in less clean code when I, in a later patch that is not part of this series, added a function the setup the default parameters into user parameters struct. main() will now pass that span_str for it to do "symmetrical" free inside main(). > For example, if memory > is allocated at the beginning of main(), then it is freed on exit of main(), you make it sound easier than the reality is. There's no singular point that is "exit of main()". It has way too many exit paths because of how selftests framework works. It doesn't give you back control when you ask it to exit the tests. You'll see how complicated this gets once we get to the user parameters structure patch but I'll use asprintf()+free() for now ;-). We can revisit this discussion if you feel like it when we get to that patch. ...And to think this all is because C cannot easily make known constant int -> string conversion w/o some runtime code. > or if there is a "test_resources_alloc()" that is called before a test is > run then there could be a "test_resources_free()" that is called > after a test is run. > > > default benchmark command needs to be assigned, so no alloc for span is > > necessary. Thus, there either is unnecessary alloc with -b or _no > > symmetry_. > > > > But I've already converted to asprintf() so no need to continue this > > discussion. > > Please note that asprintf() allocates memory that needs to be freed. Of course.
diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 9d8e38e995ef..a40e12c3b1a7 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -68,14 +68,16 @@ void cmt_test_cleanup(void) remove(RESULT_FILE_NAME); } -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd) { + const char *cmd[BENCHMARK_ARGS]; unsigned long cache_size = 0; unsigned long long_mask; + char *span_str = NULL; char cbm_mask[256]; int count_of_bits; size_t span; - int ret; + int ret, i; if (!validate_resctrl_feature_request(CMT_STR)) return -1; @@ -111,12 +113,22 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) }; span = cache_size * n / count_of_bits; - if (strcmp(benchmark_cmd[0], "fill_buf") == 0) - sprintf(benchmark_cmd[1], "%zu", span); + /* Duplicate the command to be able to replace span in it */ + for (i = 0; benchmark_cmd[i]; i++) + cmd[i] = benchmark_cmd[i]; + cmd[i] = NULL; + + if (strcmp(cmd[0], "fill_buf") == 0) { + span_str = malloc(SIZE_MAX_DECIMAL_SIZE); + if (!span_str) + return -1; + snprintf(span_str, SIZE_MAX_DECIMAL_SIZE, "%zu", span); + cmd[1] = span_str; + } remove(RESULT_FILE_NAME); - ret = resctrl_val(benchmark_cmd, ¶m); + ret = resctrl_val(cmd, ¶m); if (ret) goto out; @@ -124,6 +136,7 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) out: cmt_test_cleanup(); + free(span_str); return ret; } diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c index 094424d835d0..cf8284dadcb2 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) remove(RESULT_FILE_NAME); } -int mba_schemata_change(int cpu_no, char **benchmark_cmd) +int mba_schemata_change(int cpu_no, const char * const *benchmark_cmd) { struct resctrl_val_param param = { .resctrl_val = MBA_STR, diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c index b830fc84338b..1ae131a2e246 100644 --- a/tools/testing/selftests/resctrl/mbm_test.c +++ b/tools/testing/selftests/resctrl/mbm_test.c @@ -109,7 +109,7 @@ void mbm_test_cleanup(void) remove(RESULT_FILE_NAME); } -int mbm_bw_change(int cpu_no, char **benchmark_cmd) +int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd) { struct resctrl_val_param param = { .resctrl_val = MBM_STR, diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index bcd0d2060f81..ddb1e83a3a64 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -6,6 +6,7 @@ #include <math.h> #include <errno.h> #include <sched.h> +#include <stdint.h> #include <stdlib.h> #include <unistd.h> #include <string.h> @@ -38,7 +39,14 @@ #define END_OF_TESTS 1 +#define BENCHMARK_ARGS 64 + +/* Approximate %zu max length */ +#define SIZE_MAX_DECIMAL_SIZE (sizeof(SIZE_MAX) * 8 / 3 + 2) + +/* Define default span both as integer and string, these should match */ #define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000" #define PARENT_EXIT(err_msg) \ do { \ @@ -97,11 +105,11 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu, int group_fd, unsigned long flags); int run_fill_buf(size_t span, int memflush, int op, bool once); -int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param); -int mbm_bw_change(int cpu_no, char **benchmark_cmd); +int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param); +int mbm_bw_change(int cpu_no, const char * const *benchmark_cmd); void tests_cleanup(void); void mbm_test_cleanup(void); -int mba_schemata_change(int cpu_no, char **benchmark_cmd); +int mba_schemata_change(int cpu_no, const char *const *benchmark_cmd); void mba_test_cleanup(void); int get_cbm_mask(char *cache_type, char *cbm_mask); int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size); @@ -111,7 +119,7 @@ void signal_handler_unregister(void); int cat_val(struct resctrl_val_param *param, size_t span); void cat_test_cleanup(void); int cat_perf_miss_val(int cpu_no, int no_of_bits, char *cache_type); -int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd); +int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd); unsigned int count_bits(unsigned long n); void cmt_test_cleanup(void); int get_core_sibling(int cpu_no); diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 903167a192d7..74a10abeb01d 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -10,8 +10,9 @@ */ #include "resctrl.h" -#define BENCHMARK_ARGS 64 -#define BENCHMARK_ARG_SIZE 64 +/* Define default span both as integer and string, these should match */ +#define DEFAULT_SPAN (250 * MB) +#define DEFAULT_SPAN_STR "262144000" static int detect_vendor(void) { @@ -70,7 +71,7 @@ void tests_cleanup(void) cat_test_cleanup(); } -static void run_mbm_test(char **benchmark_cmd, int cpu_no) +static void run_mbm_test(const char **benchmark_cmd, int cpu_no) { int res; @@ -96,7 +97,7 @@ static void run_mbm_test(char **benchmark_cmd, int cpu_no) umount_resctrlfs(); } -static void run_mba_test(char **benchmark_cmd, int cpu_no) +static void run_mba_test(const char **benchmark_cmd, int cpu_no) { int res; @@ -120,7 +121,7 @@ static void run_mba_test(char **benchmark_cmd, int cpu_no) umount_resctrlfs(); } -static void run_cmt_test(char **benchmark_cmd, int cpu_no) +static void run_cmt_test(const char **benchmark_cmd, int cpu_no) { int res; @@ -173,9 +174,8 @@ static void run_cat_test(int cpu_no, int no_of_bits) int main(int argc, char **argv) { bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true; - char benchmark_cmd_area[BENCHMARK_ARGS][BENCHMARK_ARG_SIZE]; int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0; - char *benchmark_cmd[BENCHMARK_ARGS]; + const char *benchmark_cmd[BENCHMARK_ARGS]; int ben_ind, ben_count, tests = 0; bool cat_test = true; @@ -257,21 +257,16 @@ int main(int argc, char **argv) ksft_exit_fail_msg("Too long benchmark command"); /* Extract benchmark command from command line. */ - for (i = ben_ind; i < argc; i++) { - benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i]; - sprintf(benchmark_cmd[i - ben_ind], "%s", argv[i]); - } + for (i = 0; i < argc - ben_ind; i++) + benchmark_cmd[i] = argv[i + ben_ind]; benchmark_cmd[ben_count] = NULL; } else { /* If no benchmark is given by "-b" argument, use fill_buf. */ - for (i = 0; i < 5; i++) - benchmark_cmd[i] = benchmark_cmd_area[i]; - - strcpy(benchmark_cmd[0], "fill_buf"); - sprintf(benchmark_cmd[1], "%zu", (size_t)DEFAULT_SPAN); - strcpy(benchmark_cmd[2], "1"); - strcpy(benchmark_cmd[3], "0"); - strcpy(benchmark_cmd[4], "false"); + benchmark_cmd[0] = "fill_buf"; + benchmark_cmd[1] = DEFAULT_SPAN_STR; + benchmark_cmd[2] = "1"; + benchmark_cmd[3] = "0"; + benchmark_cmd[4] = "false"; benchmark_cmd[5] = NULL; } diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index f0f6c5f6e98b..51963a6f2186 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -629,7 +629,7 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start) * * Return: 0 on success. non-zero on failure. */ -int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) +int resctrl_val(const char * const *benchmark_cmd, struct resctrl_val_param *param) { char *resctrl_val = param->resctrl_val; unsigned long bw_resc_start = 0; @@ -710,7 +710,13 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param) if (ret) goto out; - value.sival_ptr = benchmark_cmd; + /* + * The cast removes constness but nothing mutates benchmark_cmd within + * the context of this process. At the receiving process, it becomes + * argv, which is mutable, on exec() but that's after fork() so it + * doesn't matter for the process running the tests. + */ + value.sival_ptr = (void *)benchmark_cmd; /* Taskset benchmark to specified cpu */ ret = taskset_benchmark(bm_pid, param->cpu_no);
Benchmark parameter uses fixed-size buffers in stack which is slightly dangerous. As benchmark command is used in multiple tests, it should not be mutated by the tests. Due to the order of tests, mutating the span argument in CMT test does not trigger any real problems currently. Mark benchmark_cmd strings as const and setup the benchmark command using pointers. As span is constant in main(), just provide the default span also as string to be used in setting up the default fill_buf argument so no malloc() is required for it. CMT test has to create a copy of the benchmark command before altering the benchmark command. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- tools/testing/selftests/resctrl/cmt_test.c | 23 ++++++++++--- tools/testing/selftests/resctrl/mba_test.c | 2 +- tools/testing/selftests/resctrl/mbm_test.c | 2 +- tools/testing/selftests/resctrl/resctrl.h | 16 ++++++--- .../testing/selftests/resctrl/resctrl_tests.c | 33 ++++++++----------- tools/testing/selftests/resctrl/resctrl_val.c | 10 ++++-- 6 files changed, 54 insertions(+), 32 deletions(-)