Re: [PATCH 2/3] io_uring: put flag checking for needing req cleanup in one spot

[Pavel Begunkov <asml.silence@gmail.com>]

On 16/04/2021 02:25, Jens Axboe wrote:
> We have this in two spots right now, which is a bit fragile. In
> preparation for moving REQ_F_POLLED cleanup into the same spot, move
> the check into io_clean_op() itself so we only have it once.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  fs/io_uring.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 87ce3dbcd4ca..a668d6a3319c 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1601,8 +1601,7 @@ static void io_req_complete_post(struct io_kiocb *req, long res,
>  static void io_req_complete_state(struct io_kiocb *req, long res,
>  				  unsigned int cflags)
>  {
> -	if (req->flags & (REQ_F_NEED_CLEANUP | REQ_F_BUFFER_SELECTED))
> -		io_clean_op(req);
> +	io_clean_op(req);
>  	req->result = res;
>  	req->compl.cflags = cflags;
>  	req->flags |= REQ_F_COMPLETE_INLINE;
> @@ -1713,16 +1712,12 @@ static void io_dismantle_req(struct io_kiocb *req)
>  
>  	if (!(flags & REQ_F_FIXED_FILE))
>  		io_put_file(req->file);
> -	if (flags & (REQ_F_NEED_CLEANUP | REQ_F_BUFFER_SELECTED |
> -		     REQ_F_INFLIGHT)) {
> -		io_clean_op(req);
> +	io_clean_op(req);
> +	if (req->flags & REQ_F_INFLIGHT) {
> +		struct io_uring_task *tctx = req->task->io_uring;

Not in particular happy about it.
1. adds extra if
2. adds extra function call
3. extra memory load in that function call.

Pushes us back in terms of performance. I'd suggest to have
a helper, which is pretty much optimisable and may be coalesced by a compiler with
adjacent flag checks.

static inline bool io_need_cleanup(unsigned flags)
{
	return flags & (REQ_F_NEED_CLEANUP | REQ_F_BUFFER_SELECTED);
}

if (io_need_cleanup(flags) || (flags & INFLIGHT)) {
    io_clean_op();
    if (INFLIGHT) {}
}



>  
> -		if (req->flags & REQ_F_INFLIGHT) {
> -			struct io_uring_task *tctx = req->task->io_uring;
> -
> -			atomic_dec(&tctx->inflight_tracked);
> -			req->flags &= ~REQ_F_INFLIGHT;
> -		}
> +		atomic_dec(&tctx->inflight_tracked);
> +		req->flags &= ~REQ_F_INFLIGHT;
>  	}
>  	if (req->fixed_rsrc_refs)
>  		percpu_ref_put(req->fixed_rsrc_refs);
> @@ -5995,6 +5990,8 @@ static int io_req_defer(struct io_kiocb *req)
>  
>  static void io_clean_op(struct io_kiocb *req)
>  {
> +	if (!(req->flags & (REQ_F_BUFFER_SELECTED | REQ_F_NEED_CLEANUP)))
> +		return;
>  	if (req->flags & REQ_F_BUFFER_SELECTED) {
>  		switch (req->opcode) {
>  		case IORING_OP_READV:
> 

-- 
Pavel Begunkov