| Message ID | c9a7d3ddbe3fb890bee0c95d207f2ce431001075.1730979658.git.gnault@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] geneve: Use pcpu stats to update rx_dropped counter. | expand |
On Thu, Nov 07, 2024 at 12:41:44PM +0100, Guillaume Nault wrote: > Use the core_stats rx_dropped counter to avoid the cost of atomic > increments. > > Signed-off-by: Guillaume Nault <gnault@redhat.com> Reviewed-by: Simon Horman <horms@kernel.org>
On 11/7/24 12:41, Guillaume Nault wrote: > Use the core_stats rx_dropped counter to avoid the cost of atomic > increments. > > Signed-off-by: Guillaume Nault <gnault@redhat.com> It looks like other UDP tunnels devices could benefit from a similar change (vxlan, bareudp). Would you mind to also touch them, to keep such implementations aligned? > --- > drivers/net/geneve.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index 2f29b1386b1c..671ca5260e92 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -235,7 +235,7 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs, > vni_to_tunnel_id(gnvh->vni), > gnvh->opt_len * 4); > if (!tun_dst) { > - DEV_STATS_INC(geneve->dev, rx_dropped); > + dev_core_stats_rx_dropped_inc(geneve->dev); How about switching to NETDEV_PCPU_STAT_DSTATS instead, so there is a single percpu struct allocated x device (geneve already uses NETDEV_PCPU_STAT_TSTATS): stats fetching will be faster, and possibly memory usage lower. /P
On 11/12/24 11:53, Paolo Abeni wrote: > On 11/7/24 12:41, Guillaume Nault wrote: >> Use the core_stats rx_dropped counter to avoid the cost of atomic >> increments. >> >> Signed-off-by: Guillaume Nault <gnault@redhat.com> > > It looks like other UDP tunnels devices could benefit from a similar > change (vxlan, bareudp). Would you mind to also touch them, to keep such > implementations aligned? > >> --- >> drivers/net/geneve.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c >> index 2f29b1386b1c..671ca5260e92 100644 >> --- a/drivers/net/geneve.c >> +++ b/drivers/net/geneve.c >> @@ -235,7 +235,7 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs, >> vni_to_tunnel_id(gnvh->vni), >> gnvh->opt_len * 4); >> if (!tun_dst) { >> - DEV_STATS_INC(geneve->dev, rx_dropped); >> + dev_core_stats_rx_dropped_inc(geneve->dev); > > How about switching to NETDEV_PCPU_STAT_DSTATS instead, so there is a > single percpu struct allocated x device (geneve already uses > NETDEV_PCPU_STAT_TSTATS): stats fetching will be faster, and possibly > memory usage lower. I was not aware of the previous discussion on this same topic: https://lore.kernel.org/netdev/20240903113402.41d19129@kernel.org/ and I missed the previous change on bareudp.c I still think that avoiding the double per-cpu traversal when fetching the stats could be useful, especially on large multi-numa nodes systems. I guess it's better to be consistent and keep geneve and bareudp aligned. We can eventually consolidate the stats later. /P
On Tue, 12 Nov 2024 17:53:36 +0100 Paolo Abeni wrote: > > How about switching to NETDEV_PCPU_STAT_DSTATS instead, so there is a > > single percpu struct allocated x device (geneve already uses > > NETDEV_PCPU_STAT_TSTATS): stats fetching will be faster, and possibly > > memory usage lower. > > I was not aware of the previous discussion on this same topic: > > https://lore.kernel.org/netdev/20240903113402.41d19129@kernel.org/ > > and I missed the previous change on bareudp.c > > I still think that avoiding the double per-cpu traversal when fetching > the stats could be useful, especially on large multi-numa nodes systems. > > I guess it's better to be consistent and keep geneve and bareudp > aligned. We can eventually consolidate the stats later. We merged the bareudp changes... begrudgingly. You're the third maintainer in a row to make a similar suggestion, which is pretty strong signal that it's a better direction.
On Tue, Nov 12, 2024 at 06:15:18PM -0800, Jakub Kicinski wrote: > On Tue, 12 Nov 2024 17:53:36 +0100 Paolo Abeni wrote: > > > How about switching to NETDEV_PCPU_STAT_DSTATS instead, so there is a > > > single percpu struct allocated x device (geneve already uses > > > NETDEV_PCPU_STAT_TSTATS): stats fetching will be faster, and possibly > > > memory usage lower. > > > > I was not aware of the previous discussion on this same topic: > > > > https://lore.kernel.org/netdev/20240903113402.41d19129@kernel.org/ > > > > and I missed the previous change on bareudp.c > > > > I still think that avoiding the double per-cpu traversal when fetching > > the stats could be useful, especially on large multi-numa nodes systems. > > > > I guess it's better to be consistent and keep geneve and bareudp > > aligned. We can eventually consolidate the stats later. > > We merged the bareudp changes... begrudgingly. You're the third > maintainer in a row to make a similar suggestion, which is pretty > strong signal that it's a better direction. No problem, I can work on DSTAT conversion.
diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index 2f29b1386b1c..671ca5260e92 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -235,7 +235,7 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs, vni_to_tunnel_id(gnvh->vni), gnvh->opt_len * 4); if (!tun_dst) { - DEV_STATS_INC(geneve->dev, rx_dropped); + dev_core_stats_rx_dropped_inc(geneve->dev); goto drop; } /* Update tunnel dst according to Geneve options. */ @@ -387,14 +387,14 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb) if (unlikely((!geneve->cfg.inner_proto_inherit && inner_proto != htons(ETH_P_TEB)))) { - DEV_STATS_INC(geneve->dev, rx_dropped); + dev_core_stats_rx_dropped_inc(geneve->dev); goto drop; } opts_len = geneveh->opt_len * 4; if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len, inner_proto, !net_eq(geneve->net, dev_net(geneve->dev)))) { - DEV_STATS_INC(geneve->dev, rx_dropped); + dev_core_stats_rx_dropped_inc(geneve->dev); goto drop; } @@ -1023,7 +1023,7 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb, struct net_device *dev) if (unlikely(!info || !(info->mode & IP_TUNNEL_INFO_TX))) { netdev_dbg(dev, "no tunnel metadata\n"); dev_kfree_skb(skb); - DEV_STATS_INC(dev, tx_dropped); + dev_core_stats_tx_dropped_inc(dev); return NETDEV_TX_OK; } } else {
Use the core_stats rx_dropped counter to avoid the cost of atomic increments. Signed-off-by: Guillaume Nault <gnault@redhat.com> --- drivers/net/geneve.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)