netdev - Re: [PATCH 1/1] forcedeth: use per cpu to collect xmit/recv statistics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 31 Aug 2019 12:24:53 +0800
From:   Zhu Yanjun <yanjun.zhu@...cle.com>
To:     Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org,
        davem@...emloft.net, nan.1986san@...il.com
Subject: Re: [PATCH 1/1] forcedeth: use per cpu to collect xmit/recv
 statistics


On 2019/8/30 17:32, Eric Dumazet wrote:
>
> On 8/30/19 10:35 AM, Zhu Yanjun wrote:
>> When testing with a background iperf pushing 1Gbit/sec traffic and running
>> both ifconfig and netstat to collect statistics, some deadlocks occurred.
>>
> This is quite a heavy patch trying to fix a bug...
This is to use per-cpu variable. Perhaps the changes are big.
>
> I suspect the root cause has nothing to do with stat
> collection since on 64bit arches there is no additional synchronization.
This bug is similar to the one that the commit 5f6b4e14cada ("net: dsa: 
User per-cpu 64-bit statistics") tries to fix.
So a similar patch is to fix this similar bug in forcedeth.
> (u64_stats_update_begin(), u64_stats_update_end() are nops)
Sure. Exactly.
>
>> +static inline void nv_get_stats(int cpu, struct fe_priv *np,
>> +				struct rtnl_link_stats64 *storage)
>> +{
>> +	struct nv_txrx_stats *src = per_cpu_ptr(np->txrx_stats, cpu);
>> +	unsigned int syncp_start;
>> +
>> +	do {
>> +		syncp_start = u64_stats_fetch_begin_irq(&np->swstats_rx_syncp);
>> +		storage->rx_packets       += src->stat_rx_packets;
>> +		storage->rx_bytes         += src->stat_rx_bytes;
>> +		storage->rx_dropped       += src->stat_rx_dropped;
>> +		storage->rx_missed_errors += src->stat_rx_missed_errors;
>> +	} while (u64_stats_fetch_retry_irq(&np->swstats_rx_syncp, syncp_start));
>> +
>> +	do {
>> +		syncp_start = u64_stats_fetch_begin_irq(&np->swstats_tx_syncp);
>> +		storage->tx_packets += src->stat_tx_packets;
>> +		storage->tx_bytes   += src->stat_tx_bytes;
>> +		storage->tx_dropped += src->stat_tx_dropped;
>> +	} while (u64_stats_fetch_retry_irq(&np->swstats_tx_syncp, syncp_start));
>> +}
>> +
>>
> This is buggy :
> If the loops are ever restarted, the storage->fields will have
> been modified multiple times.
Sure. Sorry. My bad.
A similar changes in the commit 5f6b4e14cada ("net: dsa: User per-cpu 
64-bit statistics").
I will use this similar changes.
I will send V2 soon.

Thanks a lot for your comments.

Zhu Yanjun

>
>