Hi Willy,
Le 20/03/2017 à 07:15, Willy Tarreau a écrit :
Hi Cyril,
A few comments below. First, I'd prefer not to add anything in
run_poll_loop() since we have to pass there a lot of times and it's on
the critical path. Instead I think we could create a task upon receipt
of SIGUSR1 that will either kill all connections or simply exit after
expiration (and emit a log). This can still be discussed, if the resulting
code is more fragile or more complex, but that's a general principle I
mention here.
I agree, while I modified the code, I didn't like having to add some new
tests in run_pool_loop(). Using a new task does not seem to make the
code too complex. It may even be clearer.
I already have something usable but I want to spend some more imes to
test some cases.
On Thu, Mar 16, 2017 at 12:44:40AM +0100, Cyril Bonté wrote:
@@ -1585,6 +1587,12 @@ static void run_poll_loop()
/* Check if we can expire some tasks */
next = wake_expired_tasks();
+ if (stopping && (grace != TICK_ETERNITY)) {
You can use tick_isset(grace) instead of comparing with TICK_ETERNITY.
+ if (tick_is_expired(grace, now_ms))
+ break;
+ if (next == TICK_ETERNITY || tick_is_expired(next,
grace))
+ next = grace;
+ }
I could be wrong but I fear that it can cause some longer poll() pauses
because here you're postponing "next" if it was supposed to trigger
earlier.
I'm not sure, because "next" is modified only if it's greater than
"grace". The goal was exactly the contrary to wake up earlier if the
grace time triggers before "next".
But using a task, this is now obsolete and less error-prone :-)
You should have used tick_first(next, grace), this would give
something like this :
if (unlikely(stopping)) {
if (tick_is_expired(grace, now_ms))
break;
next = tick_first(next, grace);
}
Oh indeed, I didn't notice the tick_first function :) It also becomes
obsolete using a task ;-)
/* stop when there's nothing left to do */
if (jobs == 0)
diff --git a/src/proxy.c b/src/proxy.c
index c76d55d5..024b36fa 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -925,6 +925,8 @@ void soft_stop(void)
struct peers *prs;
stopping = 1;
+ if (global.grace != TICK_ETERNITY)
+ grace = tick_add(now_ms, global.grace);
You can use the following which does these two operations :
grace = tick_add_ifset(now_ms, global.grace);
Right, and obsolete too :-)
--
Cyril Bonté
