Re: [PATCH] io_uring: ensure cq_entries is at least equal to or greater than sq_entries

[Jens Axboe <axboe@kernel.dk>]

On 10/23/19 6:22 PM, Jackie Liu wrote:
> 
> 
>> 2019年10月24日 03:41，Jens Axboe <axboe@kernel.dk> 写道：
>>
>> On 10/23/19 12:42 PM, Jeff Moyer wrote:
>>> Jackie Liu <liuyun01@kylinos.cn> writes:
>>>
>>>> If cq_entries is smaller than sq_entries, it will cause a lot of overflow
>>>> to appear. when customizing cq_entries, at least let him be no smaller than
>>>> sq_entries.
>>>>
>>>> Fixes: 95d8765bd9f2 ("io_uring: allow application controlled CQ ring size")
>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>> ---
>>>>   fs/io_uring.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index b64cd2c..dfa9731 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -3784,7 +3784,7 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p)
>>>>   		 * to a power-of-two, if it isn't already. We do NOT impose
>>>>   		 * any cq vs sq ring sizing.
>>>>   		 */
>>>> -		if (!p->cq_entries || p->cq_entries > IORING_MAX_CQ_ENTRIES)
>>>> +		if (p->cq_entries < p->sq_entries || p->cq_entries > IORING_MAX_CQ_ENTRIES)
>>>
>>> What if they're both zero?  I think you want to keep that check.
>>
>> sq_entries being zero is already checked and failed at this point.
>> So I think the patch looks fine from that perspective.
>>
>> Is there really a strong reason to disallow this? Yes, it could
>> cause overflows, but it's just doing what was asked for. The
>> normal case is of course cq_entries being much larger than
>> sq_entries.
>>
> 
> There are actually no other stronger reasons. I think it would be better to do a
> print job in liburing, but the kernel should still make a limit. Too many overflows
> will cause less efficiency.

Taken to the extreme, it's clearly an issue. You could setup sq 128
entries, with 1 cq entry. That'd work as long as you never drive more
than 1 sq entry, but it makes very little sense.

Since we used to have cq == 2 * sq (and still do, by default), I think
the change to ensure that cq >= sq makes sense. I'll apply it, thanks.

-- 
Jens Axboe