Message-ID: <c94ad517-59c6-461c-98dc-1f1706c76389@collabora.com>
Date: Fri, 24 May 2024 11:13:24 -0700
From: Muhammad Usama Anjum <usama.anjum@...labora.com>
To: Donet Tom <donettom@...ux.ibm.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Shuah Khan <shuah@...nel.org>,
 Matthew Wilcox <willy@...radead.org>, Tony Battersby <tonyb@...ernetics.com>
Cc: Muhammad Usama Anjum <usama.anjum@...labora.com>, linux-mm@...ck.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 Ritesh Harjani <ritesh.list@...il.com>, Mike Rapoport <rppt@...nel.org>,
 Muchun Song <songmuchun@...edance.com>, David Hildenbrand
 <david@...hat.com>, stable@...r.kernel.org
Subject: Re: [PATCH] selftest: mm: Test if hugepage does not get leaked during
 __bio_release_pages()

Thank you for submitting a patch.

On 5/22/24 11:39 PM, 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
> 
> Signed-off-by: Donet Tom <donettom@...ux.ibm.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
> ---
>  tools/testing/selftests/mm/Makefile      |   1 +
>  tools/testing/selftests/mm/hugetlb_dio.c | 118 +++++++++++++++++++++++
Add this test to vm_runtest.sh as all the tests are run from this script in
CIs.

>  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 eb5f39a2668b..87d8130b3376 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -71,6 +71,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..6f6587c7913c
> --- /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;
> +
> +	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);
> +	if (fd < 0)
> +		ksft_exit_fail_msg("Error opening file");
Use ksft_exit_fail_perror to print the info about the error
> +
> +	/* 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, PROT_READ | PROT_WRITE, MAP_PRIVATE
> +			| MAP_ANONYMOUS | MAP_HUGETLB, -1, 0);
Better align the arguments. Put all flags in one line instead of slitting
like this

> +	if (orig_buffer == MAP_FAILED) {
> +		close(fd);
> +		ksft_exit_fail_msg("Error mapping memory");
nit: "\n" is missing from here.

> +	}
> +	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_msg("Error writing to file");
> +	}
> +
> +	/* 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) {
> +		printf("No. Free pages before allocation : %d\n", free_hpage_b);
Use ksft_print_msg instead

> +		printf("No. Free pages after munmap : %d\n", free_hpage_a);
> +		ksft_test_result_fail(": Huge pages not freed!\n");
> +	} else {
> +		printf("No. Free pages before allocation : %d\n", free_hpage_b);
> +		printf("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();
ksft_finished() never returns. Remove the following line.

> +	return 0;
> +}
> +

-- 
BR,
Muhammad Usama Anjum

Powered by blists - more mailing lists