On 7/7/20 10:36 AM, Xiaoguang Wang wrote:
> hi,
>
>> On 7/7/20 8:28 AM, Jens Axboe wrote:
>>> On 7/7/20 7:24 AM, Xiaoguang Wang wrote:
>>>> For those applications which are not willing to use io_uring_enter()
>>>> to reap and handle cqes, they may completely rely on liburing's
>>>> io_uring_peek_cqe(), but if cq ring has overflowed, currently because
>>>> io_uring_peek_cqe() is not aware of this overflow, it won't enter
>>>> kernel to flush cqes, below test program can reveal this bug:
>>>>
>>>> static void test_cq_overflow(struct io_uring *ring)
>>>> {
>>>> struct io_uring_cqe *cqe;
>>>> struct io_uring_sqe *sqe;
>>>> int issued = 0;
>>>> int ret = 0;
>>>>
>>>> do {
>>>> sqe = io_uring_get_sqe(ring);
>>>> if (!sqe) {
>>>> fprintf(stderr, "get sqe failed\n");
>>>> break;;
>>>> }
>>>> ret = io_uring_submit(ring);
>>>> if (ret <= 0) {
>>>> if (ret != -EBUSY)
>>>> fprintf(stderr, "sqe submit failed: %d\n", ret);
>>>> break;
>>>> }
>>>> issued++;
>>>> } while (ret > 0);
>>>> assert(ret == -EBUSY);
>>>>
>>>> printf("issued requests: %d\n", issued);
>>>>
>>>> while (issued) {
>>>> ret = io_uring_peek_cqe(ring, &cqe);
>>>> if (ret) {
>>>> if (ret != -EAGAIN) {
>>>> fprintf(stderr, "peek completion failed: %s\n",
>>>> strerror(ret));
>>>> break;
>>>> }
>>>> printf("left requets: %d\n", issued);
>>>> continue;
>>>> }
>>>> io_uring_cqe_seen(ring, cqe);
>>>> issued--;
>>>> printf("left requets: %d\n", issued);
>>>> }
>>>> }
>>>>
>>>> int main(int argc, char *argv[])
>>>> {
>>>> int ret;
>>>> struct io_uring ring;
>>>>
>>>> ret = io_uring_queue_init(16, &ring, 0);
>>>> if (ret) {
>>>> fprintf(stderr, "ring setup failed: %d\n", ret);
>>>> return 1;
>>>> }
>>>>
>>>> test_cq_overflow(&ring);
>>>> return 0;
>>>> }
>>>>
>>>> To fix this issue, export cq overflow status to userspace, then
>>>> helper functions() in liburing, such as io_uring_peek_cqe, can be
>>>> aware of this cq overflow and do flush accordingly.
>>>
>>> Is there any way we can accomplish the same without exporting
>>> another set of flags? Would it be enough for the SQPOLl thread to set
>>> IORING_SQ_NEED_WAKEUP if we're in overflow condition? That should
>>> result in the app entering the kernel when it's flushed the user CQ
>>> side, and then the sqthread could attempt to flush the pending
>>> events as well.
>>>
>>> Something like this, totally untested...
>>
>> OK, took a closer look at this, it's a generic thing, not just
>> SQPOLL related. My bad!
>>
>> Anyway, my suggestion would be to add IORING_SQ_CQ_OVERFLOW to the
>> existing flags, and then make a liburing change almost identical to
>> what you had.
> Thanks.
> It's somewhat late today, I'll test and send these two patches tomorrow.
Sounds good, thanks.
--
Jens Axboe
