[PATCH net] bareudp: Fix device stats updates.

[PATCH net] bareudp: Fix device stats updates.

[Guillaume Nault]

Bareudp devices update their stats concurrently.
Therefore they need proper atomic increments.

Fixes: 571912c69f0e ("net: UDP tunnel encapsulation module for tunnelling different protocols like MPLS, IP, NSH etc.")
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 drivers/net/bareudp.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
index d5c56ca91b77..7aca0544fb29 100644
--- a/drivers/net/bareudp.c
+++ b/drivers/net/bareudp.c
@@ -83,7 +83,7 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 
 		if (skb_copy_bits(skb, BAREUDP_BASE_HLEN, &ipversion,
 				  sizeof(ipversion))) {
-			bareudp->dev->stats.rx_dropped++;
+			DEV_STATS_INC(bareudp->dev, rx_dropped);
 			goto drop;
 		}
 		ipversion >>= 4;
@@ -93,7 +93,7 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 		} else if (ipversion == 6 && bareudp->multi_proto_mode) {
 			proto = htons(ETH_P_IPV6);
 		} else {
-			bareudp->dev->stats.rx_dropped++;
+			DEV_STATS_INC(bareudp->dev, rx_dropped);
 			goto drop;
 		}
 	} else if (bareudp->ethertype == htons(ETH_P_MPLS_UC)) {
@@ -107,7 +107,7 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 				   ipv4_is_multicast(tunnel_hdr->daddr)) {
 				proto = htons(ETH_P_MPLS_MC);
 			} else {
-				bareudp->dev->stats.rx_dropped++;
+				DEV_STATS_INC(bareudp->dev, rx_dropped);
 				goto drop;
 			}
 		} else {
@@ -123,7 +123,7 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 				   (addr_type & IPV6_ADDR_MULTICAST)) {
 				proto = htons(ETH_P_MPLS_MC);
 			} else {
-				bareudp->dev->stats.rx_dropped++;
+				DEV_STATS_INC(bareudp->dev, rx_dropped);
 				goto drop;
 			}
 		}
@@ -135,7 +135,7 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 				 proto,
 				 !net_eq(bareudp->net,
 				 dev_net(bareudp->dev)))) {
-		bareudp->dev->stats.rx_dropped++;
+		DEV_STATS_INC(bareudp->dev, rx_dropped);
 		goto drop;
 	}
 
@@ -143,7 +143,7 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 
 	tun_dst = udp_tun_rx_dst(skb, family, key, 0, 0);
 	if (!tun_dst) {
-		bareudp->dev->stats.rx_dropped++;
+		DEV_STATS_INC(bareudp->dev, rx_dropped);
 		goto drop;
 	}
 	skb_dst_set(skb, &tun_dst->dst);
@@ -169,8 +169,8 @@ static int bareudp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 						     &((struct ipv6hdr *)oiph)->saddr);
 		}
 		if (err > 1) {
-			++bareudp->dev->stats.rx_frame_errors;
-			++bareudp->dev->stats.rx_errors;
+			DEV_STATS_INC(bareudp->dev, rx_frame_errors);
+			DEV_STATS_INC(bareudp->dev, rx_errors);
 			goto drop;
 		}
 	}
@@ -467,11 +467,11 @@ static netdev_tx_t bareudp_xmit(struct sk_buff *skb, struct net_device *dev)
 	dev_kfree_skb(skb);
 
 	if (err == -ELOOP)
-		dev->stats.collisions++;
+		DEV_STATS_INC(dev, collisions);
 	else if (err == -ENETUNREACH)
-		dev->stats.tx_carrier_errors++;
+		DEV_STATS_INC(dev, tx_carrier_errors);
 
-	dev->stats.tx_errors++;
+	DEV_STATS_INC(dev, tx_errors);
 	return NETDEV_TX_OK;
 }
 
-- 
2.39.2

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

[Willem de Bruijn]

Guillaume Nault wrote:
> Bareudp devices update their stats concurrently.
> Therefore they need proper atomic increments.
> 
> Fixes: 571912c69f0e ("net: UDP tunnel encapsulation module for tunnelling different protocols like MPLS, IP, NSH etc.")
> Signed-off-by: Guillaume Nault <gnault@redhat.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

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

[Jakub Kicinski]

On Fri, 30 Aug 2024 17:31:07 +0200 Guillaume Nault wrote:
> Bareudp devices update their stats concurrently.
> Therefore they need proper atomic increments.

The driver already uses struct pcpu_sw_netstats, would it make sense to
bump it up to struct pcpu_dstats and have per CPU rx drops as well?

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

[Guillaume Nault]

On Tue, Sep 03, 2024 at 11:34:02AM -0700, Jakub Kicinski wrote:
> On Fri, 30 Aug 2024 17:31:07 +0200 Guillaume Nault wrote:
> > Bareudp devices update their stats concurrently.
> > Therefore they need proper atomic increments.
> 
> The driver already uses struct pcpu_sw_netstats, would it make sense to
> bump it up to struct pcpu_dstats and have per CPU rx drops as well?

Long term, I was considering moving bareudp to use dev->tstats for
packets/bytes and dev->core_stats for drops. It looks like dev->dstats
is only used for VRF, so I didn't consider it.

Should we favour dev->dstats for tunnels instead of combining ->tstats
and ->core_stats? (vxlan uses the later for example).

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

[Jakub Kicinski]

On Wed, 4 Sep 2024 14:29:44 +0200 Guillaume Nault wrote:
> > The driver already uses struct pcpu_sw_netstats, would it make sense to
> > bump it up to struct pcpu_dstats and have per CPU rx drops as well?  
> 
> Long term, I was considering moving bareudp to use dev->tstats for
> packets/bytes and dev->core_stats for drops. It looks like dev->dstats
> is only used for VRF, so I didn't consider it.

Right, d stands for dummy so I guess they also were used by dummy 
at some stage? Mostly I think it's a matter of the other stats being
less recent.

> Should we favour dev->dstats for tunnels instead of combining ->tstats
> and ->core_stats? (vxlan uses the later for example).

Seems reasonable to me. Not important enough to convert existing
drivers, maybe, unless someone sees contention. But in new code,
or if we're touching the relevant lines I reckon we should consider it?
No strong feelings tho, LMK if you want to send v2 or keep this patch
as is.

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

[Guillaume Nault]

[Adding David Ahern for the vrf/dstats discussion]

On Wed, Sep 04, 2024 at 07:57:32AM -0700, Jakub Kicinski wrote:
> On Wed, 4 Sep 2024 14:29:44 +0200 Guillaume Nault wrote:
> > > The driver already uses struct pcpu_sw_netstats, would it make sense to
> > > bump it up to struct pcpu_dstats and have per CPU rx drops as well?
> > 
> > Long term, I was considering moving bareudp to use dev->tstats for
> > packets/bytes and dev->core_stats for drops. It looks like dev->dstats
> > is only used for VRF, so I didn't consider it.
> 
> Right, d stands for dummy so I guess they also were used by dummy
> at some stage? Mostly I think it's a matter of the other stats being
> less recent.

Looks like dummy had its own dstats, yes. But those dstats were really
like the current lstats (packets and bytes counters, nothing for
drops). Dummy was later converted to lstats by commit 4a43b1f96b1d
("net: dummy: use standard dev_lstats_add() and dev_lstats_read()").

The dstats we have now really come from vrf (different counters for tx
and rx and counters for packet drops), which had its own implementation
at that time.

My understanding is that vrf implemented its own dstats in order to
have per-cpu counters for regular bytes/packets counters and also for
packet drops.

But when vrf's dstats got moved to the core (commits
79e0c5be8c73 ("net, vrf: Move dstats structure to core") and
34d21de99cea ("net: Move {l,t,d}stats allocation to core and convert
veth & vrf")), the networking core had caught up and had also gained
support for pcpu drop counters (commit 625788b58445 ("net: add per-cpu
storage and net->core_stats")).

In this context, I feel that dstats is now just a mix of tstats and
core_stats.

> > Should we favour dev->dstats for tunnels instead of combining ->tstats
> > and ->core_stats? (vxlan uses the later for example).
> 
> Seems reasonable to me. Not important enough to convert existing
> drivers, maybe, unless someone sees contention. But in new code,
> or if we're touching the relevant lines I reckon we should consider it?

Given that we now have pcpu stats for packet drops anyway, what does
dstats bring compared to tstats?

Shouldn't we go the other way around and convert vrf to tstats and
core_stats? Then we could drop dstats entirely.

Back to bareudp, for the moment, I'd prefer to convert it to tstats
rather than dstats. The reason is that vxlan (and geneve to a lesser
extent) use tstats and I'd like to ease potential future code
consolidation between those three modules.

> No strong feelings tho, LMK if you want to send v2 or keep this patch
> as is.

I'd prefer to have this patch merged as is in -net. I have other
patches pending that have to update stats and I'd like to do that
correctly (that is, in a non-racy way) and consistently with existing
code. I feel that converting bareudp to either tstats or dstats is
something for net-next.

After -net will merge into net-next, I'll can convert bareudp to either
dstats or tstats, depending on the outcome of this conversation.

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

[Jakub Kicinski]

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

bareudp is a driver.

> After -net will merge into net-next, I'll can convert bareudp to either
> dstats or tstats, depending on the outcome of this conversation.

Sure.

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

[Guillaume Nault]

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.

> bareudp is a driver.
> 
> > After -net will merge into net-next, I'll can convert bareudp to either
> > dstats or tstats, depending on the outcome of this conversation.
> 
> Sure.
> 

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

[Eric Dumazet]

On Fri, Sep 6, 2024 at 12:42 PM Guillaume Nault <gnault@redhat.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.

commit caf586e5f23cebb2a68cbaf288d59dbbf2d74052
Author: Eric Dumazet <eric.dumazet@gmail.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@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

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

[Guillaume Nault]

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@redhat.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@gmail.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@gmail.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 

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

[David Ahern]

On 9/4/24 11:54 AM, Guillaume Nault wrote:
> [Adding David Ahern for the vrf/dstats discussion]
> 
> On Wed, Sep 04, 2024 at 07:57:32AM -0700, Jakub Kicinski wrote:
>> On Wed, 4 Sep 2024 14:29:44 +0200 Guillaume Nault wrote:
>>>> The driver already uses struct pcpu_sw_netstats, would it make sense to
>>>> bump it up to struct pcpu_dstats and have per CPU rx drops as well?
>>>
>>> Long term, I was considering moving bareudp to use dev->tstats for
>>> packets/bytes and dev->core_stats for drops. It looks like dev->dstats
>>> is only used for VRF, so I didn't consider it.
>>
>> Right, d stands for dummy so I guess they also were used by dummy
>> at some stage? Mostly I think it's a matter of the other stats being
>> less recent.
> 
> Looks like dummy had its own dstats, yes. But those dstats were really
> like the current lstats (packets and bytes counters, nothing for
> drops). Dummy was later converted to lstats by commit 4a43b1f96b1d
> ("net: dummy: use standard dev_lstats_add() and dev_lstats_read()").
> 
> The dstats we have now really come from vrf (different counters for tx
> and rx and counters for packet drops), which had its own implementation
> at that time.
> 
> My understanding is that vrf implemented its own dstats in order to
> have per-cpu counters for regular bytes/packets counters and also for
> packet drops.

VRF was following other per-cpu counters that existed in 2015-2016
timeframe.

I have no preference on the naming; just wanted per-cpu counters.

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

[Guillaume Nault]

On Wed, Sep 04, 2024 at 07:50:55PM -0600, David Ahern wrote:
> On 9/4/24 11:54 AM, Guillaume Nault wrote:
> > [Adding David Ahern for the vrf/dstats discussion]
> > 
> > On Wed, Sep 04, 2024 at 07:57:32AM -0700, Jakub Kicinski wrote:
> >> On Wed, 4 Sep 2024 14:29:44 +0200 Guillaume Nault wrote:
> >>>> The driver already uses struct pcpu_sw_netstats, would it make sense to
> >>>> bump it up to struct pcpu_dstats and have per CPU rx drops as well?
> >>>
> >>> Long term, I was considering moving bareudp to use dev->tstats for
> >>> packets/bytes and dev->core_stats for drops. It looks like dev->dstats
> >>> is only used for VRF, so I didn't consider it.
> >>
> >> Right, d stands for dummy so I guess they also were used by dummy
> >> at some stage? Mostly I think it's a matter of the other stats being
> >> less recent.
> > 
> > Looks like dummy had its own dstats, yes. But those dstats were really
> > like the current lstats (packets and bytes counters, nothing for
> > drops). Dummy was later converted to lstats by commit 4a43b1f96b1d
> > ("net: dummy: use standard dev_lstats_add() and dev_lstats_read()").
> > 
> > The dstats we have now really come from vrf (different counters for tx
> > and rx and counters for packet drops), which had its own implementation
> > at that time.
> > 
> > My understanding is that vrf implemented its own dstats in order to
> > have per-cpu counters for regular bytes/packets counters and also for
> > packet drops.
> 
> VRF was following other per-cpu counters that existed in 2015-2016
> timeframe.

Thanks. That was my impression as well.

> I have no preference on the naming; just wanted per-cpu counters.
> 

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 30 Aug 2024 17:31:07 +0200 you wrote:
> Bareudp devices update their stats concurrently.
> Therefore they need proper atomic increments.
> 
> Fixes: 571912c69f0e ("net: UDP tunnel encapsulation module for tunnelling different protocols like MPLS, IP, NSH etc.")
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> ---
>  drivers/net/bareudp.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

Here is the summary with links:
  - [net] bareudp: Fix device stats updates.
    https://git.kernel.org/netdev/net/c/4963d2343af8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html