Hi Ilpo, On 11/3/2023 4:24 AM, Ilpo Järvinen wrote: > On Thu, 2 Nov 2023, Reinette Chatre wrote: >> On 10/24/2023 2:26 AM, Ilpo Järvinen wrote: >> >>> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c >>> index d3bf4368341e..5157a3f74fee 100644 >>> --- a/tools/testing/selftests/resctrl/mba_test.c >>> +++ b/tools/testing/selftests/resctrl/mba_test.c >>> @@ -141,13 +141,13 @@ void mba_test_cleanup(void) >>> remove(RESULT_FILE_NAME); >>> } >>> >>> -int mba_schemata_change(int cpu_no, const char * const *benchmark_cmd) >>> +int mba_schemata_change(const struct user_params *uparams) >>> { >>> struct resctrl_val_param param = { >>> .resctrl_val = MBA_STR, >>> .ctrlgrp = "c1", >>> .mongrp = "m1", >>> - .cpu_no = cpu_no, >>> + .cpu_no = uparams->cpu, >>> .filename = RESULT_FILE_NAME, >>> .bw_report = "reads", >>> .setup = mba_setup >>> @@ -156,7 +156,7 @@ int mba_schemata_change(int cpu_no, const char * const *benchmark_cmd) >>> >>> remove(RESULT_FILE_NAME); >>> >>> - ret = resctrl_val(benchmark_cmd, ¶m); >>> + ret = resctrl_val(uparams->benchmark_cmd, ¶m); >>> if (ret) >>> goto out; >>> >> >> How about a new member of struct resctrl_val_param that points to >> uparams? That would remove cpu_no from resctrl_val_param >> and have everything available when a test needs to run ... not copying >> some user parameters into struct resctrl_val_param and passing >> others as parameters. > > I'm a bit allergic to adding more stuff into resctrl_val_param. It seems > a structure where random stuff has been thrown at just because it exists. > In general, your point is very valid though because the members of > resctrl_val_param should be auditted through to see how many of them are > even useful after adding uparams and struct resctrl_test. > > I could get rid of copying parameters from uparams to params and just > passing uparams instead of benchmark_cmd into resctrl_val(). Would you be > okay with that? I am ok with that. I assume this implies that cpu_no will be removed from resctrl_val_param? > Oh, and I really should rename resctrl_val() one day to something more > meaningful too. :-) (but it won't be part of this series and will likely > be another conflicty nightmare because resctrl_val_param too needs to > be renamed...). "Naming only" changes that are not part of something more substantive are not very appealing though. Reinette