[RFC PATCH 0/3] net: Improve snmp6_fill_stats

[RFC PATCH 0/3] net: Improve snmp6_fill_stats

[Jia He]

This is the follow up work of commit a3a773726c9f ("net: Optimize 
snmp stat aggregation by walking all the percpu data at once") 

Jia He (3):
  net: Remove unnecessary memset in __snmp6_fill_stats64
  net: Replace for_each_possible_cpu with for_each_online_cpu
  net: Remove the useless parameter of __snmp6_fill_statsdev

 net/ipv6/addrconf.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

-- 
2.5.0

[RFC PATCH 1/3] net: Remove unnecessary memset in __snmp6_fill_stats64

[Jia He]

buff[] will be assigned later, so memset is not necessary.

Signed-off-by: Jia He <hejianet@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
---
 net/ipv6/addrconf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ab3e796..43fa8d0 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4967,7 +4967,6 @@ static inline void __snmp6_fill_stats64(u64 *stats, void __percpu *mib,
 
 	BUG_ON(pad < 0);
 
-	memset(buff, 0, sizeof(buff));
 	buff[0] = IPSTATS_MIB_MAX;
 
 	for_each_possible_cpu(c) {
-- 
2.5.0

Re: [RFC PATCH 1/3] net: Remove unnecessary memset in __snmp6_fill_stats64

[Florian Westphal]

Jia He <hejianet@gmail.com> wrote:
> buff[] will be assigned later, so memset is not necessary.
> 
> Signed-off-by: Jia He <hejianet@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> Cc: James Morris <jmorris@namei.org>
> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> Cc: Patrick McHardy <kaber@trash.net>
> ---
>  net/ipv6/addrconf.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index ab3e796..43fa8d0 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4967,7 +4967,6 @@ static inline void __snmp6_fill_stats64(u64 *stats, void __percpu *mib,
>  
>  	BUG_ON(pad < 0);
>  
> -	memset(buff, 0, sizeof(buff));
>  	buff[0] = IPSTATS_MIB_MAX;
>  
>  	for_each_possible_cpu(c) {
                for (i = 1; i < IPSTATS_MIB_MAX; i++)
                        buff[i] += snmp_get_cpu_field64(mib, c, i, syncpoff);

Without memset result of buff[i] += ... is undefined.

Re: [RFC PATCH 1/3] net: Remove unnecessary memset in __snmp6_fill_stats64

[hejianet]

Yes, sorry about it，I am too hasty

B.R.

Jia He

On 8/8/16 7:12 PM, Florian Westphal wrote:
> Jia He <hejianet@gmail.com> wrote:
>> buff[] will be assigned later, so memset is not necessary.
>>
>> Signed-off-by: Jia He <hejianet@gmail.com>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
>> Cc: James Morris <jmorris@namei.org>
>> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
>> Cc: Patrick McHardy <kaber@trash.net>
>> ---
>>   net/ipv6/addrconf.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index ab3e796..43fa8d0 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -4967,7 +4967,6 @@ static inline void __snmp6_fill_stats64(u64 *stats, void __percpu *mib,
>>   
>>   	BUG_ON(pad < 0);
>>   
>> -	memset(buff, 0, sizeof(buff));
>>   	buff[0] = IPSTATS_MIB_MAX;
>>   
>>   	for_each_possible_cpu(c) {
>                  for (i = 1; i < IPSTATS_MIB_MAX; i++)
>                          buff[i] += snmp_get_cpu_field64(mib, c, i, syncpoff);
>
> Without memset result of buff[i] += ... is undefined.
>

Re: [RFC PATCH 1/3] net: Remove unnecessary memset in __snmp6_fill_stats64

[Eric Dumazet]

On Mon, 2016-08-08 at 18:22 +0800, Jia He wrote:
> buff[] will be assigned later, so memset is not necessary.
> 
> Signed-off-by: Jia He <hejianet@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> Cc: James Morris <jmorris@namei.org>
> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> Cc: Patrick McHardy <kaber@trash.net>
> ---
>  net/ipv6/addrconf.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index ab3e796..43fa8d0 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4967,7 +4967,6 @@ static inline void __snmp6_fill_stats64(u64 *stats, void __percpu *mib,
>  
>  	BUG_ON(pad < 0);
>  
> -	memset(buff, 0, sizeof(buff));
>  	buff[0] = IPSTATS_MIB_MAX;
>  
>  	for_each_possible_cpu(c) {

This is completely buggy, since we performs additions, not assignments :



buff[i] += snmp_get_cpu_field64(mib, c, i, syncpoff);


Please do not send untested patches.

[RFC PATCH 2/3] net: Replace for_each_possible_cpu with for_each_online_cpu

[Jia He]

In PowerPC server with large number cpus, the loop index in smt=1 could be 
reduced to 1/8 compared with smt=8.
Thus cache misses can be reduced.

Signed-off-by: Jia He <hejianet@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
---
 net/ipv6/addrconf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 43fa8d0..1fce613 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4969,7 +4969,7 @@ static inline void __snmp6_fill_stats64(u64 *stats, void __percpu *mib,
 
 	buff[0] = IPSTATS_MIB_MAX;
 
-	for_each_possible_cpu(c) {
+	for_each_online_cpu(c) {
 		for (i = 1; i < IPSTATS_MIB_MAX; i++)
 			buff[i] += snmp_get_cpu_field64(mib, c, i, syncpoff);
 	}
-- 
2.5.0

Re: [RFC PATCH 2/3] net: Replace for_each_possible_cpu with for_each_online_cpu

[David Miller]

From: Jia He <hejianet@gmail.com>
Date: Mon,  8 Aug 2016 18:22:21 +0800

> In PowerPC server with large number cpus, the loop index in smt=1 could be 
> reduced to 1/8 compared with smt=8.
> Thus cache misses can be reduced.

You can't do this, if cpus go down we still want to report the statistics
they collected while they were up.

So we must use the possible cpu list here.

Re: [RFC PATCH 2/3] net: Replace for_each_possible_cpu with for_each_online_cpu

[Eric Dumazet]

On Mon, 2016-08-08 at 18:22 +0800, Jia He wrote:
> In PowerPC server with large number cpus, the loop index in smt=1 could be 
> reduced to 1/8 compared with smt=8.
> Thus cache misses can be reduced.
> 
> Signed-off-by: Jia He <hejianet@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
> Cc: James Morris <jmorris@namei.org>
> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> Cc: Patrick McHardy <kaber@trash.net>
> ---
>  net/ipv6/addrconf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 43fa8d0..1fce613 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4969,7 +4969,7 @@ static inline void __snmp6_fill_stats64(u64 *stats, void __percpu *mib,
>  
>  	buff[0] = IPSTATS_MIB_MAX;
>  
> -	for_each_possible_cpu(c) {
> +	for_each_online_cpu(c) {
>  		for (i = 1; i < IPSTATS_MIB_MAX; i++)
>  			buff[i] += snmp_get_cpu_field64(mib, c, i, syncpoff);
>  	}

This will break on machines with cpu hotplug.

[RFC PATCH 3/3] net: Remove the useless parameter of __snmp6_fill_statsdev

[Jia He]

In commit a3a773726c9f ("net: Optimize snmp stat aggregation by walking 
all the percpu data at once"), __snmp6_fill_stats64 had been optimized 
by removing parameter items, so do the same for __snmp6_fill_statsdev.

Signed-off-by: Jia He <hejianet@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
Cc: Patrick McHardy <kaber@trash.net>
---
 net/ipv6/addrconf.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 1fce613..37ea2bb 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4944,18 +4944,18 @@ static inline size_t inet6_if_nlmsg_size(void)
 }
 
 static inline void __snmp6_fill_statsdev(u64 *stats, atomic_long_t *mib,
-				      int items, int bytes)
+					int bytes)
 {
 	int i;
-	int pad = bytes - sizeof(u64) * items;
+	int pad = bytes - sizeof(u64) * ICMP6_MIB_MAX;
 	BUG_ON(pad < 0);
 
 	/* Use put_unaligned() because stats may not be aligned for u64. */
-	put_unaligned(items, &stats[0]);
-	for (i = 1; i < items; i++)
+	put_unaligned(ICMP6_MIB_MAX, &stats[0]);
+	for (i = 1; i < ICMP6_MIB_MAX; i++)
 		put_unaligned(atomic_long_read(&mib[i]), &stats[i]);
 
-	memset(&stats[items], 0, pad);
+	memset(&stats[ICMP6_MIB_MAX], 0, pad);
 }
 
 static inline void __snmp6_fill_stats64(u64 *stats, void __percpu *mib,
@@ -4987,7 +4987,7 @@ static void snmp6_fill_stats(u64 *stats, struct inet6_dev *idev, int attrtype,
 				     offsetof(struct ipstats_mib, syncp));
 		break;
 	case IFLA_INET6_ICMP6STATS:
-		__snmp6_fill_statsdev(stats, idev->stats.icmpv6dev->mibs, ICMP6_MIB_MAX, bytes);
+		__snmp6_fill_statsdev(stats, idev->stats.icmpv6dev->mibs, bytes);
 		break;
 	}
 }
-- 
2.5.0