diff mbox series

[Openvpn-devel,v2] management: ensure consistent BYTECOUNT timing on server

Message ID 20251021070825.20773-1-gert@greenie.muc.de
State New
Headers show
Series [Openvpn-devel,v2] management: ensure consistent BYTECOUNT timing on server | expand

Commit Message

Gert Doering Oct. 21, 2025, 7:08 a.m. UTC 
From: Ralf Lici <ralf@mandelbit.com>

The BYTECOUNT notification is expected to be emitted every N seconds
when a management client issues the 'bytecount N' command. However, the
server currently relies on timeouts from unrelated periodic operations,
resulting in irregular notification timing.

This issue is especially noticeable with low bytecount intervals and DCO
enabled, as openvpn handles less traffic in userspace, causing the main
loop to run less frequently.

To address this, refactor the timeout logic and pass the timeval
reference to management_check_bytecount_server so that the timeout is
correctly set and notifications adhere to the specified interval.

Change-Id: Ifb1c49fce75e671f699f5db5f6da7246f6e0b519
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Acked-by: Lev Stipakov <lstipakov@gmail.com>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1285
This mail reflects revision 2 of this Change.

Acked-by according to Gerrit (reflected above):
Lev Stipakov <lstipakov@gmail.com>

Comments

Gert Doering Oct. 28, 2025, 6:53 p.m. UTC | #1 
Took a bit between +2 and merge, but here we are :-)

Tested on FreeBSD+DCO with "bytecount 2", which used to do "10 seconds
and no less".  New code does 2s if 2s is ordered...  whether or not that
is *useful* depends on the management client, I'd say :-) - but having
consistent behaviour is a good thing.

Looking at the code, I did wonder why multi_get_timeout_instance()
lives in multi.h, when it is only used from multi.c - this is actually
a multisocket artefact.  2.6 calls this from mudp.c and mtcp.c, so
it made sense.  master/2.7 only calls this from multi.c, so we could
move it in there eventually.

Your patch has been applied to the master branch.

commit a5ee6a1d642ae365eae78fd04d51976c365b2b59
Author: Ralf Lici
Date:   Tue Oct 21 09:08:20 2025 +0200

     management: ensure consistent BYTECOUNT timing on server

     Signed-off-by: Ralf Lici <ralf@mandelbit.com>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20251021070825.20773-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg33812.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index 1cb5c63..685b137 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -4184,16 +4184,14 @@ 
 }
 
 void
-management_check_bytecount_server(struct multi_context *multi)
+management_check_bytecount_server(struct multi_context *multi, struct timeval *timeval)
 {
     if (!(management->persist.callback.flags & MCF_SERVER))
     {
         return;
     }
 
-    struct timeval null;
-    CLEAR(null);
-    if (event_timeout_trigger(&management->connection.bytecount_update_interval, &null, ETT_DEFAULT))
+    if (event_timeout_trigger(&management->connection.bytecount_update_interval, timeval, ETT_DEFAULT))
     {
         /* fetch counters from dco */
         if (dco_enabled(&multi->top.options))
diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
index a31eb06..fe460bf 100644
--- a/src/openvpn/manage.h
+++ b/src/openvpn/manage.h
@@ -493,7 +493,7 @@ 
 
 void management_check_bytecount_client(struct context *c, struct management *man, struct timeval *timeval);
 
-void management_check_bytecount_server(struct multi_context *multi);
+void management_check_bytecount_server(struct multi_context *multi, struct timeval *timeval);
 
 void man_persist_client_stats(struct management *man, struct context *c);
 
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index e907524..d512c98 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3809,13 +3809,6 @@ 
     {
         check_stale_routes(m);
     }
-
-#ifdef ENABLE_MANAGEMENT
-    if (management)
-    {
-        management_check_bytecount_server(m);
-    }
-#endif /* ENABLE_MANAGEMENT */
 }
 
 static void
@@ -4166,6 +4159,29 @@ 
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
 }
 
+/**
+ * @brief Determines the earliest wakeup interval based on periodic operations.
+ *
+ * Updates the \c timeval to reflect the next scheduled wakeup time.
+ * Also sets \c multi->earliest_wakeup to the instance with the earliest wakeup.
+ *
+ * @param multi     Pointer to the multi context
+ * @param timeval   Pointer to the timeval structure to be updated with the
+ *                  next wakeup time
+ */
+static void
+multi_get_timeout(struct multi_context *multi, struct timeval *timeval)
+{
+    multi_get_timeout_instance(multi, timeval);
+
+#ifdef ENABLE_MANAGEMENT
+    if (management)
+    {
+        management_check_bytecount_server(multi, timeval);
+    }
+#endif /* ENABLE_MANAGEMENT */
+}
+
 /**************************************************************************/
 /**
  * Main event loop for OpenVPN in point-to-multipoint server mode.
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 97bbc4a..960e0be 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -582,15 +582,16 @@ 
 }
 
 /*
- * Compute earliest timeout expiry from the set of
- * all instances.  Output:
+ * Updates \c dest with the earliest timeout as a delta relative to the current
+ * time and sets \c m->earliest_wakeup to the \c multi_instance with the
+ * soonest scheduled wakeup.
  *
- * m->earliest_wakeup : instance needing the earliest service.
- * dest               : earliest timeout as a delta in relation
- *                      to current time.
+ * @param m     Pointer to the multi context
+ * @param dest  Pointer to a timeval struct that will hold the earliest timeout
+ *              delta.
  */
 static inline void
-multi_get_timeout(struct multi_context *m, struct timeval *dest)
+multi_get_timeout_instance(struct multi_context *m, struct timeval *dest)
 {
     struct timeval tv, current;