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
