Hi,
On Tue, Jun 14, 2016 at 3:13 AM, Gert Doering <[email protected]> wrote:
> On Mon, Jun 13, 2016 at 10:34:49PM -0400, Selva Nair wrote:
> > @@ -759,6 +762,8 @@ void
> > netcmd_semaphore_release (void)
> > {
> > semaphore_release (&netcmd_semaphore);
> > + /* netcmd_semaphore has max count of 1 - safe to close after release
> */
> > + semaphore_close (&netcmd_semaphore);
> > }
>
> This should work, but it is more "aggressive" than I had in mind -
> releasing
> the semaphore afer every single call, so when setting up lots of routes
> with
> netsh.exe, we might end up opening and closing the semaphore "dozens" of
> times.
>
> How expensive / slow is this? Has this significant impact?
>
My idea was to close the semaphore when the initialization phase is over,
> like in init.c, initialization_sequence_completed(), in the existing
> WIN32 block
>
> #ifdef WIN32
> netcmd_semaphore_close(); /* release handles */
> fork_register_dns_action (c->c1.tuntap);
> #endif
>
> or something like that. Keeping your change to the "lock" call so it
> is then reopened at tunnel teardown, but is not open while openvpn is
> running in steady state.
>
If we go this route we'll also need a netcmd_semaphore_close() when the
SIGUSR1 loop exits, as a restart before initialization_completed could wait
at management-hold with an open handle.
Or a SIGHUP will open the semaphore to delete routes and the restart could
end up in management hold.
> But I'm open to "semaphores are cheap, we can just open/lock/release/close
> it on every call" arguments :-)
>
Very unlikely to be a drag on performance -- I think createprocess will be
where time is spent [*].
That said, calling semaphore_close() inside netcmd_semaphore_unlock() is
not a style to be proud of. So I'm kind of inclined to do a version 2
along the lines you suggested but some testing will be needed to get it
right.
Selva
P.S:
[*] FWIW, I did a timing test: Run using a modified openvpn that just does
openvpn_execve of a route add and route delete inside a for loop. The times
below are for a loop count of 100.
No semaphore: 2.45 to 2.49 seconds
With semaphore lock/unlock : 2.52 to 2.53 seconds
With semaphore closed after reach unlock: 2.52 to 2.54 seconds
(Not enough statistics to give a precise number, but the overhead of using
the semaphore is small and the open/close appears to make little
difference). I'm surprised that it takes 2.5 seconds to add and delete a
route just 100 times. Never use --route-method exe
Note that all times are real time, not CPU time. And the times are for just
the for loop and scales linearly -- tested with 200 and 300 counts as well.
