diff mbox series

[Openvpn-devel,v2] management: add timer to output BYTECOUNT

Message ID 20221214224220.307-1-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v2] management: add timer to output BYTECOUNT | expand

Commit Message

Lev Stipakov Dec. 14, 2022, 10:42 p.m. UTC 
From: Lev Stipakov <lev@openvpn.net>

BYTECOUNT on management interface is used to display client stats,
for example by openvpn-gui. At the moment BYTECOUNT is sent if
there is a traffic. With DCO, userspace process doesn't see data
channel traffic, BYTECOUNT is not sent and therefore stats
are not updated.

Fix displaying DCO client stats by adding a timer, which is triggerd
every n seconds, where n is set by existing management command
bytecount <n>. Output stats, taking into account stats from DCO,
when timer is triggered.

While on it, simplify bytecount routines call chains - inlining
functions which are used only once.

DCO stats fetching is not yet implemented.

Stats for the server mode (BYTECOUNT_CLI) are unaffected
by this change - to output those in timer callback we would need to
enumerate all peers, and I am not sure we want to output stats
for all peers every <n> seconds.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 v2: handle "bytecount 0" case and disable timer
 
 src/openvpn/forward.c | 11 ++++++--
 src/openvpn/manage.c  | 42 ++++++++++++++++++++++++------
 src/openvpn/manage.h  | 60 ++++++++++++-------------------------------
 3 files changed, 59 insertions(+), 54 deletions(-)

Comments

Arne Schwabe Dec. 15, 2022, 10:44 a.m. UTC | #1 
Am 14.12.22 um 23:42 schrieb Lev Stipakov:
> From: Lev Stipakov <lev@openvpn.net>
> 
> BYTECOUNT on management interface is used to display client stats,
> for example by openvpn-gui. At the moment BYTECOUNT is sent if
> there is a traffic. With DCO, userspace process doesn't see data
> channel traffic, BYTECOUNT is not sent and therefore stats
> are not updated.
> 
> Fix displaying DCO client stats by adding a timer, which is triggerd
> every n seconds, where n is set by existing management command
> bytecount <n>. Output stats, taking into account stats from DCO,
> when timer is triggered.
> 
> While on it, simplify bytecount routines call chains - inlining
> functions which are used only once.
> 
> DCO stats fetching is not yet implemented.
> 
> Stats for the server mode (BYTECOUNT_CLI) are unaffected
> by this change - to output those in timer callback we would need to
> enumerate all peers, and I am not sure we want to output stats
> for all peers every <n> seconds.
> 

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering Dec. 15, 2022, 12:34 p.m. UTC | #2 
Arne, thanks for the review and ACK.

I have done quite a bit of stare-at-code on this patch myself, and think
I understand what it does - change from "whenever userland updates the
counters, check if mgmt needs to be informed" to "inform mgmt via the
coarse timers", because there are no more userland packets to trigger
this check.  But having the review from someone who understands mgmt 
much better is very welcome :-)

As a side effect, management_bytes_in() and _out() are consolidated
into a single function, which (due to being inlined) should do the
same thing with the same efficiency, but it's a bit easier to see
what happens.

I have subjected this to the full server/client tests on Linux and 
FreeBSD, with/without DCO - and everything works as before.

I have also tested this by connecting to the management interface to
verify that the counters + event stuff works.  No DCO involved, so 
counters were displayed in the expected intervals and values matched
what I expected.  Also, "bytecount 0" works to disable it again.

On a (FreeBSD) DCO system, the effect was not exactly what I expected
- I expected to see >BYTECOUNT: updates "with no interesting content",
but actually after setting "bytecount 10", nothing happened for a 
few minutes - and then it started to work as ordered (without data packets,
of course).  So it seems as the timer activation got delayed somehow
-> this is something we need to come back to, when Linux/FreeBSD DCO
stats get implemented.  Starting with "management-hold", then "bytecount",
then "hold release" gave me counters right away.  Mysteries...


Your patch has been applied to the master and release/2.6 branch.

commit a9991b3eb6644785421398bff8cb3a728d131713 (master)
commit 59a96c3ee05cdb842af0956de0a2ba3ffd993980 (release/2.6)
Author: Lev Stipakov
Date:   Thu Dec 15 00:42:20 2022 +0200

     management: add timer to output BYTECOUNT

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20221214224220.307-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25707.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Lev Stipakov Dec. 15, 2022, 12:48 p.m. UTC | #3 
Hi,

> So it seems as the timer activation got delayed somehow
> -> this is something we need to come back to, when Linux/FreeBSD DCO
> stats get implemented.  Starting with "management-hold", then "bytecount",
> then "hold release" gave me counters right away.  Mysteries...

This is because we do not trigger this new timer (for example by
calling reset_coarse_timers)
when we init it in man_bytecount(). We do not have "struct context"
there to do it.

Follow-up calls to reset_coarse_timers() or process_coarse_timers()
will do the trick. However, in DCO case nothing might happen in userspace
for a while, hence delay. GUI does "management-hold", sends "bytecount 5"
and after that "hold-release".

-Lev
Gert Doering Dec. 15, 2022, 2:16 p.m. UTC | #4 
Hi,

On Thu, Dec 15, 2022 at 02:48:49PM +0200, Lev Stipakov wrote:
> > So it seems as the timer activation got delayed somehow
> > -> this is something we need to come back to, when Linux/FreeBSD DCO
> > stats get implemented.  Starting with "management-hold", then "bytecount",
> > then "hold release" gave me counters right away.  Mysteries...
> 
> This is because we do not trigger this new timer (for example by
> calling reset_coarse_timers)
> when we init it in man_bytecount(). We do not have "struct context"
> there to do it.

Ah.  I guessed something like this - thanks for explaining why this would
not actually be easy to achive.

> Follow-up calls to reset_coarse_timers() or process_coarse_timers()
> will do the trick. However, in DCO case nothing might happen in userspace
> for a while, hence delay. GUI does "management-hold", sends "bytecount 5"
> and after that "hold-release".

This works (tested!), so the point above is not a serious problem.

gert
diff mbox series

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 8c1e49a3..7924fd5c 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -771,6 +771,13 @@  process_coarse_timers(struct context *c)
 
     /* Should we ping the remote? */
     check_ping_send(c);
+
+#ifdef ENABLE_MANAGEMENT
+    if (management)
+    {
+        management_check_bytecount(c, management, &c->c2.timeval);
+    }
+#endif /* ENABLE_MANAGEMENT */
 }
 
 static void
@@ -953,7 +960,7 @@  process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
 #ifdef ENABLE_MANAGEMENT
         if (management)
         {
-            management_bytes_in(management, c->c2.buf.len);
+            management_bytes_client(management, c->c2.buf.len, 0);
             management_bytes_server(management, &c->c2.link_read_bytes, &c->c2.link_write_bytes, &c->c2.mda_context);
         }
 #endif
@@ -1793,7 +1800,7 @@  process_outgoing_link(struct context *c)
 #ifdef ENABLE_MANAGEMENT
                 if (management)
                 {
-                    management_bytes_out(management, size);
+                    management_bytes_client(management, 0, size);
                     management_bytes_server(management, &c->c2.link_read_bytes, &c->c2.link_write_bytes, &c->c2.mda_context);
                 }
 #endif
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index 5b288eab..c2c5da56 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -42,6 +42,7 @@ 
 #include "ssl.h"
 #include "common.h"
 #include "manage.h"
+#include "openvpn.h"
 
 #include "memdbg.h"
 
@@ -460,32 +461,37 @@  man_status(struct management *man, const int version, struct status_output *so)
 static void
 man_bytecount(struct management *man, const int update_seconds)
 {
-    if (update_seconds >= 0)
+    if (update_seconds > 0)
     {
         man->connection.bytecount_update_seconds = update_seconds;
+        event_timeout_init(&man->connection.bytecount_update_interval,
+                           man->connection.bytecount_update_seconds,
+                           now);
     }
     else
     {
         man->connection.bytecount_update_seconds = 0;
+        event_timeout_clear(&man->connection.bytecount_update_interval);
     }
     msg(M_CLIENT, "SUCCESS: bytecount interval changed");
 }
 
-void
-man_bytecount_output_client(struct management *man)
+static void
+man_bytecount_output_client(struct management *man,
+                            counter_type dco_read_bytes,
+                            counter_type dco_write_bytes)
 {
     char in[32];
     char out[32];
+
     /* do in a roundabout way to work around possible mingw or mingw-glibc bug */
-    openvpn_snprintf(in, sizeof(in), counter_format, man->persist.bytes_in);
-    openvpn_snprintf(out, sizeof(out), counter_format, man->persist.bytes_out);
+    openvpn_snprintf(in, sizeof(in), counter_format, man->persist.bytes_in + dco_read_bytes);
+    openvpn_snprintf(out, sizeof(out), counter_format, man->persist.bytes_out + dco_write_bytes);
     msg(M_CLIENT, ">BYTECOUNT:%s,%s", in, out);
-    man->connection.bytecount_last_update = now;
 }
 
 void
-man_bytecount_output_server(struct management *man,
-                            const counter_type *bytes_in_total,
+man_bytecount_output_server(const counter_type *bytes_in_total,
                             const counter_type *bytes_out_total,
                             struct man_def_auth_context *mdac)
 {
@@ -2542,6 +2548,8 @@  man_connection_close(struct management *man)
     command_line_free(mc->in);
     buffer_list_free(mc->out);
 
+    event_timeout_clear(&mc->bytecount_update_interval);
+
     in_extra_reset(&man->connection, IER_RESET);
     buffer_list_free(mc->ext_key_input);
     man_connection_clear(mc);
@@ -4037,6 +4045,24 @@  management_sleep(const int n)
     }
 }
 
+void
+management_check_bytecount(struct context *c, struct management *man, struct timeval *timeval)
+{
+    if (event_timeout_trigger(&man->connection.bytecount_update_interval,
+                              timeval, ETT_DEFAULT))
+    {
+        /* TODO: get stats from DCO */
+
+        counter_type dco_read_bytes = 0;
+        counter_type dco_write_bytes = 0;
+
+        if (!(man->persist.callback.flags & MCF_SERVER))
+        {
+            man_bytecount_output_client(man, dco_read_bytes, dco_write_bytes);
+        }
+    }
+}
+
 #else  /* ifdef ENABLE_MANAGEMENT */
 
 void
diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
index f46274e6..700b15cf 100644
--- a/src/openvpn/manage.h
+++ b/src/openvpn/manage.h
@@ -295,7 +295,7 @@  struct man_connection {
     bool log_realtime;
     bool echo_realtime;
     int bytecount_update_seconds;
-    time_t bytecount_last_update;
+    struct event_timeout bytecount_update_interval;
 
     const char *up_query_type;
     int up_query_mode;
@@ -512,55 +512,27 @@  void management_auth_token(struct management *man, const char *token);
  * These functions drive the bytecount in/out counters.
  */
 
-void man_bytecount_output_client(struct management *man);
-
-static inline void
-man_bytecount_possible_output_client(struct management *man)
-{
-    if (man->connection.bytecount_update_seconds > 0
-        && now >= man->connection.bytecount_last_update
-        + man->connection.bytecount_update_seconds)
-    {
-        man_bytecount_output_client(man);
-    }
-}
-
-static inline void
-management_bytes_out_client(struct management *man, const int size)
-{
-    man->persist.bytes_out += size;
-    man_bytecount_possible_output_client(man);
-}
-
-static inline void
-management_bytes_in_client(struct management *man, const int size)
-{
-    man->persist.bytes_in += size;
-    man_bytecount_possible_output_client(man);
-}
-
-static inline void
-management_bytes_out(struct management *man, const int size)
-{
-    if (!(man->persist.callback.flags & MCF_SERVER))
-    {
-        management_bytes_out_client(man, size);
-    }
-}
+void
+management_check_bytecount(struct context *c,
+                           struct management *man,
+                           struct timeval *timeval);
 
 static inline void
-management_bytes_in(struct management *man, const int size)
+management_bytes_client(struct management *man,
+                        const int size_in,
+                        const int size_out)
 {
     if (!(man->persist.callback.flags & MCF_SERVER))
     {
-        management_bytes_in_client(man, size);
+        man->persist.bytes_in += size_in;
+        man->persist.bytes_out += size_out;
     }
 }
 
-void man_bytecount_output_server(struct management *man,
-                                 const counter_type *bytes_in_total,
-                                 const counter_type *bytes_out_total,
-                                 struct man_def_auth_context *mdac);
+void
+man_bytecount_output_server(const counter_type *bytes_in_total,
+                            const counter_type *bytes_out_total,
+                            struct man_def_auth_context *mdac);
 
 static inline void
 management_bytes_server(struct management *man,
@@ -570,9 +542,9 @@  management_bytes_server(struct management *man,
 {
     if (man->connection.bytecount_update_seconds > 0
         && now >= mdac->bytecount_last_update + man->connection.bytecount_update_seconds
-        && (mdac->flags & (DAF_CONNECTION_ESTABLISHED|DAF_CONNECTION_CLOSED)) == DAF_CONNECTION_ESTABLISHED)
+        && (mdac->flags & (DAF_CONNECTION_ESTABLISHED | DAF_CONNECTION_CLOSED)) == DAF_CONNECTION_ESTABLISHED)
     {
-        man_bytecount_output_server(man, bytes_in_total, bytes_out_total, mdac);
+        man_bytecount_output_server(bytes_in_total, bytes_out_total, mdac);
     }
 }