Re: [PATCH 2/2] io_uring: kbuf: add comments for some tricky code

[Jens Axboe <axboe@kernel.dk>]

On 6/14/22 6:01 AM, Hao Xu wrote:
> From: Hao Xu <howeyxu@tencent.com>
> 
> Add comments to explain why it is always under uring lock when
> incrementing head in __io_kbuf_recycle. And rectify one comemnt about
> kbuf consuming in iowq case.

Was there a 1/2 patch in this series? This one has a subject of 2/2...

> Signed-off-by: Hao Xu <howeyxu@tencent.com>
> ---
>  io_uring/kbuf.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
> index 9cdbc018fd64..37f06456bf30 100644
> --- a/io_uring/kbuf.c
> +++ b/io_uring/kbuf.c
> @@ -50,6 +50,13 @@ void __io_kbuf_recycle(struct io_kiocb *req, unsigned issue_flags)
>  	if (req->flags & REQ_F_BUFFER_RING) {
>  		if (req->buf_list) {
>  			if (req->flags & REQ_F_PARTIAL_IO) {
> +				/*
> +				 * if we reach here, uring_lock has been
> +				?* holden. Because in iowq, we already
> +				?* cleared req->buf_list to NULL when got
> +				?* the buffer from the ring, which means
> +				?* we cannot be here in that case.
> +				 */

There's a weird character before the '*' in most lines? I'd rephrase the
above as:

If we end up here, then the io_uring_lock has been kept held since we
retrieved the buffer. For the io-wq case, we already cleared
req->buf_list when the buffer was retrieved, hence it cannot be set
here for that case.

And make sure it lines up around 80 chars, your lines look very short.

> @@ -128,12 +135,13 @@ static void __user *io_ring_buffer_select(struct io_kiocb *req, size_t *len,
>  	if (issue_flags & IO_URING_F_UNLOCKED) {
>  		/*
>  		 * If we came in unlocked, we have no choice but to consume the
> -		 * buffer here. This does mean it'll be pinned until the IO
> -		 * completes. But coming in unlocked means we're in io-wq
> -		 * context, hence there should be no further retry. For the
> -		 * locked case, the caller must ensure to call the commit when
> -		 * the transfer completes (or if we get -EAGAIN and must poll
> -		 * or retry).
> +		 * buffer here otherwise nothing ensures the buffer not being
> +		 * used by others. This does mean it'll be pinned until the IO
> +		 * completes though coming in unlocked means we're in io-wq
> +		 * context and there may be further retries in async hybrid mode.
> +		 * For the locked case, the caller must ensure to call the commit
> +		 * when the transfer completes (or if we get -EAGAIN and must
> +		 * poll or retry).

and similarly:

buffer here, otherwise nothing ensures that the buffer won't get used by
others. This does mean it'll be pinned until the IO completes, coming in
unlocked means we're being called from io-wq context and there may be
further retries in async hybrid mode. For the locked case, the caller
must call commit when the transfer completes (or if we get -EAGAIN and
must poll of retry).

-- 
Jens Axboe