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

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

[Hao Xu]

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.

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.
+				 */
 				req->buf_list->head++;
 				req->buf_list = NULL;
 			} else {
@@ -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).
 		 */
 		req->buf_list = NULL;
 		bl->head++;
-- 
2.25.1

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

[Hao Xu]

Ping this one..
On 6/14/22 20:01, 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.
> 
> 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.
> +				 */
>   				req->buf_list->head++;
>   				req->buf_list = NULL;
>   			} else {
> @@ -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).
>   		 */
>   		req->buf_list = NULL;
>   		bl->head++;

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

[Jens Axboe]

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

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

[Hao Xu]

On 6/17/22 04:37, Jens Axboe wrote:
> 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...

Apologize for this, 1/2 and this should be separate, and I'm not going
to send 1/2 for now for some reason. I should have change the subject.

> 
>> 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.

I'll do the change, as well as figuring out the weird char stuff.
Thanks.
> 
>> @@ -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).
> 
Gotcha.