Re: [Openvpn-devel] [PATCH 6/7] argv: do fewer memory re-allocations
On Wednesday, November 9, 2016 11:58:21 PM CET David Sommerseth wrote:
> > argv_init (struct argv *a)
> > {
> >
> >a->capacity = 0;
> >a->argc = 0;
> >a->argv = NULL;
> >
> > + argv_extend (a, 8);
>
> Why 8? Done any performance and/or memory utilization tests? Does the
> overall memory consumption increase with this?
I figured it is a fine initial length for the argv vector. No science behind it
whatsoever. If 8 doesn't suffice the argv is reallocated.
> > @@ -126,10 +138,7 @@ argv_insert_head (const struct argv *a, const char
> > *head)>
> > const char *
> > argv_str (const struct argv *a, struct gc_arena *gc, const unsigned int
> > flags) {
> >
> > - if (a->argv)
> > -return print_argv ((const char **)a->argv, gc, flags);
> > - else
> > -return "";
> > + return print_argv ((const char **)a->argv, gc, flags);
>
> There are very few callers of print_argv() (defined in buffer.c).
> Should we migrate print_argv() into this function and move all callers
> over to argv_str() instead?
print_argv actually prints a char** and is not affiliated with struct argv,
even
though its name suggests it. So, if we want to do it it should be an add-on
done after this patchset.
> > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> > --- a/src/openvpn/tun.c
> > +++ b/src/openvpn/tun.c
>
> tun.c:2360 close_tun() seems to be missing a argv_free(), or?
> The same goes for close_tun() in tun.c:2482 and tun.c:2956
> (tun.c:2821 does have it though).
>
> Similarly, in open_tun() [tun.c:2926], argv_new() is called but no
> argv_free() calls.
Nice catch, they were missing back in the argv_reset days as well. While at
it, also removed a couple of unused struct gc_arena.
> On a not so related note. I noticed that init.c have a
> #ifdef ARGV_TEST block. That should probably also be killed; no need
> for that as we have unit tests - and the argv_test() function it calls
> no longer exists.
Aaand it's gone.
Cheers
Heiko
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 6/7] argv: do fewer memory re-allocations
On 28/10/16 18:42, Heiko Hund wrote:
> Prevent the re-allocations of memory when the internal argv grows
> beyond 2 and 4 arguments by initially allocating argv to hold up to
> 7 (+ trailing NULL) pointers.
>
> While at it rename argv_reset to argv_free to actually express
> what's going on. Redo the argv_reset functionality so that it can
> be used to actually reset the argv without re-allocation.
>
> Signed-off-by: Heiko Hund
> ---
> src/openvpn/argv.c | 45
>
> src/openvpn/argv.h | 2 +-
> src/openvpn/console_systemd.c| 2 +-
> src/openvpn/init.c | 2 +-
> src/openvpn/lladdr.c | 2 +-
> src/openvpn/misc.c | 4 ++--
> src/openvpn/multi.c | 10
> src/openvpn/options.c| 2 +-
> src/openvpn/plugin.c | 2 +-
> src/openvpn/route.c | 8 +++
> src/openvpn/socket.c | 4 ++--
> src/openvpn/ssl_verify.c | 6 ++---
> src/openvpn/tun.c| 23 --
> tests/unit_tests/openvpn/test_argv.c | 41 +---
> 14 files changed, 85 insertions(+), 68 deletions(-)
>
Code looks fairly reasonable, but I have a few comments ...
> argv_init (struct argv *a)
> {
>a->capacity = 0;
>a->argc = 0;
>a->argv = NULL;
> + argv_extend (a, 8);
Why 8? Done any performance and/or memory utilization tests? Does the
overall memory consumption increase with this?
> void
> -argv_reset (struct argv *a)
> +argv_free (struct argv *a)
This makes more sense :)
> @@ -126,10 +138,7 @@ argv_insert_head (const struct argv *a, const char *head)
> const char *
> argv_str (const struct argv *a, struct gc_arena *gc, const unsigned int
> flags)
> {
> - if (a->argv)
> -return print_argv ((const char **)a->argv, gc, flags);
> - else
> -return "";
> + return print_argv ((const char **)a->argv, gc, flags);
There are very few callers of print_argv() (defined in buffer.c).
Should we migrate print_argv() into this function and move all callers
over to argv_str() instead?
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
tun.c:2360 close_tun() seems to be missing a argv_free(), or?
The same goes for close_tun() in tun.c:2482 and tun.c:2956
(tun.c:2821 does have it though).
Similarly, in open_tun() [tun.c:2926], argv_new() is called but no
argv_free() calls.
On a not so related note. I noticed that init.c have a
#ifdef ARGV_TEST block. That should probably also be killed; no need
for that as we have unit tests - and the argv_test() function it calls
no longer exists.
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
--
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
