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

[Hao Xu <hao.xu@linux.dev>]

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.