Re: [PATCH v2] io_uring: add timeout support for io_uring_enter()

[Jens Axboe <axboe@kernel.dk>]

On 8/4/20 3:28 AM, Jiufei Xue wrote:
> @@ -6569,7 +6578,14 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
>  		}
>  		if (io_should_wake(&iowq, false))
>  			break;
> -		schedule();
> +		if (uts) {
> +			if ((timeout = schedule_timeout(timeout)) == 0) {
> +				ret = -ETIME;
> +				break;
> +			}

Please don't combine lines, this is a lot more readable as:

timeout = schedule_timeout(timeout);
if (!timeout) {
	...
}

> @@ -7993,19 +8009,36 @@ static unsigned long io_uring_nommu_get_unmapped_area(struct file *file,
>  #endif /* !CONFIG_MMU */
>  
>  SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
> -		u32, min_complete, u32, flags, const sigset_t __user *, sig,
> +		u32, min_complete, u32, flags, const void __user *, argp,
>  		size_t, sigsz)
>  {
>  	struct io_ring_ctx *ctx;
>  	long ret = -EBADF;
>  	int submitted = 0;
>  	struct fd f;
> +	const sigset_t __user *sig;
> +	struct __kernel_timespec __user *ts;
> +	struct io_uring_getevents_arg arg;
>  
>  	io_run_task_work();
>  
> -	if (flags & ~(IORING_ENTER_GETEVENTS | IORING_ENTER_SQ_WAKEUP))
> +	if (flags & ~(IORING_ENTER_GETEVENTS | IORING_ENTER_SQ_WAKEUP |
> +		      IORING_ENTER_GETEVENTS_TIMEOUT))
>  		return -EINVAL;
>  
> +	/* deal with IORING_ENTER_GETEVENTS_TIMEOUT */
> +	if (flags & IORING_ENTER_GETEVENTS_TIMEOUT) {
> +		if (!(flags & IORING_ENTER_GETEVENTS))
> +			return -EINVAL;
> +		if (copy_from_user(&arg, argp, sizeof(arg)))
> +			return -EFAULT;
> +		sig = arg.sigmask;
> +		ts = arg.ts;
> +	} else {

I like the idea of the arg structure, but why don't we utilize the
size_t argument for that struct? That would allow flexibility on
potentially changing that structure in the future. You can use it for
versioning, basically. So I'd require that to be sizeof(arg), and the
above should then add:

	if (flags & IORING_ENTER_GETEVENTS_TIMEOUT) {
		if (!(flags & IORING_ENTER_GETEVENTS))
			return -EINVAL;
		if (sigsz != sizeof(arg))
			return -EINVAL;
		...

> @@ -290,4 +292,9 @@ struct io_uring_probe {
>  	struct io_uring_probe_op ops[0];
>  };
>  
> +struct io_uring_getevents_arg {
> +	sigset_t *sigmask;
> +	struct __kernel_timespec *ts;
> +};
> +

This structure isn't the same size on 32-bit and 64-bit, that should be
rectified. I'd make the sigmask a u64 type of some sort.

So little things - but please resend with the suggested changes and we
can move forward with this patch for 5.11.

-- 
Jens Axboe