<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
</head>
<body>
Neil,<br>
<br>
Does your statement imply this would spin? It definitely doesn’t just spin (that behavior in a main “wait for work” spot of a (depending on settings) ~per-CPU daemon would render systems unusable and this patch has been in testing for a while). So what is
the detailed behavior of a “timeout that expires immediately”?<br>
<br>
- Patrick<br>
<br>
<br>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of NeilBrown <neilb@suse.com><br>
<b>Sent:</b> Sunday, October 28, 2018 7:03:02 PM<br>
<b>To:</b> James Simmons; Andreas Dilger; Oleg Drokin<br>
<b>Cc:</b> Lustre Development List<br>
<b>Subject:</b> Re: [lustre-devel] [PATCH 12/28] lustre: ptlrpc: do not wakeup every second</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">On Sun, Oct 14 2018, James Simmons wrote:<br>
<br>
> From: Alex Zhuravlev <bzzz@whamcloud.com><br>
><br>
> Even if there are no RPC requests on the set, there is no need to<br>
> wake up every second. The thread is woken up when a request is added<br>
> to the set or when the STOP bit is set, so it is sufficient to only<br>
> wake up when there are requests on the set to worry about.<br>
><br>
> Signed-off-by: Alex Zhuravlev <bzzz@whamcloud.com><br>
> WC-bug-id: <a href="https://jira.whamcloud.com/browse/LU-9660">https://jira.whamcloud.com/browse/LU-9660</a><br>
> Reviewed-on: <a href="https://review.whamcloud.com/28776">https://review.whamcloud.com/28776</a><br>
> Reviewed-by: Andreas Dilger <adilger@whamcloud.com><br>
> Reviewed-by: Patrick Farrell <paf@cray.com><br>
> Reviewed-by: Oleg Drokin <green@whamcloud.com><br>
> Signed-off-by: James Simmons <jsimmons@infradead.org><br>
> ---<br>
> drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c | 4 ++--<br>
> 1 file changed, 2 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c b/drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c<br>
> index c201a88..5b4977b 100644<br>
> --- a/drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c<br>
> +++ b/drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c<br>
> @@ -371,7 +371,7 @@ static int ptlrpcd_check(struct lu_env *env, struct ptlrpcd_ctl *pc)<br>
> }<br>
> }<br>
> <br>
> - return rc;<br>
> + return rc || test_bit(LIOD_STOP, &pc->pc_flags);<br>
> }<br>
> <br>
> /**<br>
> @@ -441,7 +441,7 @@ static int ptlrpcd(void *arg)<br>
> lu_context_enter(env.le_ses);<br>
> if (wait_event_idle_timeout(set->set_waitq,<br>
> ptlrpcd_check(&env, pc),<br>
> - (timeout ? timeout : 1) * HZ) == 0)<br>
> + timeout * HZ) == 0)<br>
> ptlrpc_expired_set(set);<br>
<br>
This is incorrect.<br>
A timeout of zero means the timeout happens after zero jiffies<br>
(immediately), it doesn't mean there is no timeout.<br>
If we want a "timeout" of zero to mean "Wait forever", we need something<br>
like:<br>
<br>
wait_event_idle_timeout(.....,<br>
timeout ? (timeout * HZ) : MAX_SCHEDULE_TIMEOUT) == 0<br>
<br>
I've updated the patch accordingly.<br>
<br>
Thanks,<br>
NeilBrown<br>
<br>
> <br>
> lu_context_exit(&env.le_ctx);<br>
> -- <br>
> 1.8.3.1<br>
</div>
</span></font></div>
</body>
</html>