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

[Jens Axboe <axboe@kernel.dk>]

On 3/4/20 6:27 AM, Xiaoguang Wang wrote:
> hi，
> 
>> 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
> In this patch, seesms that you forgot to delete:
>      if (wait_nr)
>          wait_nr = 0;
> 
> With these two codes removed, my original test case still produces the same amount
> of io_uring_enter syscalls, so you can just remove them safely.

Ah indeed, thanks for letting me know! Killed those two lines.

-- 
Jens Axboe