| Message ID | 7679d70a0ea939db13ae9dac20de56644460d6df.1718035091.git.babu.moger@amd.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 48236960c06d32370bfa6f2cc408e786873262c8 |
| Headers | show |
| Series | [v2] selftests/resctrl: Fix noncont_cat_run_test for AMD | expand |
On Mon, 10 Jun 2024, Babu Moger wrote: > The selftest noncont_cat_run_test fails on AMD with the warnings. Reason noncont_cat_run_test() (In general, use () when refering to a function, same thing in the shortlog). "the warnings" sounds like I should know about what warning it fails with but there's no previous context which tells that information. I suggest you either use "a warning" or quote the warning it fails with into the commit message. > is, AMD supports non contiguous CBM masks but does not report it via CPUID. non-contiguous > Update noncont_cat_run_test to check for the vendor when verifying CPUID. () > Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT test") > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > v2: Moved the non contiguous verification to a new function > arch_supports_noncont_cat. > > v1: > This was part of the series > https://lore.kernel.org/lkml/cover.1708637563.git.babu.moger@amd.com/ > Sending this as a separate fix per review comments. > --- > tools/testing/selftests/resctrl/cat_test.c | 32 +++++++++++++++------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c > index d4dffc934bc3..742782438ca3 100644 > --- a/tools/testing/selftests/resctrl/cat_test.c > +++ b/tools/testing/selftests/resctrl/cat_test.c > @@ -288,11 +288,30 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param > return ret; > } > > +static bool arch_supports_noncont_cat(const struct resctrl_test *test) > +{ > + unsigned int eax, ebx, ecx, edx; > + > + /* AMD always supports non-contiguous CBM. */ > + if (get_vendor() == ARCH_AMD) > + return true; > + > + /* Intel support for non-contiguous CBM needs to be discovered. */ > + if (!strcmp(test->resource, "L3")) > + __cpuid_count(0x10, 1, eax, ebx, ecx, edx); > + else if (!strcmp(test->resource, "L2")) > + __cpuid_count(0x10, 2, eax, ebx, ecx, edx); > + else > + return false; > + > + return ((ecx >> 3) & 1); > +} > + > static int noncont_cat_run_test(const struct resctrl_test *test, > const struct user_params *uparams) > { > unsigned long full_cache_mask, cont_mask, noncont_mask; > - unsigned int eax, ebx, ecx, edx, sparse_masks; > + unsigned int sparse_masks; > int bit_center, ret; > char schemata[64]; > > @@ -301,15 +320,8 @@ static int noncont_cat_run_test(const struct resctrl_test *test, > if (ret) > return ret; > > - if (!strcmp(test->resource, "L3")) > - __cpuid_count(0x10, 1, eax, ebx, ecx, edx); > - else if (!strcmp(test->resource, "L2")) > - __cpuid_count(0x10, 2, eax, ebx, ecx, edx); > - else > - return -EINVAL; > - > - if (sparse_masks != ((ecx >> 3) & 1)) { > - ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n"); > + if (arch_supports_noncont_cat(test) != sparse_masks) { > + ksft_print_msg("Hardware and kernel differ on non-contiguous CBM support!\n"); > return 1; This looks better than the previous version, thanks.
Hi On 6/10/24 11:20, Ilpo Järvinen wrote: > On Mon, 10 Jun 2024, Babu Moger wrote: > >> The selftest noncont_cat_run_test fails on AMD with the warnings. Reason > > noncont_cat_run_test() I want to mention the test here. not function. How about this? "The selftest non-contiguous CBM test fails on AMD with the warnings." > > (In general, use () when refering to a function, same thing in the > shortlog). > > "the warnings" sounds like I should know about what warning it fails with > but there's no previous context which tells that information. I suggest > you either use "a warning" or quote the warning it fails with into the > commit message. > >> is, AMD supports non contiguous CBM masks but does not report it via CPUID. > > non-contiguous Sure. > >> Update noncont_cat_run_test to check for the vendor when verifying CPUID. > > () Sure. > >> Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT test") >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- >> v2: Moved the non contiguous verification to a new function >> arch_supports_noncont_cat. >> >> v1: >> This was part of the series >> https://lore.kernel.org/lkml/cover.1708637563.git.babu.moger@amd.com/ >> Sending this as a separate fix per review comments. >> --- >> tools/testing/selftests/resctrl/cat_test.c | 32 +++++++++++++++------- >> 1 file changed, 22 insertions(+), 10 deletions(-) >> >> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c >> index d4dffc934bc3..742782438ca3 100644 >> --- a/tools/testing/selftests/resctrl/cat_test.c >> +++ b/tools/testing/selftests/resctrl/cat_test.c >> @@ -288,11 +288,30 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param >> return ret; >> } >> >> +static bool arch_supports_noncont_cat(const struct resctrl_test *test) >> +{ >> + unsigned int eax, ebx, ecx, edx; >> + >> + /* AMD always supports non-contiguous CBM. */ >> + if (get_vendor() == ARCH_AMD) >> + return true; >> + >> + /* Intel support for non-contiguous CBM needs to be discovered. */ >> + if (!strcmp(test->resource, "L3")) >> + __cpuid_count(0x10, 1, eax, ebx, ecx, edx); >> + else if (!strcmp(test->resource, "L2")) >> + __cpuid_count(0x10, 2, eax, ebx, ecx, edx); >> + else >> + return false; >> + >> + return ((ecx >> 3) & 1); >> +} >> + >> static int noncont_cat_run_test(const struct resctrl_test *test, >> const struct user_params *uparams) >> { >> unsigned long full_cache_mask, cont_mask, noncont_mask; >> - unsigned int eax, ebx, ecx, edx, sparse_masks; >> + unsigned int sparse_masks; >> int bit_center, ret; >> char schemata[64]; >> >> @@ -301,15 +320,8 @@ static int noncont_cat_run_test(const struct resctrl_test *test, >> if (ret) >> return ret; >> >> - if (!strcmp(test->resource, "L3")) >> - __cpuid_count(0x10, 1, eax, ebx, ecx, edx); >> - else if (!strcmp(test->resource, "L2")) >> - __cpuid_count(0x10, 2, eax, ebx, ecx, edx); >> - else >> - return -EINVAL; >> - >> - if (sparse_masks != ((ecx >> 3) & 1)) { >> - ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n"); >> + if (arch_supports_noncont_cat(test) != sparse_masks) { >> + ksft_print_msg("Hardware and kernel differ on non-contiguous CBM support!\n"); >> return 1; > > This looks better than the previous version, thanks. Thanks. Babu Moger
Hi Babu, On 6/10/24 10:51 AM, Moger, Babu wrote: > Hi > > On 6/10/24 11:20, Ilpo Järvinen wrote: >> On Mon, 10 Jun 2024, Babu Moger wrote: >> >>> The selftest noncont_cat_run_test fails on AMD with the warnings. Reason >> >> noncont_cat_run_test() > > I want to mention the test here. not function. How about this? > > "The selftest non-contiguous CBM test fails on AMD with the warnings." > This does not take Ilpo's feedback regarding "the warnings" into account. I agree with Ilpo (keeping his comment below) that "with the warnings" sounds incomplete. Also, it is not necessary to mention both "selftest" and "test" in "The selftest non-contiguous CBM test". You can change it to something like: The non-contiguous CBM test fails on AMD with: /* insert error message here */ AMD always supports non-contiguous CBM but does not report it via CPUID. Fix the non-contiguous CBM test to use CPUID to discover non-contiguous CBM support only on Intel. >> >> (In general, use () when refering to a function, same thing in the >> shortlog). >> >> "the warnings" sounds like I should know about what warning it fails with >> but there's no previous context which tells that information. I suggest >> you either use "a warning" or quote the warning it fails with into the >> commit message. >> >>> is, AMD supports non contiguous CBM masks but does not report it via CPUID. The "M" in CBM is for "mask" so there is no need to write "mask" after CBM. Reinette
Hi Babu, (please do not send new version of patch in response to previous version) On 6/10/24 9:00 AM, Babu Moger wrote: > The selftest noncont_cat_run_test fails on AMD with the warnings. Reason > is, AMD supports non contiguous CBM masks but does not report it via CPUID. > > Update noncont_cat_run_test to check for the vendor when verifying CPUID. > > Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT test") > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- The changelog needs to be reworked based on discussion in other thread. With that complete, for this patch you can add: | Reviewed-by: Reinette Chatre <reinette.chatre@intel.com> Reinette
Hi Reinette, On 6/10/2024 4:32 PM, Reinette Chatre wrote: > Hi Babu, > > (please do not send new version of patch in response to previous version) Yes. I saw your comments on other thread. I will take care of it. > > On 6/10/24 9:00 AM, Babu Moger wrote: >> The selftest noncont_cat_run_test fails on AMD with the warnings. Reason >> is, AMD supports non contiguous CBM masks but does not report it via >> CPUID. >> >> Update noncont_cat_run_test to check for the vendor when verifying CPUID. >> >> Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT >> test") >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- > > The changelog needs to be reworked based on discussion in other thread. Sure. > With that complete, for this patch you can add: > | Reviewed-by: Reinette Chatre <reinette.chatre@intel.com> Thanks
On Mon, 10 Jun 2024, Moger, Babu wrote: > On 6/10/24 11:20, Ilpo Järvinen wrote: > > On Mon, 10 Jun 2024, Babu Moger wrote: > > > >> The selftest noncont_cat_run_test fails on AMD with the warnings. Reason > > > > noncont_cat_run_test() > > I want to mention the test here. not function. How about this? > > "The selftest non-contiguous CBM test fails on AMD with the warnings." Yes, it's possible to refer to something with natural language (in fact, I personally find that better than using a function name when both options exist). The underscores are C artifacts to replace spaces that do not belong to natural language so if one uses underscores, I'll always take it as a direct reference to C code. The quote still has "the warnings" though (but I see Reinette already noted that). > > (In general, use () when refering to a function, same thing in the > > shortlog). > > > > "the warnings" sounds like I should know about what warning it fails with > > but there's no previous context which tells that information. I suggest > > you either use "a warning" or quote the warning it fails with into the > > commit message. > > > >> is, AMD supports non contiguous CBM masks but does not report it via CPUID.
On 6/10/24 4:11 PM, Moger, Babu wrote: > Hi Reinette, > > On 6/10/2024 4:32 PM, Reinette Chatre wrote: >> Hi Babu, >> >> (please do not send new version of patch in response to previous version) > > Yes. I saw your comments on other thread. I will take care of it. > ... and yet you proceed to send v3 in response to previous version again.
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index d4dffc934bc3..742782438ca3 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -288,11 +288,30 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param return ret; } +static bool arch_supports_noncont_cat(const struct resctrl_test *test) +{ + unsigned int eax, ebx, ecx, edx; + + /* AMD always supports non-contiguous CBM. */ + if (get_vendor() == ARCH_AMD) + return true; + + /* Intel support for non-contiguous CBM needs to be discovered. */ + if (!strcmp(test->resource, "L3")) + __cpuid_count(0x10, 1, eax, ebx, ecx, edx); + else if (!strcmp(test->resource, "L2")) + __cpuid_count(0x10, 2, eax, ebx, ecx, edx); + else + return false; + + return ((ecx >> 3) & 1); +} + static int noncont_cat_run_test(const struct resctrl_test *test, const struct user_params *uparams) { unsigned long full_cache_mask, cont_mask, noncont_mask; - unsigned int eax, ebx, ecx, edx, sparse_masks; + unsigned int sparse_masks; int bit_center, ret; char schemata[64]; @@ -301,15 +320,8 @@ static int noncont_cat_run_test(const struct resctrl_test *test, if (ret) return ret; - if (!strcmp(test->resource, "L3")) - __cpuid_count(0x10, 1, eax, ebx, ecx, edx); - else if (!strcmp(test->resource, "L2")) - __cpuid_count(0x10, 2, eax, ebx, ecx, edx); - else - return -EINVAL; - - if (sparse_masks != ((ecx >> 3) & 1)) { - ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n"); + if (arch_supports_noncont_cat(test) != sparse_masks) { + ksft_print_msg("Hardware and kernel differ on non-contiguous CBM support!\n"); return 1; }
The selftest noncont_cat_run_test fails on AMD with the warnings. Reason is, AMD supports non contiguous CBM masks but does not report it via CPUID. Update noncont_cat_run_test to check for the vendor when verifying CPUID. Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT test") Signed-off-by: Babu Moger <babu.moger@amd.com> --- v2: Moved the non contiguous verification to a new function arch_supports_noncont_cat. v1: This was part of the series https://lore.kernel.org/lkml/cover.1708637563.git.babu.moger@amd.com/ Sending this as a separate fix per review comments. --- tools/testing/selftests/resctrl/cat_test.c | 32 +++++++++++++++------- 1 file changed, 22 insertions(+), 10 deletions(-)