Re: [PATCH bpf-next v2 08/10] xsk: Support UMEM chunk_size > PAGE_SIZE
From: Kal Cutter Conley <hidden>
Date: 2023-04-04 08:15:48
Also in:
bpf, linux-doc, lkml
Is not the max 64K as you test against XDP_UMEM_MAX_CHUNK_SIZE in xdp_umem_reg()?
The absolute max is 64K. In the case of HPAGE_SIZE < 64K, then it would be HPAGE_SIZE.
quoted
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h index e96a1151ec75..ed88880d4b68 100644 --- a/include/net/xdp_sock.h +++ b/include/net/xdp_sock.h@@ -28,6 +28,9 @@ struct xdp_umem { struct user_struct *user; refcount_t users; u8 flags; +#ifdef CONFIG_HUGETLB_PAGESanity check: have you tried compiling your code without this config set?
Yes. The CI does this also on one of the platforms (hence some of the bot errors in v1).
quoted
static int xdp_umem_pin_pages(struct xdp_umem *umem, unsigned long address) { +#ifdef CONFIG_HUGETLB_PAGELet us try to get rid of most of these #ifdefs sprinkled around the code. How about hiding this inside xdp_umem_is_hugetlb() and get rid of these #ifdefs below? Since I believe it is quite uncommon not to have this config enabled, we could simplify things by always using the page_size in the pool, for example. And dito for the one in struct xdp_umem. What do you think?
I used #ifdef for `page_size` in the pool for maximum performance when huge pages are disabled. We could also not worry about optimizing this uncommon case though since the performance impact is very small. However, I don't find the #ifdefs excessive either.
quoted
+static void xp_check_dma_contiguity(struct xsk_dma_map *dma_map, u32 page_size) { - u32 i; + u32 stride = page_size >> PAGE_SHIFT; /* in order-0 pages */ + u32 i, j; - for (i = 0; i < dma_map->dma_pages_cnt - 1; i++) { - if (dma_map->dma_pages[i] + PAGE_SIZE == dma_map->dma_pages[i + 1]) - dma_map->dma_pages[i] |= XSK_NEXT_PG_CONTIG_MASK; - else - dma_map->dma_pages[i] &= ~XSK_NEXT_PG_CONTIG_MASK; + for (i = 0; i + stride < dma_map->dma_pages_cnt;) { + if (dma_map->dma_pages[i] + page_size == dma_map->dma_pages[i + stride]) { + for (j = 0; j < stride; i++, j++) + dma_map->dma_pages[i] |= XSK_NEXT_PG_CONTIG_MASK; + } else { + for (j = 0; j < stride; i++, j++) + dma_map->dma_pages[i] &= ~XSK_NEXT_PG_CONTIG_MASK; + }Still somewhat too conservative :-). If your page size is large you will waste a lot of the umem. For the last page mark all the 4K "pages" that cannot cross the end of the umem due to the max size of a packet with the XSK_NEXT_PG_CONTIG_MASK bit. So you only need to add one more for-loop here to mark this, and then adjust the last for-loop below so it only marks the last bunch of 4K pages at the end of the umem as not contiguous.
I don't understand the issue. The XSK_NEXT_PG_CONTIG_MASK bit is only looked at if the descriptor actually crosses a page boundary. I don't think the current implementation wastes any UMEM.