Re: [PATCH net-next v3 12/24] ovpn: store tunnel and transport statistics

Sabrina Dubroca
2024-05-13T09:19:37Z

2024-05-13, 09:25:29 +0200, Antonio Quartulli wrote:
> On 12/05/2024 10:47, Sabrina Dubroca wrote:
> > 2024-05-06, 03:16:25 +0200, Antonio Quartulli wrote:
> > > Byte/packet counters for in-tunnel and transport streams
> > > are now initialized and updated as needed.
> > >
> > > To be exported via netlink.
> > >
> > > Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
> > > ---
> > > drivers/net/ovpn/Makefile | 1 +
> > > drivers/net/ovpn/io.c | 10 ++++++++
> > > drivers/net/ovpn/peer.c | 3 +++
> > > drivers/net/ovpn/peer.h | 13 +++++++---
> > > drivers/net/ovpn/stats.c | 21 ++++++++++++++++
> > > drivers/net/ovpn/stats.h | 52 +++++++++++++++++++++++++++++++++++++++
> >
> > What I'm seeing in this patch are "success" counters. I don't see any
> > stats for dropped packets that would help the user figure out why
> > their VPN isn't working, or why their CPU is burning up decrypting
> > packets that don't show up on the host, etc. You can guess there are
> > issues by subtracting the link and vpn stats, but that's very limited.
>
> This stats are just the bare minimum to make our current userspace happy :-)
>
> But we can always extend the stats reporting later on, no?
>
> >
> > For example:
> > - counter for packets dropped during the udp encap/decap
> > - counter for failed encrypt/decrypt (especially failed decrypt)
> > - counter for replay protection failures
> > - counter for malformed packets
> >
> > Maybe not a separate counter for each of the prints you added in the
> > rx/tx code, but at least enough of them to start figuring out what's
> > going on without enabling all the prints and parsing dmesg.
>
> Definitely a good suggestion! I'd just postpone it for later, unless you
> think it's a blocker.

I'm not sure. It's not strictly necessary to make the driver work, but
from a user/admin's point of view, I think counters would be really
useful.

Maybe at least increment the rx_dropped/rx_errors/etc counters from
rtnl_link_stats on the netdevice?

> indeed, darn. Juggling this many patches has been quite tedious
>
>
>
> Yeah..I guess I'll write a small script to catch all these things..it's easy
> to lose them across the whole patchset.
>
> Thanks for spotting them! I will make sure they all go away

Thanks. I know it's painful :(

--
Sabrina