public inbox for io-uring@vger.kernel.org
 help / color / mirror / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Pavel Begunkov <asml.silence@gmail.com>, io-uring@vger.kernel.org
Subject: Re: [PATCH 6/7] io_uring/kbuf: add support for mapping type KBUF_MODE_BVEC
Date: Thu, 24 Oct 2024 09:27:55 -0600	[thread overview]
Message-ID: <673f2f66-cf18-44f1-878d-db2a6ffe335b@kernel.dk> (raw)
In-Reply-To: <84c8f280-09eb-425d-a47f-69117438ae55@gmail.com>

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@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.

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

  reply	other threads:[~2024-10-24 15:27 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-23 16:07 [PATCHSET RFC 0/7] Add support for provided registered buffers Jens Axboe
2024-10-23 16:07 ` [PATCH 1/7] io_uring/kbuf: mark buf_sel_arg mode as KBUF_MODE_FREE once allocated Jens Axboe
2024-10-23 16:07 ` [PATCH 2/7] io_uring/kbuf: change io_provided_buffers_select() calling convention Jens Axboe
2024-10-23 16:07 ` [PATCH 3/7] io_uring/net: abstract out io_send_import() helper Jens Axboe
2024-10-23 16:07 ` [PATCH 4/7] io_uring/net: move send zc fixed buffer import into helper Jens Axboe
2024-10-23 16:07 ` [PATCH 5/7] io_uring: add ability for provided buffer to index registered buffers Jens Axboe
2024-10-24 15:44   ` Pavel Begunkov
2024-10-24 15:57     ` Jens Axboe
2024-10-24 16:17       ` Pavel Begunkov
2024-10-24 17:16         ` Jens Axboe
2024-10-24 18:20           ` Pavel Begunkov
2024-10-24 19:53             ` Jens Axboe
2024-10-24 22:46               ` Jens Axboe
2024-10-23 16:07 ` [PATCH 6/7] io_uring/kbuf: add support for mapping type KBUF_MODE_BVEC Jens Axboe
2024-10-24 15:22   ` Pavel Begunkov
2024-10-24 15:27     ` Jens Axboe [this message]
2024-10-24 15:40       ` Pavel Begunkov
2024-10-24 15:49         ` Jens Axboe
2024-10-23 16:07 ` [PATCH 7/7] io_uring/net: add provided buffer and bundle support to send zc Jens Axboe
2024-10-24 14:44   ` Pavel Begunkov
2024-10-24 14:48     ` Jens Axboe
2024-10-24 15:36       ` Pavel Begunkov
2024-10-24 14:36 ` [PATCHSET RFC 0/7] Add support for provided registered buffers Pavel Begunkov
2024-10-24 14:43   ` Jens Axboe
2024-10-24 15:04     ` Pavel Begunkov
2024-10-24 15:11       ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=673f2f66-cf18-44f1-878d-db2a6ffe335b@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=asml.silence@gmail.com \
    --cc=io-uring@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox