| Message ID | 20240311135230.7007-1-ilpo.jarvinen@linux.intel.com |
|---|---|
| Headers | show |
| Series | selftests/resctrl: resctrl_val() related cleanups & improvements | expand |
Hi Ilpo, On 3/11/2024 6:52 AM, Ilpo Järvinen wrote: > The open() side handles fds in a for loop but close() is based on two > fixed indexes READ and WRITE. > > Match the close() side with the open() side by using for loop for > consistency. I find the close() side to be more appropriate. I say this for two reasons: (a) looking at the close() calls as they are now it is obvious what the close() applies to and transitioning to a loop adds a layer of unnecessary indirection, (b) I do not think a loop is appropriate for the READ/WRITE define that just happen to be 0 and 1 ... there should not be an assumption about their underlying value. Reinette
Hi Ilpo, On 3/11/2024 6:52 AM, Ilpo Järvinen wrote: > The struct resctrl_val_param has control and monitor groups as char > arrays but they are not supposed to be mutated within resctrl_val(). > > Convert the ctrlgrp and mongrp char array within resctrl_val_param to > plain const char pointers and adjust the strlen() based checks to > check NULL instead. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > tools/testing/selftests/resctrl/resctrl.h | 4 ++-- > tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++---- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index 52769b075233..54e5bce4c698 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -89,8 +89,8 @@ struct resctrl_test { > */ > struct resctrl_val_param { > char *resctrl_val; > - char ctrlgrp[64]; > - char mongrp[64]; > + const char *ctrlgrp; > + const char *mongrp; > char filename[64]; > unsigned long mask; > int num_of_runs; > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > index 79cf1c593106..dbe0cc6d74fa 100644 > --- a/tools/testing/selftests/resctrl/resctrlfs.c > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > @@ -469,7 +469,7 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp) > * length of grp_name == 0, it means, user wants to use root con_mon > * grp, so do nothing > */ Could you please confirm that the comments are still accurate? > - if (strlen(grp_name) == 0) > + if (!grp_name) > return 0; > Reinette
On Tue, 19 Mar 2024, Reinette Chatre wrote: > On 3/11/2024 6:52 AM, Ilpo Järvinen wrote: > > The open() side handles fds in a for loop but close() is based on two > > fixed indexes READ and WRITE. > > > > Match the close() side with the open() side by using for loop for > > consistency. > > I find the close() side to be more appropriate. I say this for two > reasons: (a) looking at the close() calls as they are now it is > obvious what the close() applies to and transitioning to a loop > adds a layer of unnecessary indirection, (b) I do not think a loop > is appropriate for the READ/WRITE define that just happen to be 0 > and 1 ... there should not be an assumption about their underlying > value. Hi, So to confirm are you suggesting I should remove all the other loops instead?
On Fri, 22 Mar 2024, Ilpo Järvinen wrote: > On Tue, 19 Mar 2024, Reinette Chatre wrote: > > On 3/11/2024 6:52 AM, Ilpo Järvinen wrote: > > > The open() side handles fds in a for loop but close() is based on two > > > fixed indexes READ and WRITE. > > > > > > Match the close() side with the open() side by using for loop for > > > consistency. > > > > I find the close() side to be more appropriate. I say this for two > > reasons: (a) looking at the close() calls as they are now it is > > obvious what the close() applies to and transitioning to a loop > > adds a layer of unnecessary indirection, (b) I do not think a loop > > is appropriate for the READ/WRITE define that just happen to be 0 > > and 1 ... there should not be an assumption about their underlying > > value. > > Hi, > > So to confirm are you suggesting I should remove all the other loops > instead? Nevermind, I read the comment to second patch, so the answer is yes. :-)
On Wed, 20 Mar 2024, Reinette Chatre wrote: > On 3/11/2024 6:52 AM, Ilpo Järvinen wrote: > > The struct resctrl_val_param has control and monitor groups as char > > arrays but they are not supposed to be mutated within resctrl_val(). > > > > Convert the ctrlgrp and mongrp char array within resctrl_val_param to > > plain const char pointers and adjust the strlen() based checks to > > check NULL instead. > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > tools/testing/selftests/resctrl/resctrl.h | 4 ++-- > > tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++---- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > > index 52769b075233..54e5bce4c698 100644 > > --- a/tools/testing/selftests/resctrl/resctrl.h > > +++ b/tools/testing/selftests/resctrl/resctrl.h > > @@ -89,8 +89,8 @@ struct resctrl_test { > > */ > > struct resctrl_val_param { > > char *resctrl_val; > > - char ctrlgrp[64]; > > - char mongrp[64]; > > + const char *ctrlgrp; > > + const char *mongrp; > > char filename[64]; > > unsigned long mask; > > int num_of_runs; > > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > > index 79cf1c593106..dbe0cc6d74fa 100644 > > --- a/tools/testing/selftests/resctrl/resctrlfs.c > > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > > @@ -469,7 +469,7 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp) > > * length of grp_name == 0, it means, user wants to use root con_mon > > * grp, so do nothing > > */ > > Could you please confirm that the comments are still accurate? It's not, I missed it. > > - if (strlen(grp_name) == 0) > > + if (!grp_name) > > return 0; But now when looking into the surrounding code, to me it looks the correct action here is to remove the comment and return -1 instead of 0. It makes this just an internal sanity check that grp_name is provided by the caller.
Hi Ilpo, On 3/22/2024 5:30 AM, Ilpo Järvinen wrote: > On Wed, 20 Mar 2024, Reinette Chatre wrote: >> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote: >>> The struct resctrl_val_param has control and monitor groups as char >>> arrays but they are not supposed to be mutated within resctrl_val(). >>> >>> Convert the ctrlgrp and mongrp char array within resctrl_val_param to >>> plain const char pointers and adjust the strlen() based checks to >>> check NULL instead. >>> >>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >>> --- >>> tools/testing/selftests/resctrl/resctrl.h | 4 ++-- >>> tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++---- >>> 2 files changed, 6 insertions(+), 6 deletions(-) >>> >>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h >>> index 52769b075233..54e5bce4c698 100644 >>> --- a/tools/testing/selftests/resctrl/resctrl.h >>> +++ b/tools/testing/selftests/resctrl/resctrl.h >>> @@ -89,8 +89,8 @@ struct resctrl_test { >>> */ >>> struct resctrl_val_param { >>> char *resctrl_val; >>> - char ctrlgrp[64]; >>> - char mongrp[64]; >>> + const char *ctrlgrp; >>> + const char *mongrp; >>> char filename[64]; >>> unsigned long mask; >>> int num_of_runs; >>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c >>> index 79cf1c593106..dbe0cc6d74fa 100644 >>> --- a/tools/testing/selftests/resctrl/resctrlfs.c >>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c >>> @@ -469,7 +469,7 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp) >>> * length of grp_name == 0, it means, user wants to use root con_mon >>> * grp, so do nothing >>> */ >> >> Could you please confirm that the comments are still accurate? > > It's not, I missed it. > >>> - if (strlen(grp_name) == 0) >>> + if (!grp_name) >>> return 0; > > But now when looking into the surrounding code, to me it looks the correct > action here is to remove the comment and return -1 instead of 0. It makes > this just an internal sanity check that grp_name is provided by the > caller. > hmmm ... this should not be an error because the caller is not required to provide grp_name. Not providing grp_name has a specific meaning of this operating on the CON_MON group and a failure would break flows operating on the CON_MON group. Reinette
On Fri, 22 Mar 2024, Reinette Chatre wrote: > On 3/22/2024 5:30 AM, Ilpo Järvinen wrote: > > On Wed, 20 Mar 2024, Reinette Chatre wrote: > >> On 3/11/2024 6:52 AM, Ilpo Järvinen wrote: > >>> The struct resctrl_val_param has control and monitor groups as char > >>> arrays but they are not supposed to be mutated within resctrl_val(). > >>> > >>> Convert the ctrlgrp and mongrp char array within resctrl_val_param to > >>> plain const char pointers and adjust the strlen() based checks to > >>> check NULL instead. > >>> > >>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > >>> --- > >>> tools/testing/selftests/resctrl/resctrl.h | 4 ++-- > >>> tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++---- > >>> 2 files changed, 6 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > >>> index 52769b075233..54e5bce4c698 100644 > >>> --- a/tools/testing/selftests/resctrl/resctrl.h > >>> +++ b/tools/testing/selftests/resctrl/resctrl.h > >>> @@ -89,8 +89,8 @@ struct resctrl_test { > >>> */ > >>> struct resctrl_val_param { > >>> char *resctrl_val; > >>> - char ctrlgrp[64]; > >>> - char mongrp[64]; > >>> + const char *ctrlgrp; > >>> + const char *mongrp; > >>> char filename[64]; > >>> unsigned long mask; > >>> int num_of_runs; > >>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > >>> index 79cf1c593106..dbe0cc6d74fa 100644 > >>> --- a/tools/testing/selftests/resctrl/resctrlfs.c > >>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c > >>> @@ -469,7 +469,7 @@ static int create_grp(const char *grp_name, char *grp, const char *parent_grp) > >>> * length of grp_name == 0, it means, user wants to use root con_mon > >>> * grp, so do nothing > >>> */ > >> > >> Could you please confirm that the comments are still accurate? > > > > It's not, I missed it. > > > >>> - if (strlen(grp_name) == 0) > >>> + if (!grp_name) > >>> return 0; > > > > But now when looking into the surrounding code, to me it looks the correct > > action here is to remove the comment and return -1 instead of 0. It makes > > this just an internal sanity check that grp_name is provided by the > > caller. > > > > hmmm ... this should not be an error because the caller is not required > to provide grp_name. Not providing grp_name has a specific meaning > of this operating on the CON_MON group and a failure would break flows > operating on the CON_MON group. write_bm_pid_to_resctrl() checks for non-NULL mongrp before it calls into create_grp() so with current code, I don't think it changes anything. And param->ctrlgrp is always non-NULL too so I don't think the return ever triggers with the current codebase. However, I was more talking from API point of view. It feels more natural for "create group" function to return error if the caller is inconsistent with itself by asking to create a group but doesn't want to create a group.
Hi all, This series does a number of cleanups into resctrl_val() and generalizes it by removing test name specific handling from the function. One of the changes improves MBA/MBM measurement by narrowing down the period the resctrl FS derived memory bandwidth numbers are measured over. My feel is it didn't cause noticeable difference into the numbers because they're generally good anyway except for the small number of outliers. To see the impact on outliers, I'd need to setup a test to run large number of replications and do a statistical analysis, which I've not spent my time on. Even without the statistical analysis, the new way to measure seems obviously better and makes sense even if I cannot see a major improvement with the setup I'm using. This series has some conflicts with SNC series from Maciej and also with the MBA/MBM series from Babu. -- i. v2: - Resolved conflicts with kselftest/next - Spaces -> tabs correction Ilpo Järvinen (13): selftests/resctrl: Convert get_mem_bw_imc() fd close to for loop selftests/resctrl: Calculate resctrl FS derived mem bw over sleep(1) only selftests/resctrl: Consolidate get_domain_id() into resctrl_val() selftests/resctrl: Use correct type for pids selftests/resctrl: Cleanup bm_pid and ppid usage & limit scope selftests/resctrl: Rename measure_vals() to measure_mem_bw_vals() & document selftests/resctrl: Add ->measure() callback to resctrl_val_param selftests/resctrl: Add ->init() callback into resctrl_val_param selftests/resctrl: Simplify bandwidth report type handling selftests/resctrl: Make some strings passed to resctrlfs functions const selftests/resctrl: Convert ctrlgrp & mongrp to pointers selftests/resctrl: Remove mongrp from MBA test selftests/resctrl: Remove test name comparing from write_bm_pid_to_resctrl() tools/testing/selftests/resctrl/cache.c | 6 +- tools/testing/selftests/resctrl/cat_test.c | 5 +- tools/testing/selftests/resctrl/cmt_test.c | 21 +- tools/testing/selftests/resctrl/mba_test.c | 34 ++- tools/testing/selftests/resctrl/mbm_test.c | 33 ++- tools/testing/selftests/resctrl/resctrl.h | 48 ++-- tools/testing/selftests/resctrl/resctrl_val.c | 269 ++++++------------ tools/testing/selftests/resctrl/resctrlfs.c | 55 ++-- 8 files changed, 224 insertions(+), 247 deletions(-)