| Message ID | 20230322113249.2039-1-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Print DCO client stats on SIGUSR2 | expand |
Hi, On Wed, Mar 22, 2023 at 7:34 AM Lev Stipakov <lstipakov@gmail.com> wrote: > From: Lev Stipakov <lev@openvpn.net> > > Change-Id: I465febdf7ee5fe573e88255844f718efb60f8e8a > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > src/openvpn/sig.c | 13 +++++++++---- > src/openvpn/sig.h | 2 +- > 2 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c > index 5b89bb42..05c0054b 100644 > --- a/src/openvpn/sig.c > +++ b/src/openvpn/sig.c > @@ -300,18 +300,23 @@ restore_signal_state(void) > * Triggered by SIGUSR2 or F2 on Windows. > */ > void > -print_status(const struct context *c, struct status_output *so) > +print_status(struct context *c, struct status_output *so) { > struct gc_arena gc = gc_new(); > > status_reset(so); > > + if (dco_enabled(&c->options)) > + { > + dco_get_peer_stats(c); > + } > + > While this is expedient, adding dco dependence to parts of code which should be independent of such inner details look wrong style to me. Finding a way to keep struct context a const here will lead to a better logic, I think. Can't we have a coarse timer that keeps the stats updated? It doesn't have to be frequent as this is just for information. > status_printf(so, "OpenVPN STATISTICS"); > status_printf(so, "Updated,%s", time_string(0, 0, false, &gc)); > status_printf(so, "TUN/TAP read bytes," counter_format, > c->c2.tun_read_bytes); > status_printf(so, "TUN/TAP write bytes," counter_format, > c->c2.tun_write_bytes); > - status_printf(so, "TCP/UDP read bytes," counter_format, > c->c2.link_read_bytes); > - status_printf(so, "TCP/UDP write bytes," counter_format, > c->c2.link_write_bytes); > + status_printf(so, "TCP/UDP read bytes," counter_format, > c->c2.link_read_bytes + c->c2.dco_read_bytes); > + status_printf(so, "TCP/UDP write bytes," counter_format, > c->c2.link_write_bytes + c->c2.dco_write_bytes); > Same here -- have a place to read the total count from independent of dco. Selva
Hi, > While this is expedient, adding dco dependence to parts of code which should be independent of such inner details look wrong style to me. Finding a way to keep struct context a const here will lead to a better logic, I think. We have the same approach in multi_print_status(). Not saying that "that's why this is right", but we should at least be consistent. > Can't we have a coarse timer that keeps the stats updated? It doesn't have to be frequent as this is just for information. I don't think that "purity" would justify the complexity of adding an additional timer for fetching dco-specific stats. >> + status_printf(so, "TCP/UDP read bytes," counter_format, c->c2.link_read_bytes + c->c2.dco_read_bytes); >> + status_printf(so, "TCP/UDP write bytes," counter_format, c->c2.link_write_bytes + c->c2.dco_write_bytes); > > Same here -- have a place to read the total count from independent of dco. But what is wrong in fetching dco stats when we need them? What is the advantage of timer? -Lev
Hi, On Wed, Mar 22, 2023 at 09:03:02AM -0400, Selva Nair wrote: > Can't we have a coarse timer that keeps the stats updated? It doesn't have > to be frequent as this is just for information. I can see why you do not like the current style - but doing this with timers is something I'm not really happy with. If nobody ever presses F2 / sends SIGUSR2, we spend useless going in and out of the kernel to get statistics that nobody cares about. *If* someone wants to see what happens, and sends SIGUSR2 in short intervals ("ok, so we have 4 kbyte now, let's run $testprogram, and see what the value is afterwards"), the counters printed will be in accurate, because we've not polled the now-current numbers. (For the status file stats, the writing-of-the file is timer triggered, so then using the same event to poll the data makes sense) > > + status_printf(so, "TCP/UDP read bytes," counter_format, > > c->c2.link_read_bytes + c->c2.dco_read_bytes); > > + status_printf(so, "TCP/UDP write bytes," counter_format, > > c->c2.link_write_bytes + c->c2.dco_write_bytes); > > Same here -- have a place to read the total count from independent of dco. This is something the FreeBSD DCO counter patch introduced, and which might not have been the best idea - I think the idea was "leave the userland counters alone", but thinking about it, these should always be 0 anyway, no? So we could just overwrite them. I tend to merge this patch "as is", because it did not introduce the debatable way to do DCO counters - and come back later when we have all platforms and features working and have a better understanding of the code paths involved (like, --inactive) and then try to make it more nice. (Yes, I know, good intentions and all that) gert
Acked-by: Gert Doering <gert@greenie.muc.de> I have listened to the discussion, and I think we all agree that we need to revisit this "DCO counter" business: - definition of c2 structure elements - do we need extra fields for "dco counters"? - do we need more counters? Windows currently has "TransportBytes*" and "TunBytes*", while FreeBSD only has "bytes" (which are what, exactly?) -> *I* definitely need a better understanding of these - when and where do we query the kernel for counters (timer, signal, ...?) This said, we want the functionality now :-) - so, I have tested this (on FreeBSD), and it does what it says, and the code looks reasonable according to "it will not access undefined pointers, will not leak memory" etc. Your patch has been applied to the master and release/2.6 branch. commit d5238627e4fab93a6c09816c60eb90e237b626c3 (master) commit d598871ca9fb7b4814ee8d8edfb26d20479bb6ed (release/2.6) Author: Lev Stipakov Date: Wed Mar 22 13:32:49 2023 +0200 Print DCO client stats on SIGUSR2 Signed-off-by: Lev Stipakov <lev@openvpn.net> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20230322113249.2039-1-lstipakov@gmail.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26471.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
I didn't realize it until Lev pointed out that this reply yesterday didn't go to the list. FTR, copying to the list. ---------- Forwarded message --------- From: Selva Nair <selva.nair@gmail.com> Date: Wed, Mar 22, 2023 at 9:42 AM Subject: Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2 To: Lev Stipakov <lev@openvpn.net> > > > >> + status_printf(so, "TCP/UDP read bytes," counter_format, > c->c2.link_read_bytes + c->c2.dco_read_bytes); > >> + status_printf(so, "TCP/UDP write bytes," counter_format, > c->c2.link_write_bytes + c->c2.dco_write_bytes); > > > > Same here -- have a place to read the total count from independent of > dco. > > But what is wrong in fetching dco stats when we need them? What is the > advantage of timer? > What happens when we have three other drivers like DCO? Not saying that will ever happen, but using that as an example to clarify why I think this is "wrong" style. That said, I agree why a timer may not be a good idea either. If an approach that can keep internal details "private" is going to be too complex, we can live with this. Selva
For reference, my comments.
---------- Forwarded message ---------
From: Lev Stipakov <lev@openvpn.net>
Date: Thu, Mar 23, 2023 at 9:39 AM
Subject: Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2
To: Selva Nair <selva.nair@gmail.com>
I see the point - we now have driver-specific code in functions
{multi}_print_status which are supposed to be driver-agnostic and it
doesn't scale well when/if we add another driver. Also in general it
feels wrong that
print_ functions do print-unrelated things, like calls to the network driver.
I think in the long run we might want to get rid of
dco_read/write_bytes and just store dco stats into
link_read/write_bytes. I remember that in Linux (and likely in
FreeBSD?) we do initial handshake in userspace and
then switch to dco - probably that was the rationale of keeping link
and dco stats separately. Not sure if stats granularity justifies
complexity of adding driver-specific counters.
Your reply wasn't sent to -devel, not sure if it was intentional.
-Lev
diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c index 5b89bb42..05c0054b 100644 --- a/src/openvpn/sig.c +++ b/src/openvpn/sig.c @@ -300,18 +300,23 @@ restore_signal_state(void) * Triggered by SIGUSR2 or F2 on Windows. */ void -print_status(const struct context *c, struct status_output *so) +print_status(struct context *c, struct status_output *so) { struct gc_arena gc = gc_new(); status_reset(so); + if (dco_enabled(&c->options)) + { + dco_get_peer_stats(c); + } + status_printf(so, "OpenVPN STATISTICS"); status_printf(so, "Updated,%s", time_string(0, 0, false, &gc)); status_printf(so, "TUN/TAP read bytes," counter_format, c->c2.tun_read_bytes); status_printf(so, "TUN/TAP write bytes," counter_format, c->c2.tun_write_bytes); - status_printf(so, "TCP/UDP read bytes," counter_format, c->c2.link_read_bytes); - status_printf(so, "TCP/UDP write bytes," counter_format, c->c2.link_write_bytes); + status_printf(so, "TCP/UDP read bytes," counter_format, c->c2.link_read_bytes + c->c2.dco_read_bytes); + status_printf(so, "TCP/UDP write bytes," counter_format, c->c2.link_write_bytes + c->c2.dco_write_bytes); status_printf(so, "Auth read bytes," counter_format, c->c2.link_read_bytes_auth); #ifdef USE_COMP if (c->c2.comp_context) @@ -402,7 +407,7 @@ remap_signal(struct context *c) } static void -process_sigusr2(const struct context *c) +process_sigusr2(struct context *c) { struct status_output *so = status_open(NULL, 0, M_INFO, NULL, 0); print_status(c, so); diff --git a/src/openvpn/sig.h b/src/openvpn/sig.h index 4858eb93..b09dfab6 100644 --- a/src/openvpn/sig.h +++ b/src/openvpn/sig.h @@ -69,7 +69,7 @@ void restore_signal_state(void); void print_signal(const struct signal_info *si, const char *title, int msglevel); -void print_status(const struct context *c, struct status_output *so); +void print_status(struct context *c, struct status_output *so); void remap_signal(struct context *c);