netdev - Re: [PATCH net] bareudp: Fix device stats updates.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZuAfQFbzYchyrybw@debian>
Date: Tue, 10 Sep 2024 12:28:16 +0200
From: Guillaume Nault <gnault@...hat.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>, David Miller <davem@...emloft.net>,
	Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
	Martin Varghese <martin.varghese@...ia.com>,
	Willem de Bruijn <willemb@...gle.com>,
	David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH net] bareudp: Fix device stats updates.

On Fri, Sep 06, 2024 at 02:47:15PM +0200, Eric Dumazet wrote:
> On Fri, Sep 6, 2024 at 12:42 PM Guillaume Nault <gnault@...hat.com> wrote:
> >
> > On Wed, Sep 04, 2024 at 02:48:39PM -0700, Jakub Kicinski wrote:
> > > On Wed, 4 Sep 2024 19:54:40 +0200 Guillaume Nault wrote:
> > > > In this context, I feel that dstats is now just a mix of tstats and
> > > > core_stats.
> > >
> > > I don't know the full background but:
> > >
> > >  *    @core_stats:    core networking counters,
> > >  *                    do not use this in drivers
> >
> > Hum, I didn't realise that :/.
> >
> > I'd really like to understand why drivers shouldn't use core_stats.
> >
> > I mean, what makes driver and core networking counters so different
> > that they need to be handled in two different ways (but finally merged
> > together when exporting stats to user space)?
> >
> > Does that prevent any contention on the counters or optimise cache line
> > access? I can't see how, so I'm probably missing something important
> > here.
> 
> Some archeology might help.
> 
> Before we had tracing, having separate fields could help for diagnostics.

Interesting, I didn't think about it from a diagnostic perspective.

Considering that we now have tracing and skb_drop_reason, does it still
make sense to keep this distinction between core and driver stats?

I find this approach elegant, but since no UDP tunnel respects it and
that dstats are only used by one driver (vrf) I wonder what's the best
path forward.

I'm restricting this discussion to UDP tunnels, as I'd like them to
keep their implementation as consistent as possible (to hopefully ease
code consolidation in the future). But feel free to broaden the scope
if necessary.

I can think of two possibilities:

  1- Follow the core/driver stats policy and convert vxlan, geneve and
     bareudp to dstats (NETDEV_PCPU_STAT_DSTATS).

  2- Give up on that policy, let vxlan and geneve as is and convert
     bareudp to tstats (NETDEV_PCPU_STAT_TSTATS). Then convert vrf to
     tstats too and drop NETDEV_PCPU_STAT_DSTATS which becomes unused.

Any opinion?

> commit caf586e5f23cebb2a68cbaf288d59dbbf2d74052
> Author: Eric Dumazet <eric.dumazet@...il.com>
> Date:   Thu Sep 30 21:06:55 2010 +0000
> 
>     net: add a core netdev->rx_dropped counter
> 
>     In various situations, a device provides a packet to our stack and we
>     drop it before it enters protocol stack :
>     - softnet backlog full (accounted in /proc/net/softnet_stat)
>     - bad vlan tag (not accounted)
>     - unknown/unregistered protocol (not accounted)
> 
>     We can handle a per-device counter of such dropped frames at core level,
>     and automatically adds it to the device provided stats (rx_dropped), so
>     that standard tools can be used (ifconfig, ip link, cat /proc/net/dev)
> 
>     This is a generalization of commit 8990f468a (net: rx_dropped
>     accounting), thus reverting it.
> 
>     Signed-off-by: Eric Dumazet <eric.dumazet@...il.com>
>     Signed-off-by: David S. Miller <davem@...emloft.net>
>