Re: [PATCH net-next] net: dsa: Add support for 64-bit statistics
On 08/03/2017 11:11 AM, Andrew Lunn wrote: > On Thu, Aug 03, 2017 at 10:30:56AM -0700, Florian Fainelli wrote: >> On 08/02/2017 04:49 PM, David Miller wrote: >>> From: Florian Fainelli >>> Date: Tue, 1 Aug 2017 15:00:36 -0700 >>> DSA slave network devices maintain a pair of bytes and packets counters for each directions, but these are not 64-bit capable. Re-use pcpu_sw_netstats which contains exactly what we need for that purpose and update the code path to report 64-bit capable statistics. Signed-off-by: Florian Fainelli >>> >>> Applied, thanks. >>> >>> I would run ethtool -S and ifconfig under perf to see where it is >>> spending so much time. >>> >> >> This appears to be way worse than I thought, will keep digging, but for >> now, I may have to send a revert. Andrew, Vivien can you see if you have >> the same problems on your boards? Thanks! >> >> # killall iperf >> # [ ID] Interval Transfer Bandwidth >> [ 3] 0.0-19.1 sec 500 MBytes 220 Mbits/sec >> # while true; do ethtool -S gphy; ifconfig gphy; done >> ^C^C >> >> >> [ 64.566226] INFO: rcu_sched self-detected stall on CPU >> [ 64.571487] 0-...: (25999 ticks this GP) idle=006/141/0 > > Hi Florian Hi Andrew, > > I don't get anything so bad, but i think that is because of hardware > restrictions. I see the ethtool; ifconfig loop goes a lot slower when > there is iperf traffic, but i don't get an RCU stall. However, the > board i tested on only has a 100Mbps CPU interface, and it can handle > all that traffic without pushing the CPU to 100%. What is the CPU load > when you run your test? Even if you are going to 100% CPU load, we > still don't want RCU stalls. This is a quad core 1.5 Ghz board pushing 1Gbit/sec worth of traffic, this is about 25% loaded. What is needed to reproduce this is basically: iperf -c 192.168.1.1 -t 30 & while true; do ifconfig gphy; ethtool -S gphy; done when iperf terminates, the lock-up reliably occurs. I just converted net/dsa/ to use per-cpu statistics and of course, I can no longer reproduce this problem now... -- Florian
Re: [PATCH net-next] net: dsa: Add support for 64-bit statistics
On Thu, Aug 03, 2017 at 10:30:56AM -0700, Florian Fainelli wrote: > On 08/02/2017 04:49 PM, David Miller wrote: > > From: Florian Fainelli > > Date: Tue, 1 Aug 2017 15:00:36 -0700 > > > >> DSA slave network devices maintain a pair of bytes and packets counters > >> for each directions, but these are not 64-bit capable. Re-use > >> pcpu_sw_netstats which contains exactly what we need for that purpose > >> and update the code path to report 64-bit capable statistics. > >> > >> Signed-off-by: Florian Fainelli > > > > Applied, thanks. > > > > I would run ethtool -S and ifconfig under perf to see where it is > > spending so much time. > > > > This appears to be way worse than I thought, will keep digging, but for > now, I may have to send a revert. Andrew, Vivien can you see if you have > the same problems on your boards? Thanks! > > # killall iperf > # [ ID] Interval Transfer Bandwidth > [ 3] 0.0-19.1 sec 500 MBytes 220 Mbits/sec > # while true; do ethtool -S gphy; ifconfig gphy; done > ^C^C > > > [ 64.566226] INFO: rcu_sched self-detected stall on CPU > [ 64.571487] 0-...: (25999 ticks this GP) idle=006/141/0 Hi Florian I don't get anything so bad, but i think that is because of hardware restrictions. I see the ethtool; ifconfig loop goes a lot slower when there is iperf traffic, but i don't get an RCU stall. However, the board i tested on only has a 100Mbps CPU interface, and it can handle all that traffic without pushing the CPU to 100%. What is the CPU load when you run your test? Even if you are going to 100% CPU load, we still don't want RCU stalls. Andrew
Re: [PATCH net-next] net: dsa: Add support for 64-bit statistics
On 08/02/2017 04:49 PM, David Miller wrote: > From: Florian Fainelli > Date: Tue, 1 Aug 2017 15:00:36 -0700 > >> DSA slave network devices maintain a pair of bytes and packets counters >> for each directions, but these are not 64-bit capable. Re-use >> pcpu_sw_netstats which contains exactly what we need for that purpose >> and update the code path to report 64-bit capable statistics. >> >> Signed-off-by: Florian Fainelli > > Applied, thanks. > > I would run ethtool -S and ifconfig under perf to see where it is > spending so much time. > This appears to be way worse than I thought, will keep digging, but for now, I may have to send a revert. Andrew, Vivien can you see if you have the same problems on your boards? Thanks! # killall iperf # [ ID] Interval Transfer Bandwidth [ 3] 0.0-19.1 sec 500 MBytes 220 Mbits/sec # while true; do ethtool -S gphy; ifconfig gphy; done ^C^C [ 64.566226] INFO: rcu_sched self-detected stall on CPU [ 64.571487] 0-...: (25999 ticks this GP) idle=006/141/0 softirq=965/965 fqs=6495 [ 64.580214] (t=26000 jiffies g=205 c=204 q=51) [ 64.584958] NMI backtrace for cpu 0 [ 64.588571] CPU: 0 PID: 1515 Comm: ethtool Not tainted 4.13.0-rc3-00534-g5a4d148f0d78 #328 [ 64.596951] Hardware name: Broadcom STB (Flattened Device Tree) [ 64.602973] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 64.610836] [] (show_stack) from [] (dump_stack+0xb0/0xdc) [ 64.618172] [] (dump_stack) from [] (nmi_cpu_backtrace+0x11c/0x120) [ 64.626295] [] (nmi_cpu_backtrace) from [] (nmi_trigger_cpumask_backtrace+0x118/0x158) [ 64.636087] [] (nmi_trigger_cpumask_backtrace) from [] (rcu_dump_cpu_stacks+0xa0/0xd4) [ 64.645875] [] (rcu_dump_cpu_stacks) from [] (rcu_check_callbacks+0xb8c/0xbfc) [ 64.654965] [] (rcu_check_callbacks) from [] (update_process_times+0x30/0x5c) [ 64.663966] [] (update_process_times) from [] (tick_sched_timer+0x40/0x90) [ 64.672702] [] (tick_sched_timer) from [] (__hrtimer_run_queues+0x198/0x6f0) [ 64.681615] [] (__hrtimer_run_queues) from [] (hrtimer_interrupt+0x98/0x1f4) [ 64.690530] [] (hrtimer_interrupt) from [] (arch_timer_handler_virt+0x28/0x30) [ 64.699628] [] (arch_timer_handler_virt) from [] (handle_percpu_devid_irq+0xc8/0x484) [ 64.709329] [] (handle_percpu_devid_irq) from [] (generic_handle_irq+0x24/0x34) [ 64.718502] [] (generic_handle_irq) from [] (__handle_domain_irq+0x5c/0xb0) [ 64.727325] [] (__handle_domain_irq) from [] (gic_handle_irq+0x48/0x8c) [ 64.735794] [] (gic_handle_irq) from [] (__irq_svc+0x5c/0x7c) [ 64.743384] Exception stack(0xc283fd40 to 0xc283fd88) [ 64.748510] fd40: 0001 0011a9ad edf37000 ecddb800 f0d95000 ecddbe6c c08e87f4 [ 64.756801] fd60: ed6b8010 0660 0658 600f0013 0001 c283fd90 c027a39c c09a8c24 [ 64.765091] fd80: 200f0013 [ 64.768647] [] (__irq_svc) from [] (dsa_slave_get_ethtool_stats+0x100/0x104) [ 64.777562] [] (dsa_slave_get_ethtool_stats) from [] (dev_ethtool+0x768/0x2840) [ 64.786742] [] (dev_ethtool) from [] (dev_ioctl+0x5f8/0xa50) [ 64.794251] [] (dev_ioctl) from [] (do_vfs_ioctl+0xac/0x8d0) [ 64.801755] [] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c) [ 64.809175] [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x1c) [ 64.816901] INFO: rcu_sched detected stalls on CPUs/tasks: [ 64.822480] 0-...: (26006 ticks this GP) idle=006/140/0 softirq=965/965 fqs=6495 [ 64.831206] (detected by 2, t=26264 jiffies, g=205, c=204, q=51) [ 64.837390] Sending NMI from CPU 2 to CPUs 0: [ 64.841811] NMI backtrace for cpu 0 [ 64.841818] CPU: 0 PID: 1515 Comm: ethtool Not tainted 4.13.0-rc3-00534-g5a4d148f0d78 #328 [ 64.841821] Hardware name: Broadcom STB (Flattened Device Tree) [ 64.841824] task: edf37000 task.stack: c283e000 [ 64.841832] PC is at dsa_slave_get_ethtool_stats+0x100/0x104 [ 64.841842] LR is at mark_held_locks+0x68/0x90 [ 64.841846] pc : []lr : []psr: 200f0013 [ 64.841850] sp : c283fd90 ip : 0001 fp : 600f0013 [ 64.841853] r10: 0658 r9 : 0660 r8 : ed6b8010 [ 64.841856] r7 : c08e87f4 r6 : ecddbe6c r5 : f0d95000 r4 : ecddb800 [ 64.841860] r3 : edf37000 r2 : r1 : 0011a9ad r0 : 0001 [ 64.841865] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [ 64.841869] Control: 30c5387d Table: 2dddf500 DAC: fffd [ 64.841874] CPU: 0 PID: 1515 Comm: ethtool Not tainted 4.13.0-rc3-00534-g5a4d148f0d78 #328 [ 64.841876] Hardware name: Broadcom STB (Flattened Device Tree) [ 64.841885] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 64.841892] [] (show_stack) from [] (dump_stack+0xb0/0xdc) [ 64.841900] [] (dump_stack) from [] (nmi_cpu_backtrace+0xc0/0x120) [ 64.841907] [] (nmi_cpu_backtrace) from [] (handle_IPI+0x108/0x424) [ 64.841914] [] (handle_IPI) from [] (gic_handle_irq+0x88/0x8c) [ 64.841919] [] (gic_handle_irq) from [] (__irq_svc+0x5c/0x7c) [ 64.841922] Exception stack(0xc283fd40 to 0xc283fd88)
Re: [PATCH net-next] net: dsa: Add support for 64-bit statistics
From: Florian Fainelli Date: Tue, 1 Aug 2017 15:00:36 -0700 > DSA slave network devices maintain a pair of bytes and packets counters > for each directions, but these are not 64-bit capable. Re-use > pcpu_sw_netstats which contains exactly what we need for that purpose > and update the code path to report 64-bit capable statistics. > > Signed-off-by: Florian Fainelli Applied, thanks. I would run ethtool -S and ifconfig under perf to see where it is spending so much time.
Re: [PATCH net-next] net: dsa: Add support for 64-bit statistics
On 08/01/2017 03:00 PM, Florian Fainelli wrote:
> DSA slave network devices maintain a pair of bytes and packets counters
> for each directions, but these are not 64-bit capable. Re-use
> pcpu_sw_netstats which contains exactly what we need for that purpose
> and update the code path to report 64-bit capable statistics.
Humm, I do see a long time for ifconfig/ethtool -S to complete after
having transfered over 250GiB of data, the locking looks correct to me
in that statistics for the RX path are updated from softIRQ context
(napi_gro_receive/netif_receive_skb) but maybe I did misunderstand
whether the softirq/IRQ context applies to the producer and not the
reader...
>
> Signed-off-by: Florian Fainelli
> ---
> net/dsa/dsa.c | 8 ++--
> net/dsa/dsa_priv.h | 2 ++
> net/dsa/slave.c| 38 +++---
> 3 files changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index a55e2e4087a4..0ba842c08dd3 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -190,6 +190,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct
> net_device *dev,
> {
> struct dsa_switch_tree *dst = dev->dsa_ptr;
> struct sk_buff *nskb = NULL;
> + struct dsa_slave_priv *p;
>
> if (unlikely(dst == NULL)) {
> kfree_skb(skb);
> @@ -207,12 +208,15 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct
> net_device *dev,
> }
>
> skb = nskb;
> + p = netdev_priv(skb->dev);
> skb_push(skb, ETH_HLEN);
> skb->pkt_type = PACKET_HOST;
> skb->protocol = eth_type_trans(skb, skb->dev);
>
> - skb->dev->stats.rx_packets++;
> - skb->dev->stats.rx_bytes += skb->len;
> + u64_stats_update_begin(&p->stats64.syncp);
> + p->stats64.rx_packets++;
> + p->stats64.rx_bytes += skb->len;
> + u64_stats_update_end(&p->stats64.syncp);
>
> netif_receive_skb(skb);
>
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 55982cc39b24..7aa0656296c2 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -77,6 +77,8 @@ struct dsa_slave_priv {
> struct sk_buff *(*xmit)(struct sk_buff *skb,
> struct net_device *dev);
>
> + struct pcpu_sw_netstats stats64;
> +
> /* DSA port data, such as switch, port index, etc. */
> struct dsa_port *dp;
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 9507bd38cf04..65f3cef85976 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -354,8 +354,10 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb,
> struct net_device *dev)
> struct dsa_slave_priv *p = netdev_priv(dev);
> struct sk_buff *nskb;
>
> - dev->stats.tx_packets++;
> - dev->stats.tx_bytes += skb->len;
> + u64_stats_update_begin(&p->stats64.syncp);
> + p->stats64.tx_packets++;
> + p->stats64.tx_bytes += skb->len;
> + u64_stats_update_end(&p->stats64.syncp);
>
> /* Transmit function may have to reallocate the original SKB,
>* in which case it must have freed it. Only free it here on error.
> @@ -594,11 +596,15 @@ static void dsa_slave_get_ethtool_stats(struct
> net_device *dev,
> {
> struct dsa_slave_priv *p = netdev_priv(dev);
> struct dsa_switch *ds = p->dp->ds;
> -
> - data[0] = dev->stats.tx_packets;
> - data[1] = dev->stats.tx_bytes;
> - data[2] = dev->stats.rx_packets;
> - data[3] = dev->stats.rx_bytes;
> + unsigned int start;
> +
> + do {
> + start = u64_stats_fetch_begin_irq(&p->stats64.syncp);
> + data[0] = p->stats64.tx_packets;
> + data[1] = p->stats64.tx_bytes;
> + data[2] = p->stats64.rx_packets;
> + data[3] = p->stats64.rx_bytes;
> + } while (u64_stats_fetch_retry_irq(&p->stats64.syncp, start));
> if (ds->ops->get_ethtool_stats)
> ds->ops->get_ethtool_stats(ds, p->dp->index, data + 4);
> }
> @@ -861,6 +867,22 @@ static int dsa_slave_setup_tc(struct net_device *dev,
> u32 handle,
> }
> }
>
> +static void dsa_slave_get_stats64(struct net_device *dev,
> + struct rtnl_link_stats64 *stats)
> +{
> + struct dsa_slave_priv *p = netdev_priv(dev);
> + unsigned int start;
> +
> + netdev_stats_to_stats64(stats, &dev->stats);
> + do {
> + start = u64_stats_fetch_begin_irq(&p->stats64.syncp);
> + stats->tx_packets = p->stats64.tx_packets;
> + stats->tx_bytes = p->stats64.tx_bytes;
> + stats->rx_packets = p->stats64.rx_packets;
> + stats->rx_bytes = p->stats64.rx_bytes;
> + } while (u64_stats_fetch_retry_irq(&p->stats64.syncp, start));
> +}
> +
> void dsa_cpu_port_ethtool_init(struct ethtool_ops *ops)
> {
> ops->get_sset_count = dsa_cpu_port_get_sset_count;
> @@ -936,6 +958,7 @@ static const struct net_device_ops dsa_slave_netdev_ops =
> {
> .ndo_bridge_dellink =
[PATCH net-next] net: dsa: Add support for 64-bit statistics
DSA slave network devices maintain a pair of bytes and packets counters
for each directions, but these are not 64-bit capable. Re-use
pcpu_sw_netstats which contains exactly what we need for that purpose
and update the code path to report 64-bit capable statistics.
Signed-off-by: Florian Fainelli
---
net/dsa/dsa.c | 8 ++--
net/dsa/dsa_priv.h | 2 ++
net/dsa/slave.c| 38 +++---
3 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index a55e2e4087a4..0ba842c08dd3 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -190,6 +190,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct
net_device *dev,
{
struct dsa_switch_tree *dst = dev->dsa_ptr;
struct sk_buff *nskb = NULL;
+ struct dsa_slave_priv *p;
if (unlikely(dst == NULL)) {
kfree_skb(skb);
@@ -207,12 +208,15 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct
net_device *dev,
}
skb = nskb;
+ p = netdev_priv(skb->dev);
skb_push(skb, ETH_HLEN);
skb->pkt_type = PACKET_HOST;
skb->protocol = eth_type_trans(skb, skb->dev);
- skb->dev->stats.rx_packets++;
- skb->dev->stats.rx_bytes += skb->len;
+ u64_stats_update_begin(&p->stats64.syncp);
+ p->stats64.rx_packets++;
+ p->stats64.rx_bytes += skb->len;
+ u64_stats_update_end(&p->stats64.syncp);
netif_receive_skb(skb);
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 55982cc39b24..7aa0656296c2 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -77,6 +77,8 @@ struct dsa_slave_priv {
struct sk_buff *(*xmit)(struct sk_buff *skb,
struct net_device *dev);
+ struct pcpu_sw_netstats stats64;
+
/* DSA port data, such as switch, port index, etc. */
struct dsa_port *dp;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9507bd38cf04..65f3cef85976 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -354,8 +354,10 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb,
struct net_device *dev)
struct dsa_slave_priv *p = netdev_priv(dev);
struct sk_buff *nskb;
- dev->stats.tx_packets++;
- dev->stats.tx_bytes += skb->len;
+ u64_stats_update_begin(&p->stats64.syncp);
+ p->stats64.tx_packets++;
+ p->stats64.tx_bytes += skb->len;
+ u64_stats_update_end(&p->stats64.syncp);
/* Transmit function may have to reallocate the original SKB,
* in which case it must have freed it. Only free it here on error.
@@ -594,11 +596,15 @@ static void dsa_slave_get_ethtool_stats(struct net_device
*dev,
{
struct dsa_slave_priv *p = netdev_priv(dev);
struct dsa_switch *ds = p->dp->ds;
-
- data[0] = dev->stats.tx_packets;
- data[1] = dev->stats.tx_bytes;
- data[2] = dev->stats.rx_packets;
- data[3] = dev->stats.rx_bytes;
+ unsigned int start;
+
+ do {
+ start = u64_stats_fetch_begin_irq(&p->stats64.syncp);
+ data[0] = p->stats64.tx_packets;
+ data[1] = p->stats64.tx_bytes;
+ data[2] = p->stats64.rx_packets;
+ data[3] = p->stats64.rx_bytes;
+ } while (u64_stats_fetch_retry_irq(&p->stats64.syncp, start));
if (ds->ops->get_ethtool_stats)
ds->ops->get_ethtool_stats(ds, p->dp->index, data + 4);
}
@@ -861,6 +867,22 @@ static int dsa_slave_setup_tc(struct net_device *dev, u32
handle,
}
}
+static void dsa_slave_get_stats64(struct net_device *dev,
+ struct rtnl_link_stats64 *stats)
+{
+ struct dsa_slave_priv *p = netdev_priv(dev);
+ unsigned int start;
+
+ netdev_stats_to_stats64(stats, &dev->stats);
+ do {
+ start = u64_stats_fetch_begin_irq(&p->stats64.syncp);
+ stats->tx_packets = p->stats64.tx_packets;
+ stats->tx_bytes = p->stats64.tx_bytes;
+ stats->rx_packets = p->stats64.rx_packets;
+ stats->rx_bytes = p->stats64.rx_bytes;
+ } while (u64_stats_fetch_retry_irq(&p->stats64.syncp, start));
+}
+
void dsa_cpu_port_ethtool_init(struct ethtool_ops *ops)
{
ops->get_sset_count = dsa_cpu_port_get_sset_count;
@@ -936,6 +958,7 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
.ndo_bridge_dellink = switchdev_port_bridge_dellink,
.ndo_get_phys_port_name = dsa_slave_get_phys_port_name,
.ndo_setup_tc = dsa_slave_setup_tc,
+ .ndo_get_stats64= dsa_slave_get_stats64,
};
static const struct switchdev_ops dsa_slave_switchdev_ops = {
@@ -1171,6 +1194,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device
*parent,
slave_dev->vlan_features = master->vlan_features;
p = netdev_priv(slave_dev);
+ u64_stats_init(&p->stats64.syncp);
p->dp = &ds->p
