io_uring: export cq overflow status to userspace

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.

Quoted 63 lines · Xiaoguang Wang

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;
}

Quoted 3 lines

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

Quoted 12 lines

Quoted 4 lines

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.

Quoted 1 line

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.

...

Quoted 17 lines

Quoted 5 lines

Quoted 4 lines

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.

Quoted 8 lines

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!

Quoted 3 lines

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.

How about CQ being full as an indicator that flush is required? With a properly set CQ size there shouldn't be much false positives.

Quoted 75 lines · Xiaoguang Wang

Quoted 63 lines

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;
}

Quoted 3 lines

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.

Quoted 1 line

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.

Hence kernel side:

fs/io_uring.c +12 -4

index d37d7ea5ebe5..af9fd5cefc51 100644
@@ -1234,11 +1234,12 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
  	struct io_uring_cqe *cqe;
  	struct io_kiocb *req;
  	unsigned long flags;
+	bool ret = true;
  	LIST_HEAD(list);
 
  	if (!force) {
  		if (list_empty_careful(&ctx->cq_overflow_list))
-			return true;
+			goto done;
  		if ((ctx->cached_cq_tail - READ_ONCE(rings->cq.head) ==
  		    rings->cq_ring_entries))
  			return false;

@@ -1284,7 +1285,11 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
  		io_put_req(req);
  	}
 
-	return cqe != NULL;
+	ret = cqe != NULL;
+done:
+	if (ret)
+		ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
+	return ret;
  }
 
  static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)

@@ -5933,10 +5938,13 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
  	int i, submitted = 0;
 
  	/* if we have a backlog and couldn't flush it all, return BUSY */
-	if (test_bit(0, &ctx->sq_check_overflow)) {
+	if (unlikely(test_bit(0, &ctx->sq_check_overflow))) {
  		if (!list_empty(&ctx->cq_overflow_list) &&
-		    !io_cqring_overflow_flush(ctx, false))
+		    !io_cqring_overflow_flush(ctx, false)) {
+			ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;
+			smp_mb();
  			return -EBUSY;
+		}
  	}
 
  	/* make sure SQ entry isn't read before tail */

include/uapi/linux/io_uring.h +1 -0

index 92c22699a5a7..9c7e028beda5 100644
@@ -197,6 +197,7 @@ struct io_sqring_offsets {
   * sq_ring->flags
   */
  #define IORING_SQ_NEED_WAKEUP	(1U << 0) /* needs io_uring_enter wakeup */
+#define IORING_SQ_CQ_OVERFLOW	(1U << 1) /* app needs to enter kernel */
 
  struct io_cqring_offsets {
  	__u32 head;

and then this for the liburing side:

src/include/liburing/io_uring.h +1 -0

index 6a73522..e4314ed 100644
@@ -202,6 +202,7 @@ struct io_sqring_offsets {
   * sq_ring->flags
   */
  #define IORING_SQ_NEED_WAKEUP	(1U << 0) /* needs io_uring_enter wakeup */
+#define IORING_SQ_CQ_OVERFLOW	(1U << 1)
 
  struct io_cqring_offsets {
  	__u32 head;

src/queue.c +13 -4

index 88e0294..1f00251 100644
@@ -32,6 +32,11 @@ static inline bool sq_ring_needs_enter(struct io_uring *ring,
  	return false;
  }
 
+static inline bool cq_ring_needs_flush(struct io_uring *ring)
+{
+	return IO_URING_READ_ONCE(*ring->sq.kflags) & IORING_SQ_CQ_OVERFLOW;
+}
+
  static int __io_uring_peek_cqe(struct io_uring *ring,
  			       struct io_uring_cqe **cqe_ptr)
  {

@@ -67,22 +72,26 @@ int __io_uring_get_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_ptr,
  	int ret = 0, err;
 
  	do {
+		bool cq_overflow_flush = false;
  		unsigned flags = 0;
 
  		err = __io_uring_peek_cqe(ring, &cqe);
  		if (err)
  			break;
  		if (!cqe && !to_wait && !submit) {
-			err = -EAGAIN;
-			break;
+			if (!cq_ring_needs_flush(ring)) {
+				err = -EAGAIN;
+				break;
+			}
+			cq_overflow_flush = true;
  		}
  		if (wait_nr && cqe)
  			wait_nr--;
-		if (wait_nr)
+		if (wait_nr || cq_overflow_flush)
  			flags = IORING_ENTER_GETEVENTS;
  		if (submit)
  			sq_ring_needs_enter(ring, submit, &flags);
-		if (wait_nr || submit)
+		if (wait_nr || submit || cq_overflow_flush)
  			ret = __sys_io_uring_enter(ring->ring_fd, submit,
  						   wait_nr, flags, sigmask);
  		if (ret < 0) {

If you agree with this approach, could you test this and resubmit the two patches?

hi,

Quoted 78 lines · Jens Axboe

Quoted 76 lines · Xiaoguang Wang

Quoted 63 lines

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;
}

Quoted 3 lines

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

Quoted 6 lines

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.

Quoted 68 lines · Xiaoguang Wang

Quoted 63 lines

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;
}

Quoted 3 lines

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.

Quoted 8 lines

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.

Hence kernel side:

fs/io_uring.c +12 -4

index d37d7ea5ebe5..af9fd5cefc51 100644
@@ -1234,11 +1234,12 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
  	struct io_uring_cqe *cqe;
  	struct io_kiocb *req;
  	unsigned long flags;
+	bool ret = true;
  	LIST_HEAD(list);
 
  	if (!force) {
  		if (list_empty_careful(&ctx->cq_overflow_list))
-			return true;
+			goto done;
  		if ((ctx->cached_cq_tail - READ_ONCE(rings->cq.head) ==
  		    rings->cq_ring_entries))
  			return false;

@@ -1284,7 +1285,11 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
  		io_put_req(req);
  	}
 
-	return cqe != NULL;
+	ret = cqe != NULL;
+done:
+	if (ret)
+		ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
+	return ret;
  }
 
  static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)

@@ -5933,10 +5938,13 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
  	int i, submitted = 0;
 
  	/* if we have a backlog and couldn't flush it all, return BUSY */
-	if (test_bit(0, &ctx->sq_check_overflow)) {
+	if (unlikely(test_bit(0, &ctx->sq_check_overflow))) {
  		if (!list_empty(&ctx->cq_overflow_list) &&
-		    !io_cqring_overflow_flush(ctx, false))
+		    !io_cqring_overflow_flush(ctx, false)) {
+			ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;
+			smp_mb();
  			return -EBUSY;
+		}
  	}
 
  	/* make sure SQ entry isn't read before tail */

include/uapi/linux/io_uring.h +0 -0

index 92c22699a5a7..9c7e028beda5 100644

and then this for the liburing side:

src/include/liburing/io_uring.h +1 -0

index 6a73522..e4314ed 100644
@@ -202,6 +202,7 @@ struct io_sqring_offsets {
   * sq_ring->flags
   */
  #define IORING_SQ_NEED_WAKEUP	(1U << 0) /* needs io_uring_enter wakeup */
+#define IORING_SQ_CQ_OVERFLOW	(1U << 1)
 
  struct io_cqring_offsets {
  	__u32 head;

src/queue.c +13 -4

index 88e0294..1f00251 100644
@@ -32,6 +32,11 @@ static inline bool sq_ring_needs_enter(struct io_uring *ring,
  	return false;
  }
 
+static inline bool cq_ring_needs_flush(struct io_uring *ring)
+{
+	return IO_URING_READ_ONCE(*ring->sq.kflags) & IORING_SQ_CQ_OVERFLOW;
+}
+
  static int __io_uring_peek_cqe(struct io_uring *ring,
  			       struct io_uring_cqe **cqe_ptr)
  {

@@ -67,22 +72,26 @@ int __io_uring_get_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_ptr,
  	int ret = 0, err;
 
  	do {
+		bool cq_overflow_flush = false;
  		unsigned flags = 0;
 
  		err = __io_uring_peek_cqe(ring, &cqe);
  		if (err)
  			break;
  		if (!cqe && !to_wait && !submit) {
-			err = -EAGAIN;
-			break;
+			if (!cq_ring_needs_flush(ring)) {
+				err = -EAGAIN;
+				break;
+			}
+			cq_overflow_flush = true;
  		}
  		if (wait_nr && cqe)
  			wait_nr--;
-		if (wait_nr)
+		if (wait_nr || cq_overflow_flush)
  			flags = IORING_ENTER_GETEVENTS;
  		if (submit)
  			sq_ring_needs_enter(ring, submit, &flags);
-		if (wait_nr || submit)
+		if (wait_nr || submit || cq_overflow_flush)
  			ret = __sys_io_uring_enter(ring->ring_fd, submit,
  						   wait_nr, flags, sigmask);
  		if (ret < 0) {

If you agree with this approach, could you test this and resubmit the two patches?

hi,

Quoted 132 lines

Quoted 69 lines · Jens Axboe

Quoted 63 lines · Xiaoguang Wang

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;
}

Quoted 3 lines

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.

Quoted 8 lines

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.
Hence kernel side:

fs/io_uring.c +12 -3

index d37d7ea5ebe5..af9fd5cefc51 100644
@@ -1234,11 +1234,12 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
     struct io_uring_cqe *cqe;
     struct io_kiocb *req;
     unsigned long flags;
+    bool ret = true;
     LIST_HEAD(list);
       if (!force) {
         if (list_empty_careful(&ctx->cq_overflow_list))
-            return true;
+            goto done;
         if ((ctx->cached_cq_tail - READ_ONCE(rings->cq.head) ==
             rings->cq_ring_entries))
             return false;

@@ -1284,7 +1285,11 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
         io_put_req(req);
     }
 -    return cqe != NULL;
+    ret = cqe != NULL;
+done:
+    if (ret)
+        ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
+    return ret;
 }
   static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)

@@ -5933,10 +5938,13 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
     int i, submitted = 0;
       /* if we have a backlog and couldn't flush it all, return BUSY */
-    if (test_bit(0, &ctx->sq_check_overflow)) {
+    if (unlikely(test_bit(0, &ctx->sq_check_overflow))) {
         if (!list_empty(&ctx->cq_overflow_list) &&
-            !io_cqring_overflow_flush(ctx, false))
+            !io_cqring_overflow_flush(ctx, false)) {
+            ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;
+            smp_mb();
             return -EBUSY;
+        }
     }
       /* make sure SQ entry isn't read before tail */

include/uapi/linux/io_uring.h +0 -0

index 92c22699a5a7..9c7e028beda5 100644
@@ -197,6 +197,7 @@ struct io_sqring_offsets {
  * sq_ring->flags
  */

include/uapi/linux/io_uring.h struct io_sqring_offsets { · line 203

Quoted 4 lines

include/uapi/linux/io_uring.h +1 -0

 #define IORING_SQ_NEED_WAKEUP    (1U << 0) /* needs io_uring_enter wakeup */
+#define IORING_SQ_CQ_OVERFLOW    (1U << 1) /* app needs to enter kernel */
   struct io_cqring_offsets {
     __u32 head;

I think above codes are still not correct, you only set IORING_SQ_CQ_OVERFLOW in io_submit_sqes, but if cq ring has been overflowed and applications don't do io submit anymore, just calling io_uring_peek_cqe continuously, then applications still won't be aware of the cq ring overflow.

We can put the IORING_SQ_CQ_OVERFLOW set in __io_cqring_fill_event() when setting cq_check_overflow. In non-sqpoll, this will be safe, but in sqpoll mode, there maybe concurrent modifications to sq_flags, which is a race condition and may need extral lock to protect IORING_SQ_NEED_WAKEP and IORING_SQ_CQ_OVERFLOW.

Quoted 137 lines · Xiaoguang Wang

hi,

Quoted 132 lines

Quoted 69 lines · Jens Axboe

Quoted 63 lines · Xiaoguang Wang

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;
}

Quoted 3 lines

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.

Quoted 8 lines

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.
Hence kernel side:

fs/io_uring.c +12 -3

index d37d7ea5ebe5..af9fd5cefc51 100644
@@ -1234,11 +1234,12 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
      struct io_uring_cqe *cqe;
      struct io_kiocb *req;
      unsigned long flags;
+    bool ret = true;
      LIST_HEAD(list);
        if (!force) {
          if (list_empty_careful(&ctx->cq_overflow_list))
-            return true;
+            goto done;
          if ((ctx->cached_cq_tail - READ_ONCE(rings->cq.head) ==
              rings->cq_ring_entries))
              return false;

@@ -1284,7 +1285,11 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
          io_put_req(req);
      }
  -    return cqe != NULL;
+    ret = cqe != NULL;
+done:
+    if (ret)
+        ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
+    return ret;
  }
    static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)

@@ -5933,10 +5938,13 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
      int i, submitted = 0;
        /* if we have a backlog and couldn't flush it all, return BUSY */
-    if (test_bit(0, &ctx->sq_check_overflow)) {
+    if (unlikely(test_bit(0, &ctx->sq_check_overflow))) {
          if (!list_empty(&ctx->cq_overflow_list) &&
-            !io_cqring_overflow_flush(ctx, false))
+            !io_cqring_overflow_flush(ctx, false)) {
+            ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;
+            smp_mb();
              return -EBUSY;
+        }
      }
        /* make sure SQ entry isn't read before tail */

include/uapi/linux/io_uring.h +0 -0

index 92c22699a5a7..9c7e028beda5 100644
@@ -197,6 +197,7 @@ struct io_sqring_offsets {
   * sq_ring->flags
   */

Quoted 8 lines

include/uapi/linux/io_uring.h struct io_sqring_offsets { · line 203

Quoted 4 lines

include/uapi/linux/io_uring.h +1 -0

  #define IORING_SQ_NEED_WAKEUP    (1U << 0) /* needs io_uring_enter wakeup */
+#define IORING_SQ_CQ_OVERFLOW    (1U << 1) /* app needs to enter kernel */
    struct io_cqring_offsets {
      __u32 head;

I think above codes are still not correct, you only set IORING_SQ_CQ_OVERFLOW in io_submit_sqes, but if cq ring has been overflowed and applications don't do io submit anymore, just calling io_uring_peek_cqe continuously, then applications still won't be aware of the cq ring overflow.

With the associated liburing change in that same email, the peek will enter the kernel just to flush the overflow. So not sure I see your concern?

Quoted 5 lines

We can put the IORING_SQ_CQ_OVERFLOW set in __io_cqring_fill_event() when setting cq_check_overflow. In non-sqpoll, this will be safe, but in sqpoll mode, there maybe concurrent modifications to sq_flags, which is a race condition and may need extral lock to protect IORING_SQ_NEED_WAKEP and IORING_SQ_CQ_OVERFLOW.

That’s why I wanted to keep it in the shared submission path, as that’s properly synchronized.

I modify above test program a bit:
#include <errno.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <fcntl.h>
#include <assert.h>

#include "liburing.h"

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;
         int i;

         for (i = 0; i < 33; i++) {
                 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++;
         }

         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 %d\n", issued, IO_URING_READ_ONCE(*ring->sq.kflags));
                         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;
}

Though with your patches applied, we still can not peek the last cqe. This test program will only issue 33 sqes, so it won't get EBUSY error.

Quoted 68 lines

I modify above test program a bit:
#include <errno.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <fcntl.h>
#include <assert.h>

#include "liburing.h"

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;
          int i;

          for (i = 0; i < 33; i++) {
                  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++;
          }

          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 %d\n", issued, IO_URING_READ_ONCE(*ring->sq.kflags));
                          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;
}

Quoted 2 lines

Though with your patches applied, we still can not peek the last cqe. This test program will only issue 33 sqes, so it won't get EBUSY error.

How about we make this even simpler, then - make the IORING_SQ_CQ_OVERFLOW actually track the state, rather than when we fail on submission. The liburing change would be the same, the kernel side would then look like the below.

fs/io_uring.c +6 -2

index 4c9a494c9f9f..01981926cdf4 100644
@@ -1342,6 +1342,7 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
  	if (cqe) {
  		clear_bit(0, &ctx->sq_check_overflow);
  		clear_bit(0, &ctx->cq_check_overflow);
+		ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
  	}
  	spin_unlock_irqrestore(&ctx->completion_lock, flags);
  	io_cqring_ev_posted(ctx);

@@ -1379,6 +1380,7 @@ static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)
  		if (list_empty(&ctx->cq_overflow_list)) {
  			set_bit(0, &ctx->sq_check_overflow);
  			set_bit(0, &ctx->cq_check_overflow);
+			ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;
  		}
  		req->flags |= REQ_F_OVERFLOW;
  		refcount_inc(&req->refs);

@@ -6375,9 +6377,9 @@ static int io_sq_thread(void *data)
  			}
 
  			/* Tell userspace we may need a wakeup call */
+			spin_lock_irq(&ctx->completion_lock);
  			ctx->rings->sq_flags |= IORING_SQ_NEED_WAKEUP;
-			/* make sure to read SQ tail after writing flags */
-			smp_mb();
+			spin_unlock_irq(&ctx->completion_lock);
 
  			to_submit = io_sqring_entries(ctx);
  			if (!to_submit || ret == -EBUSY) {

@@ -6400,7 +6402,9 @@ static int io_sq_thread(void *data)
  			}
  			finish_wait(&ctx->sqo_wait, &wait);
 
+			spin_lock_irq(&ctx->completion_lock);
  			ctx->rings->sq_flags &= ~IORING_SQ_NEED_WAKEUP;
+			spin_unlock_irq(&ctx->completion_lock);
  		}
 
  		mutex_lock(&ctx->uring_lock);

include/uapi/linux/io_uring.h +0 -0

index 8d033961cb78..91953b543125 100644

hi,

Quoted 130 lines · Xiaoguang Wang

Quoted 68 lines

I modify above test program a bit:
#include <errno.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <fcntl.h>
#include <assert.h>

#include "liburing.h"

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;
          int i;

          for (i = 0; i < 33; i++) {
                  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++;
          }

          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 %d\n", issued, IO_URING_READ_ONCE(*ring->sq.kflags));
                          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;
}

Quoted 2 lines

Though with your patches applied, we still can not peek the last cqe. This test program will only issue 33 sqes, so it won't get EBUSY error.

How about we make this even simpler, then - make the IORING_SQ_CQ_OVERFLOW actually track the state, rather than when we fail on submission. The liburing change would be the same, the kernel side would then look like the below.

fs/io_uring.c +6 -2

index 4c9a494c9f9f..01981926cdf4 100644
@@ -1342,6 +1342,7 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
  	if (cqe) {
  		clear_bit(0, &ctx->sq_check_overflow);
  		clear_bit(0, &ctx->cq_check_overflow);
+		ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
  	}
  	spin_unlock_irqrestore(&ctx->completion_lock, flags);
  	io_cqring_ev_posted(ctx);

@@ -1379,6 +1380,7 @@ static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)
  		if (list_empty(&ctx->cq_overflow_list)) {
  			set_bit(0, &ctx->sq_check_overflow);
  			set_bit(0, &ctx->cq_check_overflow);
+			ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;
  		}
  		req->flags |= REQ_F_OVERFLOW;
  		refcount_inc(&req->refs);

@@ -6375,9 +6377,9 @@ static int io_sq_thread(void *data)
  			}
 
  			/* Tell userspace we may need a wakeup call */
+			spin_lock_irq(&ctx->completion_lock);
  			ctx->rings->sq_flags |= IORING_SQ_NEED_WAKEUP;
-			/* make sure to read SQ tail after writing flags */
-			smp_mb();
+			spin_unlock_irq(&ctx->completion_lock);
 
  			to_submit = io_sqring_entries(ctx);
  			if (!to_submit || ret == -EBUSY) {

@@ -6400,7 +6402,9 @@ static int io_sq_thread(void *data)
  			}
  			finish_wait(&ctx->sqo_wait, &wait);
 
+			spin_lock_irq(&ctx->completion_lock);
  			ctx->rings->sq_flags &= ~IORING_SQ_NEED_WAKEUP;
+			spin_unlock_irq(&ctx->completion_lock);
  		}
 
  		mutex_lock(&ctx->uring_lock);

include/uapi/linux/io_uring.h +1 -0

index 8d033961cb78..91953b543125 100644
@@ -198,6 +198,7 @@ struct io_sqring_offsets {
   * sq_ring->flags
   */
  #define IORING_SQ_NEED_WAKEUP	(1U << 0) /* needs io_uring_enter wakeup */
+#define IORING_SQ_CQ_OVERFLOW	(1U << 1) /* app needs to enter kernel */
 

include/uapi/linux/io_uring.h struct io_sqring_offsets { · line 204

Quoted 3 lines

include/uapi/linux/io_uring.h +0 -0

  struct io_cqring_offsets {
  	__u32 head;

Looks good, I think it'll work now.
I'll test and send patches soon, thanks.

Quoted 89 lines

hi,

Quoted 87 lines · Xiaoguang Wang

Quoted 68 lines

I modify above test program a bit:
#include <errno.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <fcntl.h>
#include <assert.h>

#include "liburing.h"

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;
           int i;

           for (i = 0; i < 33; i++) {
                   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++;
           }

           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 %d\n", issued, IO_URING_READ_ONCE(*ring->sq.kflags));
                           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;
}

Quoted 2 lines

Though with your patches applied, we still can not peek the last cqe. This test program will only issue 33 sqes, so it won't get EBUSY error.

How about we make this even simpler, then - make the IORING_SQ_CQ_OVERFLOW actually track the state, rather than when we fail on submission. The liburing change would be the same, the kernel side would then look like the below.

fs/io_uring.c +1 -0

index 4c9a494c9f9f..01981926cdf4 100644
@@ -1342,6 +1342,7 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
   	if (cqe) {
   		clear_bit(0, &ctx->sq_check_overflow);
   		clear_bit(0, &ctx->cq_check_overflow);
+		ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;

Quoted 8 lines

fs/io_uring.c static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags) · line 1383

Quoted 8 lines

fs/io_uring.c +1 -0

   	}
   	spin_unlock_irqrestore(&ctx->completion_lock, flags);
   	io_cqring_ev_posted(ctx);

@@ -1379,6 +1380,7 @@ static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)
   		if (list_empty(&ctx->cq_overflow_list)) {
   			set_bit(0, &ctx->sq_check_overflow);
   			set_bit(0, &ctx->cq_check_overflow);
+			ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;

Quoted 35 lines

Quoted 35 lines

fs/io_uring.c +4 -2

   		}
   		req->flags |= REQ_F_OVERFLOW;
   		refcount_inc(&req->refs);

@@ -6375,9 +6377,9 @@ static int io_sq_thread(void *data)
   			}
 
   			/* Tell userspace we may need a wakeup call */
+			spin_lock_irq(&ctx->completion_lock);
   			ctx->rings->sq_flags |= IORING_SQ_NEED_WAKEUP;
-			/* make sure to read SQ tail after writing flags */
-			smp_mb();
+			spin_unlock_irq(&ctx->completion_lock);
 
   			to_submit = io_sqring_entries(ctx);
   			if (!to_submit || ret == -EBUSY) {

@@ -6400,7 +6402,9 @@ static int io_sq_thread(void *data)
   			}
   			finish_wait(&ctx->sqo_wait, &wait);
 
+			spin_lock_irq(&ctx->completion_lock);
   			ctx->rings->sq_flags &= ~IORING_SQ_NEED_WAKEUP;
+			spin_unlock_irq(&ctx->completion_lock);
   		}
 
   		mutex_lock(&ctx->uring_lock);

include/uapi/linux/io_uring.h +1 -0

index 8d033961cb78..91953b543125 100644
@@ -198,6 +198,7 @@ struct io_sqring_offsets {
    * sq_ring->flags
    */
   #define IORING_SQ_NEED_WAKEUP	(1U << 0) /* needs io_uring_enter wakeup */
+#define IORING_SQ_CQ_OVERFLOW	(1U << 1) /* app needs to enter kernel */
 

Quoted 5 lines

include/uapi/linux/io_uring.h struct io_sqring_offsets { · line 204

Quoted 3 lines

include/uapi/linux/io_uring.h +0 -0

   struct io_cqring_offsets {
   	__u32 head;

Looks good, I think it'll work now.
I'll test and send patches soon, thanks.

One missing bit clear, and I corrected that last comment. Just base on this one, thanks!

fs/io_uring.c +7 -2

index 4c9a494c9f9f..0b6a4b2d7e76 100644
@@ -1342,6 +1342,7 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
  	if (cqe) {
  		clear_bit(0, &ctx->sq_check_overflow);
  		clear_bit(0, &ctx->cq_check_overflow);
+		ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
  	}
  	spin_unlock_irqrestore(&ctx->completion_lock, flags);
  	io_cqring_ev_posted(ctx);

@@ -1379,6 +1380,7 @@ static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)
  		if (list_empty(&ctx->cq_overflow_list)) {
  			set_bit(0, &ctx->sq_check_overflow);
  			set_bit(0, &ctx->cq_check_overflow);
+			ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;
  		}
  		req->flags |= REQ_F_OVERFLOW;
  		refcount_inc(&req->refs);

@@ -6375,9 +6377,9 @@ static int io_sq_thread(void *data)
  			}
 
  			/* Tell userspace we may need a wakeup call */
+			spin_lock_irq(&ctx->completion_lock);
  			ctx->rings->sq_flags |= IORING_SQ_NEED_WAKEUP;
-			/* make sure to read SQ tail after writing flags */
-			smp_mb();
+			spin_unlock_irq(&ctx->completion_lock);
 
  			to_submit = io_sqring_entries(ctx);
  			if (!to_submit || ret == -EBUSY) {

@@ -6400,7 +6402,9 @@ static int io_sq_thread(void *data)
  			}
  			finish_wait(&ctx->sqo_wait, &wait);
 
+			spin_lock_irq(&ctx->completion_lock);
  			ctx->rings->sq_flags &= ~IORING_SQ_NEED_WAKEUP;
+			spin_unlock_irq(&ctx->completion_lock);
  		}
 
  		mutex_lock(&ctx->uring_lock);

@@ -7810,6 +7814,7 @@ static void io_uring_cancel_files(struct io_ring_ctx *ctx,
  			if (list_empty(&ctx->cq_overflow_list)) {
  				clear_bit(0, &ctx->sq_check_overflow);
  				clear_bit(0, &ctx->cq_check_overflow);
+				ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
  			}
  			spin_unlock_irq(&ctx->completion_lock);
 

include/uapi/linux/io_uring.h +1 -0

index 8d033961cb78..d65fde732518 100644
@@ -198,6 +198,7 @@ struct io_sqring_offsets {
   * sq_ring->flags
   */
  #define IORING_SQ_NEED_WAKEUP	(1U << 0) /* needs io_uring_enter wakeup */
+#define IORING_SQ_CQ_OVERFLOW	(1U << 1) /* CQ ring is overflown */
 
  struct io_cqring_offsets {
  	__u32 head;

hi,

Quoted 95 lines · Xiaoguang Wang

Quoted 94 lines

hi,

Quoted 92 lines · Xiaoguang Wang

Quoted 68 lines

I modify above test program a bit:
#include <errno.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <fcntl.h>
#include <assert.h>

#include "liburing.h"

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;
           int i;

           for (i = 0; i < 33; i++) {
                   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++;
           }

           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 %d\n", issued, IO_URING_READ_ONCE(*ring->sq.kflags));
                           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;
}

Quoted 2 lines

Though with your patches applied, we still can not peek the last cqe. This test program will only issue 33 sqes, so it won't get EBUSY error.

How about we make this even simpler, then - make the IORING_SQ_CQ_OVERFLOW actually track the state, rather than when we fail on submission. The liburing change would be the same, the kernel side would then look like the below.

fs/io_uring.c +1 -0

index 4c9a494c9f9f..01981926cdf4 100644
@@ -1342,6 +1342,7 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
   	if (cqe) {
   		clear_bit(0, &ctx->sq_check_overflow);
   		clear_bit(0, &ctx->cq_check_overflow);
+		ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
   	}
   	spin_unlock_irqrestore(&ctx->completion_lock, flags);
   	io_cqring_ev_posted(ctx);

@@ -1379,6 +1380,7 @@ static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)
   		if (list_empty(&ctx->cq_overflow_list)) {

Quoted 3 lines

Quoted 3 lines

fs/io_uring.c static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags) · line 1383

Quoted 3 lines

fs/io_uring.c +1 -0

   			set_bit(0, &ctx->sq_check_overflow);
   			set_bit(0, &ctx->cq_check_overflow);
+			ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;

Some callers to __io_cqring_fill_event() don't hold completion_lock, for example:
==> io_iopoll_complete
====> __io_cqring_fill_event()
So this patch maybe still not safe when SQPOLL is enabled.
Do you perfer adding a new lock or just do completion_lock here only when cq ring is overflowed?

Quoted 97 lines

hi,

Quoted 95 lines · Xiaoguang Wang

Quoted 94 lines

hi,

Quoted 92 lines · Xiaoguang Wang

Quoted 68 lines

I modify above test program a bit:
#include <errno.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <string.h>
#include <fcntl.h>
#include <assert.h>

#include "liburing.h"

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;
            int i;

            for (i = 0; i < 33; i++) {
                    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++;
            }

            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 %d\n", issued, IO_URING_READ_ONCE(*ring->sq.kflags));
                            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;
}

Quoted 2 lines

Though with your patches applied, we still can not peek the last cqe. This test program will only issue 33 sqes, so it won't get EBUSY error.

How about we make this even simpler, then - make the IORING_SQ_CQ_OVERFLOW actually track the state, rather than when we fail on submission. The liburing change would be the same, the kernel side would then look like the below.

fs/io_uring.c +1 -0

index 4c9a494c9f9f..01981926cdf4 100644
@@ -1342,6 +1342,7 @@ static bool io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
    	if (cqe) {
    		clear_bit(0, &ctx->sq_check_overflow);
    		clear_bit(0, &ctx->cq_check_overflow);
+		ctx->rings->sq_flags &= ~IORING_SQ_CQ_OVERFLOW;
    	}
    	spin_unlock_irqrestore(&ctx->completion_lock, flags);
    	io_cqring_ev_posted(ctx);

@@ -1379,6 +1380,7 @@ static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags)
    		if (list_empty(&ctx->cq_overflow_list)) {

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

fs/io_uring.c static void __io_cqring_fill_event(struct io_kiocb *req, long res, long cflags) · line 1383

Quoted 3 lines

fs/io_uring.c +1 -0

    			set_bit(0, &ctx->sq_check_overflow);
    			set_bit(0, &ctx->cq_check_overflow);
+			ctx->rings->sq_flags |= IORING_SQ_CQ_OVERFLOW;

Some callers to __io_cqring_fill_event() don't hold completion_lock, for example:
==> io_iopoll_complete
====> __io_cqring_fill_event()
So this patch maybe still not safe when SQPOLL is enabled.
Do you perfer adding a new lock or just do completion_lock here only when cq ring is overflowed?

The polled side isn't IRQ driven, so should be serialized separately. This works because we don't allow non-polled IO on a polled context, and vice versa. If not, we'd have bigger issues than just the flags modification.

So it should be fine as-is.

Quoted 63 lines

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;
}

Quoted 3 lines

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

fs/io_uring.c +11 -1

index d37d7ea5ebe5..d409bd68553f 100644
@@ -6110,8 +6110,18 @@ static int io_sq_thread(void *data)
  		}
 
  		mutex_lock(&ctx->uring_lock);
-		if (likely(!percpu_ref_is_dying(&ctx->refs)))
+		if (likely(!percpu_ref_is_dying(&ctx->refs))) {
+retry:
  			ret = io_submit_sqes(ctx, to_submit, NULL, -1);
+			if (unlikely(ret == -EBUSY)) {
+				ctx->rings->sq_flags |= IORING_SQ_NEED_WAKEUP;
+				smp_mb();
+				if (io_cqring_overflow_flush(ctx, false)) {
+					ctx->rings->sq_flags &= ~IORING_SQ_NEED_WAKEUP;
+					goto retry;
+				}
+			}
+		}
  		mutex_unlock(&ctx->uring_lock);
  		timeout = jiffies + ctx->sq_thread_idle;
  	}

hi,

Quoted 68 lines · Xiaoguang Wang

Quoted 66 lines

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.

Quoted 2 lines

Is there any way we can accomplish the same without exporting another set of flags?

I understand your concerns and will try to find some better methods later, but not sure there're some better :)