| Message ID | 20241127-kselftest-mm-hugetlb-dio-names-v1-1-22aab01bf550@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftest: hugetlb_dio: Fix test naming | expand |
On 11/27/24 21:44, Mark Brown wrote: > The string logged when a test passes or fails is used by the selftest > framework to identify which test is being reported. The hugetlb_dio test > not only uses the same strings for every test that is run but it also uses > different strings for test passes and failures which means that test > automation is unable to follow what the test is doing at all. > > Pull the existing duplicated logging of the number of free huge pages > before and after the test out of the conditional and replace that and the > logging of the result with a single ksft_print_result() which incorporates > the parameters passed into the test into the output. > > Fixes: fae1980347bf ("selftests: hugetlb_dio: fixup check for initial conditions to skip in the start") > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > tools/testing/selftests/mm/hugetlb_dio.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/mm/hugetlb_dio.c b/tools/testing/selftests/mm/hugetlb_dio.c > index 432d5af15e66b7d6cac0273fb244d6696d7c9ddc..db63abe5ee5e85ff7795d3ea176c3ac47184bf4f 100644 > --- a/tools/testing/selftests/mm/hugetlb_dio.c > +++ b/tools/testing/selftests/mm/hugetlb_dio.c > @@ -76,19 +76,15 @@ void run_dio_using_hugetlb(unsigned int start_off, unsigned int end_off) > /* Get the free huge pages after unmap*/ > free_hpage_a = get_free_hugepages(); > > + ksft_print_msg("No. Free pages before allocation : %d\n", free_hpage_b); > + ksft_print_msg("No. Free pages after munmap : %d\n", free_hpage_a); > + > /* > * If the no. of free hugepages before allocation and after unmap does > * not match - that means there could still be a page which is pinned. > */ > - if (free_hpage_a != free_hpage_b) { > - ksft_print_msg("No. Free pages before allocation : %d\n", free_hpage_b); > - ksft_print_msg("No. Free pages after munmap : %d\n", free_hpage_a); > - ksft_test_result_fail(": Huge pages not freed!\n"); > - } else { > - ksft_print_msg("No. Free pages before allocation : %d\n", free_hpage_b); > - ksft_print_msg("No. Free pages after munmap : %d\n", free_hpage_a); > - ksft_test_result_pass(": Huge pages freed successfully !\n"); > - } > + ksft_test_result(free_hpage_a == free_hpage_b, > + "free huge pages from %u-%u\n", start_off, end_off); Hi Mark This test allocates a hugetlb buffer and adjusts the start and end offsets of the buffer based on|start_off| and|end_off|. The adjusted buffer is then used for Direct I/O (DIO). If I understand correctly,|start_off| and|end_off| are not free huge pages but rather DIO buffer offsets. Should we change this message to "Hugetlb DIO buffer offset"? Thanks Donet > } > > int main(void) > > --- > base-commit: 6f3d2b5299b0a8bcb8a9405a8d3fceb24f79c4f0 > change-id: 20241127-kselftest-mm-hugetlb-dio-names-1ebccbe8183d > > Best regards,
On 11/27/24 9:14 PM, Mark Brown wrote: > The string logged when a test passes or fails is used by the selftest > framework to identify which test is being reported. The hugetlb_dio test > not only uses the same strings for every test that is run but it also uses > different strings for test passes and failures which means that test > automation is unable to follow what the test is doing at all. > > Pull the existing duplicated logging of the number of free huge pages > before and after the test out of the conditional and replace that and the > logging of the result with a single ksft_print_result() which incorporates > the parameters passed into the test into the output. > > Fixes: fae1980347bf ("selftests: hugetlb_dio: fixup check for initial conditions to skip in the start") > Signed-off-by: Mark Brown <broonie@kernel.org> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > --- > tools/testing/selftests/mm/hugetlb_dio.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/mm/hugetlb_dio.c b/tools/testing/selftests/mm/hugetlb_dio.c > index 432d5af15e66b7d6cac0273fb244d6696d7c9ddc..db63abe5ee5e85ff7795d3ea176c3ac47184bf4f 100644 > --- a/tools/testing/selftests/mm/hugetlb_dio.c > +++ b/tools/testing/selftests/mm/hugetlb_dio.c > @@ -76,19 +76,15 @@ void run_dio_using_hugetlb(unsigned int start_off, unsigned int end_off) > /* Get the free huge pages after unmap*/ > free_hpage_a = get_free_hugepages(); > > + ksft_print_msg("No. Free pages before allocation : %d\n", free_hpage_b); > + ksft_print_msg("No. Free pages after munmap : %d\n", free_hpage_a); > + > /* > * If the no. of free hugepages before allocation and after unmap does > * not match - that means there could still be a page which is pinned. > */ > - if (free_hpage_a != free_hpage_b) { > - ksft_print_msg("No. Free pages before allocation : %d\n", free_hpage_b); > - ksft_print_msg("No. Free pages after munmap : %d\n", free_hpage_a); > - ksft_test_result_fail(": Huge pages not freed!\n"); > - } else { > - ksft_print_msg("No. Free pages before allocation : %d\n", free_hpage_b); > - ksft_print_msg("No. Free pages after munmap : %d\n", free_hpage_a); > - ksft_test_result_pass(": Huge pages freed successfully !\n"); > - } > + ksft_test_result(free_hpage_a == free_hpage_b, > + "free huge pages from %u-%u\n", start_off, end_off); > } > > int main(void) > > --- > base-commit: 6f3d2b5299b0a8bcb8a9405a8d3fceb24f79c4f0 > change-id: 20241127-kselftest-mm-hugetlb-dio-names-1ebccbe8183d > > Best regards,
On Thu, Nov 28, 2024 at 10:46:56AM +0530, Donet Tom wrote: > On 11/27/24 21:44, Mark Brown wrote: > > + ksft_test_result(free_hpage_a == free_hpage_b, > > + "free huge pages from %u-%u\n", start_off, end_off); > This test allocates a hugetlb buffer and adjusts the start and end offsets of the buffer based > on|start_off| and|end_off|. The adjusted buffer is then used for Direct I/O (DIO). If I understand > correctly,|start_off| and|end_off| are not free huge pages but rather DIO buffer offsets. Should we > change this message to "Hugetlb DIO buffer offset"? Sure, so long as the message is consistent it doesn't really matter what it is from the point of view of the tooling. I also noticed while doing this that the test doesn't verify that a huge page is actually used at any point, I was thinking about doing an incremental change for that too. Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.
diff --git a/tools/testing/selftests/mm/hugetlb_dio.c b/tools/testing/selftests/mm/hugetlb_dio.c index 432d5af15e66b7d6cac0273fb244d6696d7c9ddc..db63abe5ee5e85ff7795d3ea176c3ac47184bf4f 100644 --- a/tools/testing/selftests/mm/hugetlb_dio.c +++ b/tools/testing/selftests/mm/hugetlb_dio.c @@ -76,19 +76,15 @@ void run_dio_using_hugetlb(unsigned int start_off, unsigned int end_off) /* Get the free huge pages after unmap*/ free_hpage_a = get_free_hugepages(); + ksft_print_msg("No. Free pages before allocation : %d\n", free_hpage_b); + ksft_print_msg("No. Free pages after munmap : %d\n", free_hpage_a); + /* * If the no. of free hugepages before allocation and after unmap does * not match - that means there could still be a page which is pinned. */ - if (free_hpage_a != free_hpage_b) { - ksft_print_msg("No. Free pages before allocation : %d\n", free_hpage_b); - ksft_print_msg("No. Free pages after munmap : %d\n", free_hpage_a); - ksft_test_result_fail(": Huge pages not freed!\n"); - } else { - ksft_print_msg("No. Free pages before allocation : %d\n", free_hpage_b); - ksft_print_msg("No. Free pages after munmap : %d\n", free_hpage_a); - ksft_test_result_pass(": Huge pages freed successfully !\n"); - } + ksft_test_result(free_hpage_a == free_hpage_b, + "free huge pages from %u-%u\n", start_off, end_off); } int main(void)
The string logged when a test passes or fails is used by the selftest framework to identify which test is being reported. The hugetlb_dio test not only uses the same strings for every test that is run but it also uses different strings for test passes and failures which means that test automation is unable to follow what the test is doing at all. Pull the existing duplicated logging of the number of free huge pages before and after the test out of the conditional and replace that and the logging of the result with a single ksft_print_result() which incorporates the parameters passed into the test into the output. Fixes: fae1980347bf ("selftests: hugetlb_dio: fixup check for initial conditions to skip in the start") Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/mm/hugetlb_dio.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) --- base-commit: 6f3d2b5299b0a8bcb8a9405a8d3fceb24f79c4f0 change-id: 20241127-kselftest-mm-hugetlb-dio-names-1ebccbe8183d Best regards,