There are few issues with it:
- when using DCO, the server part doesn't output BYTECOUNT_CLI since
process_incoming_link_part1/process_outgoing_link are not called
- when using DCO, the server part applies bytecount timer to the each
connection, unneccessary making too many calls to the kernel and also
uses incorrect BYTECOUNT output.
- client part outputs counters using timer, server part utilizes
traffic activity -> inconsistency
Following changes have been made:
- Use timer to output counters in client and server mode. Code which
deals with bytecount on traffic activity has been removed. This unifies
DCO and non-DCO, as well as client and server mode
- In server mode, peers stats are fetched with the single ioctl call
- Per-packet stats are not persisted anymore in the client mode during
traffic activity. Instead cumulative stats (including DCO stats) are
persisted when the session closes.
GitHub: closes OpenVPN/openvpn#820
Change-Id: I43a93f0d84f01fd808a64115e1b8c3b806706491
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20250902103606.22181-1-gert@greenie.muc.de>
URL: https://sourceforge.net/p/openvpn/mailman/message/59228150/
Signed-off-by: Gert Doering <gert@greenie.muc.de>
| ... | ... |
@@ -818,7 +818,7 @@ process_coarse_timers(struct context *c) |
| 818 | 818 |
#ifdef ENABLE_MANAGEMENT |
| 819 | 819 |
if (management) |
| 820 | 820 |
{
|
| 821 |
- management_check_bytecount(c, management, &c->c2.timeval); |
|
| 821 |
+ management_check_bytecount_client(c, management, &c->c2.timeval); |
|
| 822 | 822 |
} |
| 823 | 823 |
#endif /* ENABLE_MANAGEMENT */ |
| 824 | 824 |
} |
| ... | ... |
@@ -998,14 +998,6 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo |
| 998 | 998 |
} |
| 999 | 999 |
#endif |
| 1000 | 1000 |
c->c2.original_recv_size = c->c2.buf.len; |
| 1001 |
-#ifdef ENABLE_MANAGEMENT |
|
| 1002 |
- if (management) |
|
| 1003 |
- {
|
|
| 1004 |
- management_bytes_client(management, c->c2.buf.len, 0); |
|
| 1005 |
- management_bytes_server(management, &c->c2.link_read_bytes, &c->c2.link_write_bytes, |
|
| 1006 |
- &c->c2.mda_context); |
|
| 1007 |
- } |
|
| 1008 |
-#endif |
|
| 1009 | 1001 |
} |
| 1010 | 1002 |
else |
| 1011 | 1003 |
{
|
| ... | ... |
@@ -1823,14 +1815,6 @@ process_outgoing_link(struct context *c, struct link_socket *sock) |
| 1823 | 1823 |
mmap_stats->link_write_bytes = link_write_bytes_global; |
| 1824 | 1824 |
} |
| 1825 | 1825 |
#endif |
| 1826 |
-#ifdef ENABLE_MANAGEMENT |
|
| 1827 |
- if (management) |
|
| 1828 |
- {
|
|
| 1829 |
- management_bytes_client(management, 0, size); |
|
| 1830 |
- management_bytes_server(management, &c->c2.link_read_bytes, |
|
| 1831 |
- &c->c2.link_write_bytes, &c->c2.mda_context); |
|
| 1832 |
- } |
|
| 1833 |
-#endif |
|
| 1834 | 1826 |
} |
| 1835 | 1827 |
} |
| 1836 | 1828 |
|
| ... | ... |
@@ -41,6 +41,7 @@ |
| 41 | 41 |
#include "manage.h" |
| 42 | 42 |
#include "openvpn.h" |
| 43 | 43 |
#include "dco.h" |
| 44 |
+#include "multi.h" |
|
| 44 | 45 |
|
| 45 | 46 |
#include "memdbg.h" |
| 46 | 47 |
|
| ... | ... |
@@ -517,29 +518,27 @@ man_bytecount(struct management *man, const int update_seconds) |
| 517 | 517 |
} |
| 518 | 518 |
|
| 519 | 519 |
static void |
| 520 |
-man_bytecount_output_client(struct management *man, counter_type dco_read_bytes, |
|
| 521 |
- counter_type dco_write_bytes) |
|
| 520 |
+man_bytecount_output_client(counter_type bytes_in_total, counter_type bytes_out_total) |
|
| 522 | 521 |
{
|
| 523 | 522 |
char in[32]; |
| 524 | 523 |
char out[32]; |
| 525 | 524 |
|
| 526 | 525 |
/* do in a roundabout way to work around possible mingw or mingw-glibc bug */ |
| 527 |
- snprintf(in, sizeof(in), counter_format, man->persist.bytes_in + dco_read_bytes); |
|
| 528 |
- snprintf(out, sizeof(out), counter_format, man->persist.bytes_out + dco_write_bytes); |
|
| 526 |
+ snprintf(in, sizeof(in), counter_format, bytes_in_total); |
|
| 527 |
+ snprintf(out, sizeof(out), counter_format, bytes_out_total); |
|
| 529 | 528 |
msg(M_CLIENT, ">BYTECOUNT:%s,%s", in, out); |
| 530 | 529 |
} |
| 531 | 530 |
|
| 532 |
-void |
|
| 533 |
-man_bytecount_output_server(const counter_type *bytes_in_total, const counter_type *bytes_out_total, |
|
| 531 |
+static void |
|
| 532 |
+man_bytecount_output_server(const counter_type bytes_in_total, const counter_type bytes_out_total, |
|
| 534 | 533 |
struct man_def_auth_context *mdac) |
| 535 | 534 |
{
|
| 536 | 535 |
char in[32]; |
| 537 | 536 |
char out[32]; |
| 538 | 537 |
/* do in a roundabout way to work around possible mingw or mingw-glibc bug */ |
| 539 |
- snprintf(in, sizeof(in), counter_format, *bytes_in_total); |
|
| 540 |
- snprintf(out, sizeof(out), counter_format, *bytes_out_total); |
|
| 538 |
+ snprintf(in, sizeof(in), counter_format, bytes_in_total); |
|
| 539 |
+ snprintf(out, sizeof(out), counter_format, bytes_out_total); |
|
| 541 | 540 |
msg(M_CLIENT, ">BYTECOUNT_CLI:%lu,%s,%s", mdac->cid, in, out); |
| 542 |
- mdac->bytecount_last_update = now; |
|
| 543 | 541 |
} |
| 544 | 542 |
|
| 545 | 543 |
static void |
| ... | ... |
@@ -4065,42 +4064,82 @@ management_sleep(const int n) |
| 4065 | 4065 |
} |
| 4066 | 4066 |
|
| 4067 | 4067 |
void |
| 4068 |
-management_check_bytecount(struct context *c, struct management *man, struct timeval *timeval) |
|
| 4068 |
+management_check_bytecount_client(struct context *c, struct management *man, struct timeval *timeval) |
|
| 4069 | 4069 |
{
|
| 4070 |
- if (event_timeout_trigger(&man->connection.bytecount_update_interval, timeval, ETT_DEFAULT)) |
|
| 4070 |
+ if (man->persist.callback.flags & MCF_SERVER) |
|
| 4071 | 4071 |
{
|
| 4072 |
- counter_type dco_read_bytes = 0; |
|
| 4073 |
- counter_type dco_write_bytes = 0; |
|
| 4072 |
+ return; |
|
| 4073 |
+ } |
|
| 4074 | 4074 |
|
| 4075 |
+ if (event_timeout_trigger(&man->connection.bytecount_update_interval, timeval, ETT_DEFAULT)) |
|
| 4076 |
+ {
|
|
| 4075 | 4077 |
if (dco_enabled(&c->options)) |
| 4076 | 4078 |
{
|
| 4077 | 4079 |
if (dco_get_peer_stats(c, true) < 0) |
| 4078 | 4080 |
{
|
| 4079 | 4081 |
return; |
| 4080 | 4082 |
} |
| 4083 |
+ } |
|
| 4084 |
+ |
|
| 4085 |
+ man_bytecount_output_client(c->c2.dco_read_bytes + man->persist.bytes_in + c->c2.link_read_bytes, |
|
| 4086 |
+ c->c2.dco_write_bytes + man->persist.bytes_out + c->c2.link_write_bytes); |
|
| 4087 |
+ } |
|
| 4088 |
+} |
|
| 4089 |
+ |
|
| 4090 |
+void |
|
| 4091 |
+management_check_bytecount_server(struct multi_context *multi) |
|
| 4092 |
+{
|
|
| 4093 |
+ if (!(management->persist.callback.flags & MCF_SERVER)) |
|
| 4094 |
+ {
|
|
| 4095 |
+ return; |
|
| 4096 |
+ } |
|
| 4081 | 4097 |
|
| 4082 |
- dco_read_bytes = c->c2.dco_read_bytes; |
|
| 4083 |
- dco_write_bytes = c->c2.dco_write_bytes; |
|
| 4098 |
+ struct timeval null; |
|
| 4099 |
+ CLEAR(null); |
|
| 4100 |
+ if (event_timeout_trigger(&management->connection.bytecount_update_interval, &null, ETT_DEFAULT)) |
|
| 4101 |
+ {
|
|
| 4102 |
+ /* fetch counters from dco */ |
|
| 4103 |
+ if (dco_enabled(&multi->top.options)) |
|
| 4104 |
+ {
|
|
| 4105 |
+ if (dco_get_peer_stats_multi(&multi->top.c1.tuntap->dco, true) < 0) |
|
| 4106 |
+ {
|
|
| 4107 |
+ return; |
|
| 4108 |
+ } |
|
| 4084 | 4109 |
} |
| 4085 | 4110 |
|
| 4086 |
- if (!(man->persist.callback.flags & MCF_SERVER)) |
|
| 4111 |
+ /* iterate over peers and report counters for each connected peer */ |
|
| 4112 |
+ struct hash_iterator hi; |
|
| 4113 |
+ struct hash_element *he; |
|
| 4114 |
+ hash_iterator_init(multi->hash, &hi); |
|
| 4115 |
+ while ((he = hash_iterator_next(&hi))) |
|
| 4087 | 4116 |
{
|
| 4088 |
- man_bytecount_output_client(man, dco_read_bytes, dco_write_bytes); |
|
| 4117 |
+ struct multi_instance *mi = (struct multi_instance *)he->value; |
|
| 4118 |
+ struct context_2 *c2 = &mi->context.c2; |
|
| 4119 |
+ |
|
| 4120 |
+ if ((c2->mda_context.flags & (DAF_CONNECTION_ESTABLISHED | DAF_CONNECTION_CLOSED)) == DAF_CONNECTION_ESTABLISHED) |
|
| 4121 |
+ {
|
|
| 4122 |
+ man_bytecount_output_server(c2->dco_read_bytes + c2->link_read_bytes, c2->dco_write_bytes + c2->link_write_bytes, &c2->mda_context); |
|
| 4123 |
+ } |
|
| 4089 | 4124 |
} |
| 4125 |
+ hash_iterator_free(&hi); |
|
| 4090 | 4126 |
} |
| 4091 | 4127 |
} |
| 4092 | 4128 |
|
| 4093 |
-/* DCO resets stats on reconnect. Since client expects stats |
|
| 4094 |
- * to be preserved across reconnects, we need to save DCO |
|
| 4129 |
+/* context_2 stats are reset on reconnect. Since client expects stats |
|
| 4130 |
+ * to be preserved across reconnects, we need to save context_2 |
|
| 4095 | 4131 |
* stats before tearing the tunnel down. |
| 4096 | 4132 |
*/ |
| 4097 | 4133 |
void |
| 4098 | 4134 |
man_persist_client_stats(struct management *man, struct context *c) |
| 4099 | 4135 |
{
|
| 4100 |
- /* no need to raise SIGUSR1 since we are already closing the instance */ |
|
| 4136 |
+ man->persist.bytes_in += c->c2.link_read_bytes; |
|
| 4137 |
+ man->persist.bytes_out += c->c2.link_write_bytes; |
|
| 4138 |
+ |
|
| 4139 |
+ /* no need to raise SIGUSR1 on error since we are already closing the instance */ |
|
| 4101 | 4140 |
if (dco_enabled(&c->options) && (dco_get_peer_stats(c, false) == 0)) |
| 4102 | 4141 |
{
|
| 4103 |
- management_bytes_client(man, c->c2.dco_read_bytes, c->c2.dco_write_bytes); |
|
| 4142 |
+ man->persist.bytes_in += c->c2.dco_read_bytes; |
|
| 4143 |
+ man->persist.bytes_out += c->c2.dco_write_bytes; |
|
| 4104 | 4144 |
} |
| 4105 | 4145 |
} |
| 4106 | 4146 |
|
| ... | ... |
@@ -70,8 +70,6 @@ struct man_def_auth_context |
| 70 | 70 |
unsigned int flags; |
| 71 | 71 |
|
| 72 | 72 |
unsigned int mda_key_id_counter; |
| 73 |
- |
|
| 74 |
- time_t bytecount_last_update; |
|
| 75 | 73 |
}; |
| 76 | 74 |
|
| 77 | 75 |
/* |
| ... | ... |
@@ -492,34 +490,9 @@ void management_auth_token(struct management *man, const char *token); |
| 492 | 492 |
* These functions drive the bytecount in/out counters. |
| 493 | 493 |
*/ |
| 494 | 494 |
|
| 495 |
-void management_check_bytecount(struct context *c, struct management *man, struct timeval *timeval); |
|
| 496 |
- |
|
| 497 |
-static inline void |
|
| 498 |
-management_bytes_client(struct management *man, const int size_in, const int size_out) |
|
| 499 |
-{
|
|
| 500 |
- if (!(man->persist.callback.flags & MCF_SERVER)) |
|
| 501 |
- {
|
|
| 502 |
- man->persist.bytes_in += size_in; |
|
| 503 |
- man->persist.bytes_out += size_out; |
|
| 504 |
- } |
|
| 505 |
-} |
|
| 495 |
+void management_check_bytecount_client(struct context *c, struct management *man, struct timeval *timeval); |
|
| 506 | 496 |
|
| 507 |
-void man_bytecount_output_server(const counter_type *bytes_in_total, |
|
| 508 |
- const counter_type *bytes_out_total, |
|
| 509 |
- struct man_def_auth_context *mdac); |
|
| 510 |
- |
|
| 511 |
-static inline void |
|
| 512 |
-management_bytes_server(struct management *man, const counter_type *bytes_in_total, |
|
| 513 |
- const counter_type *bytes_out_total, struct man_def_auth_context *mdac) |
|
| 514 |
-{
|
|
| 515 |
- if (man->connection.bytecount_update_seconds > 0 |
|
| 516 |
- && now >= mdac->bytecount_last_update + man->connection.bytecount_update_seconds |
|
| 517 |
- && (mdac->flags & (DAF_CONNECTION_ESTABLISHED | DAF_CONNECTION_CLOSED)) |
|
| 518 |
- == DAF_CONNECTION_ESTABLISHED) |
|
| 519 |
- {
|
|
| 520 |
- man_bytecount_output_server(bytes_in_total, bytes_out_total, mdac); |
|
| 521 |
- } |
|
| 522 |
-} |
|
| 497 |
+void management_check_bytecount_server(struct multi_context *multi); |
|
| 523 | 498 |
|
| 524 | 499 |
void man_persist_client_stats(struct management *man, struct context *c); |
| 525 | 500 |
|
| ... | ... |
@@ -3812,6 +3812,13 @@ multi_process_per_second_timers_dowork(struct multi_context *m) |
| 3812 | 3812 |
{
|
| 3813 | 3813 |
check_stale_routes(m); |
| 3814 | 3814 |
} |
| 3815 |
+ |
|
| 3816 |
+#ifdef ENABLE_MANAGEMENT |
|
| 3817 |
+ if (management) |
|
| 3818 |
+ {
|
|
| 3819 |
+ management_check_bytecount_server(m); |
|
| 3820 |
+ } |
|
| 3821 |
+#endif /* ENABLE_MANAGEMENT */ |
|
| 3815 | 3822 |
} |
| 3816 | 3823 |
|
| 3817 | 3824 |
static void |