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. -- Jens Axboe