Hi Ilpo,
On 10/4/24 7:05 AM, Ilpo Järvinen wrote:
> On Thu, 12 Sep 2024, Reinette Chatre wrote:
>> tools/testing/selftests/resctrl/cmt_test.c | 32 ++----
>> tools/testing/selftests/resctrl/mba_test.c | 13 ++-
>> tools/testing/selftests/resctrl/mbm_test.c | 22 ++--
>> tools/testing/selftests/resctrl/resctrl.h | 54 ++++++---
>> .../testing/selftests/resctrl/resctrl_tests.c | 104 +++++++++++++-----
>> tools/testing/selftests/resctrl/resctrl_val.c | 43 ++++----
>> 6 files changed, 176 insertions(+), 92 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>> index 0c045080d808..a7effe76b419 100644
>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>> @@ -116,15 +116,13 @@ static void cmt_test_cleanup(void)
>>
>> static int cmt_run_test(const struct resctrl_test *test, const struct user_params *uparams)
>> {
>> - const char * const *cmd = uparams->benchmark_cmd;
>> - const char *new_cmd[BENCHMARK_ARGS];
>> + struct fill_buf_param fill_buf = {0};
>
> Empty initializer is enough to have the fields initialized to zero.
Sure.
...
>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
>> index 82801245e4c1..98462752cb46 100644
>> --- a/tools/testing/selftests/resctrl/resctrl.h
>> +++ b/tools/testing/selftests/resctrl/resctrl.h
>> @@ -43,16 +43,35 @@
>>
>> #define DEFAULT_SPAN (250 * MB)
>>
>> +/*
>> + * fill_buf_param: "fill_buf" benchmark parameters
>> + * @buf_size: Size (in bytes) of buffer used in benchmark.
>> + * "fill_buf" allocates and initializes buffer of
>> + * @buf_size. User can change value via command line.
>> + * @memflush: If 0 then the buffer will not be flushed after
>> + * allocation and initialization, otherwise the
>> + * buffer will be flushed. User can change value via
>> + * command line.
>> + */
>> +struct fill_buf_param {
>> + size_t buf_size;
>> + int memflush;
>
> bool?
>
> Or is there a plan to use other values than 0 and 1?
Sure. There is no need to keep the original user provided values. I am not aware of
any other planned values.
>> +static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
>> +{
>> + struct fill_buf_param *fill_param = NULL;
>> + char *endptr = NULL;
>> +
>> + if (!uparams->benchmark_cmd[0] || strcmp(uparams->benchmark_cmd[0], "fill_buf"))
>> + return NULL;
>> +
>> + fill_param = malloc(sizeof(*fill_param));
>> + if (!fill_param)
>> + ksft_exit_skip("Unable to allocate memory for fill_buf parameters.\n");
>> +
>> + if (uparams->benchmark_cmd[1]) {
>> + errno = 0;
>> + fill_param->buf_size = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
>> + if (errno || uparams->benchmark_cmd[1] == endptr) {
>
> Should this also check that there is no extra garbage?
Good point. Instead of adding another check I plan to replace existing endptr check
with a different test that ensures that "*endptr == '\0'".
>> static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
>> {
>> - char **benchmark_cmd;
>> - int ret, memflush;
>> - size_t span;
>> + struct benchmark_info *benchmark_info;
>> + const struct user_params *uparams;
>> + struct resctrl_val_param *param;
>> FILE *fp;
>> + int ret;
>>
>> - benchmark_cmd = info->si_ptr;
>> + benchmark_info = info->si_ptr;
>
> I'd just assign this directly while defining the local variable.
>
Sure.
Thank you.
Reinette
