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

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

[Jackie Liu]

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)
 			return -EINVAL;
 		p->cq_entries = roundup_pow_of_two(p->cq_entries);
 	} else {
-- 
2.7.4

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

[Jeff Moyer]

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.

-Jeff

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

[Jens Axboe]

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.

-- 
Jens Axboe

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

[Jackie Liu]

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

--
Jackie Liu

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

[Jens Axboe]

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