Re: [PATCH] io_uring: export cq overflow status to userspace

[Jens Axboe <axboe@kernel.dk>]

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