On Fri, 2017-08-04 at 08:51 -0700, Florian Fainelli wrote:
On 08/03/2017 10:36 PM, Eric Dumazet wrote:
quoted
On Thu, 2017-08-03 at 21:33 -0700, Florian Fainelli wrote:
quoted
During testing with a background iperf pushing 1Gbit/sec worth of
traffic and having both ifconfig and ethtool collect statistics, we
could see quite frequent deadlocks. Convert the often accessed DSA slave
network devices statistics to per-cpu 64-bit statistics to remove these
deadlocks and provide fast efficient statistics updates.
This seems to be a bug fix, it would be nice to get a proper tag like :
Fixes: f613ed665bb3 ("net: dsa: Add support for 64-bit statistics")
Right, should have been added, thanks!
quoted
Problem here is that if multiple cpus can call dsa_switch_rcv() at the
same time, then u64_stats_update_begin() contract is not respected.
This is really where I struggled understanding what is wrong in the
non-per CPU version, my understanding is that we have:
- writers for xmit executes in process context
- writers for receive executes from NAPI (from the DSA's master network
device through it's own NAPI doing netif_receive_skb -> netdev_uses_dsa
-> netif_receive_skb)
readers should all execute in process context. The test scenario that
led to a deadlock involved running iperf in the background, having a
while loop with both ifconfig and ethtool reading stats, and somehow
when iperf exited, either reader would just be locked. So I guess this
leaves us with the two writers not being mutually excluded then, right?
You could add a debug version of u64_stats_update_begin()
doing
int ret = atomic_inc((atomic_t *)syncp);
BUG_ON(ret & 1);
And u64_stats_update_end()
int ret = atomic_inc((atomic_t *)syncp);
BUG_ON(!(ret & 1));
We probably could have a CONFIG_DEBUG_U64_STATS that could be used on
64bit kernels as well...