Re: [PATCH liburing v2] test: add test cases for hugepage registered buffers

[Jens Axboe <axboe@kernel.dk>]

On 5/29/24 9:15 PM, Chenliang Li wrote:
> Add a test file for hugepage registered buffers, to make sure the
> fixed buffer coalescing feature works safe and soundly.
> 
> Testcases include read/write with single/multiple/unaligned/non-2MB
> hugepage fixed buffers, and also a should-not coalesce case where
> buffer is a mixture of different size'd pages.

Thanks for improving the test case. Note on the commit message - use
'---' as the separator, not a random number of '-' as it would otherwise
need hand editing after being applied.

This is against a really old base, can you resend it so it applies to
the current tree? Would not be hard to hand apply, but it's a bit
worrying if your tree is that old.

Outside of that, if you get ENOMEM on mmap'ing a huge page because the
system doesn't have huge pages allocated (quite common), the test case
should print an information message and return T_EXIT_SKIP to skip the
test case rather than hard failing.

A few other comments below.

> diff --git a/test/fixed-hugepage.c b/test/fixed-hugepage.c
> new file mode 100644
> index 0000000..a5a0947
> --- /dev/null
> +++ b/test/fixed-hugepage.c
> @@ -0,0 +1,391 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Test fixed buffers consisting of hugepages.
> + */
> +#include <stdio.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <sys/mman.h>
> +#include <linux/mman.h>
> +#include <sys/shm.h>
> +
> +#include "liburing.h"
> +#include "helpers.h"
> +
> +/*
> + * Before testing
> + * echo madvise > /sys/kernel/mm/transparent_hugepage/enabled
> + * echo always > /sys/kernel/mm/transparent_hugepage/hugepages-16kB/enabled
> + *
> + * Not 100% guaranteed to get THP-backed memory, but in general it does.
> + */
> +#define MTHP_16KB	(16UL * 1024)
> +#define HUGEPAGE_SIZE	(2UL * 1024 * 1024)
> +#define NR_BUFS		1
> +#define IN_FD		"/dev/urandom"
> +#define OUT_FD		"/dev/zero"
> +
> +static int open_files(int *fd_in, int *fd_out)
> +{
> +	*fd_in = open(IN_FD, O_RDONLY, 0644);
> +	if (*fd_in < 0) {
> +		perror("open in");
> +		return -1;
> +	}
> +
> +	*fd_out = open(OUT_FD, O_RDWR, 0644);
> +	if (*fd_out < 0) {
> +		perror("open out");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void unmap(struct iovec *iov, int nr_bufs, size_t offset)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_bufs; i++)
> +		munmap(iov[i].iov_base - offset, iov[i].iov_len + offset);
> +
> +	return;
> +}

Don't need a return, just remove that.

> +static int mmap_hugebufs(struct iovec *iov, int nr_bufs, size_t buf_size, size_t offset)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_bufs; i++) {
> +		void *base = NULL;
> +
> +		base = mmap(NULL, buf_size, PROT_READ | PROT_WRITE,
> +				MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB, -1, 0);
> +		if (!base || base == MAP_FAILED) {
> +			fprintf(stderr, "Error in mmapping the %dth buffer: %s\n", i, strerror(errno));
> +			unmap(iov, i, offset);
> +			return -1;
> +		}

You just need to check MAP_FAILED here.

> +/* map a hugepage and smaller page to a contiguous memory */
> +static int mmap_mixture(struct iovec *iov, int nr_bufs, size_t buf_size)
> +{
> +	int i;
> +	void *small_base = NULL, *huge_base = NULL, *start = NULL;
> +	size_t small_size = buf_size - HUGEPAGE_SIZE;
> +	size_t seg_size = ((buf_size / HUGEPAGE_SIZE) + 1) * HUGEPAGE_SIZE;
> +
> +	start = mmap(NULL, seg_size * nr_bufs, PROT_NONE, 

Trailing whitespace here after "PROT_NONE,".

> +static void free_bufs(struct iovec *iov, int nr_bufs, size_t offset)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_bufs; i++)
> +		free(iov[i].iov_base - offset);
> +
> +	return;
> +}

Redundant return again.

> +int main(int argc, char *argv[])
> +{
> +	struct io_uring ring;
> +	int ret, fd_in, fd_out;
> +
> +	if (argc > 1)
> +		return T_EXIT_SKIP;

Since it just takes a generic input file, you could have the test case
use argv[1] as the input file to read from rather than just skip if one
is provided.

-- 
Jens Axboe