tinc devel - May 2011 - nearly-tickless-tinc

Hi,

    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 ?

     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.

     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.

     Tell me if you are interested and whether I should modify
  anything.

     Thanks a lot,

           Lo?c

PS: patch against 1.0.14
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tinc.diff
Type: text/x-patch
Size: 3124 bytes
Desc: not available
URL:
<http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20110529/292c7e8a/attachment.bin>

29.05.2011 21:43, Lo?c Greni? wrote:
>     Hi,
> 
>     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 ?
First of all, there's another branch, tinc-1.1, which reworks whole
event model of tinc to use other, more efficient mechanisms.
>      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.
> 
>      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.
Usual trick in such cases is to use selfpipe - create a pipe(2),
add its read end to the select list, and write a byte to the write
end of it in the signal handler, to make select() to notice it.

But your version is not an improvement.  You eliminate a every-second
wakeup during idle, but you add quite several extra system calls into
the critical path around select() call.  In other words, you optimize
idle loop by making real stuff less efficient.

The selfpipe trick may help here to get things back for sure.

But I'm not really convinced this is the right course - instead,
the 1.1 branch should be made live.  In my opinion anyway.

/mjt

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.
>      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, but to prevent race conditions, only pselect() should change the
signal mask in the main loop. 

-- 
Met vriendelijke groet / with kind regards,
     Guus Sliepen <guus at tinc-vpn.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL:
<http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20110529/593ef696/attachment.pgp>