Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1457687 > unrolled thread

[RFC PATCH 0/3] net: Improve snmp6_fill_stats

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/3] net: Improve snmp6_fill_stats Jia He <hejianet@gmail.com> - 2016-08-08 12:30 +0200
    [RFC PATCH 1/3] net: Remove unnecessary memset in __snmp6_fill_stats64 Jia He <hejianet@gmail.com> - 2016-08-08 12:30 +0200
      Re: [RFC PATCH 1/3] net: Remove unnecessary memset in  __snmp6_fill_stats64 Florian Westphal <fw@strlen.de> - 2016-08-08 13:20 +0200
        Re: [RFC PATCH 1/3] net: Remove unnecessary memset in  __snmp6_fill_stats64 hejianet <hejianet@gmail.com> - 2016-08-08 15:10 +0200
      Re: [RFC PATCH 1/3] net: Remove unnecessary memset in  __snmp6_fill_stats64 Eric Dumazet <eric.dumazet@gmail.com> - 2016-08-09 12:20 +0200
    [RFC PATCH 2/3] net: Replace for_each_possible_cpu with for_each_online_cpu Jia He <hejianet@gmail.com> - 2016-08-08 12:30 +0200
      Re: [RFC PATCH 2/3] net: Replace for_each_possible_cpu with  for_each_online_cpu David Miller <davem@davemloft.net> - 2016-08-08 20:00 +0200
      Re: [RFC PATCH 2/3] net: Replace for_each_possible_cpu with  for_each_online_cpu Eric Dumazet <eric.dumazet@gmail.com> - 2016-08-09 12:20 +0200
    [RFC PATCH 3/3] net: Remove the useless parameter of __snmp6_fill_statsdev Jia He <hejianet@gmail.com> - 2016-08-08 12:30 +0200

#1457687 — [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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.
>

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web