Re: [PATCH net] net: systemport: Fix 64-bit stats deadlock

[David Miller]

From: Florian Fainelli 
Date: Fri, 15 Sep 2017 14:28:40 -0700

> FYI, there is another patch needed to ensure consistency between
> ethtool reported stats and netdevice stats, will submit that after
> some more testing. Thanks!

Yes, I saw that, th anks.

Re: [PATCH net] net: systemport: Fix 64-bit stats deadlock

[Florian Fainelli]

On September 15, 2017 2:25:11 PM PDT, David Miller  wrote:
>From: Florian Fainelli 
>Date: Tue, 12 Sep 2017 13:14:26 -0700
>
>> We can enter a deadlock situation because there is no sufficient
>protection
>> when ndo_get_stats64() runs in process context to guard against RX or
>TX NAPI
>> contexts running in softirq, this can lead to the following lockdep
>splat and
>> actual deadlock was experienced as well with an iperf session in the
>background
>> and a while loop doing ifconfig + ethtool.
> ...
>> So just remove the u64_stats_update_begin()/end() pair in
>ndo_get_stats64()
>> since it does not appear to be useful for anything. No inconsistency
>was
>> observed with either ifconfig or ethtool, global TX counts equal the
>sum of
>> per-queue TX counts on a 32-bit architecture.
>> 
>> Fixes: 10377ba7673d ("net: systemport: Support 64bit statistics")
>> Signed-off-by: Florian Fainelli 
>
>Applied.

FYI, there is another patch needed to ensure consistency between ethtool 
reported stats and netdevice stats, will submit that after some more testing. 
Thanks!

-- 
Florian

Re: [PATCH net] net: systemport: Fix 64-bit stats deadlock

[David Miller]

From: Florian Fainelli 
Date: Tue, 12 Sep 2017 13:14:26 -0700

> We can enter a deadlock situation because there is no sufficient protection
> when ndo_get_stats64() runs in process context to guard against RX or TX NAPI
> contexts running in softirq, this can lead to the following lockdep splat and
> actual deadlock was experienced as well with an iperf session in the 
> background
> and a while loop doing ifconfig + ethtool.
 ...
> So just remove the u64_stats_update_begin()/end() pair in ndo_get_stats64()
> since it does not appear to be useful for anything. No inconsistency was
> observed with either ifconfig or ethtool, global TX counts equal the sum of
> per-queue TX counts on a 32-bit architecture.
> 
> Fixes: 10377ba7673d ("net: systemport: Support 64bit statistics")
> Signed-off-by: Florian Fainelli 

Applied.

Re: [PATCH net] net: systemport: Fix 64-bit stats deadlock

[Florian Fainelli]

On 09/12/2017 02:38 PM, Eric Dumazet wrote:
> On Tue, 2017-09-12 at 13:14 -0700, Florian Fainelli wrote:
>> We can enter a deadlock situation because there is no sufficient protection
>> when ndo_get_stats64() runs in process context to guard against RX or TX NAPI
>> contexts running in softirq, this can lead to the following lockdep splat and
>> actual deadlock was experienced as well with an iperf session in the 
>> background
>> and a while loop doing ifconfig + ethtool.
> 
>> So just remove the u64_stats_update_begin()/end() pair in ndo_get_stats64()
>> since it does not appear to be useful for anything. No inconsistency was
>> observed with either ifconfig or ethtool, global TX counts equal the sum of
>> per-queue TX counts on a 32-bit architecture.
>>
>> Fixes: 10377ba7673d ("net: systemport: Support 64bit statistics")
>> Signed-off-by: Florian Fainelli 
>> ---
>>  drivers/net/ethernet/broadcom/bcmsysport.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
>> b/drivers/net/ethernet/broadcom/bcmsysport.c
>> index a6572b51435a..c3c53f6cd9e6 100644
>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>> @@ -1735,11 +1735,8 @@ static void bcm_sysport_get_stats64(struct net_device 
>> *dev,
>>  stats->tx_packets += tx_packets;
>>  }
>>  
>> -/* lockless update tx_bytes and tx_packets */
>> -u64_stats_update_begin(>syncp);
> 
> Yes, this u64_stats_update_begin()/u64_stats_update_end() is bogus
> 
> But why do we even write on tx_bytes/tx_packets here ??? 

That's for the ethtool -S netdev stats copy (that's on me, I added that
in the driver initial version), so yes, not very robust...

> 
> Seems very wrong anyway.
> 
> (ethtool -S does not call bcm_sysport_get_stats64() to refresh them )

Yes that might actually be the simplest way to get this fixed.

> 
>>  stats64->tx_bytes = stats->tx_bytes;
>>  stats64->tx_packets = stats->tx_packets;
>> -u64_stats_update_end(>syncp);
>>  
>>  do {
>>  start = u64_stats_fetch_begin_irq(>syncp);
> 
> 


-- 
Florian

Re: [PATCH net] net: systemport: Fix 64-bit stats deadlock

[Eric Dumazet]

On Tue, 2017-09-12 at 13:14 -0700, Florian Fainelli wrote:
> We can enter a deadlock situation because there is no sufficient protection
> when ndo_get_stats64() runs in process context to guard against RX or TX NAPI
> contexts running in softirq, this can lead to the following lockdep splat and
> actual deadlock was experienced as well with an iperf session in the 
> background
> and a while loop doing ifconfig + ethtool.

> So just remove the u64_stats_update_begin()/end() pair in ndo_get_stats64()
> since it does not appear to be useful for anything. No inconsistency was
> observed with either ifconfig or ethtool, global TX counts equal the sum of
> per-queue TX counts on a 32-bit architecture.
> 
> Fixes: 10377ba7673d ("net: systemport: Support 64bit statistics")
> Signed-off-by: Florian Fainelli 
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index a6572b51435a..c3c53f6cd9e6 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -1735,11 +1735,8 @@ static void bcm_sysport_get_stats64(struct net_device 
> *dev,
>   stats->tx_packets += tx_packets;
>   }
>  
> - /* lockless update tx_bytes and tx_packets */
> - u64_stats_update_begin(>syncp);

Yes, this u64_stats_update_begin()/u64_stats_update_end() is bogus

But why do we even write on tx_bytes/tx_packets here ??? 

Seems very wrong anyway.

(ethtool -S does not call bcm_sysport_get_stats64() to refresh them )

>   stats64->tx_bytes = stats->tx_bytes;
>   stats64->tx_packets = stats->tx_packets;
> - u64_stats_update_end(>syncp);
>  
>   do {
>   start = u64_stats_fetch_begin_irq(>syncp);