Re: [ovs-dev] [PATCH 1/2] timeval: Don't require signals for time_alarm().

[Ben Pfaff]

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 <et...@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)".
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev