[ovs-dev] [PATCH 1/2] timeval: Don't require signals for time_alarm().
Ben Pfaff
blp at nicira.com
Fri Oct 5 17:14:24 UTC 2012
On Thu, Oct 04, 2012 at 07:15:42PM -0700, Ethan Jackson wrote:
> Before this patch, time_alarm() used the SIGALRM handler to notify
> the poll loop that it should exit the program. Instead, this patch
> simply implements time_alarm() directly in the pool loop. This
> significantly simplifies the code, while removing a call to
> timer_create() which is not currently supported on ESX.
>
> Signed-off-by: Ethan Jackson <ethan at nicira.com>
This looks good. I have only a few comments.
First, when I try this out, I get a new error in the log when a
program terminates due to the timeout expiring, like this:
reconnect|WARN|tcp:127.0.0.1: connection attempt failed (Connection refused)
reconnect|WARN|tcp:127.0.0.1: connection attempt failed (Connection refused)
reconnect|WARN|tcp:127.0.0.1: connection attempt failed (Connection refused)
poll_loop|ERR|poll: Interrupted system call
fatal_signal|WARN|terminating with signal 14 (Alarm clock)
Alarm clock
I figure that this is because on 32-bit x86 (where I develop) the
periodic SIGALRM signal tends to get processed before the deadline.
This incremental fixes it:
diff --git a/lib/timeval.c b/lib/timeval.c
index 9b81cd3..16aded4 100644
--- a/lib/timeval.c
+++ b/lib/timeval.c
@@ -336,6 +336,9 @@ time_poll(struct pollfd *pollfds, int n_pollfds, long long int timeout_when,
time_refresh();
if (deadline <= time_msec()) {
fatal_signal_handler(SIGALRM);
+ if (retval < 0) {
+ retval = 0;
+ }
break;
}
Looking closer, the reason that EINTR is waking up the process is that
we have a special case to never block SIGALRM if there's a deadline.
That's obsolete with this patch, since SIGALRM isn't what we need to
wake us up for the deadline anymore, so we can also apply:
diff --git a/lib/timeval.c b/lib/timeval.c
index 9b81cd3..7f369b0 100644
--- a/lib/timeval.c
+++ b/lib/timeval.c
@@ -343,7 +343,7 @@ time_poll(struct pollfd *pollfds, int n_pollfds, long long int timeout_when,
break;
}
- if (!blocked && deadline == LLONG_MAX) {
+ if (!blocked) {
block_sigalrm(&oldsigs);
blocked = true;
}
Come to think of it, separately, if we're caching time then blocking
SIGALRM is not beneficial so we might as well change the condition to
"if (!blocked && !CACHE_TIME)".
More information about the dev
mailing list