Re: [PATCH 0/3] io_uring: fixes for provided buffer ring

[Hao Xu <hao.xu@linux.dev>]

On 6/13/22 22:05, Pavel Begunkov wrote:
> On 6/13/22 14:16, Dylan Yudaken wrote:
>> On Mon, 2022-06-13 at 13:59 +0100, Pavel Begunkov wrote:
>>> On 6/13/22 12:08, Hao Xu wrote:
>>>> On 6/13/22 18:11, Dylan Yudaken wrote:
>>>>> This fixes two problems in the new provided buffer ring feature.
>>>>> One
>>>>> is a simple arithmetic bug (I think this came out from a
>>>>> refactor).
>>>>> The other is due to type differences between head & tail, which
>>>>> causes
>>>>> it to sometimes reuse an old buffer incorrectly.
>>>>>
>>>>> Patch 1&2 fix bugs
>>>>> Patch 3 limits the size of the ring as it's not
>>>>> possible to address more entries with 16 bit head/tail
>>>>
>>>> Reviewed-by: Hao Xu <howeyxu@tencent.com>
>>>>
>>>>>
>>>>> I will send test cases for liburing shortly.
>>>>>
>>>>> One question might be if we should change the type of
>>>>> ring_entries
>>>>> to uint16_t in struct io_uring_buf_reg?
>>>>
>>>> Why not? 5.19 is just rc2 now. So we can assume there is no users
>>>> using
>>>> it right now I think?
>>>
>>> It's fine to change, but might be better if we want to extend it
>>> in the future. Do other pbuf bits allow more than 2^16 buffers?
>>>
> 
> might be better to leave it u32 *
> 
>> I guess with
>>
>> +    if (reg.ring_entries >= 65536)
>> +        return -EINVAL;
>>
>> it doesn't matter either way. we can always use those bits later if we
>> need?
> 
> I think so as well.
> 
> I was also wondering whether pbufs can potentially allow >16 bits,
> but taking a quick look IORING_CQE_BUFFER_SHIFT=16, so we only have
> 16 bits in cqe::flags for indexes we return.
> 

Yea, the 16 bits return index in cqe->flags is a hard limit for
pbuf ring feature, but I do think it's ok since 1<<16 is already big