public inbox for [email protected]
 help / color / mirror / Atom feed
From: Jens Axboe <[email protected]>
To: Hao Xu <[email protected]>, [email protected]
Cc: Pavel Begunkov <[email protected]>, [email protected]
Subject: Re: [PATCH 5/5] io_uring: implement multishot mode for accept
Date: Fri, 6 May 2022 08:42:34 -0600	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 5/6/22 1:01 AM, Hao Xu wrote:
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 0a83ecc457d1..9febe7774dc3 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1254,6 +1254,7 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
>  static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
>  static void io_eventfd_signal(struct io_ring_ctx *ctx);
>  static void io_req_tw_post_queue(struct io_kiocb *req, s32 res, u32 cflags);
> +static void io_poll_remove_entries(struct io_kiocb *req);
>  
>  static struct kmem_cache *req_cachep;
>  
> @@ -5690,24 +5691,29 @@ static int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>  static int io_accept_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  {
>  	struct io_accept *accept = &req->accept;
> +	bool multishot;
>  
>  	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
>  		return -EINVAL;
> -	if (sqe->ioprio || sqe->len || sqe->buf_index)
> +	if (sqe->len || sqe->buf_index)
>  		return -EINVAL;
>  
>  	accept->addr = u64_to_user_ptr(READ_ONCE(sqe->addr));
>  	accept->addr_len = u64_to_user_ptr(READ_ONCE(sqe->addr2));
>  	accept->flags = READ_ONCE(sqe->accept_flags);
>  	accept->nofile = rlimit(RLIMIT_NOFILE);
> +	multishot = !!(READ_ONCE(sqe->ioprio) & IORING_ACCEPT_MULTISHOT);

I tend to like:

	multishot = READ_ONCE(sqe->ioprio) & IORING_ACCEPT_MULTISHOT) != 0;

as I think it's more readable. But I think we really want it ala:

	u16 poll_flags;

	poll_flags = READ_ONCE(sqe->ioprio);
	if (poll_flags & ~IORING_ACCEPT_MULTISHOT)
		return -EINVAL;

	...

to ensure that we can add more flags later, hence only accepting this
single flag right now.

Do we need REQ_F_APOLL_MULTI_POLLED, or can we just store whether this
is a multishot request in struct io_accept?

> @@ -5760,7 +5774,35 @@ static int io_accept(struct io_kiocb *req, unsigned int issue_flags)
>  		ret = io_install_fixed_file(req, file, issue_flags,
>  					    accept->file_slot - 1);
>  	}
> -	__io_req_complete(req, issue_flags, ret, 0);
> +
> +	if (req->flags & REQ_F_APOLL_MULTISHOT) {
> +		if (ret >= 0) {
> +			bool filled;
> +
> +			spin_lock(&ctx->completion_lock);
> +			filled = io_fill_cqe_aux(ctx, req->cqe.user_data, ret,
> +						 IORING_CQE_F_MORE);
> +			io_commit_cqring(ctx);
> +			spin_unlock(&ctx->completion_lock);
> +			if (unlikely(!filled)) {
> +				io_poll_clean(req);
> +				return -ECANCELED;
> +			}
> +			io_cqring_ev_posted(ctx);
> +			goto retry;
> +		} else {
> +			/*
> +			 * the apoll multishot req should handle poll
> +			 * cancellation by itself since the upper layer
> +			 * who called io_queue_sqe() cannot get errors
> +			 * happened here.
> +			 */
> +			io_poll_clean(req);
> +			return ret;
> +		}
> +	} else {
> +		__io_req_complete(req, issue_flags, ret, 0);
> +	}
>  	return 0;
>  }

I'd probably just make that:

	if (!(req->flags & REQ_F_APOLL_MULTISHOT)) {
		__io_req_complete(req, issue_flags, ret, 0);
		return 0;
	}
	if (ret >= 0) {
		bool filled;

		spin_lock(&ctx->completion_lock);
		filled = io_fill_cqe_aux(ctx, req->cqe.user_data, ret,
					 IORING_CQE_F_MORE);
		io_commit_cqring(ctx);
		spin_unlock(&ctx->completion_lock);
		if (filled) {
			io_cqring_ev_posted(ctx);
			goto retry;
		}
		/* fall through to error case */
		ret = -ECANCELED;
	}

	/*
	 * the apoll multishot req should handle poll
	 * cancellation by itself since the upper layer
	 * who called io_queue_sqe() cannot get errors
	 * happened here.
	 */
	io_poll_clean(req);
	return ret;

which I think is a lot easier to read and keeps the indentation at a
manageable level and reduces duplicate code.

-- 
Jens Axboe


  reply	other threads:[~2022-05-06 14:42 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-06  7:00 [PATCH v2 0/5] fast poll multishot mode Hao Xu
2022-05-06  7:00 ` [PATCH 1/5] io_uring: add IORING_ACCEPT_MULTISHOT for accept Hao Xu
2022-05-06 14:32   ` Jens Axboe
2022-05-07  4:05     ` Hao Xu
2022-05-06  7:00 ` [PATCH 2/5] io_uring: add REQ_F_APOLL_MULTISHOT for requests Hao Xu
2022-05-06  7:01 ` [PATCH 3/5] io_uring: let fast poll support multishot Hao Xu
2022-05-06 17:19   ` Pavel Begunkov
2022-05-06 22:02     ` Jens Axboe
2022-05-07  6:32       ` Hao Xu
2022-05-07  9:26       ` Pavel Begunkov
2022-05-07  7:08     ` Hao Xu
2022-05-07  9:47       ` Pavel Begunkov
2022-05-07 11:06         ` Hao Xu
2022-05-06 18:02   ` kernel test robot
2022-05-06  7:01 ` [PATCH 4/5] io_uring: add a helper for poll clean Hao Xu
2022-05-06 11:04   ` kernel test robot
2022-05-06 12:47   ` kernel test robot
2022-05-06 14:36   ` Jens Axboe
2022-05-07  6:37     ` Hao Xu
2022-05-06 16:22   ` Pavel Begunkov
2022-05-07  6:43     ` Hao Xu
2022-05-07  9:29       ` Pavel Begunkov
2022-05-06  7:01 ` [PATCH 5/5] io_uring: implement multishot mode for accept Hao Xu
2022-05-06 14:42   ` Jens Axboe [this message]
2022-05-07  9:13     ` Hao Xu
2022-05-06 20:50   ` Jens Axboe
2022-05-06 21:29     ` Jens Axboe
2022-05-06  7:36 ` [PATCH v2 0/5] fast poll multishot mode Hao Xu
2022-05-06 14:18   ` Jens Axboe
2022-05-06 16:01     ` Pavel Begunkov
2022-05-06 16:03       ` Jens Axboe
2022-05-06 22:23 ` Jens Axboe
2022-05-06 23:26   ` Jens Axboe
2022-05-07  2:33     ` Jens Axboe
2022-05-07  3:08       ` Jens Axboe
2022-05-07 16:01         ` Hao Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox