Re: [PATCH v1 10/15] io_uring/zcrx: add io_zcrx_area

Jens Axboe
2024-10-09T18:02:06Z

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