Message ID | 20240607055046.138650-1-donettom@linux.ibm.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [v3] selftest: mm: Test if hugepage does not get leaked during __bio_release_pages() | expand |
On 6/7/24 10:50 AM, Donet Tom wrote: > Commit 1b151e2435fc ("block: Remove special-casing of compound > pages") caused a change in behaviour when releasing the pages > if the buffer does not start at the beginning of the page. This > was because the calculation of the number of pages to release > was incorrect. > This was fixed by commit 38b43539d64b ("block: Fix page refcounts > for unaligned buffers in __bio_release_pages()"). > > We pin the user buffer during direct I/O writes. If this buffer is a > hugepage, bio_release_page() will unpin it and decrement all references > and pin counts at ->bi_end_io. However, if any references to the hugepage > remain post-I/O, the hugepage will not be freed upon unmap, leading > to a memory leak. > > This patch verifies that a hugepage, used as a user buffer for DIO > operations, is correctly freed upon unmapping, regardless of whether > the offsets are aligned or unaligned w.r.t page boundary. > > Test Result Fail Scenario (Without the fix) > -------------------------------------------------------- > []# ./hugetlb_dio > TAP version 13 > 1..4 > No. Free pages before allocation : 7 > No. Free pages after munmap : 7 > ok 1 : Huge pages freed successfully ! > No. Free pages before allocation : 7 > No. Free pages after munmap : 7 > ok 2 : Huge pages freed successfully ! > No. Free pages before allocation : 7 > No. Free pages after munmap : 7 > ok 3 : Huge pages freed successfully ! > No. Free pages before allocation : 7 > No. Free pages after munmap : 6 > not ok 4 : Huge pages not freed! > Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0 > > Test Result PASS Scenario (With the fix) > --------------------------------------------------------- > []#./hugetlb_dio > TAP version 13 > 1..4 > No. Free pages before allocation : 7 > No. Free pages after munmap : 7 > ok 1 : Huge pages freed successfully ! > No. Free pages before allocation : 7 > No. Free pages after munmap : 7 > ok 2 : Huge pages freed successfully ! > No. Free pages before allocation : 7 > No. Free pages after munmap : 7 > ok 3 : Huge pages freed successfully ! > No. Free pages before allocation : 7 > No. Free pages after munmap : 7 > ok 4 : Huge pages freed successfully ! > Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0 > > V3: > - Fixed the build error when it is compiled with _FORTIFY_SOURCE. > > V2: > - Addressed all review commets from Muhammad Usama Anjum > https://lore.kernel.org/all/20240604132801.23377-1-donettom@linux.ibm.com/ > > V1: > https://lore.kernel.org/all/20240523063905.3173-1-donettom@linux.ibm.com/#t > > Signed-off-by: Donet Tom <donettom@linux.ibm.com> > Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > --- > tools/testing/selftests/mm/Makefile | 1 + > tools/testing/selftests/mm/hugetlb_dio.c | 118 +++++++++++++++++++++++ Missed my feedback on adding the test to vm_test.sh Other than this, LGTM. Please add following tag after the above change: Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > 2 files changed, 119 insertions(+) > create mode 100644 tools/testing/selftests/mm/hugetlb_dio.c > > diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile > index 3b49bc3d0a3b..a1748a4c7df1 100644 > --- a/tools/testing/selftests/mm/Makefile > +++ b/tools/testing/selftests/mm/Makefile > @@ -73,6 +73,7 @@ TEST_GEN_FILES += ksm_functional_tests > TEST_GEN_FILES += mdwe_test > TEST_GEN_FILES += hugetlb_fault_after_madv > TEST_GEN_FILES += hugetlb_madv_vs_map > +TEST_GEN_FILES += hugetlb_dio > > ifneq ($(ARCH),arm64) > TEST_GEN_FILES += soft-dirty > diff --git a/tools/testing/selftests/mm/hugetlb_dio.c b/tools/testing/selftests/mm/hugetlb_dio.c > new file mode 100644 > index 000000000000..986f3b6c7f7b > --- /dev/null > +++ b/tools/testing/selftests/mm/hugetlb_dio.c > @@ -0,0 +1,118 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * This program tests for hugepage leaks after DIO writes to a file using a > + * hugepage as the user buffer. During DIO, the user buffer is pinned and > + * should be properly unpinned upon completion. This patch verifies that the > + * kernel correctly unpins the buffer at DIO completion for both aligned and > + * unaligned user buffer offsets (w.r.t page boundary), ensuring the hugepage > + * is freed upon unmapping. > + */ > + > +#define _GNU_SOURCE > +#include <stdio.h> > +#include <sys/stat.h> > +#include <stdlib.h> > +#include <fcntl.h> > +#include <stdint.h> > +#include <unistd.h> > +#include <string.h> > +#include <sys/mman.h> > +#include "vm_util.h" > +#include "../kselftest.h" > + > +void run_dio_using_hugetlb(unsigned int start_off, unsigned int end_off) > +{ > + int fd; > + char *buffer = NULL; > + char *orig_buffer = NULL; > + size_t h_pagesize = 0; > + size_t writesize; > + int free_hpage_b = 0; > + int free_hpage_a = 0; > + const int mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB; > + const int mmap_prot = PROT_READ | PROT_WRITE; > + > + writesize = end_off - start_off; > + > + /* Get the default huge page size */ > + h_pagesize = default_huge_page_size(); > + if (!h_pagesize) > + ksft_exit_fail_msg("Unable to determine huge page size\n"); > + > + /* Open the file to DIO */ > + fd = open("/tmp", O_TMPFILE | O_RDWR | O_DIRECT, 0664); > + if (fd < 0) > + ksft_exit_fail_perror("Error opening file\n"); > + > + /* Get the free huge pages before allocation */ > + free_hpage_b = get_free_hugepages(); > + if (free_hpage_b == 0) { > + close(fd); > + ksft_exit_skip("No free hugepage, exiting!\n"); > + } > + > + /* Allocate a hugetlb page */ > + orig_buffer = mmap(NULL, h_pagesize, mmap_prot, mmap_flags, -1, 0); > + if (orig_buffer == MAP_FAILED) { > + close(fd); > + ksft_exit_fail_perror("Error mapping memory\n"); > + } > + buffer = orig_buffer; > + buffer += start_off; > + > + memset(buffer, 'A', writesize); > + > + /* Write the buffer to the file */ > + if (write(fd, buffer, writesize) != (writesize)) { > + munmap(orig_buffer, h_pagesize); > + close(fd); > + ksft_exit_fail_perror("Error writing to file\n"); > + } > + > + /* unmap the huge page */ > + munmap(orig_buffer, h_pagesize); > + close(fd); > + > + /* Get the free huge pages after unmap*/ > + free_hpage_a = get_free_hugepages(); > + > + /* > + * 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"); > + } > +} > + > +int main(void) > +{ > + size_t pagesize = 0; > + > + ksft_print_header(); > + ksft_set_plan(4); > + > + /* Get base page size */ > + pagesize = psize(); > + > + /* start and end is aligned to pagesize */ > + run_dio_using_hugetlb(0, (pagesize * 3)); > + > + /* start is aligned but end is not aligned */ > + run_dio_using_hugetlb(0, (pagesize * 3) - (pagesize / 2)); > + > + /* start is unaligned and end is aligned */ > + run_dio_using_hugetlb(pagesize / 2, (pagesize * 3)); > + > + /* both start and end are unaligned */ > + run_dio_using_hugetlb(pagesize / 2, (pagesize * 3) + (pagesize / 2)); > + > + ksft_finished(); > +} > +
On 6/7/24 11:53, Muhammad Usama Anjum wrote: > On 6/7/24 10:50 AM, Donet Tom wrote: >> Commit 1b151e2435fc ("block: Remove special-casing of compound >> pages") caused a change in behaviour when releasing the pages >> if the buffer does not start at the beginning of the page. This >> was because the calculation of the number of pages to release >> was incorrect. >> This was fixed by commit 38b43539d64b ("block: Fix page refcounts >> for unaligned buffers in __bio_release_pages()"). >> >> We pin the user buffer during direct I/O writes. If this buffer is a >> hugepage, bio_release_page() will unpin it and decrement all references >> and pin counts at ->bi_end_io. However, if any references to the hugepage >> remain post-I/O, the hugepage will not be freed upon unmap, leading >> to a memory leak. >> >> This patch verifies that a hugepage, used as a user buffer for DIO >> operations, is correctly freed upon unmapping, regardless of whether >> the offsets are aligned or unaligned w.r.t page boundary. >> >> Test Result Fail Scenario (Without the fix) >> -------------------------------------------------------- >> []# ./hugetlb_dio >> TAP version 13 >> 1..4 >> No. Free pages before allocation : 7 >> No. Free pages after munmap : 7 >> ok 1 : Huge pages freed successfully ! >> No. Free pages before allocation : 7 >> No. Free pages after munmap : 7 >> ok 2 : Huge pages freed successfully ! >> No. Free pages before allocation : 7 >> No. Free pages after munmap : 7 >> ok 3 : Huge pages freed successfully ! >> No. Free pages before allocation : 7 >> No. Free pages after munmap : 6 >> not ok 4 : Huge pages not freed! >> Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0 >> >> Test Result PASS Scenario (With the fix) >> --------------------------------------------------------- >> []#./hugetlb_dio >> TAP version 13 >> 1..4 >> No. Free pages before allocation : 7 >> No. Free pages after munmap : 7 >> ok 1 : Huge pages freed successfully ! >> No. Free pages before allocation : 7 >> No. Free pages after munmap : 7 >> ok 2 : Huge pages freed successfully ! >> No. Free pages before allocation : 7 >> No. Free pages after munmap : 7 >> ok 3 : Huge pages freed successfully ! >> No. Free pages before allocation : 7 >> No. Free pages after munmap : 7 >> ok 4 : Huge pages freed successfully ! >> Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0 >> >> V3: >> - Fixed the build error when it is compiled with _FORTIFY_SOURCE. >> >> V2: >> - Addressed all review commets from Muhammad Usama Anjum >> https://lore.kernel.org/all/20240604132801.23377-1-donettom@linux.ibm.com/ >> >> V1: >> https://lore.kernel.org/all/20240523063905.3173-1-donettom@linux.ibm.com/#t >> >> Signed-off-by: Donet Tom <donettom@linux.ibm.com> >> Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> >> --- >> tools/testing/selftests/mm/Makefile | 1 + >> tools/testing/selftests/mm/hugetlb_dio.c | 118 +++++++++++++++++++++++ > Missed my feedback on adding the test to vm_test.sh I was not able to find vm_test.sh file to add this test in selftests/mm. could you please help me to get the correct vm_test.sh to add this? Thanks Donet > > Other than this, LGTM. Please add following tag after the above change: > > Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > >> 2 files changed, 119 insertions(+) >> create mode 100644 tools/testing/selftests/mm/hugetlb_dio.c >> >> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile >> index 3b49bc3d0a3b..a1748a4c7df1 100644 >> --- a/tools/testing/selftests/mm/Makefile >> +++ b/tools/testing/selftests/mm/Makefile >> @@ -73,6 +73,7 @@ TEST_GEN_FILES += ksm_functional_tests >> TEST_GEN_FILES += mdwe_test >> TEST_GEN_FILES += hugetlb_fault_after_madv >> TEST_GEN_FILES += hugetlb_madv_vs_map >> +TEST_GEN_FILES += hugetlb_dio >> >> ifneq ($(ARCH),arm64) >> TEST_GEN_FILES += soft-dirty >> diff --git a/tools/testing/selftests/mm/hugetlb_dio.c b/tools/testing/selftests/mm/hugetlb_dio.c >> new file mode 100644 >> index 000000000000..986f3b6c7f7b >> --- /dev/null >> +++ b/tools/testing/selftests/mm/hugetlb_dio.c >> @@ -0,0 +1,118 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * This program tests for hugepage leaks after DIO writes to a file using a >> + * hugepage as the user buffer. During DIO, the user buffer is pinned and >> + * should be properly unpinned upon completion. This patch verifies that the >> + * kernel correctly unpins the buffer at DIO completion for both aligned and >> + * unaligned user buffer offsets (w.r.t page boundary), ensuring the hugepage >> + * is freed upon unmapping. >> + */ >> + >> +#define _GNU_SOURCE >> +#include <stdio.h> >> +#include <sys/stat.h> >> +#include <stdlib.h> >> +#include <fcntl.h> >> +#include <stdint.h> >> +#include <unistd.h> >> +#include <string.h> >> +#include <sys/mman.h> >> +#include "vm_util.h" >> +#include "../kselftest.h" >> + >> +void run_dio_using_hugetlb(unsigned int start_off, unsigned int end_off) >> +{ >> + int fd; >> + char *buffer = NULL; >> + char *orig_buffer = NULL; >> + size_t h_pagesize = 0; >> + size_t writesize; >> + int free_hpage_b = 0; >> + int free_hpage_a = 0; >> + const int mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB; >> + const int mmap_prot = PROT_READ | PROT_WRITE; >> + >> + writesize = end_off - start_off; >> + >> + /* Get the default huge page size */ >> + h_pagesize = default_huge_page_size(); >> + if (!h_pagesize) >> + ksft_exit_fail_msg("Unable to determine huge page size\n"); >> + >> + /* Open the file to DIO */ >> + fd = open("/tmp", O_TMPFILE | O_RDWR | O_DIRECT, 0664); >> + if (fd < 0) >> + ksft_exit_fail_perror("Error opening file\n"); >> + >> + /* Get the free huge pages before allocation */ >> + free_hpage_b = get_free_hugepages(); >> + if (free_hpage_b == 0) { >> + close(fd); >> + ksft_exit_skip("No free hugepage, exiting!\n"); >> + } >> + >> + /* Allocate a hugetlb page */ >> + orig_buffer = mmap(NULL, h_pagesize, mmap_prot, mmap_flags, -1, 0); >> + if (orig_buffer == MAP_FAILED) { >> + close(fd); >> + ksft_exit_fail_perror("Error mapping memory\n"); >> + } >> + buffer = orig_buffer; >> + buffer += start_off; >> + >> + memset(buffer, 'A', writesize); >> + >> + /* Write the buffer to the file */ >> + if (write(fd, buffer, writesize) != (writesize)) { >> + munmap(orig_buffer, h_pagesize); >> + close(fd); >> + ksft_exit_fail_perror("Error writing to file\n"); >> + } >> + >> + /* unmap the huge page */ >> + munmap(orig_buffer, h_pagesize); >> + close(fd); >> + >> + /* Get the free huge pages after unmap*/ >> + free_hpage_a = get_free_hugepages(); >> + >> + /* >> + * 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"); >> + } >> +} >> + >> +int main(void) >> +{ >> + size_t pagesize = 0; >> + >> + ksft_print_header(); >> + ksft_set_plan(4); >> + >> + /* Get base page size */ >> + pagesize = psize(); >> + >> + /* start and end is aligned to pagesize */ >> + run_dio_using_hugetlb(0, (pagesize * 3)); >> + >> + /* start is aligned but end is not aligned */ >> + run_dio_using_hugetlb(0, (pagesize * 3) - (pagesize / 2)); >> + >> + /* start is unaligned and end is aligned */ >> + run_dio_using_hugetlb(pagesize / 2, (pagesize * 3)); >> + >> + /* both start and end are unaligned */ >> + run_dio_using_hugetlb(pagesize / 2, (pagesize * 3) + (pagesize / 2)); >> + >> + ksft_finished(); >> +} >> +
On 6/7/24 9:30 PM, Donet Tom wrote: ... >>> tools/testing/selftests/mm/Makefile | 1 + >>> tools/testing/selftests/mm/hugetlb_dio.c | 118 +++++++++++++++++++++++ >> Missed my feedback on adding the test to vm_test.sh > I was not able to find vm_test.sh file to add this test in selftests/mm. > could you please help me to get the correct vm_test.sh to add this? Here is the file path: tools/testing/selftests/mm/run_vmtests.sh
On 6/7/24 22:02, Muhammad Usama Anjum wrote: > On 6/7/24 9:30 PM, Donet Tom wrote: > ... >>>> tools/testing/selftests/mm/Makefile | 1 + >>>> tools/testing/selftests/mm/hugetlb_dio.c | 118 +++++++++++++++++++++++ >>> Missed my feedback on adding the test to vm_test.sh >> I was not able to find vm_test.sh file to add this test in selftests/mm. >> could you please help me to get the correct vm_test.sh to add this? > Here is the file path: tools/testing/selftests/mm/run_vmtests.sh Thanks I will add this. >
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 3b49bc3d0a3b..a1748a4c7df1 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -73,6 +73,7 @@ TEST_GEN_FILES += ksm_functional_tests TEST_GEN_FILES += mdwe_test TEST_GEN_FILES += hugetlb_fault_after_madv TEST_GEN_FILES += hugetlb_madv_vs_map +TEST_GEN_FILES += hugetlb_dio ifneq ($(ARCH),arm64) TEST_GEN_FILES += soft-dirty diff --git a/tools/testing/selftests/mm/hugetlb_dio.c b/tools/testing/selftests/mm/hugetlb_dio.c new file mode 100644 index 000000000000..986f3b6c7f7b --- /dev/null +++ b/tools/testing/selftests/mm/hugetlb_dio.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * This program tests for hugepage leaks after DIO writes to a file using a + * hugepage as the user buffer. During DIO, the user buffer is pinned and + * should be properly unpinned upon completion. This patch verifies that the + * kernel correctly unpins the buffer at DIO completion for both aligned and + * unaligned user buffer offsets (w.r.t page boundary), ensuring the hugepage + * is freed upon unmapping. + */ + +#define _GNU_SOURCE +#include <stdio.h> +#include <sys/stat.h> +#include <stdlib.h> +#include <fcntl.h> +#include <stdint.h> +#include <unistd.h> +#include <string.h> +#include <sys/mman.h> +#include "vm_util.h" +#include "../kselftest.h" + +void run_dio_using_hugetlb(unsigned int start_off, unsigned int end_off) +{ + int fd; + char *buffer = NULL; + char *orig_buffer = NULL; + size_t h_pagesize = 0; + size_t writesize; + int free_hpage_b = 0; + int free_hpage_a = 0; + const int mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB; + const int mmap_prot = PROT_READ | PROT_WRITE; + + writesize = end_off - start_off; + + /* Get the default huge page size */ + h_pagesize = default_huge_page_size(); + if (!h_pagesize) + ksft_exit_fail_msg("Unable to determine huge page size\n"); + + /* Open the file to DIO */ + fd = open("/tmp", O_TMPFILE | O_RDWR | O_DIRECT, 0664); + if (fd < 0) + ksft_exit_fail_perror("Error opening file\n"); + + /* Get the free huge pages before allocation */ + free_hpage_b = get_free_hugepages(); + if (free_hpage_b == 0) { + close(fd); + ksft_exit_skip("No free hugepage, exiting!\n"); + } + + /* Allocate a hugetlb page */ + orig_buffer = mmap(NULL, h_pagesize, mmap_prot, mmap_flags, -1, 0); + if (orig_buffer == MAP_FAILED) { + close(fd); + ksft_exit_fail_perror("Error mapping memory\n"); + } + buffer = orig_buffer; + buffer += start_off; + + memset(buffer, 'A', writesize); + + /* Write the buffer to the file */ + if (write(fd, buffer, writesize) != (writesize)) { + munmap(orig_buffer, h_pagesize); + close(fd); + ksft_exit_fail_perror("Error writing to file\n"); + } + + /* unmap the huge page */ + munmap(orig_buffer, h_pagesize); + close(fd); + + /* Get the free huge pages after unmap*/ + free_hpage_a = get_free_hugepages(); + + /* + * 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"); + } +} + +int main(void) +{ + size_t pagesize = 0; + + ksft_print_header(); + ksft_set_plan(4); + + /* Get base page size */ + pagesize = psize(); + + /* start and end is aligned to pagesize */ + run_dio_using_hugetlb(0, (pagesize * 3)); + + /* start is aligned but end is not aligned */ + run_dio_using_hugetlb(0, (pagesize * 3) - (pagesize / 2)); + + /* start is unaligned and end is aligned */ + run_dio_using_hugetlb(pagesize / 2, (pagesize * 3)); + + /* both start and end are unaligned */ + run_dio_using_hugetlb(pagesize / 2, (pagesize * 3) + (pagesize / 2)); + + ksft_finished(); +} +