Re: [PATCH 7/7] io_uring/net: add provided buffer and bundle support to send zc

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

On 10/24/24 15:48, Jens Axboe wrote:
> On 10/24/24 8:44 AM, Pavel Begunkov wrote:
>> On 10/23/24 17:07, Jens Axboe wrote:
>>> Provided buffers inform the kernel which buffer group ID to pick a
>>> buffer from for transfer. Normally that buffer contains the usual
>>> addr + length information, as well as a buffer ID that is passed back
>>> at completion time to inform the application of which buffer was used
>>> for the transfer.
>>>
>>> However, if registered and provided buffers are combined, then the
>>> provided buffer must instead tell the kernel which registered buffer
>>> index should be used, and the length/offset within that buffer. Rather
>>> than store the addr + length, the application must instead store this
>>> information instead.
>>>
>>> If provided buffers are used with send zc, then those buffers must be
>>> an index into a registered buffer. Change the mapping type to use
>>> KBUF_MODE_BVEC, which tells the kbuf handlers to turn the mappings
>>> into bio_vecs rather than iovecs. Then all that is needed is to
>>> setup our iov_iterator to use iov_iter_bvec().
>>>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>> ---
>> ...
>>> diff --git a/io_uring/net.h b/io_uring/net.h
>>> index 52bfee05f06a..e052762cf85d 100644
>>> --- a/io_uring/net.h
>>> +++ b/io_uring/net.h
>>> @@ -5,9 +5,15 @@
>>>      struct io_async_msghdr {
>>>    #if defined(CONFIG_NET)
>>> -    struct iovec            fast_iov;
>>> +    union {
>>> +        struct iovec        fast_iov;
>>> +        struct bio_vec        fast_bvec;
>>> +    };
>>>        /* points to an allocated iov, if NULL we use fast_iov instead */
>>> -    struct iovec            *free_iov;
>>> +    union {
>>> +        struct iovec        *free_iov;
>>> +        struct bio_vec        *free_bvec;
>>
>> I'd rather not do it like that, aliasing with reusing memory and
>> counting the number is a recipe for disaster when scattered across
>> code. E.g. seems you change all(?) iovec allocations to allocate
>> based on the size of the larger structure.
>>
>> Counting bytes as in my series is less fragile, otherwise it needs
>> a new structure and a set of helpers that can be kept together.
> 
> I have been pondering this, because I'm not a huge fan either. But
> outside of the space side, it does come out pretty nicely/clean. This
> series is really just a WIP posting as per the RFC, mostly just so we
> can come up with something that's clean enough and works for both cases,
> as it does have the caching that your series does not. And to
> facilitate some more efficient TX/RX zero copy testing.

The thing is, I know how to implement caching on top of my series,
but as I commented in the other thread, I don't think it can reuse
the incremental helper well. At least it can try to unify the imu
checking / parsing, bvec copy-setup, but that's minor.

-- 
Pavel Begunkov