<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<p style="margin-top:0;margin-bottom:0">Ah, OK!<br>
<br>
Thanks, Neil. That makes sense. I just knew there was no way that had been missed in the WC branch - Just as I'm sure you two noticed it immediately in your testing.</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">Also useful to know about the timeout functions and 0.</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">Regards,</p>
<p style="margin-top:0;margin-bottom:0">Patrick</p>
</div>
<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> James Simmons <jsimmons@infradead.org><br>
<b>Sent:</b> Sunday, October 28, 2018 10:42:10 PM<br>
<b>To:</b> NeilBrown<br>
<b>Cc:</b> Patrick Farrell; Andreas Dilger; Oleg Drokin; 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"><br>
> > Neil,<br>
> ><br>
> > Does your statement imply this would spin? It definitely doesn’t just<br>
> > spin (that behavior in a main “wait for work” spot of a (depending on<br>
> > settings) ~per-CPU daemon would render systems unusable and this patch<br>
> > has been in testing for a while). So what is the detailed behavior of<br>
> > a “timeout that expires immediately”?<br>
> <br>
> Hi Patrick,<br>
> it definitely spins for me.<br>
<br>
Ah that is where the high cpu load is coming from.<br>
<br>
> I should have clarified that the SFS patch<br>
> <br>
> e81847bd0651 LU-9660 ptlrpc: do not wakeup every second<br>
> <br>
> is correct, as __l_wait_event() treats a timeout value of 0 as meaning an<br>
> indefinite timeout.<br>
> The error was in the conversion to wait_event_idle_timeout(). The<br>
> various wait_event*timeout() functions treat 0 as 1 less than 1.<br>
> If you want to not have a timeout, you need to not use the *_timeout()<br>
> version.<br>
> If a timeout is undesirable rather than fatal, then<br>
> MAX_SCHEDULE_TIMEOUT can be used. In this case, that seemed best.<br>
<br>
I missed that in the conversion. Now that I know that the mapping I will<br>
do the future server code port correctly ;-)<br>
<br>
> > - Patrick<br>
> ><br>
> ><br>
> > ________________________________<br>
> > From: lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of NeilBrown <neilb@suse.com><br>
> > Sent: Sunday, October 28, 2018 7:03:02 PM<br>
> > To: James Simmons; Andreas Dilger; Oleg Drokin<br>
> > Cc: Lustre Development List<br>
> > Subject: Re: [lustre-devel] [PATCH 12/28] lustre: ptlrpc: do not wakeup every second<br>
> ><br>
> > 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>