On 2/5/21 5:46 AM, Pavel Begunkov wrote: > On 04/02/2021 16:50, Jens Axboe wrote: >> On 2/3/21 4:49 AM, Pavel Begunkov wrote: >>> On 02/02/2021 20:56, Pavel Begunkov wrote: >>>> On 02/02/2021 20:48, Jens Axboe wrote: >>>>> On 2/2/21 1:34 PM, Pavel Begunkov wrote: >>>>>> On 02/02/2021 17:41, Pavel Begunkov wrote: >>>>>>> On 02/02/2021 17:24, Jens Axboe wrote: >>>>>>>> On 2/2/21 10:10 AM, Victor Stewart wrote: >>>>>>>>>> Can you send the updated test app? >>>>>>>>> >>>>>>>>> https://gist.github.com/victorstewart/98814b65ed702c33480487c05b40eb56 >>>>>>>>> >>>>>>>>> same link i just updated the same gist >>>>>>>> >>>>>>>> And how are you running it? >>>>>>> >>>>>>> with SQPOLL with FIXED FLAG -> FAILURE: failed with error = ??? >>>>>>> -> io_uring_wait_cqe_timeout() strangely returns -1, (-EPERM??) >>>>>> >>>>>> Ok, _io_uring_get_cqe() is just screwed twice >>>>>> >>>>>> TL;DR >>>>>> we enter into it with submit=0, do an iteration, which decrements it, >>>>>> then a second iteration passes submit=-1, which is returned back by >>>>>> the kernel as a result and propagated back from liburing... >>>>> >>>>> Yep, that's what I came up with too. We really just need a clear way >>>>> of knowing when to break out, and when to keep going. Eg if we've >>>>> done a loop and don't end up calling the system call, then there's >>>>> no point in continuing. >>>> >>>> We can bodge something up (and forget about it), and do much cleaner >>>> for IORING_FEAT_EXT_ARG, because we don't have LIBURING_UDATA_TIMEOUT >>>> reqs for it and so can remove peek and so on. >>> >>> This version looks reasonably simple, and even passes tests and all >>> issues found by Victor's test. Didn't test it yet, but should behave >>> similarly in regard of internal timeouts (pre IORING_FEAT_EXT_ARG). >>> >>> static int _io_uring_get_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_ptr, >>> struct get_data *data) >>> { >>> struct io_uring_cqe *cqe = NULL; >>> int ret = 0, err; >>> >>> do { >>> unsigned flags = 0; >>> unsigned nr_available; >>> bool enter = false; >>> >>> err = __io_uring_peek_cqe(ring, &cqe, &nr_available); >>> if (err) >>> break; >>> >>> /* IOPOLL won't proceed when there're not reaped CQEs */ >>> if (cqe && (ring->flags & IORING_SETUP_IOPOLL)) >>> data->wait_nr = 0; >>> >>> if (data->wait_nr > nr_available || cq_ring_needs_flush(ring)) { >>> flags = IORING_ENTER_GETEVENTS | data->get_flags; >>> enter = true; >>> } >>> if (data->submit) { >>> sq_ring_needs_enter(ring, &flags); >>> enter = true; >>> } >>> if (!enter) >>> break; >>> >>> ret = __sys_io_uring_enter2(ring->ring_fd, data->submit, >>> data->wait_nr, flags, data->arg, >>> data->sz); >>> if (ret < 0) { >>> err = -errno; >>> break; >>> } >>> data->submit -= ret; >>> } while (1); >>> >>> *cqe_ptr = cqe; >>> return err; >>> } >> >> So here's my take on this - any rewrite of _io_uring_get_cqe() is going >> to end up adding special cases, that's unfortunately just the nature of >> the game. And since we're going to be doing a new liburing release very >> shortly, this isn't a great time to add a rewrite of it. It'll certainly >> introduce more bugs than it solves, and hence regressions, no matter how >> careful we are. >> >> Hence my suggestion is to just patch this in a trivial kind of fashion, >> even if it doesn't really make the function any prettier. But it'll be >> safer for a release, and then we can rework the function after. >> >> With that in mind, here's my suggestion. The premise is if we go through >> the loop and don't do io_uring_enter(), then there's no point in >> continuing. That's the trivial fix. > > Your idea but imho cleaner below. > +1 comment inline Shouldn't be hard, it was just a quick hack :-) >> diff --git a/src/queue.c b/src/queue.c >> index 94f791e..4161aa7 100644 >> --- a/src/queue.c >> +++ b/src/queue.c >> @@ -89,12 +89,13 @@ static int _io_uring_get_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_pt >> { >> struct io_uring_cqe *cqe = NULL; >> const int to_wait = data->wait_nr; >> - int ret = 0, err; >> + int err; >> >> do { >> bool cq_overflow_flush = false; >> unsigned flags = 0; >> unsigned nr_available; >> + int ret = -2; >> >> err = __io_uring_peek_cqe(ring, &cqe, &nr_available); >> if (err) >> @@ -117,7 +118,9 @@ static int _io_uring_get_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_pt >> ret = __sys_io_uring_enter2(ring->ring_fd, data->submit, >> data->wait_nr, flags, data->arg, >> data->sz); >> - if (ret < 0) { >> + if (ret == -2) { >> + break; > > peek/wait_cqe expect that cqe_ptr is filled on return=0. Looks we need > to return an error or hack up those functions. Right good point, we'd need -EAGAIN. >> + } else if (ret < 0) { >> err = -errno; >> } else if (ret == (int)data->submit) { >> data->submit = 0; >> > > > diff --git a/src/queue.c b/src/queue.c > index 94f791e..7d6f31d 100644 > --- a/src/queue.c > +++ b/src/queue.c > @@ -112,11 +112,15 @@ static int _io_uring_get_cqe(struct io_uring *ring, struct io_uring_cqe **cqe_pt > flags = IORING_ENTER_GETEVENTS | data->get_flags; > if (data->submit) > sq_ring_needs_enter(ring, &flags); > - if (data->wait_nr > nr_available || data->submit || > - cq_overflow_flush) > - ret = __sys_io_uring_enter2(ring->ring_fd, data->submit, > - data->wait_nr, flags, data->arg, > - data->sz); > + > + if (data->wait_nr <= nr_available && !data->submit && > + !cq_overflow_flush) { > + err = ?; which I guess is the actual error missing from here? > + break; > + } > + ret = __sys_io_uring_enter2(ring->ring_fd, data->submit, > + data->wait_nr, flags, data->arg, > + data->sz); > if (ret < 0) { > err = -errno; > } else if (ret == (int)data->submit) { > -- Jens Axboe
