Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2
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 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2
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
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2
Hi,
On Wed, Mar 22, 2023 at 7:34 AM Lev Stipakov wrote:
> From: Lev Stipakov
>
> Change-Id: I465febdf7ee5fe573e88255844f718efb60f8e8a
> Signed-off-by: Lev Stipakov
> ---
> 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(>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, ));
> 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
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Print DCO client stats on SIGUSR2
From: Lev Stipakov
Change-Id: I465febdf7ee5fe573e88255844f718efb60f8e8a
Signed-off-by: Lev Stipakov
---
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(>options))
+{
+dco_get_peer_stats(c);
+}
+
status_printf(so, "OpenVPN STATISTICS");
status_printf(so, "Updated,%s", time_string(0, 0, false, ));
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);
--
2.38.1.windows.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
