| Message ID | 20241007221603.1703699-11-dw@davidwei.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | io_uring zero copy rx | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On 10/7/24 4:15 PM, David Wei wrote: > +static int io_zcrx_create_area(struct io_ring_ctx *ctx, > + struct io_zcrx_ifq *ifq, > + struct io_zcrx_area **res, > + struct io_uring_zcrx_area_reg *area_reg) > +{ > + struct io_zcrx_area *area; > + int i, ret, nr_pages; > + struct iovec iov; > + > + if (area_reg->flags || area_reg->rq_area_token) > + return -EINVAL; > + if (area_reg->__resv1 || area_reg->__resv2[0] || area_reg->__resv2[1]) > + return -EINVAL; > + if (area_reg->addr & ~PAGE_MASK || area_reg->len & ~PAGE_MASK) > + return -EINVAL; > + > + iov.iov_base = u64_to_user_ptr(area_reg->addr); > + iov.iov_len = area_reg->len; > + ret = io_buffer_validate(&iov); > + if (ret) > + return ret; > + > + ret = -ENOMEM; > + area = kzalloc(sizeof(*area), GFP_KERNEL); > + if (!area) > + goto err; This should probably just be a: area = kzalloc(sizeof(*area), GFP_KERNEL); if (!area) return -ENOMEM; Minor it... > diff --git a/io_uring/zcrx.h b/io_uring/zcrx.h > index 4ef94e19d36b..2fcbeb3d5501 100644 > --- a/io_uring/zcrx.h > +++ b/io_uring/zcrx.h > @@ -3,10 +3,26 @@ > #define IOU_ZC_RX_H > > #include <linux/io_uring_types.h> > +#include <net/page_pool/types.h> > + > +struct io_zcrx_area { > + struct net_iov_area nia; > + struct io_zcrx_ifq *ifq; > + > + u16 area_id; > + struct page **pages; > + > + /* freelist */ > + spinlock_t freelist_lock ____cacheline_aligned_in_smp; > + u32 free_count; > + u32 *freelist; > +}; I'm wondering if this really needs an aligned lock? Since it's only a single structure, probably not a big deal. But unless there's evidence to the contrary, might not be a bad idea to just kill that. Apart from that, looks fine to me.
On 10/9/24 19:02, Jens Axboe wrote: > On 10/7/24 4:15 PM, David Wei wrote: ... >> +struct io_zcrx_area { >> + struct net_iov_area nia; >> + struct io_zcrx_ifq *ifq; >> + >> + u16 area_id; >> + struct page **pages; >> + >> + /* freelist */ >> + spinlock_t freelist_lock ____cacheline_aligned_in_smp; >> + u32 free_count; >> + u32 *freelist; >> +}; > > I'm wondering if this really needs an aligned lock? Since it's only a > single structure, probably not a big deal. But unless there's evidence > to the contrary, might not be a bad idea to just kill that. napi and IORING_OP_RECV_ZC can run on different CPUs, I wouldn't want the fields before the lock being contended by the lock because of cache line sharing, would especially hurt until it's warmed up well. Not really profiled, but not like we need to care about space here.
On 10/9/24 1:05 PM, Pavel Begunkov wrote: > On 10/9/24 19:02, Jens Axboe wrote: >> On 10/7/24 4:15 PM, David Wei wrote: > ... >>> +struct io_zcrx_area { >>> + struct net_iov_area nia; >>> + struct io_zcrx_ifq *ifq; >>> + >>> + u16 area_id; >>> + struct page **pages; >>> + >>> + /* freelist */ >>> + spinlock_t freelist_lock ____cacheline_aligned_in_smp; >>> + u32 free_count; >>> + u32 *freelist; >>> +}; >> >> I'm wondering if this really needs an aligned lock? Since it's only a >> single structure, probably not a big deal. But unless there's evidence >> to the contrary, might not be a bad idea to just kill that. > > napi and IORING_OP_RECV_ZC can run on different CPUs, I wouldn't > want the fields before the lock being contended by the lock > because of cache line sharing, would especially hurt until it's > warmed up well. Not really profiled, but not like we need to > care about space here. Right, as mentioned it's just a single struct, so doesn't matter that much. I guess my testing all ran with same cpu for napi + rx, so would not have seen it regardless. We can keep it as-is.
On Mon, Oct 7, 2024 at 3:16 PM David Wei <dw@davidwei.uk> wrote: > > From: David Wei <davidhwei@meta.com> > > Add io_zcrx_area that represents a region of userspace memory that is > used for zero copy. During ifq registration, userspace passes in the > uaddr and len of userspace memory, which is then pinned by the kernel. > Each net_iov is mapped to one of these pages. > > The freelist is a spinlock protected list that keeps track of all the > net_iovs/pages that aren't used. > > For now, there is only one area per ifq and area registration happens > implicitly as part of ifq registration. There is no API for > adding/removing areas yet. The struct for area registration is there for > future extensibility once we support multiple areas and TCP devmem. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > Signed-off-by: David Wei <dw@davidwei.uk> This patch, and the later patch to add the io_uring memory provider are what I was referring to in the other thread as changes I would like to reuse for a socket extension for this. In my mind it would be nice to decouple the memory being bound to the page_pool from io_uring, so I can bind a malloced/pinned block of memory to the rx queue and use it with regular sockets. Seems a lot of this patch and the provider can be reused. The biggest issue AFAICT I see is that there are io_uring specific calls to set up the region like io_buffer_validate/io_pin_pages, but these in turn seem to call generic mm helpers and from a quick look I don't see much iouring specific. Seems fine to leave this to a future extension. -- Thanks, Mina
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 567cdb89711e..ffd315d8c6b5 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -831,6 +831,15 @@ struct io_uring_zcrx_offsets { __u64 __resv[2]; }; +struct io_uring_zcrx_area_reg { + __u64 addr; + __u64 len; + __u64 rq_area_token; + __u32 flags; + __u32 __resv1; + __u64 __resv2[2]; +}; + /* * Argument for IORING_REGISTER_ZCRX_IFQ */ diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c index 453867add7ca..42606404019e 100644 --- a/io_uring/rsrc.c +++ b/io_uring/rsrc.c @@ -85,7 +85,7 @@ static int io_account_mem(struct io_ring_ctx *ctx, unsigned long nr_pages) return 0; } -static int io_buffer_validate(struct iovec *iov) +int io_buffer_validate(struct iovec *iov) { unsigned long tmp, acct_len = iov->iov_len + (PAGE_SIZE - 1); diff --git a/io_uring/rsrc.h b/io_uring/rsrc.h index c032ca3436ca..e691e8ed849b 100644 --- a/io_uring/rsrc.h +++ b/io_uring/rsrc.h @@ -74,6 +74,7 @@ int io_register_rsrc_update(struct io_ring_ctx *ctx, void __user *arg, unsigned size, unsigned type); int io_register_rsrc(struct io_ring_ctx *ctx, void __user *arg, unsigned int size, unsigned int type); +int io_buffer_validate(struct iovec *iov); static inline void io_put_rsrc_node(struct io_ring_ctx *ctx, struct io_rsrc_node *node) { diff --git a/io_uring/zcrx.c b/io_uring/zcrx.c index 79d79b9b8df8..8382129402ac 100644 --- a/io_uring/zcrx.c +++ b/io_uring/zcrx.c @@ -10,6 +10,7 @@ #include "kbuf.h" #include "memmap.h" #include "zcrx.h" +#include "rsrc.h" #define IO_RQ_MAX_ENTRIES 32768 @@ -40,6 +41,83 @@ static void io_free_rbuf_ring(struct io_zcrx_ifq *ifq) ifq->rqes = NULL; } +static void io_zcrx_free_area(struct io_zcrx_area *area) +{ + if (area->freelist) + kvfree(area->freelist); + if (area->nia.niovs) + kvfree(area->nia.niovs); + if (area->pages) { + unpin_user_pages(area->pages, area->nia.num_niovs); + kvfree(area->pages); + } + kfree(area); +} + +static int io_zcrx_create_area(struct io_ring_ctx *ctx, + struct io_zcrx_ifq *ifq, + struct io_zcrx_area **res, + struct io_uring_zcrx_area_reg *area_reg) +{ + struct io_zcrx_area *area; + int i, ret, nr_pages; + struct iovec iov; + + if (area_reg->flags || area_reg->rq_area_token) + return -EINVAL; + if (area_reg->__resv1 || area_reg->__resv2[0] || area_reg->__resv2[1]) + return -EINVAL; + if (area_reg->addr & ~PAGE_MASK || area_reg->len & ~PAGE_MASK) + return -EINVAL; + + iov.iov_base = u64_to_user_ptr(area_reg->addr); + iov.iov_len = area_reg->len; + ret = io_buffer_validate(&iov); + if (ret) + return ret; + + ret = -ENOMEM; + area = kzalloc(sizeof(*area), GFP_KERNEL); + if (!area) + goto err; + + area->pages = io_pin_pages((unsigned long)area_reg->addr, area_reg->len, + &nr_pages); + if (IS_ERR(area->pages)) { + ret = PTR_ERR(area->pages); + area->pages = NULL; + goto err; + } + area->nia.num_niovs = nr_pages; + + area->nia.niovs = kvmalloc_array(nr_pages, sizeof(area->nia.niovs[0]), + GFP_KERNEL | __GFP_ZERO); + if (!area->nia.niovs) + goto err; + + area->freelist = kvmalloc_array(nr_pages, sizeof(area->freelist[0]), + GFP_KERNEL | __GFP_ZERO); + if (!area->freelist) + goto err; + + for (i = 0; i < nr_pages; i++) { + area->freelist[i] = i; + } + + area->free_count = nr_pages; + area->ifq = ifq; + /* we're only supporting one area per ifq for now */ + area->area_id = 0; + area_reg->rq_area_token = (u64)area->area_id << IORING_ZCRX_AREA_SHIFT; + spin_lock_init(&area->freelist_lock); + *res = area; + return 0; +err: + if (area) + io_zcrx_free_area(area); + return ret; +} + static struct io_zcrx_ifq *io_zcrx_ifq_alloc(struct io_ring_ctx *ctx) { struct io_zcrx_ifq *ifq; @@ -55,6 +133,9 @@ static struct io_zcrx_ifq *io_zcrx_ifq_alloc(struct io_ring_ctx *ctx) static void io_zcrx_ifq_free(struct io_zcrx_ifq *ifq) { + if (ifq->area) + io_zcrx_free_area(ifq->area); + io_free_rbuf_ring(ifq); kfree(ifq); } @@ -62,6 +143,7 @@ static void io_zcrx_ifq_free(struct io_zcrx_ifq *ifq) int io_register_zcrx_ifq(struct io_ring_ctx *ctx, struct io_uring_zcrx_ifq_reg __user *arg) { + struct io_uring_zcrx_area_reg area; struct io_uring_zcrx_ifq_reg reg; struct io_zcrx_ifq *ifq; size_t ring_sz, rqes_sz; @@ -93,7 +175,7 @@ int io_register_zcrx_ifq(struct io_ring_ctx *ctx, } reg.rq_entries = roundup_pow_of_two(reg.rq_entries); - if (!reg.area_ptr) + if (copy_from_user(&area, u64_to_user_ptr(reg.area_ptr), sizeof(area))) return -EFAULT; ifq = io_zcrx_ifq_alloc(ctx); @@ -104,6 +186,10 @@ int io_register_zcrx_ifq(struct io_ring_ctx *ctx, if (ret) goto err; + ret = io_zcrx_create_area(ctx, ifq, &ifq->area, &area); + if (ret) + goto err; + ifq->rq_entries = reg.rq_entries; ifq->if_rxq = reg.if_rxq; @@ -118,7 +204,10 @@ int io_register_zcrx_ifq(struct io_ring_ctx *ctx, ret = -EFAULT; goto err; } - + if (copy_to_user(u64_to_user_ptr(reg.area_ptr), &area, sizeof(area))) { + ret = -EFAULT; + goto err; + } ctx->ifq = ifq; return 0; err: diff --git a/io_uring/zcrx.h b/io_uring/zcrx.h index 4ef94e19d36b..2fcbeb3d5501 100644 --- a/io_uring/zcrx.h +++ b/io_uring/zcrx.h @@ -3,10 +3,26 @@ #define IOU_ZC_RX_H #include <linux/io_uring_types.h> +#include <net/page_pool/types.h> + +struct io_zcrx_area { + struct net_iov_area nia; + struct io_zcrx_ifq *ifq; + + u16 area_id; + struct page **pages; + + /* freelist */ + spinlock_t freelist_lock ____cacheline_aligned_in_smp; + u32 free_count; + u32 *freelist; +}; struct io_zcrx_ifq { struct io_ring_ctx *ctx; struct net_device *dev; + struct io_zcrx_area *area; + struct io_uring *rq_ring; struct io_uring_zcrx_rqe *rqes; u32 rq_entries;