On 10/24/24 9:22 AM, Pavel Begunkov wrote:
> 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@xxxxxxxxx>
>> ---
>> 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.
How would that work? You need to pass in addr + buffer index for that.
The usual approach is doing that, and then 'addr' tells you the offset
within the buffer, eg you can just do a subtraction to get your offset.
But you can't pass in both addr + index in a provided buffer, which is
why it's using buf->addr to encode index + offset for that, rather than
rely on the addr for the offset too.
The alternative obviously is to just do the 'addr' and have that be both
index and offset, in which case you'd need to lookup the buffer. And
that's certainly a no-go.
> 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.
Yeah agree, the global node queue is getting in the way of more
important things too, like applications using registered files and not
seeing reclaim in due time. I think that's the main issue here with the
ring wide queue, and certainly something that needs sorting sooner
rather than later.
Limiting this patch to just dealing with a single imu would be perfectly
fine I think, the intended use case here is really large registered
buffers, not little ones. And having a bundle be limited to a single
buffer would be perfectly fine. I can make that change for sure.
--
Jens Axboe
