Re: [PATCH v10 2/5] io-uring: add napi busy poll support

[Stefan Roesch <shr@devkernel.io>]

Jens Axboe <axboe@kernel.dk> writes:

> On 4/26/23 7:50?PM, Jens Axboe wrote:
>> On 4/26/23 7:41?PM, Jens Axboe wrote:
>>>> +static void io_napi_multi_busy_loop(struct list_head *napi_list,
>>>> +		struct io_wait_queue *iowq)
>>>> +{
>>>> +	unsigned long start_time = busy_loop_current_time();
>>>> +
>>>> +	do {
>>>> +		if (list_is_singular(napi_list))
>>>> +			break;
>>>> +		if (!__io_napi_busy_loop(napi_list, iowq->napi_prefer_busy_poll))
>>>> +			break;
>>>> +	} while (!io_napi_busy_loop_should_end(iowq, start_time));
>>>> +}
>>>
>>> Do we need to check for an empty list here?
>>>
>>>> +static void io_napi_blocking_busy_loop(struct list_head *napi_list,
>>>> +		struct io_wait_queue *iowq)
>>>> +{
>>>> +	if (!list_is_singular(napi_list))
>>>> +		io_napi_multi_busy_loop(napi_list, iowq);
>>>> +
>>>> +	if (list_is_singular(napi_list)) {
>>>> +		struct io_napi_ht_entry *ne;
>>>> +
>>>> +		ne = list_first_entry(napi_list, struct io_napi_ht_entry, list);
>>>> +		napi_busy_loop(ne->napi_id, io_napi_busy_loop_should_end, iowq,
>>>> +			iowq->napi_prefer_busy_poll, BUSY_POLL_BUDGET);
>>>> +	}
>>>> +}
>>>
>>> Presumably io_napi_multi_busy_loop() can change the state of the list,
>>> which is why we have if (cond) and then if (!cond) here? Would probably
>>> warrant a comment as it looks a bit confusing.
>>
>> Doesn't look like that's the case? We just call into
>> io_napi_multi_busy_loop() -> napi_busy_loop() which doesn't touch it. So
>> the state should be the same?
>>
>> We also check if the list isn't singular before we call it, and then
>> io_napi_multi_busy_loop() breaks out of the loop if it is. And we know
>> it's not singular when calling, and I don't see what changes it.
>>
>> Unless I'm missing something, which is quite possible, this looks overly
>> convoluted and has extra pointless checks?
>
> All the cleanups/fixes I ended up doing are below. Not all for this
> patch probably, just for the series overall. Not tested at all, so
> please just go over them and see what makes sense and let me know which
> hunks you don't agree with.
>
>
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index a4c9a404f631..390f54c546d6 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -2617,29 +2617,17 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
>  	iowq.cq_tail = READ_ONCE(ctx->rings->cq.head) + min_events;
>  	iowq.timeout = KTIME_MAX;
>
> -	if (!io_napi(ctx)) {
> -		if (uts) {
> -			struct timespec64 ts;
> +	if (uts) {
> +		struct timespec64 ts;
>
> -			if (get_timespec64(&ts, uts))
> -				return -EFAULT;
> -			iowq.timeout = ktime_add_ns(timespec64_to_ktime(ts), ktime_get_ns());
> -		}
> -	} else {
> -		if (uts) {
> -			struct timespec64 ts;
> -
> -			if (get_timespec64(&ts, uts))
> -				return -EFAULT;
> -
> -			io_napi_adjust_timeout(ctx, &iowq, &ts);
> -			iowq.timeout = ktime_add_ns(timespec64_to_ktime(ts), ktime_get_ns());
> -		} else {
> -			io_napi_adjust_timeout(ctx, &iowq, NULL);
> -		}
> -		io_napi_busy_loop(ctx, &iowq);
> +		if (get_timespec64(&ts, uts))
> +			return -EFAULT;
> +		iowq.timeout = ktime_add_ns(timespec64_to_ktime(ts), ktime_get_ns());
> +		io_napi_adjust_timeout(ctx, &iowq, &ts);
>  	}
>
> +	io_napi_busy_loop(ctx, &iowq);
> +
>  	trace_io_uring_cqring_wait(ctx, min_events);
>
>  	do {
> diff --git a/io_uring/napi.c b/io_uring/napi.c
> index ca12ff5f5611..50b2bdb10417 100644
> --- a/io_uring/napi.c
> +++ b/io_uring/napi.c
> @@ -60,8 +60,8 @@ void __io_napi_add(struct io_ring_ctx *ctx, struct file *file)
>  	spin_unlock(&ctx->napi_lock);
>  }
>
> -static inline void adjust_timeout(unsigned int poll_to, struct timespec64 *ts,
> -		unsigned int *new_poll_to)
> +static void adjust_timeout(unsigned int poll_to, struct timespec64 *ts,
> +			  unsigned int *new_poll_to)
>  {
>  	struct timespec64 pollto = ns_to_timespec64(1000 * (s64)poll_to);
>
> @@ -95,12 +95,17 @@ static bool io_napi_busy_loop_should_end(void *p, unsigned long start_time)
>  {
>  	struct io_wait_queue *iowq = p;
>
> -	return signal_pending(current) ||
> -	       io_should_wake(iowq) ||
> -	       io_napi_busy_loop_timeout(start_time, iowq->napi_busy_poll_to);
> +	if (signal_pending(current))
> +		return true;
> +	if (io_should_wake(iowq))
> +		return true;
> +	if (io_napi_busy_loop_timeout(start_time, iowq->napi_busy_poll_to))
> +		return true;
> +	return false;
>  }
>
> -static bool __io_napi_busy_loop(struct list_head *napi_list, bool prefer_busy_poll)
> +static bool __io_napi_do_busy_loop(struct list_head *napi_list,
> +				   bool prefer_busy_poll)
>  {
>  	struct io_napi_ht_entry *e;
>  	struct io_napi_ht_entry *n;
> @@ -113,38 +118,35 @@ static bool __io_napi_busy_loop(struct list_head *napi_list, bool prefer_busy_po
>  	return !list_empty(napi_list);
>  }
>
> -static void io_napi_multi_busy_loop(struct list_head *napi_list,
> -		struct io_wait_queue *iowq)
> +static void io_napi_multi_busy_loop(struct list_head *list,
> +				   struct io_wait_queue *iowq)
>  {
>  	unsigned long start_time = busy_loop_current_time();
>
>  	do {
> -		if (list_is_singular(napi_list))
> -			break;
> -		if (!__io_napi_busy_loop(napi_list, iowq->napi_prefer_busy_poll))
> +		if (!__io_napi_do_busy_loop(list, iowq->napi_prefer_busy_poll))
>  			break;
>  	} while (!io_napi_busy_loop_should_end(iowq, start_time));
>  }
>
>  static void io_napi_blocking_busy_loop(struct list_head *napi_list,
> -		struct io_wait_queue *iowq)
> +				       struct io_wait_queue *iowq)
>  {
> -	if (!list_is_singular(napi_list))
> +	if (!list_is_singular(napi_list)) {
>  		io_napi_multi_busy_loop(napi_list, iowq);
> -
> -	if (list_is_singular(napi_list)) {
> +	} else {
>  		struct io_napi_ht_entry *ne;
>
>  		ne = list_first_entry(napi_list, struct io_napi_ht_entry, list);
>  		napi_busy_loop(ne->napi_id, io_napi_busy_loop_should_end, iowq,
> -			iowq->napi_prefer_busy_poll, BUSY_POLL_BUDGET);
> +				iowq->napi_prefer_busy_poll, BUSY_POLL_BUDGET);
>  	}
>  }
>
>  static void io_napi_remove_stale(struct io_ring_ctx *ctx)
>  {
> -	unsigned int i;
>  	struct io_napi_ht_entry *he;
> +	unsigned int i;
>
>  	hash_for_each(ctx->napi_ht, i, he, node) {
>  		if (time_after(jiffies, he->timeout)) {
> @@ -152,11 +154,10 @@ static void io_napi_remove_stale(struct io_ring_ctx *ctx)
>  			hash_del(&he->node);
>  		}
>  	}
> -
>  }
>
>  static void io_napi_merge_lists(struct io_ring_ctx *ctx,
> -		struct list_head *napi_list)
> +				struct list_head *napi_list)
>  {
>  	spin_lock(&ctx->napi_lock);
>  	list_splice(napi_list, &ctx->napi_list);
> @@ -186,9 +187,9 @@ void io_napi_init(struct io_ring_ctx *ctx)
>   */
>  void io_napi_free(struct io_ring_ctx *ctx)
>  {
> -	unsigned int i;
>  	struct io_napi_ht_entry *he;
>  	LIST_HEAD(napi_list);
> +	unsigned int i;
>
>  	spin_lock(&ctx->napi_lock);
>  	hash_for_each(ctx->napi_ht, i, he, node)
> @@ -206,8 +207,8 @@ void io_napi_free(struct io_ring_ctx *ctx)
>  int io_register_napi(struct io_ring_ctx *ctx, void __user *arg)
>  {
>  	const struct io_uring_napi curr = {
> -		.busy_poll_to = ctx->napi_busy_poll_to,
> -		.prefer_busy_poll = ctx->napi_prefer_busy_poll
> +		.busy_poll_to		= ctx->napi_busy_poll_to,
> +		.prefer_busy_poll	= ctx->napi_prefer_busy_poll
>  	};
>  	struct io_uring_napi napi;
>
> @@ -236,14 +237,12 @@ int io_register_napi(struct io_ring_ctx *ctx, void __user *arg)
>  int io_unregister_napi(struct io_ring_ctx *ctx, void __user *arg)
>  {
>  	const struct io_uring_napi curr = {
> -		.busy_poll_to = ctx->napi_busy_poll_to,
> -		.prefer_busy_poll = ctx->napi_prefer_busy_poll
> +		.busy_poll_to		= ctx->napi_busy_poll_to,
> +		.prefer_busy_poll	= ctx->napi_prefer_busy_poll
>  	};
>
> -	if (arg) {
> -		if (copy_to_user(arg, &curr, sizeof(curr)))
> -			return -EFAULT;
> -	}
> +	if (arg && copy_to_user(arg, &curr, sizeof(curr)))
> +		return -EFAULT;
>
>  	WRITE_ONCE(ctx->napi_busy_poll_to, 0);
>  	WRITE_ONCE(ctx->napi_prefer_busy_poll, false);
> @@ -251,31 +250,36 @@ int io_unregister_napi(struct io_ring_ctx *ctx, void __user *arg)
>  }
>
>  /*
> - * io_napi_adjust_timeout() - Add napi id to the busy poll list
> + * __io_napi_adjust_timeout() - Add napi id to the busy poll list
>   * @ctx: pointer to io-uring context structure
>   * @iowq: pointer to io wait queue
>   * @ts: pointer to timespec or NULL
>   *
>   * Adjust the busy loop timeout according to timespec and busy poll timeout.
>   */
> -void io_napi_adjust_timeout(struct io_ring_ctx *ctx, struct io_wait_queue *iowq,
> -		struct timespec64 *ts)
> +void __io_napi_adjust_timeout(struct io_ring_ctx *ctx,
> +			      struct io_wait_queue *iowq, struct timespec64 *ts)
>  {
> +	unsigned int poll_to;
> +
> +	if (!io_napi(ctx))
> +		return;
> +
> +	poll_to = READ_ONCE(ctx->napi_busy_poll_to);
>  	if (ts)
> -		adjust_timeout(READ_ONCE(ctx->napi_busy_poll_to), ts,
> -			&iowq->napi_busy_poll_to);
> +		adjust_timeout(poll_to, ts, &iowq->napi_busy_poll_to);
>  	else
> -		iowq->napi_busy_poll_to = READ_ONCE(ctx->napi_busy_poll_to);
> +		iowq->napi_busy_poll_to = poll_to;
>  }
>
>  /*
> - * io_napi_busy_loop() - execute busy poll loop
> + * __io_napi_busy_loop() - execute busy poll loop
>   * @ctx: pointer to io-uring context structure
>   * @iowq: pointer to io wait queue
>   *
>   * Execute the busy poll loop and merge the spliced off list.
>   */
> -void io_napi_busy_loop(struct io_ring_ctx *ctx, struct io_wait_queue *iowq)
> +void __io_napi_busy_loop(struct io_ring_ctx *ctx, struct io_wait_queue *iowq)
>  {
>  	iowq->napi_prefer_busy_poll = READ_ONCE(ctx->napi_prefer_busy_poll);
>
> @@ -302,8 +306,8 @@ void io_napi_busy_loop(struct io_ring_ctx *ctx, struct io_wait_queue *iowq)
>   */
>  int io_napi_sqpoll_busy_poll(struct io_ring_ctx *ctx)
>  {
> -	int ret = 0;
>  	LIST_HEAD(napi_list);
> +	int ret;
>
>  	if (!READ_ONCE(ctx->napi_busy_poll_to))
>  		return 0;
> @@ -312,9 +316,7 @@ int io_napi_sqpoll_busy_poll(struct io_ring_ctx *ctx)
>  	list_splice_init(&ctx->napi_list, &napi_list);
>  	spin_unlock(&ctx->napi_lock);
>
> -	if (__io_napi_busy_loop(&napi_list, ctx->napi_prefer_busy_poll))
> -		ret = 1;
> -
> +	ret = __io_napi_do_busy_loop(&napi_list, ctx->napi_prefer_busy_poll);
>  	io_napi_merge_lists(ctx, &napi_list);
>  	return ret;
>  }
> diff --git a/io_uring/napi.h b/io_uring/napi.h
> index 8da8f032a441..b5e93b3777c0 100644
> --- a/io_uring/napi.h
> +++ b/io_uring/napi.h
> @@ -17,9 +17,9 @@ int io_unregister_napi(struct io_ring_ctx *ctx, void __user *arg);
>
>  void __io_napi_add(struct io_ring_ctx *ctx, struct file *file);
>
> -void io_napi_adjust_timeout(struct io_ring_ctx *ctx,
> +void __io_napi_adjust_timeout(struct io_ring_ctx *ctx,
>  		struct io_wait_queue *iowq, struct timespec64 *ts);
> -void io_napi_busy_loop(struct io_ring_ctx *ctx, struct io_wait_queue *iowq);
> +void __io_napi_busy_loop(struct io_ring_ctx *ctx, struct io_wait_queue *iowq);
>  int io_napi_sqpoll_busy_poll(struct io_ring_ctx *ctx);
>
>  static inline bool io_napi(struct io_ring_ctx *ctx)
> @@ -27,6 +27,23 @@ static inline bool io_napi(struct io_ring_ctx *ctx)
>  	return !list_empty(&ctx->napi_list);
>  }
>
> +static inline void io_napi_adjust_timeout(struct io_ring_ctx *ctx,
> +					  struct io_wait_queue *iowq,
> +					  struct timespec64 *ts)
> +{
> +	if (!io_napi(ctx))
> +		return;
> +	__io_napi_adjust_timeout(ctx, iowq, ts);
> +}
> +
> +static inline void io_napi_busy_loop(struct io_ring_ctx *ctx,
> +				     struct io_wait_queue *iowq)
> +{
> +	if (!io_napi(ctx))
> +		return;
> +	__io_napi_busy_loop(ctx, iowq);
> +}
> +
>  /*
>   * io_napi_add() - Add napi id to the busy poll list
>   * @req: pointer to io_kiocb request

Looks good to me, only difference is to callapse
__io_napi_adjust_timeout and adjust_timeout in one function.