public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH v2] io_uring: reduce latency by reissueing the operation
@ 2021-06-20 19:05 Olivier Langlois
  2021-06-20 19:07 ` Randy Dunlap
  2021-06-20 19:56 ` Pavel Begunkov
  0 siblings, 2 replies; 14+ messages in thread
From: Olivier Langlois @ 2021-06-20 19:05 UTC (permalink / raw)
  To: Jens Axboe, Pavel Begunkov, io-uring, linux-kernel; +Cc: Olivier Langlois

It is quite frequent that when an operation fails and returns EAGAIN,
the data becomes available between that failure and the call to
vfs_poll() done by io_arm_poll_handler().

Detecting the situation and reissuing the operation is much faster
than going ahead and push the operation to the io-wq.

Signed-off-by: Olivier Langlois <[email protected]>
---
 fs/io_uring.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index fa8794c61af7..6e037304429a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5143,7 +5143,10 @@ static __poll_t __io_arm_poll_handler(struct io_kiocb *req,
 	return mask;
 }
 
-static bool io_arm_poll_handler(struct io_kiocb *req)
+#define IO_ARM_POLL_OK    0
+#define IO_ARM_POLL_ERR   1
+#define IO_ARM_POLL_READY 2
+static int io_arm_poll_handler(struct io_kiocb *req)
 {
 	const struct io_op_def *def = &io_op_defs[req->opcode];
 	struct io_ring_ctx *ctx = req->ctx;
@@ -5153,22 +5156,22 @@ static bool io_arm_poll_handler(struct io_kiocb *req)
 	int rw;
 
 	if (!req->file || !file_can_poll(req->file))
-		return false;
+		return IO_ARM_POLL_ERR;
 	if (req->flags & REQ_F_POLLED)
-		return false;
+		return IO_ARM_POLL_ERR;
 	if (def->pollin)
 		rw = READ;
 	else if (def->pollout)
 		rw = WRITE;
 	else
-		return false;
+		return IO_ARM_POLL_ERR;
 	/* if we can't nonblock try, then no point in arming a poll handler */
 	if (!io_file_supports_async(req, rw))
-		return false;
+		return IO_ARM_POLL_ERR;
 
 	apoll = kmalloc(sizeof(*apoll), GFP_ATOMIC);
 	if (unlikely(!apoll))
-		return false;
+		return IO_ARM_POLL_ERR;
 	apoll->double_poll = NULL;
 
 	req->flags |= REQ_F_POLLED;
@@ -5194,12 +5197,12 @@ static bool io_arm_poll_handler(struct io_kiocb *req)
 	if (ret || ipt.error) {
 		io_poll_remove_double(req);
 		spin_unlock_irq(&ctx->completion_lock);
-		return false;
+		return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;
 	}
 	spin_unlock_irq(&ctx->completion_lock);
 	trace_io_uring_poll_arm(ctx, req->opcode, req->user_data, mask,
 					apoll->poll.events);
-	return true;
+	return IO_ARM_POLL_OK;
 }
 
 static bool __io_poll_remove_one(struct io_kiocb *req,
@@ -6416,6 +6419,7 @@ static void __io_queue_sqe(struct io_kiocb *req)
 	struct io_kiocb *linked_timeout = io_prep_linked_timeout(req);
 	int ret;
 
+issue_sqe:
 	ret = io_issue_sqe(req, IO_URING_F_NONBLOCK|IO_URING_F_COMPLETE_DEFER);
 
 	/*
@@ -6435,12 +6439,16 @@ static void __io_queue_sqe(struct io_kiocb *req)
 			io_put_req(req);
 		}
 	} else if (ret == -EAGAIN && !(req->flags & REQ_F_NOWAIT)) {
-		if (!io_arm_poll_handler(req)) {
+		switch (io_arm_poll_handler(req)) {
+		case IO_ARM_POLL_READY:
+			goto issue_sqe;
+		case IO_ARM_POLL_ERR:
 			/*
 			 * Queued up for async execution, worker will release
 			 * submit reference when the iocb is actually submitted.
 			 */
 			io_queue_async_work(req);
+			break;
 		}
 	} else {
 		io_req_complete_failed(req, ret);
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 19:05 [PATCH v2] io_uring: reduce latency by reissueing the operation Olivier Langlois
@ 2021-06-20 19:07 ` Randy Dunlap
  2021-06-20 19:28   ` Olivier Langlois
  2021-06-20 19:56 ` Pavel Begunkov
  1 sibling, 1 reply; 14+ messages in thread
From: Randy Dunlap @ 2021-06-20 19:07 UTC (permalink / raw)
  To: Olivier Langlois, Jens Axboe, Pavel Begunkov, io-uring,
	linux-kernel

On 6/20/21 12:05 PM, Olivier Langlois wrote:
> -		return false;
> +		return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;

Hi,
Please make that return expression more readable.

thanks.
-- 
~Randy


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 19:07 ` Randy Dunlap
@ 2021-06-20 19:28   ` Olivier Langlois
  2021-06-20 20:01     ` Randy Dunlap
  2021-06-21 16:03     ` Jens Axboe
  0 siblings, 2 replies; 14+ messages in thread
From: Olivier Langlois @ 2021-06-20 19:28 UTC (permalink / raw)
  To: Randy Dunlap, Jens Axboe, Pavel Begunkov, io-uring, linux-kernel

On Sun, 2021-06-20 at 12:07 -0700, Randy Dunlap wrote:
> On 6/20/21 12:05 PM, Olivier Langlois wrote:
> > -               return false;
> > +               return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;
> 
> Hi,
> Please make that return expression more readable.
> 
> 
How exactly?

by adding spaces?
Changing the define names??



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 19:05 [PATCH v2] io_uring: reduce latency by reissueing the operation Olivier Langlois
  2021-06-20 19:07 ` Randy Dunlap
@ 2021-06-20 19:56 ` Pavel Begunkov
  2021-06-20 20:03   ` Pavel Begunkov
  2021-06-20 21:05   ` Olivier Langlois
  1 sibling, 2 replies; 14+ messages in thread
From: Pavel Begunkov @ 2021-06-20 19:56 UTC (permalink / raw)
  To: Olivier Langlois, Jens Axboe, io-uring, linux-kernel

On 6/20/21 8:05 PM, Olivier Langlois wrote:
> It is quite frequent that when an operation fails and returns EAGAIN,
> the data becomes available between that failure and the call to
> vfs_poll() done by io_arm_poll_handler().
> 
> Detecting the situation and reissuing the operation is much faster
> than going ahead and push the operation to the io-wq.
> 
> Signed-off-by: Olivier Langlois <[email protected]>
> ---
>  fs/io_uring.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index fa8794c61af7..6e037304429a 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -5143,7 +5143,10 @@ static __poll_t __io_arm_poll_handler(struct io_kiocb *req,
>  	return mask;
>  }
>  
> -static bool io_arm_poll_handler(struct io_kiocb *req)
> +#define IO_ARM_POLL_OK    0
> +#define IO_ARM_POLL_ERR   1
> +#define IO_ARM_POLL_READY 2

Please add a new line here. Can even be moved somewhere
to the top, but it's a matter of taste.

Also, how about to rename it to apoll? io_uring internal
rw/send/recv polling is often abbreviated as such around
io_uring.c
IO_APOLL_OK and so on.

> +static int io_arm_poll_handler(struct io_kiocb *req)
>  {
>  	const struct io_op_def *def = &io_op_defs[req->opcode];
>  	struct io_ring_ctx *ctx = req->ctx;
> @@ -5153,22 +5156,22 @@ static bool io_arm_poll_handler(struct io_kiocb *req)
>  	int rw;
>  
>  	if (!req->file || !file_can_poll(req->file))
> -		return false;
> +		return IO_ARM_POLL_ERR;

It's not really an error. Maybe IO_APOLL_ABORTED or so?

>  	if (req->flags & REQ_F_POLLED)
> -		return false;
> +		return IO_ARM_POLL_ERR;
>  	if (def->pollin)
>  		rw = READ;
>  	else if (def->pollout)
>  		rw = WRITE;
>  	else
> -		return false;
> +		return IO_ARM_POLL_ERR;
>  	/* if we can't nonblock try, then no point in arming a poll handler */
>  	if (!io_file_supports_async(req, rw))
> -		return false;
> +		return IO_ARM_POLL_ERR;
>  
>  	apoll = kmalloc(sizeof(*apoll), GFP_ATOMIC);
>  	if (unlikely(!apoll))
> -		return false;
> +		return IO_ARM_POLL_ERR;
>  	apoll->double_poll = NULL;
>  
>  	req->flags |= REQ_F_POLLED;
> @@ -5194,12 +5197,12 @@ static bool io_arm_poll_handler(struct io_kiocb *req)
>  	if (ret || ipt.error) {
>  		io_poll_remove_double(req);
>  		spin_unlock_irq(&ctx->completion_lock);
> -		return false;
> +		return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;

spaces would be great.

>  	}
>  	spin_unlock_irq(&ctx->completion_lock);
>  	trace_io_uring_poll_arm(ctx, req->opcode, req->user_data, mask,
>  					apoll->poll.events);
> -	return true;
> +	return IO_ARM_POLL_OK;
>  }
>  
>  static bool __io_poll_remove_one(struct io_kiocb *req,
> @@ -6416,6 +6419,7 @@ static void __io_queue_sqe(struct io_kiocb *req)
>  	struct io_kiocb *linked_timeout = io_prep_linked_timeout(req);
>  	int ret;
>  
> +issue_sqe:
>  	ret = io_issue_sqe(req, IO_URING_F_NONBLOCK|IO_URING_F_COMPLETE_DEFER);
>  
>  	/*
> @@ -6435,12 +6439,16 @@ static void __io_queue_sqe(struct io_kiocb *req)
>  			io_put_req(req);
>  		}
>  	} else if (ret == -EAGAIN && !(req->flags & REQ_F_NOWAIT)) {
> -		if (!io_arm_poll_handler(req)) {
> +		switch (io_arm_poll_handler(req)) {
> +		case IO_ARM_POLL_READY:
> +			goto issue_sqe;

Checked assembly, the fast path looks ok (i.e. not affected).
Also, a note, linked_timeout is handled correctly.

> +		case IO_ARM_POLL_ERR:
>  			/*
>  			 * Queued up for async execution, worker will release
>  			 * submit reference when the iocb is actually submitted.
>  			 */
>  			io_queue_async_work(req);
> +			break;
>  		}
>  	} else {
>  		io_req_complete_failed(req, ret);
> 

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 19:28   ` Olivier Langlois
@ 2021-06-20 20:01     ` Randy Dunlap
  2021-06-20 20:08       ` Pavel Begunkov
  2021-06-21 16:03     ` Jens Axboe
  1 sibling, 1 reply; 14+ messages in thread
From: Randy Dunlap @ 2021-06-20 20:01 UTC (permalink / raw)
  To: Olivier Langlois, Jens Axboe, Pavel Begunkov, io-uring,
	linux-kernel

On 6/20/21 12:28 PM, Olivier Langlois wrote:
> On Sun, 2021-06-20 at 12:07 -0700, Randy Dunlap wrote:
>> On 6/20/21 12:05 PM, Olivier Langlois wrote:
>>> -               return false;
>>> +               return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;
>>
>> Hi,
>> Please make that return expression more readable.
>>
>>
> How exactly?
> 
> by adding spaces?
> Changing the define names??

Adding spaces would be sufficient IMO (like Pavel suggested also).

thanks.
-- 
~Randy


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 19:56 ` Pavel Begunkov
@ 2021-06-20 20:03   ` Pavel Begunkov
  2021-06-20 20:56     ` Olivier Langlois
  2021-06-20 21:05   ` Olivier Langlois
  1 sibling, 1 reply; 14+ messages in thread
From: Pavel Begunkov @ 2021-06-20 20:03 UTC (permalink / raw)
  To: Olivier Langlois, Jens Axboe, io-uring, linux-kernel

On 6/20/21 8:56 PM, Pavel Begunkov wrote:
> On 6/20/21 8:05 PM, Olivier Langlois wrote:
>> It is quite frequent that when an operation fails and returns EAGAIN,
>> the data becomes available between that failure and the call to
>> vfs_poll() done by io_arm_poll_handler().
>>
>> Detecting the situation and reissuing the operation is much faster
>> than going ahead and push the operation to the io-wq.
>>
>> Signed-off-by: Olivier Langlois <[email protected]>
>> ---
>>  fs/io_uring.c | 26 +++++++++++++++++---------
>>  1 file changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index fa8794c61af7..6e037304429a 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -5143,7 +5143,10 @@ static __poll_t __io_arm_poll_handler(struct io_kiocb *req,
>>  	return mask;
>>  }
>>  
>> -static bool io_arm_poll_handler(struct io_kiocb *req)
>> +#define IO_ARM_POLL_OK    0
>> +#define IO_ARM_POLL_ERR   1
>> +#define IO_ARM_POLL_READY 2
> 
> Please add a new line here. Can even be moved somewhere
> to the top, but it's a matter of taste.
> 
> Also, how about to rename it to apoll? io_uring internal
> rw/send/recv polling is often abbreviated as such around
> io_uring.c
> IO_APOLL_OK and so on.
> 
>> +static int io_arm_poll_handler(struct io_kiocb *req)
>>  {
>>  	const struct io_op_def *def = &io_op_defs[req->opcode];
>>  	struct io_ring_ctx *ctx = req->ctx;
>> @@ -5153,22 +5156,22 @@ static bool io_arm_poll_handler(struct io_kiocb *req)
>>  	int rw;
>>  
>>  	if (!req->file || !file_can_poll(req->file))
>> -		return false;
>> +		return IO_ARM_POLL_ERR;
> 
> It's not really an error. Maybe IO_APOLL_ABORTED or so?

fwiw, I mean totally replacing *_ERR, not only this return

> 
>>  	if (req->flags & REQ_F_POLLED)
>> -		return false;
>> +		return IO_ARM_POLL_ERR;
>>  	if (def->pollin)
>>  		rw = READ;
>>  	else if (def->pollout)
>>  		rw = WRITE;
>>  	else
>> -		return false;
>> +		return IO_ARM_POLL_ERR;
>>  	/* if we can't nonblock try, then no point in arming a poll handler */
>>  	if (!io_file_supports_async(req, rw))
>> -		return false;
>> +		return IO_ARM_POLL_ERR;
>>  
>>  	apoll = kmalloc(sizeof(*apoll), GFP_ATOMIC);
>>  	if (unlikely(!apoll))
>> -		return false;
>> +		return IO_ARM_POLL_ERR;
>>  	apoll->double_poll = NULL;
>>  
>>  	req->flags |= REQ_F_POLLED;
>> @@ -5194,12 +5197,12 @@ static bool io_arm_poll_handler(struct io_kiocb *req)
>>  	if (ret || ipt.error) {
>>  		io_poll_remove_double(req);
>>  		spin_unlock_irq(&ctx->completion_lock);
>> -		return false;
>> +		return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;
> 
> spaces would be great.
> 
>>  	}
>>  	spin_unlock_irq(&ctx->completion_lock);
>>  	trace_io_uring_poll_arm(ctx, req->opcode, req->user_data, mask,
>>  					apoll->poll.events);
>> -	return true;
>> +	return IO_ARM_POLL_OK;
>>  }
>>  
>>  static bool __io_poll_remove_one(struct io_kiocb *req,
>> @@ -6416,6 +6419,7 @@ static void __io_queue_sqe(struct io_kiocb *req)
>>  	struct io_kiocb *linked_timeout = io_prep_linked_timeout(req);
>>  	int ret;
>>  
>> +issue_sqe:
>>  	ret = io_issue_sqe(req, IO_URING_F_NONBLOCK|IO_URING_F_COMPLETE_DEFER);
>>  
>>  	/*
>> @@ -6435,12 +6439,16 @@ static void __io_queue_sqe(struct io_kiocb *req)
>>  			io_put_req(req);
>>  		}
>>  	} else if (ret == -EAGAIN && !(req->flags & REQ_F_NOWAIT)) {
>> -		if (!io_arm_poll_handler(req)) {
>> +		switch (io_arm_poll_handler(req)) {
>> +		case IO_ARM_POLL_READY:
>> +			goto issue_sqe;
> 
> Checked assembly, the fast path looks ok (i.e. not affected).
> Also, a note, linked_timeout is handled correctly.
> 
>> +		case IO_ARM_POLL_ERR:
>>  			/*
>>  			 * Queued up for async execution, worker will release
>>  			 * submit reference when the iocb is actually submitted.
>>  			 */
>>  			io_queue_async_work(req);
>> +			break;
>>  		}
>>  	} else {
>>  		io_req_complete_failed(req, ret);
>>
> 

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 20:01     ` Randy Dunlap
@ 2021-06-20 20:08       ` Pavel Begunkov
  2021-06-20 20:54         ` Olivier Langlois
  2021-06-20 22:17         ` Randy Dunlap
  0 siblings, 2 replies; 14+ messages in thread
From: Pavel Begunkov @ 2021-06-20 20:08 UTC (permalink / raw)
  To: Randy Dunlap, Olivier Langlois, Jens Axboe, io-uring,
	linux-kernel

On 6/20/21 9:01 PM, Randy Dunlap wrote:
> On 6/20/21 12:28 PM, Olivier Langlois wrote:
>> On Sun, 2021-06-20 at 12:07 -0700, Randy Dunlap wrote:
>>> On 6/20/21 12:05 PM, Olivier Langlois wrote:
>>>> -               return false;
>>>> +               return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;
>>>
>>> Hi,
>>> Please make that return expression more readable.
>>>
>>>
>> How exactly?
>>
>> by adding spaces?
>> Changing the define names??
> 
> Adding spaces would be sufficient IMO (like Pavel suggested also).

Agree. That should be in the code style somewhere

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 20:08       ` Pavel Begunkov
@ 2021-06-20 20:54         ` Olivier Langlois
  2021-06-20 22:17         ` Randy Dunlap
  1 sibling, 0 replies; 14+ messages in thread
From: Olivier Langlois @ 2021-06-20 20:54 UTC (permalink / raw)
  To: Pavel Begunkov, Randy Dunlap, Jens Axboe, io-uring, linux-kernel

On Sun, 2021-06-20 at 21:08 +0100, Pavel Begunkov wrote:
> On 6/20/21 9:01 PM, Randy Dunlap wrote:
> > On 6/20/21 12:28 PM, Olivier Langlois wrote:
> > > On Sun, 2021-06-20 at 12:07 -0700, Randy Dunlap wrote:
> > > > On 6/20/21 12:05 PM, Olivier Langlois wrote:
> > > > > -               return false;
> > > > > +               return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;
> > > > 
> > > > Hi,
> > > > Please make that return expression more readable.
> > > > 
> > > > 
> > > How exactly?
> > > 
> > > by adding spaces?
> > > Changing the define names??
> > 
> > Adding spaces would be sufficient IMO (like Pavel suggested also).
> 
> Agree. That should be in the code style somewhere
> 
Sure no problem.

This hasn't been reported by checkpatch.pl but I have just discovered
codespell... Maybe this addon reports more issues than vanilla
checkpatch



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 20:03   ` Pavel Begunkov
@ 2021-06-20 20:56     ` Olivier Langlois
  0 siblings, 0 replies; 14+ messages in thread
From: Olivier Langlois @ 2021-06-20 20:56 UTC (permalink / raw)
  To: Pavel Begunkov, Jens Axboe, io-uring, linux-kernel

On Sun, 2021-06-20 at 21:03 +0100, Pavel Begunkov wrote:
> On 6/20/21 8:56 PM, Pavel Begunkov wrote:
> > On 6/20/21 8:05 PM, Olivier Langlois wrote:
> > > 
> > 
> > It's not really an error. Maybe IO_APOLL_ABORTED or so?
> 
> fwiw, I mean totally replacing *_ERR, not only this return
> 
this is how I understood the remark. I will change it as you suggested




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 19:56 ` Pavel Begunkov
  2021-06-20 20:03   ` Pavel Begunkov
@ 2021-06-20 21:05   ` Olivier Langlois
  2021-06-20 21:09     ` Pavel Begunkov
  2021-06-20 21:11     ` Pavel Begunkov
  1 sibling, 2 replies; 14+ messages in thread
From: Olivier Langlois @ 2021-06-20 21:05 UTC (permalink / raw)
  To: Pavel Begunkov, Jens Axboe, io-uring, linux-kernel

On Sun, 2021-06-20 at 20:56 +0100, Pavel Begunkov wrote:
> On 6/20/21 8:05 PM, Olivier Langlois wrote:
> > 
> >  
> > -static bool io_arm_poll_handler(struct io_kiocb *req)
> > +#define IO_ARM_POLL_OK    0
> > +#define IO_ARM_POLL_ERR   1
> > +#define IO_ARM_POLL_READY 2
> 
> Please add a new line here. Can even be moved somewhere
> to the top, but it's a matter of taste.

If you let me decide, I prefer to let them close to where they are
used. There is so much data definitions in the heading section that I
feel like putting very minor implementation details to it might
overwhelm newcomers instead of helping them to grasp the big picture.

but I will add an extra space as you request
> 
> Also, how about to rename it to apoll? io_uring internal
> rw/send/recv polling is often abbreviated as such around
> io_uring.c
> IO_APOLL_OK and so on.

no problem. I will.
> 
> > +static int io_arm_poll_handler(struct io_kiocb *req)
> >  {
> >         const struct io_op_def *def = &io_op_defs[req->opcode];
> >         struct io_ring_ctx *ctx = req->ctx;
> > @@ -5153,22 +5156,22 @@ static bool io_arm_poll_handler(struct
> > io_kiocb *req)
> >         int rw;
> >  
> >         if (!req->file || !file_can_poll(req->file))
> > -               return false;
> > +               return IO_ARM_POLL_ERR;
> 
> It's not really an error. Maybe IO_APOLL_ABORTED or so?

Ok.



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 21:05   ` Olivier Langlois
@ 2021-06-20 21:09     ` Pavel Begunkov
  2021-06-20 21:11     ` Pavel Begunkov
  1 sibling, 0 replies; 14+ messages in thread
From: Pavel Begunkov @ 2021-06-20 21:09 UTC (permalink / raw)
  To: Olivier Langlois, Jens Axboe, io-uring, linux-kernel

On 6/20/21 10:05 PM, Olivier Langlois wrote:
> On Sun, 2021-06-20 at 20:56 +0100, Pavel Begunkov wrote:
>> On 6/20/21 8:05 PM, Olivier Langlois wrote:
>>>
>>>  
>>> -static bool io_arm_poll_handler(struct io_kiocb *req)
>>> +#define IO_ARM_POLL_OK    0
>>> +#define IO_ARM_POLL_ERR   1
>>> +#define IO_ARM_POLL_READY 2
>>
>> Please add a new line here. Can even be moved somewhere
>> to the top, but it's a matter of taste.
> 
> If you let me decide, I prefer to let them close to where they are
> used. There is so much data definitions in the heading section that I
> feel like putting very minor implementation details to it might
> overwhelm newcomers instead of helping them to grasp the big picture.

Oh yeah, I think any is fine. And there are others
happily living in this style, like FFS_ASYNC_READ

> but I will add an extra space as you request
>>
>> Also, how about to rename it to apoll? io_uring internal
>> rw/send/recv polling is often abbreviated as such around
>> io_uring.c
>> IO_APOLL_OK and so on.
> 
> no problem. I will.
>>
>>> +static int io_arm_poll_handler(struct io_kiocb *req)
>>>  {
>>>         const struct io_op_def *def = &io_op_defs[req->opcode];
>>>         struct io_ring_ctx *ctx = req->ctx;
>>> @@ -5153,22 +5156,22 @@ static bool io_arm_poll_handler(struct
>>> io_kiocb *req)
>>>         int rw;
>>>  
>>>         if (!req->file || !file_can_poll(req->file))
>>> -               return false;
>>> +               return IO_ARM_POLL_ERR;
>>
>> It's not really an error. Maybe IO_APOLL_ABORTED or so?
> 
> Ok.
> 
> 

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 21:05   ` Olivier Langlois
  2021-06-20 21:09     ` Pavel Begunkov
@ 2021-06-20 21:11     ` Pavel Begunkov
  1 sibling, 0 replies; 14+ messages in thread
From: Pavel Begunkov @ 2021-06-20 21:11 UTC (permalink / raw)
  To: Olivier Langlois, Jens Axboe, io-uring, linux-kernel

On 6/20/21 10:05 PM, Olivier Langlois wrote:
> On Sun, 2021-06-20 at 20:56 +0100, Pavel Begunkov wrote:
>> On 6/20/21 8:05 PM, Olivier Langlois wrote:
>>>
>>>  
>>> -static bool io_arm_poll_handler(struct io_kiocb *req)
>>> +#define IO_ARM_POLL_OK    0
>>> +#define IO_ARM_POLL_ERR   1
>>> +#define IO_ARM_POLL_READY 2
>>
>> Please add a new line here. Can even be moved somewhere
>> to the top, but it's a matter of taste.
> 
> If you let me decide, I prefer to let them close to where they are
> used. There is so much data definitions in the heading section that I
> feel like putting very minor implementation details to it might
> overwhelm newcomers instead of helping them to grasp the big picture.
> 
> but I will add an extra space as you request

btw, it doesn't apply cleanly for me, conflicts
with your trace changes. Can you check that you're
on an up-to-date revision? I.e.

https://git.kernel.dk/cgit/linux-block/log/?h=for-5.14/io_uring

>>
>> Also, how about to rename it to apoll? io_uring internal
>> rw/send/recv polling is often abbreviated as such around
>> io_uring.c
>> IO_APOLL_OK and so on.
> 
> no problem. I will.
>>
>>> +static int io_arm_poll_handler(struct io_kiocb *req)
>>>  {
>>>         const struct io_op_def *def = &io_op_defs[req->opcode];
>>>         struct io_ring_ctx *ctx = req->ctx;
>>> @@ -5153,22 +5156,22 @@ static bool io_arm_poll_handler(struct
>>> io_kiocb *req)
>>>         int rw;
>>>  
>>>         if (!req->file || !file_can_poll(req->file))
>>> -               return false;
>>> +               return IO_ARM_POLL_ERR;
>>
>> It's not really an error. Maybe IO_APOLL_ABORTED or so?
> 
> Ok.
> 
> 

-- 
Pavel Begunkov

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 20:08       ` Pavel Begunkov
  2021-06-20 20:54         ` Olivier Langlois
@ 2021-06-20 22:17         ` Randy Dunlap
  1 sibling, 0 replies; 14+ messages in thread
From: Randy Dunlap @ 2021-06-20 22:17 UTC (permalink / raw)
  To: Pavel Begunkov, Olivier Langlois, Jens Axboe, io-uring,
	linux-kernel

On 6/20/21 1:08 PM, Pavel Begunkov wrote:
> On 6/20/21 9:01 PM, Randy Dunlap wrote:
>> On 6/20/21 12:28 PM, Olivier Langlois wrote:
>>> On Sun, 2021-06-20 at 12:07 -0700, Randy Dunlap wrote:
>>>> On 6/20/21 12:05 PM, Olivier Langlois wrote:
>>>>> -               return false;
>>>>> +               return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;
>>>>
>>>> Hi,
>>>> Please make that return expression more readable.
>>>>
>>>>
>>> How exactly?
>>>
>>> by adding spaces?
>>> Changing the define names??
>>
>> Adding spaces would be sufficient IMO (like Pavel suggested also).
> 
> Agree. That should be in the code style somewhere
> 

It is.

-- 
~Randy


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2] io_uring: reduce latency by reissueing the operation
  2021-06-20 19:28   ` Olivier Langlois
  2021-06-20 20:01     ` Randy Dunlap
@ 2021-06-21 16:03     ` Jens Axboe
  1 sibling, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2021-06-21 16:03 UTC (permalink / raw)
  To: Olivier Langlois, Randy Dunlap, Pavel Begunkov, io-uring,
	linux-kernel

On 6/20/21 1:28 PM, Olivier Langlois wrote:
> On Sun, 2021-06-20 at 12:07 -0700, Randy Dunlap wrote:
>> On 6/20/21 12:05 PM, Olivier Langlois wrote:
>>> -               return false;
>>> +               return ret?IO_ARM_POLL_READY:IO_ARM_POLL_ERR;
>>
>> Hi,
>> Please make that return expression more readable.
>>
>>
> How exactly?
> 
> by adding spaces?
> Changing the define names??

Not super important, but I greatly prefer:

	if (ret)
		return IO_ARM_POLL_READY;
	return IO_ARM_POLL_ERR;

as that's a lot more readable to me. This is orthogonal to the currently
missing spaces, of course.

For the defines, an enum would be preferable too. And place it near where
it's used.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-06-21 16:03 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-06-20 19:05 [PATCH v2] io_uring: reduce latency by reissueing the operation Olivier Langlois
2021-06-20 19:07 ` Randy Dunlap
2021-06-20 19:28   ` Olivier Langlois
2021-06-20 20:01     ` Randy Dunlap
2021-06-20 20:08       ` Pavel Begunkov
2021-06-20 20:54         ` Olivier Langlois
2021-06-20 22:17         ` Randy Dunlap
2021-06-21 16:03     ` Jens Axboe
2021-06-20 19:56 ` Pavel Begunkov
2021-06-20 20:03   ` Pavel Begunkov
2021-06-20 20:56     ` Olivier Langlois
2021-06-20 21:05   ` Olivier Langlois
2021-06-20 21:09     ` Pavel Begunkov
2021-06-20 21:11     ` Pavel Begunkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox