Re: [PATCH] __io_uring_get_cqe: eliminate unnecessary io_uring_enter() syscalls

[Jens Axboe <axboe@kernel.dk>]

On 3/2/20 8:24 AM, Jens Axboe wrote:
> On 3/2/20 7:05 AM, Jens Axboe wrote:
>> On 3/1/20 9:18 PM, Xiaoguang Wang wrote:
>>> When user applis programming mode, like sumbit one sqe and wait its
>>> completion event, __io_uring_get_cqe() will result in many unnecessary
>>> syscalls, see below test program:
>>>
>>>     int main(int argc, char *argv[])
>>>     {
>>>             struct io_uring ring;
>>>             int fd, ret;
>>>             struct io_uring_sqe *sqe;
>>>             struct io_uring_cqe *cqe;
>>>             struct iovec iov;
>>>             off_t offset, filesize = 0;
>>>             void *buf;
>>>
>>>             if (argc < 2) {
>>>                     printf("%s: file\n", argv[0]);
>>>                     return 1;
>>>             }
>>>
>>>             ret = io_uring_queue_init(4, &ring, 0);
>>>             if (ret < 0) {
>>>                     fprintf(stderr, "queue_init: %s\n", strerror(-ret));
>>>                     return 1;
>>>             }
>>>
>>>             fd = open(argv[1], O_RDONLY | O_DIRECT);
>>>             if (fd < 0) {
>>>                     perror("open");
>>>                     return 1;
>>>             }
>>>
>>>             if (posix_memalign(&buf, 4096, 4096))
>>>                     return 1;
>>>             iov.iov_base = buf;
>>>             iov.iov_len = 4096;
>>>
>>>             offset = 0;
>>>             do {
>>>                     sqe = io_uring_get_sqe(&ring);
>>>                     if (!sqe) {
>>>                             printf("here\n");
>>>                             break;
>>>                     }
>>>                     io_uring_prep_readv(sqe, fd, &iov, 1, offset);
>>>
>>>                     ret = io_uring_submit(&ring);
>>>                     if (ret < 0) {
>>>                             fprintf(stderr, "io_uring_submit: %s\n", strerror(-ret));
>>>                             return 1;
>>>                     }
>>>
>>>                     ret = io_uring_wait_cqe(&ring, &cqe);
>>>                     if (ret < 0) {
>>>                             fprintf(stderr, "io_uring_wait_cqe: %s\n", strerror(-ret));
>>>                             return 1;
>>>                     }
>>>
>>>                     if (cqe->res <= 0) {
>>>                             if (cqe->res < 0) {
>>>                                     fprintf(stderr, "got eror: %d\n", cqe->res);
>>>                                     ret = 1;
>>>                             }
>>>                             io_uring_cqe_seen(&ring, cqe);
>>>                             break;
>>>                     }
>>>                     offset += cqe->res;
>>>                     filesize += cqe->res;
>>>                     io_uring_cqe_seen(&ring, cqe);
>>>             } while (1);
>>>
>>>             printf("filesize: %ld\n", filesize);
>>>             close(fd);
>>>             io_uring_queue_exit(&ring);
>>>             return 0;
>>>     }
>>>
>>> dd if=/dev/zero of=testfile bs=4096 count=16
>>> ./test  testfile
>>> and use bpftrace to trace io_uring_enter syscalls, in original codes,
>>> [lege@localhost ~]$ sudo bpftrace -e "tracepoint:syscalls:sys_enter_io_uring_enter {@c[tid] = count();}"
>>> Attaching 1 probe...
>>> @c[11184]: 49
>>> Above test issues 49 syscalls, it's counterintuitive. After looking
>>> into the codes, it's because __io_uring_get_cqe issue one more syscall,
>>> indded when __io_uring_get_cqe issues the first syscall, one cqe should
>>> already be ready, we don't need to wait again.
>>>
>>> To fix this issue, after the first syscall, set wait_nr to be zero, with
>>> tihs patch, bpftrace shows the number of io_uring_enter syscall is 33.
>>
>> Thanks, that's a nice fix, we definitely don't want to be doing
>> 50% more system calls than we have to...
> 
> Actually, don't think the fix is quite safe. For one, if we get an error
> on the __io_uring_enter(), then we may not have waited for entries. Or if
> we submitted less than we thought we would, we would not have waited
> either. So we need to check for full success before deeming it safe to
> clear wait_nr.

Unrelated fix:

https://git.kernel.dk/cgit/liburing/commit/?id=0edcef5700fd558d2548532e0e5db26cb74d19ca

and then a fix for your patch on top:

https://git.kernel.dk/cgit/liburing/commit/?id=dc14e30a086082b6aebc3130948e2453e3bd3b2a

Can you double check that your original test case still produces the
same amount of system calls with the fix in place?

-- 
Jens Axboe