[PATCH net-next] ipv6: fix the use of pcpu_tstats in ip6_tunnel

[PATCH net-next] ipv6: fix the use of pcpu_tstats in ip6_tunnel

[roy.qing.li]

From: Li RongQing <roy.qing.li@gmail.com>

when read/write the 64bit data, the correct lock should be hold. and use the
BUILD_BUG_ON check to ensure the syncp is last element of struct pcpu_tstats,
then reduce the copy the syncp when get the stats

Eric Dumazet <edumazet@google.com>
Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
 net/ipv6/ip6_tunnel.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 8d7c986..8eedb0c 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -102,16 +102,25 @@ struct ip6_tnl_net {
 
 static struct net_device_stats *ip6_get_stats(struct net_device *dev)
 {
-	struct pcpu_tstats sum = { 0 };
+	struct pcpu_tstats tmp, sum = { 0 };
 	int i;
 
-	for_each_possible_cpu(i) {
-		const struct pcpu_tstats *tstats = per_cpu_ptr(dev->tstats, i);
+	BUILD_BUG_ON(offsetof(typeof(tmp), syncp) !=
+		     sizeof(tmp) - sizeof(tmp.syncp));
 
-		sum.rx_packets += tstats->rx_packets;
-		sum.rx_bytes   += tstats->rx_bytes;
-		sum.tx_packets += tstats->tx_packets;
-		sum.tx_bytes   += tstats->tx_bytes;
+	for_each_possible_cpu(i) {
+		unsigned int start;
+		const struct pcpu_tstats *stat = per_cpu_ptr(dev->tstats, i);
+
+		do {
+			start = u64_stats_fetch_begin_bh(&stat->syncp);
+			memcpy(&tmp, stat, sizeof(tmp) - sizeof(stat->syncp));
+		} while (u64_stats_fetch_retry_bh(&stat->syncp, start));
+
+		sum.rx_packets += tmp.rx_packets;
+		sum.rx_bytes   += tmp.rx_bytes;
+		sum.tx_packets += tmp.tx_packets;
+		sum.tx_bytes   += tmp.tx_bytes;
 	}
 	dev->stats.rx_packets = sum.rx_packets;
 	dev->stats.rx_bytes   = sum.rx_bytes;
@@ -823,8 +832,10 @@ static int ip6_tnl_rcv(struct sk_buff *skb, __u16 protocol,
 		}
 
 		tstats = this_cpu_ptr(t->dev->tstats);
+		u64_stats_update_begin(&tstats->syncp);
 		tstats->rx_packets++;
 		tstats->rx_bytes += skb->len;
+		u64_stats_update_end(&tstats->syncp);
 
 		netif_rx(skb);
 
-- 
1.7.10.4

Re: [PATCH net-next] ipv6: fix the use of pcpu_tstats in ip6_tunnel

[David Miller]

From: roy.qing.li@gmail.com
Date: Thu,  2 Jan 2014 08:49:40 +0800

> Eric Dumazet <edumazet@google.com>

What is this?

> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
 ...
>  {
> -	struct pcpu_tstats sum = { 0 };
> +	struct pcpu_tstats tmp, sum = { 0 };
>  	int i;
>  
> -	for_each_possible_cpu(i) {
> -		const struct pcpu_tstats *tstats = per_cpu_ptr(dev->tstats, i);
> +	BUILD_BUG_ON(offsetof(typeof(tmp), syncp) !=
> +		     sizeof(tmp) - sizeof(tmp.syncp));
>  
> -		sum.rx_packets += tstats->rx_packets;
> -		sum.rx_bytes   += tstats->rx_bytes;
> -		sum.tx_packets += tstats->tx_packets;
> -		sum.tx_bytes   += tstats->tx_bytes;
> +	for_each_possible_cpu(i) {
> +		unsigned int start;
> +		const struct pcpu_tstats *stat = per_cpu_ptr(dev->tstats, i);
> +
> +		do {
> +			start = u64_stats_fetch_begin_bh(&stat->syncp);
> +			memcpy(&tmp, stat, sizeof(tmp) - sizeof(stat->syncp));
> +		} while (u64_stats_fetch_retry_bh(&stat->syncp, start));

It's entirely wasteful and non-canonical to use an on-stack copy for this,
just surround the entire set of statistic increments within the u64 stats
updata sequence.  Then you don't need the on-stack thing.

Re: [PATCH net-next] ipv6: fix the use of pcpu_tstats in ip6_tunnel

[Eric Dumazet]

On Thu, 2014-01-02 at 08:49 +0800, roy.qing.li@gmail.com wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
> 
> when read/write the 64bit data, the correct lock should be hold. and use the
> BUILD_BUG_ON check to ensure the syncp is last element of struct pcpu_tstats,
> then reduce the copy the syncp when get the stats

No idea of why you added a BUILD_BUG_ON(), this makes no sense.

> 
> Eric Dumazet <edumazet@google.com>

Why is my name mentioned in this changelog ?

> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
> ---
>  net/ipv6/ip6_tunnel.c |   25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)

Could you find out when the bug was introduced ?

Ideally you should add :

Fixes : 12-digit SHA1 ("patch title")

Thanks

Re: [PATCH net-next] ipv6: fix the use of pcpu_tstats in ip6_tunnel

[Rongqing Li]

On 01/02/2014 11:50 AM, David Miller wrote:
> From: roy.qing.li@gmail.com
> Date: Thu,  2 Jan 2014 08:49:40 +0800
>
>> Eric Dumazet <edumazet@google.com>
>
> What is this?

Sorry, it should CC to Eric Dumazet

>
>> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
>   ...
>>   {
>> -	struct pcpu_tstats sum = { 0 };
>> +	struct pcpu_tstats tmp, sum = { 0 };
>>   	int i;
>>
>> -	for_each_possible_cpu(i) {
>> -		const struct pcpu_tstats *tstats = per_cpu_ptr(dev->tstats, i);
>> +	BUILD_BUG_ON(offsetof(typeof(tmp), syncp) !=
>> +		     sizeof(tmp) - sizeof(tmp.syncp));
>>
>> -		sum.rx_packets += tstats->rx_packets;
>> -		sum.rx_bytes   += tstats->rx_bytes;
>> -		sum.tx_packets += tstats->tx_packets;
>> -		sum.tx_bytes   += tstats->tx_bytes;
>> +	for_each_possible_cpu(i) {
>> +		unsigned int start;
>> +		const struct pcpu_tstats *stat = per_cpu_ptr(dev->tstats, i);
>> +
>> +		do {
>> +			start = u64_stats_fetch_begin_bh(&stat->syncp);
>> +			memcpy(&tmp, stat, sizeof(tmp) - sizeof(stat->syncp));
>> +		} while (u64_stats_fetch_retry_bh(&stat->syncp, start));
>
> It's entirely wasteful and non-canonical to use an on-stack copy for this,
> just surround the entire set of statistic increments within the u64 stats
> updata sequence.  Then you don't need the on-stack thing.
>

Ok, I see.

Thanks

-Roy


> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

-- 
Best Reagrds,
Roy | RongQing Li

Re: [PATCH net-next] ipv6: fix the use of pcpu_tstats in ip6_tunnel

[Ben Hutchings]

On Wed, 2014-01-01 at 22:50 -0500, David Miller wrote:
> From: roy.qing.li@gmail.com
> Date: Thu,  2 Jan 2014 08:49:40 +0800
> 
> > Eric Dumazet <edumazet@google.com>
> 
> What is this?
> 
> > Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
>  ...
> >  {
> > -	struct pcpu_tstats sum = { 0 };
> > +	struct pcpu_tstats tmp, sum = { 0 };
> >  	int i;
> >  
> > -	for_each_possible_cpu(i) {
> > -		const struct pcpu_tstats *tstats = per_cpu_ptr(dev->tstats, i);
> > +	BUILD_BUG_ON(offsetof(typeof(tmp), syncp) !=
> > +		     sizeof(tmp) - sizeof(tmp.syncp));
> >  
> > -		sum.rx_packets += tstats->rx_packets;
> > -		sum.rx_bytes   += tstats->rx_bytes;
> > -		sum.tx_packets += tstats->tx_packets;
> > -		sum.tx_bytes   += tstats->tx_bytes;
> > +	for_each_possible_cpu(i) {
> > +		unsigned int start;
> > +		const struct pcpu_tstats *stat = per_cpu_ptr(dev->tstats, i);
> > +
> > +		do {
> > +			start = u64_stats_fetch_begin_bh(&stat->syncp);
> > +			memcpy(&tmp, stat, sizeof(tmp) - sizeof(stat->syncp));
> > +		} while (u64_stats_fetch_retry_bh(&stat->syncp, start));
> 
> It's entirely wasteful and non-canonical to use an on-stack copy for this,
> just surround the entire set of statistic increments within the u64 stats
> updata sequence.  Then you don't need the on-stack thing.

Then you add them to the sum repeatedly in case of a retry...

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.