Re: [PATCH 6/7] io_uring/kbuf: add support for mapping type KBUF_MODE_BVEC

[Pavel Begunkov <asml.silence@gmail.com>]

On 10/23/24 17:07, Jens Axboe wrote:
> The provided buffer helpers always map to iovecs. Add a new mode,
> KBUF_MODE_BVEC, which instead maps it to a bio_vec array instead. For
> use with zero-copy scenarios, where the caller would want to turn it
> into a bio_vec anyway, and this avoids first iterating and filling out
> and iovec array, only for the caller to then iterate it again and turn
> it into a bio_vec array.
> 
> Since it's now managing both iovecs and bvecs, change the naming of
> buf_sel_arg->nr_iovs member to nr_vecs instead.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   io_uring/kbuf.c | 170 +++++++++++++++++++++++++++++++++++++++++++-----
>   io_uring/kbuf.h |   9 ++-
>   io_uring/net.c  |  10 +--
>   3 files changed, 165 insertions(+), 24 deletions(-)
> 
> diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
> index 42579525c4bd..10a3a7a27e9a 100644
> --- a/io_uring/kbuf.c
> +++ b/io_uring/kbuf.c
...
> +static struct io_mapped_ubuf *io_ubuf_from_buf(struct io_ring_ctx *ctx,
> +					       u64 addr, unsigned int *offset)
> +{
> +	struct io_mapped_ubuf *imu;
> +	u16 idx;
> +
> +	/*
> +	 * Get registered buffer index and offset, encoded into the
> +	 * addr base value.
> +	 */
> +	idx = addr & ((1ULL << IOU_BUF_REGBUF_BITS) - 1);
> +	addr >>= IOU_BUF_REGBUF_BITS;
> +	*offset = addr  & ((1ULL << IOU_BUF_OFFSET_BITS) - 1);

There are two ABI questions with that. First why not use just
user addresses instead of offsets? It's more consistent with
how everything else works. Surely it could've been offsets for
all registered buffers ops from the beggining, but it's not.

And the second, we need to start getting rid of the global node
queue, if we do, this will need to allocate an array of nodes,
store an imu list or something similar, which will be just
as terrible as it sounds, and then it'll need another cache,
sprinking more checks and handling into the hot path and so
on. That's the reason the vectored registered buffer patch
supports juts one registered buffer to index per request, and
I believe this one should do that as well.

-- 
Pavel Begunkov