Re: [RFC PATCH v3 07/20] io_uring: add interface queue

Pavel Begunkov
2023-12-20T16:29:08Z

On 12/20/23 16:13, Jens Axboe wrote:
> On 12/19/23 2:03 PM, David Wei wrote:
>> @@ -750,6 +753,54 @@ enum {
>> SOCKET_URING_OP_SETSOCKOPT,
>> };
>>
>> +struct io_uring_rbuf_rqe {
>> + __u32 off;
>> + __u32 len;
>> + __u16 region;
>> + __u8 __pad[6];
>> +};
>> +
>> +struct io_uring_rbuf_cqe {
>> + __u32 off;
>> + __u32 len;
>> + __u16 region;
>> + __u8 sock;
>> + __u8 flags;
>> + __u8 __pad[2];
>> +};
>
> Looks like this leaves a gap? Should be __pad[4] or probably just __u32
> __pad; For all of these, definitely worth thinking about if we'll ever
> need more than the slight padding. Might not hurt to always leave 8
> bytes extra, outside of the required padding.

Good catch, and that all should be paholed to ensure all of them
are fitted nicely.

FWIW, the format will also be revisited, e.g. max 256 sockets per
ifq is too restrictive, and most probably moved from a separate queue
into the CQ.

>> +struct io_rbuf_rqring_offsets {
>> + __u32 head;
>> + __u32 tail;
>> + __u32 rqes;
>> + __u8 __pad[4];
>> +};
>
> Ditto here, __u32 __pad;
>
>> +struct io_rbuf_cqring_offsets {
>> + __u32 head;
>> + __u32 tail;
>> + __u32 cqes;
>> + __u8 __pad[4];
>> +};
>
> And here.
>
>> +
>> +/*
>> + * Argument for IORING_REGISTER_ZC_RX_IFQ
>> + */
>> +struct io_uring_zc_rx_ifq_reg {
>> + __u32 if_idx;
>> + /* hw rx descriptor ring id */
>> + __u32 if_rxq_id;
>> + __u32 region_id;
>> + __u32 rq_entries;
>> + __u32 cq_entries;
>> + __u32 flags;
>> + __u16 cpu;
>> +
>> + __u32 mmap_sz;
>> + struct io_rbuf_rqring_offsets rq_off;
>> + struct io_rbuf_cqring_offsets cq_off;
>> +};
>
> You have rq_off starting at a 48-bit offset here, don't think this is
> going to work as it's uapi. You'd need padding to align it to 64-bits.
>
>> diff --git a/io_uring/zc_rx.c b/io_uring/zc_rx.c
>> new file mode 100644
>> index 000000000000..5fc94cad5e3a
>> --- /dev/null
>> +++ b/io_uring/zc_rx.c
>> +int io_register_zc_rx_ifq(struct io_ring_ctx *ctx,
>> + struct io_uring_zc_rx_ifq_reg __user *arg)
>> +{
>> + struct io_uring_zc_rx_ifq_reg reg;
>> + struct io_zc_rx_ifq *ifq;
>> + int ret;
>> +
>> + if (!(ctx->flags & IORING_SETUP_DEFER_TASKRUN))
>> + return -EINVAL;
>> + if (copy_from_user(&reg, arg, sizeof(reg)))
>> + return -EFAULT;
>> + if (ctx->ifq)
>> + return -EBUSY;
>> + if (reg.if_rxq_id == -1)
>> + return -EINVAL;
>> +
>> + ifq = io_zc_rx_ifq_alloc(ctx);
>> + if (!ifq)
>> + return -ENOMEM;
>> +
>> + /* TODO: initialise network interface */
>> +
>> + ret = io_allocate_rbuf_ring(ifq, &reg);
>> + if (ret)
>> + goto err;
>> +
>> + /* TODO: map zc region and initialise zc pool */
>> +
>> + ifq->rq_entries = reg.rq_entries;
>> + ifq->cq_entries = reg.cq_entries;
>> + ifq->if_rxq_id = reg.if_rxq_id;
>> + ctx->ifq = ifq;
>
> As these TODO's are removed in later patches, I think you should just
> not include them to begin with. It reads more like notes to yourself,
> doesn't really add anything to the series.
>
>> +void io_shutdown_zc_rx_ifqs(struct io_ring_ctx *ctx)
>> +{
>> + lockdep_assert_held(&ctx->uring_lock);
>> +}
>
> This is a bit odd?

Oh, this chunk actually leaked here from my rebases, which is not
a big deal as it provides the interface and a later patch implements
it, but might be better to move it there in the first place.

--
Pavel Begunkov