nearly-tickless-tinc

[Loïc Grenié]

2011/5/29 Guus Sliepen <guus at tinc-vpn.org>:
> On Sun, May 29, 2011 at 07:43:54PM +0200, Loïc Grenié wrote:
>
>>     tinc has a fixed timeout of 1 second for select() in the main
>>   loop. I think this could be improved. Do you think the following
>>   patch goes into the right direction ?
>
> Well, the reason that it has a 1 second timeout is to avoid some problems that
> you get with a tickless main loop:
>
>>      I don't know whether pselect() is standard enough (works under
>>   "current" linux, however I don't know about the other arches). Maybe
>>   a config option is necessary.
>
> pselect() is POSIX, so recent *BSDs will probably also have it. However,
> Windows does not. On the other hand, Windows has no useful signals so one
> could revert to normal select() there.
>
> Another problem is that older kernels (< 2.6.16) did not have a pselect()
> system call, and glibc has been emulating it, but of course it could not do so
> without a race condition.

    I was thinking about conditional pselect(): if there is pselect then use
  it, otherwise use standard select with a timeout.

>>      It's probably possible to simplify at least signal handling (I've
>>   tried to keep SIGHUP and SIGALRM blocking as they are,
>>   however I suspect that tincd unblocks those signals at the
>>   beginning or, at least, does not modify their blocking while
>>   running). It is also undoubtedly possible to improve the patch
>>   in other ways.
>
> A way to get around the whole signal blocking mess is to have a "self-pipe",
> where the signal handlers write a byte to, and select() checks the pipe fd.
> However, this requires somewhat more changes than using pselect().
>
> In tinc 1.1 we do not have the problem of signals anymore, since it has a
> control socket. And libevent() is used, which takes care of all the details of
> event loop handling for us anyway.
>
>>      Tell me if you are interested and whether I should modify
>>   anything.
>
> Yes, I am interested. However, it should work on all platforms supported by
> tinc, and:
>
>>  int main_loop(void) {
> [...]
>> +             /* Block SIGHUP & SIGALARM if not already blocked */
>> +             sigprocmask(SIG_BLOCK, NULL, &omask);
>> +             sigemptyset(&block_mask);
>> +             if (!sigismember(&omask, SIGHUP))
>> +                     sigaddset(&block_mask, SIGHUP);
>> +             if (!sigismember(&omask, SIGALRM))
>> +                     sigaddset(&block_mask, SIGALRM);
>> +             sigprocmask(SIG_BLOCK, &block_mask, &omask);
> [...]
>> +             r = pselect(maxfd + 1, &readset, &writeset, NULL, &tv, &block_mask);
>> +             /* Unlock SIGHUP & SIGALARM (if not already blocked) */
>> +             sigprocmask(SIG_UNBLOCK, &block_mask, NULL);
> [...]
>
> That's a lot of sigprocmask() calls in the main loop. Not only is that
> inefficient,

     It is indeed, but see below.

> but to prevent race conditions, only pselect() should change the
> signal mask in the main loop.

     I might be wrong but I think there is no race condition in my code.
  However I can eliminate all the problems if you tell me that tincd
  does not block/unblock SIGHUP and SIGALARM while running
  (block_mask can be initialized before the main loop, signals blocked
  there and never unblocked). In that case the main loop is mostly
  unchanged (except for computing "next_event") and we can
  emulate pselect() when it does not exist (or is racy) by a
  standard select() with a 1s timeout (without blocking signals
  obviously).

     The "selfpipe" trick is another option obviously if you prefer. It is
  not very elegant but has no real downside (I don't know whether
  there are pipes under Windows, but since there are no signals
  anyway...).

          Thanks,

                 Loïc