Re: [PATCHSET v2 RFC 0/11] Add support for ring mapped provided buffers

Re: [PATCHSET v2 RFC 0/11] Add support for ring mapped provided buffers

[Pavel Begunkov]

On 4/29/22 18:56, Jens Axboe wrote:
> Hi,
> 
> This series builds to adding support for a different way of doing
> provided buffers. The interesting bits here are patch 11, which also has
> some performance numbers an an explanation of it.

Jens, would be great if you can CC me for large changes, you know
how it's with mailing lists nowadays...

1) reading "io_uring: abstract out provided buffer list selection"

Let's move io_ring_submit_unlock() to where the lock call is.
In the end, it's only confusing and duplicates unlock in
io_ring_buffer_select() and io_provided_buffer_select().

2) As it's a new API, let's do bucket selection right, I quite
don't like io_buffer_get_list(). We can replace "bgid" with
indexes into an array and let the userspace to handle indexing.
Most likely it knows the index right away or can implement indexes
lookup with as many tricks and caching it needs.


> Patches 1..5 are cleanups that should just applied separately, I
> think the clean up the existing code quite nicely.
> 
> Patch 6 is a generic optimization for the buffer list lookups.
> 
> Patch 7 adds NOP support for provided buffers, just so that we can
> benchmark the last change.
> 
> Patches 8..10 are prep for patch 11.
> 
> Patch 11 finally adds the feature.
> 
> This passes the full liburing suite - obviously this just means that it
> didn't break anything existing (that I know of), the only test case for
> the ring buffers is the nop peak benchmark referenced in patch 11.
> 
> v2:	- Minor optimizations
> 	- Fix 4k PAGE_SIZE assumption
> 	- Style cleanup
> 
> Can also be found in my git repo, for-5.19/io_uring-pbuf branch:
> 
> https://git.kernel.dk/cgit/linux-block/log/?h=for-5.19/io_uring-pbuf
> 
>   fs/io_uring.c                 | 463 +++++++++++++++++++++++++---------
>   include/uapi/linux/io_uring.h |  26 ++
>   2 files changed, 370 insertions(+), 119 deletions(-)
> 

-- 
Pavel Begunkov

Re: [PATCHSET v2 RFC 0/11] Add support for ring mapped provided buffers

[Pavel Begunkov]

On 5/1/22 14:28, Jens Axboe wrote:
> On 5/1/22 7:14 AM, Pavel Begunkov wrote:
>> On 4/29/22 18:56, Jens Axboe wrote:
>>> Hi,
>>>
>>> This series builds to adding support for a different way of doing
>>> provided buffers. The interesting bits here are patch 11, which also has
>>> some performance numbers an an explanation of it.
>>
>> Jens, would be great if you can CC me for large changes, you know
>> how it's with mailing lists nowadays...
> 
> You bet, I can just add you to anything posted. Starting to lose faith
> in email ever becoming reliable again...

thanks


>> 1) reading "io_uring: abstract out provided buffer list selection"
>>
>> Let's move io_ring_submit_unlock() to where the lock call is.
>> In the end, it's only confusing and duplicates unlock in
>> io_ring_buffer_select() and io_provided_buffer_select().
> 
> Sure, I can clean that up.
> 
>> 2) As it's a new API, let's do bucket selection right, I quite
>> don't like io_buffer_get_list(). We can replace "bgid" with
>> indexes into an array and let the userspace to handle indexing.
>> Most likely it knows the index right away or can implement indexes
>> lookup with as many tricks and caching it needs.
> 
> Maybe we can just use xarray here rather than a hashed list? It's really
> just a sparse array. The downside is that xarray locking isn't always
> very convenient, eg using it with your own locking...
> 
> Any other suggestions?

I'd suggest for mapped pbuffers to have an old plain array with
sequential indexing, just how we do it for fixed buffers. Do normal
and mapped pbuffers share something that would prevent it?

-- 
Pavel Begunkov

Re: [PATCHSET v2 RFC 0/11] Add support for ring mapped provided buffers

[Jens Axboe]

On 5/1/22 7:39 AM, Pavel Begunkov wrote:
> I'd suggest for mapped pbuffers to have an old plain array with
> sequential indexing, just how we do it for fixed buffers. Do normal
> and mapped pbuffers share something that would prevent it?

Ah yes, we could do that. Registering it returns the group ID instead of
providing it up front.

-- 
Jens Axboe

Re: [PATCHSET v2 RFC 0/11] Add support for ring mapped provided buffers

[Jens Axboe]

On 5/1/22 8:25 AM, Jens Axboe wrote:
> On 5/1/22 7:39 AM, Pavel Begunkov wrote:
>> I'd suggest for mapped pbuffers to have an old plain array with
>> sequential indexing, just how we do it for fixed buffers. Do normal
>> and mapped pbuffers share something that would prevent it?
> 
> Ah yes, we could do that. Registering it returns the group ID instead of
> providing it up front.

Actually I'd rather just have the app provide it, but recommendations
can be made in terms of using mostly sequential indexes. I suspect
that's what most would naturally do anyway.

I'm thinking just straight array of X entries, and then a fallback to
xarray if we go beyond that to ensure we don't grow the buffer group
array to crazy values.

I'll do this as a prep patch, not really related to the actual change
here, but will benefit both the classic and ring buffers alike.

-- 
Jens Axboe