[PATCH] nvme/host/tcp.c: fix use of time_after()

[PATCH] nvme/host/tcp.c: fix use of time_after()

[Wunderlich, Mark]

nvme/host/tcp.c: fix use of time_after()

From: Mark Wunderlich <mark.wunderlich@intel.com>

The values provided to time_after() call used to terminate
do/while loop were reversed, causing loop to always exit
after single pass.  Believe intent was to give the worker
a 1 msec time quota.

Signed-off-by: Mark Wunderlich <mark.wunderlich@intel.com>
---
 drivers/nvme/host/tcp.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 08a2501..8f4a3f84 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1057,7 +1057,7 @@ static void nvme_tcp_io_work(struct work_struct *w)
 		if (!pending)
 			return;
 
-	} while (time_after(jiffies, start)); /* quota is exhausted */
+	} while (time_after(start, jiffies)); /* quota is exhausted */
 
 	queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);
 }


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme/host/tcp.c: fix use of time_after()

[Sagi Grimberg]

Thanks Mark for catching this!
> nvme/host/tcp.c: fix use of time_after()

title should be:
nvme-tcp: fix wrong stop condition in io_work

> From: Mark Wunderlich <mark.wunderlich@intel.com>

This should not appear in the patch message, are you using git-send-email?

> The values provided to time_after() call used to terminate
> do/while loop were reversed, causing loop to always exit
> after single pass.

They are not reversed, but rather in the right order, the problem
is that the do/while statement needs to continue as long as the
time *DID NOT* expire.

So the right fix is:
 > -	} while (time_after(jiffies, start)); /* quota is exhausted */
 > +	} while (!time_after(jiffies, start)); /* quota is exhausted */

Please also update the patch description accordingly.

I'm inclined to get this to jens asap and also send it to stable as well.

I'll be waiting for you to respin.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

RE: [PATCH] nvme/host/tcp.c: fix use of time_after()

[Wunderlich, Mark]

>Thanks Mark for catching this!
>> nvme/host/tcp.c: fix use of time_after()

>title should be:
>nvme-tcp: fix wrong stop condition in io_work
Sure, will change when I re-submit as 'v2';  [PATCH v2] nvme-tcp: fix wrong stop condition in io_work, correct?

>> From: Mark Wunderlich <mark.wunderlich@intel.com>

>This should not appear in the patch message, are you using git-send-email?
Sorry, could not send email directly from server with git tree, so had to just send via my workstation email, copying exported patch contexts into plain text mail message.
I'll remove that from the patch next time.  Is this process OK then, or will I need to replicate git tree on my workstation to use the git email path?

>> The values provided to time_after() call used to terminate do/while 
>> loop were reversed, causing loop to always exit after single pass.

>They are not reversed, but rather in the right order, the problem is that the do/while statement needs to continue as long as the time *DID NOT* expire.

>So the right fix is:
 >> -	} while (time_after(jiffies, start)); /* quota is exhausted */
 >> +	} while (!time_after(jiffies, start)); /* quota is exhausted */
Same result either way, but I think your suggestion reads better.  Will change.
Personally do not like use of label 'start' to indicate the end of time period, to me would read better if used something like 'quota_end' or 'time_end'.  But not worth changing here?

>Please also update the patch description accordingly.
Will do.

>I'm inclined to get this to jens asap and also send it to stable as well.

>I'll be waiting for you to respin.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme/host/tcp.c: fix use of time_after()

[Bart Van Assche]

On 9/18/19 11:20 AM, Wunderlich, Mark wrote:
> Personally do not like use of label 'start' to indicate the end of time period, to me would read better if used something like 'quota_end' or 'time_end'.  But not worth changing here?

How about 'deadline'?

Thanks,

Bart.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme/host/tcp.c: fix use of time_after()

[Sagi Grimberg]

>> Thanks Mark for catching this!
>>> nvme/host/tcp.c: fix use of time_after()
> 
>> title should be:
>> nvme-tcp: fix wrong stop condition in io_work
> Sure, will change when I re-submit as 'v2';  [PATCH v2] nvme-tcp: fix wrong stop condition in io_work, correct?

Yes

> 
>>> From: Mark Wunderlich <mark.wunderlich@intel.com>
> 
>> This should not appear in the patch message, are you using git-send-email?
> Sorry, could not send email directly from server with git tree, so had to just send via my workstation email, copying exported patch contexts into plain text mail message.
> I'll remove that from the patch next time.  Is this process OK then, or will I need to replicate git tree on my workstation to use the git email path?

I can fix that when applying if you have issues with that.

>>> The values provided to time_after() call used to terminate do/while
>>> loop were reversed, causing loop to always exit after single pass.
> 
>> They are not reversed, but rather in the right order, the problem is that the do/while statement needs to continue as long as the time *DID NOT* expire.
> 
>> So the right fix is:
>   >> -	} while (time_after(jiffies, start)); /* quota is exhausted */
>   >> +	} while (!time_after(jiffies, start)); /* quota is exhausted */
> Same result either way, but I think your suggestion reads better.  Will change.
> Personally do not like use of label 'start' to indicate the end of time period, to me would read better if used something like 'quota_end' or 'time_end'.  But not worth changing here?

You can use deadline as Bart suggested.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme