[RFC PATCH 0/3] net: Improve snmp6_fill_stats

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

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

Signed-off-by: Jia He <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <redacted>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <redacted>
Cc: Patrick McHardy <redacted>
---
 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

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 <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <redacted>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <redacted>
Cc: Patrick McHardy <redacted>
---
 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

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 <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <redacted>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <redacted>
Cc: Patrick McHardy <redacted>
---
 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

Jia He [off-list ref] wrote:

                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.

Yes, sorry about it，I am too hasty

B.R.

Jia He

On 8/8/16 7:12 PM, Florian Westphal wrote:

Jia He [off-list ref] wrote:

quoted

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

Signed-off-by: Jia He <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Alexey Kuznetsov <redacted>
Cc: James Morris <jmorris@namei.org>
Cc: Hideaki YOSHIFUJI <redacted>
Cc: Patrick McHardy <redacted>
---
  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.

From: Jia He <redacted>
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.

On Mon, 2016-08-08 at 18:22 +0800, Jia He wrote:

This will break on machines with cpu hotplug.

On Mon, 2016-08-08 at 18:22 +0800, Jia He wrote:

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.