In a PowerPc server with large cpu number(160), besides commit
a3a773726c9f ("net: Optimize snmp stat aggregation by walking all
the percpu data at once"), I watched several other snmp_fold_field
callsites which will cause high cache miss rate.
My simple test case, which read from the procfs items endlessly:
/***********************************************************/
#include <stdlib.h>
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#define LINELEN 2560
int main(int argc, char **argv)
{
int i;
int fd = -1 ;
int rdsize = 0;
char buf[LINELEN+1];
buf[LINELEN] = 0;
memset(buf,0,LINELEN);
if(1 >= argc) {
printf("file name empty\n");
return -1;
}
fd = open(argv[1], O_RDWR, 0644);
if(0 > fd){
printf("open error\n");
return -2;
}
for(i=0;i<0xffffffff;i++) {
while(0 < (rdsize = read(fd,buf,LINELEN))){
//nothing here
}
lseek(fd, 0, SEEK_SET);
}
close(fd);
return 0;
}
/**********************************************************/
compile and run:
gcc test.c -o test
perf stat -d -e cache-misses ./test /proc/net/snmp
perf stat -d -e cache-misses ./test /proc/net/snmp6
perf stat -d -e cache-misses ./test /proc/net/netstat
perf stat -d -e cache-misses ./test /proc/net/sctp/snmp
perf stat -d -e cache-misses ./test /proc/net/xfrm_stat
before the patch set:
====================
Performance counter stats for 'system wide':
355911097 cache-misses [40.08%]
2356829300 L1-dcache-loads [60.04%]
355642645 L1-dcache-load-misses # 15.09% of all L1-dcache hits [60.02%]
346544541 LLC-loads [59.97%]
389763 LLC-load-misses # 0.11% of all LL-cache hits [40.02%]
6.245162638 seconds time elapsed
After the patch set:
===================
Performance counter stats for 'system wide':
194992476 cache-misses [40.03%]
6718051877 L1-dcache-loads [60.07%]
194871921 L1-dcache-load-misses # 2.90% of all L1-dcache hits [60.11%]
187632232 LLC-loads [60.04%]
464466 LLC-load-misses # 0.25% of all LL-cache hits [39.89%]
6.868422769 seconds time elapsed
The cache-miss rate can be reduced from 15% to 2.9%
v3:
- introduce generic interface (suggested by Marcelo Ricardo Leitner)
- use max_t instead of self defined macro (suggested by David Miller)
v2:
- fix bug in udplite statistics.
- snmp_seq_show is split into 2 parts
Jia He (7):
net:snmp: Introduce generic interfaces for snmp_get_cpu_field{,64}
proc: Reduce cache miss in {snmp,netstat}_seq_show
proc: Reduce cache miss in snmp6_seq_show
proc: Reduce cache miss in sctp_snmp_seq_show
proc: Reduce cache miss in xfrm_statistics_seq_show
ipv6: Remove useless parameter in __snmp6_fill_statsdev
net: Suppress the "Comparison to NULL could be written" warning
include/net/ip.h | 23 +++++++++
net/ipv4/proc.c | 138 +++++++++++++++++++++++++++++++++------------------
net/ipv6/addrconf.c | 12 ++---
net/ipv6/proc.c | 32 ++++++++----
net/sctp/proc.c | 10 ++--
net/xfrm/xfrm_proc.c | 10 +++-
6 files changed, 157 insertions(+), 68 deletions(-)
--
1.8.3.1
This is to introduced the generic batch interfaces for snmp_get_cpu_field{,64}.
It exchanges the two for-loops for collecting the percpu statistics data.
This can aggregate the data by going through all the items of each cpu
sequentially.
Signed-off-by: Jia He <redacted>
Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
include/net/ip.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Signed-off-by: Jia He <redacted>
---
net/ipv6/proc.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Then snmp_seq_show and netstat_seq_show are split into 2 parts to avoid build
warning "the frame size" larger than 1024 on s390.
Signed-off-by: Jia He <redacted>
---
net/ipv4/proc.c | 106 +++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 74 insertions(+), 32 deletions(-)
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Signed-off-by: Jia He <redacted>
---
net/sctp/proc.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Signed-off-by: Jia He <redacted>
---
net/xfrm/xfrm_proc.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
@@ -4952,18 +4952,18 @@ static inline size_t inet6_if_nlmsg_size(void)}staticinlinevoid__snmp6_fill_statsdev(u64*stats,atomic_long_t*mib,-intitems,intbytes)+intbytes){inti;-intpad=bytes-sizeof(u64)*items;+intpad=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);}staticinlinevoid__snmp6_fill_stats64(u64*stats,void__percpu*mib,
This is to suppress the checkpatch.pl warning "Comparison to NULL
could be written". No functional changes here.
Signed-off-by: Jia He <redacted>
---
net/ipv4/proc.c | 32 ++++++++++++++++----------------
net/sctp/proc.c | 2 +-
2 files changed, 17 insertions(+), 17 deletions(-)
On Fri, Sep 09, 2016 at 02:33:57PM +0800, Jia He wrote:
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Then snmp_seq_show and netstat_seq_show are split into 2 parts to avoid build
warning "the frame size" larger than 1024 on s390.
Yeah about that, did you test it with stack overflow detection?
These arrays can be quite large.
One more below..
On Fri, Sep 09, 2016 at 02:33:58PM +0800, Jia He wrote:
quoted hunk
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Signed-off-by: Jia He <redacted>
---
net/ipv6/proc.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 09, 2016 at 02:33:57PM +0800, Jia He wrote:
quoted
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Then snmp_seq_show and netstat_seq_show are split into 2 parts to avoid build
warning "the frame size" larger than 1024 on s390.
Yeah about that, did you test it with stack overflow detection?
These arrays can be quite large.
One more below..
I found scripts/checkstack.pl could analyze the stack usage statically.
[root@tian-lp1 kernel]# objdump -d vmlinux | scripts/checkstack.pl ppc64|grep seq
0xc0000000007d4b18 netstat_seq_show_tcpext.isra.7 [vmlinux]:1120
0xc0000000007ccbe8 fib_triestat_seq_show [vmlinux]: 496
0xc00000000083e7a4 tcp6_seq_show [vmlinux]: 480
0xc0000000007d4908 snmp_seq_show_ipstats.isra.6 [vmlinux]:464
0xc0000000007d4d18 netstat_seq_show_ipext.isra.8 [vmlinux]:464
0xc0000000006f5bd8 proto_seq_show [vmlinux]: 416
0xc0000000007f5718 xfrm_statistics_seq_show [vmlinux]: 416
0xc0000000007405b4 dev_seq_printf_stats [vmlinux]: 400
seems the stack usage in netstat_seq_show_tcpext is too big.
Will consider how to reduce it
B.R.
Jia
On Fri, Sep 09, 2016 at 02:33:58PM +0800, Jia He wrote:
quoted
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Signed-off-by: Jia He <redacted>
---
net/ipv6/proc.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 09, 2016 at 02:33:57PM +0800, Jia He wrote:
quoted
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Then snmp_seq_show and netstat_seq_show are split into 2 parts to avoid build
warning "the frame size" larger than 1024 on s390.
Yeah about that, did you test it with stack overflow detection?
These arrays can be quite large.
One more below..
Do you think it is acceptable if the stack usage is a little larger than 1024?
e.g. 1120
I can't find any other way to reduce the stack usage except use "static" before
unsigned long buff[TCP_MIB_MAX]
PS. sizeof buff is about TCP_MIB_MAX(116)*8=928
B.R.
Hi Jia,
On Wed, Sep 14, 2016 at 01:58:42PM +0800, hejianet wrote:
Hi Marcelo
On 9/13/16 2:57 AM, Marcelo wrote:
quoted
On Fri, Sep 09, 2016 at 02:33:57PM +0800, Jia He wrote:
quoted
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Then snmp_seq_show and netstat_seq_show are split into 2 parts to avoid build
warning "the frame size" larger than 1024 on s390.
Yeah about that, did you test it with stack overflow detection?
These arrays can be quite large.
One more below..
Do you think it is acceptable if the stack usage is a little larger than 1024?
e.g. 1120
I can't find any other way to reduce the stack usage except use "static" before
unsigned long buff[TCP_MIB_MAX]
PS. sizeof buff is about TCP_MIB_MAX(116)*8=928
B.R.
That's pretty much the question. Linux has the option on some archs to
run with 4Kb (4KSTACKS option), so this function alone would be using
25% of it in this last case. While on x86_64, it uses 16Kb (6538b8ea886e
("x86_64: expand kernel stack to 16K")).
Adding static to it is not an option as it actually makes the variable
shared amongst the CPUs (and then you have concurrency issues), plus the
fact that it's always allocated, even while not in use.
Others here certainly know better than me if it's okay to make such
usage of the stach.
quoted
quoted
+static int netstat_seq_show_ipext(struct seq_file *seq, void *v)+{+ int i;+ u64 buff64[IPSTATS_MIB_MAX];+ struct net *net = seq->private; seq_puts(seq, "\nIpExt:"); for (i = 0; snmp4_ipextstats_list[i].name != NULL; i++) seq_printf(seq, " %s", snmp4_ipextstats_list[i].name); seq_puts(seq, "\nIpExt:");
You're missing a memset() call here.
Not sure if you missed this one or not..
Thanks,
Marcelo
Hi Marcelo
sorry for the late, just came back from a vacation.
On 9/14/16 7:55 PM, Marcelo wrote:
Hi Jia,
On Wed, Sep 14, 2016 at 01:58:42PM +0800, hejianet wrote:
quoted
Hi Marcelo
On 9/13/16 2:57 AM, Marcelo wrote:
quoted
On Fri, Sep 09, 2016 at 02:33:57PM +0800, Jia He wrote:
quoted
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Then snmp_seq_show and netstat_seq_show are split into 2 parts to avoid build
warning "the frame size" larger than 1024 on s390.
Yeah about that, did you test it with stack overflow detection?
These arrays can be quite large.
One more below..
Do you think it is acceptable if the stack usage is a little larger than 1024?
e.g. 1120
I can't find any other way to reduce the stack usage except use "static" before
unsigned long buff[TCP_MIB_MAX]
PS. sizeof buff is about TCP_MIB_MAX(116)*8=928
B.R.
That's pretty much the question. Linux has the option on some archs to
run with 4Kb (4KSTACKS option), so this function alone would be using
25% of it in this last case. While on x86_64, it uses 16Kb (6538b8ea886e
("x86_64: expand kernel stack to 16K")).
Adding static to it is not an option as it actually makes the variable
shared amongst the CPUs (and then you have concurrency issues), plus the
fact that it's always allocated, even while not in use.
Others here certainly know better than me if it's okay to make such
usage of the stach.
What about this patch instead?
It is a trade-off. I split the aggregation process into 2 parts, it will
increase the cache miss a little bit, but it can reduce the stack usage.
After this, stack usage is 672bytes
objdump -d vmlinux | ./scripts/checkstack.pl ppc64 | grep seq_show
0xc0000000007f7cc0 netstat_seq_show_tcpext.isra.3 [vmlinux]:672
On Thu, Sep 22, 2016 at 12:18:46AM +0800, hejianet wrote:
Hi Marcelo
sorry for the late, just came back from a vacation.
Hi, no problem. Hope your batteries are recharged now :-)
quoted hunk
On 9/14/16 7:55 PM, Marcelo wrote:
quoted
Hi Jia,
On Wed, Sep 14, 2016 at 01:58:42PM +0800, hejianet wrote:
quoted
Hi Marcelo
On 9/13/16 2:57 AM, Marcelo wrote:
quoted
On Fri, Sep 09, 2016 at 02:33:57PM +0800, Jia He wrote:
quoted
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Then snmp_seq_show and netstat_seq_show are split into 2 parts to avoid build
warning "the frame size" larger than 1024 on s390.
Yeah about that, did you test it with stack overflow detection?
These arrays can be quite large.
One more below..
Do you think it is acceptable if the stack usage is a little larger than 1024?
e.g. 1120
I can't find any other way to reduce the stack usage except use "static" before
unsigned long buff[TCP_MIB_MAX]
PS. sizeof buff is about TCP_MIB_MAX(116)*8=928
B.R.
That's pretty much the question. Linux has the option on some archs to
run with 4Kb (4KSTACKS option), so this function alone would be using
25% of it in this last case. While on x86_64, it uses 16Kb (6538b8ea886e
("x86_64: expand kernel stack to 16K")).
Adding static to it is not an option as it actually makes the variable
shared amongst the CPUs (and then you have concurrency issues), plus the
fact that it's always allocated, even while not in use.
Others here certainly know better than me if it's okay to make such
usage of the stach.
What about this patch instead?
It is a trade-off. I split the aggregation process into 2 parts, it will
increase the cache miss a little bit, but it can reduce the stack usage.
After this, stack usage is 672bytes
objdump -d vmlinux | ./scripts/checkstack.pl ppc64 | grep seq_show
0xc0000000007f7cc0 netstat_seq_show_tcpext.isra.3 [vmlinux]:672
Yep, it halves the stack usage, but it doesn't look good heh
But well, you may try to post the patchset (with or without this last
change, you pick) officially and see how it goes. As you're posting as
RFC, it's not being evaluated as seriously.
FWIW, I tested your patches, using your test and /proc/net/snmp file on
a x86_64 box, Intel(R) Xeon(R) CPU E5-2643 v3.
Before the patches:
Performance counter stats for './test /proc/net/snmp':
5.225 cache-misses
12.708.673.785 L1-dcache-loads
1.288.450.174 L1-dcache-load-misses # 10,14% of all L1-dcache hits
1.271.857.028 LLC-loads
4.122 LLC-load-misses # 0,00% of all LL-cache hits
9,174936524 seconds time elapsed
After:
Performance counter stats for './test /proc/net/snmp':
2.865 cache-misses
30.203.883.807 L1-dcache-loads
1.215.774.643 L1-dcache-load-misses # 4,03% of all L1-dcache hits
1.181.662.831 LLC-loads
2.685 LLC-load-misses # 0,00% of all LL-cache hits
13,374445056 seconds time elapsed
Numbers were steady across multiple runs.
Marcelo
quoted
quoted
quoted
quoted
+static int netstat_seq_show_ipext(struct seq_file *seq, void *v)+{+ int i;+ u64 buff64[IPSTATS_MIB_MAX];+ struct net *net = seq->private; seq_puts(seq, "\nIpExt:"); for (i = 0; snmp4_ipextstats_list[i].name != NULL; i++) seq_printf(seq, " %s", snmp4_ipextstats_list[i].name); seq_puts(seq, "\nIpExt:");
You're missing a memset() call here.
Not sure if you missed this one or not..
indeed, thanks
B.R.
Jia
quoted
Thanks,
Marcelo
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 22, 2016 at 12:18:46AM +0800, hejianet wrote:
quoted
Hi Marcelo
sorry for the late, just came back from a vacation.
Hi, no problem. Hope your batteries are recharged now :-)
quoted
On 9/14/16 7:55 PM, Marcelo wrote:
quoted
Hi Jia,
On Wed, Sep 14, 2016 at 01:58:42PM +0800, hejianet wrote:
quoted
Hi Marcelo
On 9/13/16 2:57 AM, Marcelo wrote:
quoted
On Fri, Sep 09, 2016 at 02:33:57PM +0800, Jia He wrote:
quoted
This is to use the generic interface snmp_get_cpu_field{,64}_batch to
aggregate the data by going through all the items of each cpu sequentially.
Then snmp_seq_show and netstat_seq_show are split into 2 parts to avoid build
warning "the frame size" larger than 1024 on s390.
Yeah about that, did you test it with stack overflow detection?
These arrays can be quite large.
One more below..
Do you think it is acceptable if the stack usage is a little larger than 1024?
e.g. 1120
I can't find any other way to reduce the stack usage except use "static" before
unsigned long buff[TCP_MIB_MAX]
PS. sizeof buff is about TCP_MIB_MAX(116)*8=928
B.R.
That's pretty much the question. Linux has the option on some archs to
run with 4Kb (4KSTACKS option), so this function alone would be using
25% of it in this last case. While on x86_64, it uses 16Kb (6538b8ea886e
("x86_64: expand kernel stack to 16K")).
Adding static to it is not an option as it actually makes the variable
shared amongst the CPUs (and then you have concurrency issues), plus the
fact that it's always allocated, even while not in use.
Others here certainly know better than me if it's okay to make such
usage of the stach.
What about this patch instead?
It is a trade-off. I split the aggregation process into 2 parts, it will
increase the cache miss a little bit, but it can reduce the stack usage.
After this, stack usage is 672bytes
objdump -d vmlinux | ./scripts/checkstack.pl ppc64 | grep seq_show
0xc0000000007f7cc0 netstat_seq_show_tcpext.isra.3 [vmlinux]:672
Yep, it halves the stack usage, but it doesn't look good heh
But well, you may try to post the patchset (with or without this last
change, you pick) officially and see how it goes. As you're posting as
RFC, it's not being evaluated as seriously.
Thanks for the suggestion, I will remove it in future patch version
FWIW, I tested your patches, using your test and /proc/net/snmp file on
a x86_64 box, Intel(R) Xeon(R) CPU E5-2643 v3.
Before the patches:
Performance counter stats for './test /proc/net/snmp':
5.225 cache-misses
12.708.673.785 L1-dcache-loads
1.288.450.174 L1-dcache-load-misses # 10,14% of all L1-dcache hits
1.271.857.028 LLC-loads
4.122 LLC-load-misses # 0,00% of all LL-cache hits
9,174936524 seconds time elapsed
After:
Performance counter stats for './test /proc/net/snmp':
2.865 cache-misses
30.203.883.807 L1-dcache-loads
1.215.774.643 L1-dcache-load-misses # 4,03% of all L1-dcache hits
1.181.662.831 LLC-loads
2.685 LLC-load-misses # 0,00% of all LL-cache hits
13,374445056 seconds time elapsed
Numbers were steady across multiple runs.
Marcelo
Yes, I guess your X86 machine doesn't have the large cpu number as mine (cpu#=160).
The cache misses rate difference btw before and after this patch will be more
significant if the cpu number is large.
B.R.
Jia
quoted
quoted
quoted
quoted
quoted
+static int netstat_seq_show_ipext(struct seq_file *seq, void *v)+{+ int i;+ u64 buff64[IPSTATS_MIB_MAX];+ struct net *net = seq->private; seq_puts(seq, "\nIpExt:"); for (i = 0; snmp4_ipextstats_list[i].name != NULL; i++) seq_printf(seq, " %s", snmp4_ipextstats_list[i].name); seq_puts(seq, "\nIpExt:");
You're missing a memset() call here.
Not sure if you missed this one or not..
indeed, thanks
B.R.
Jia
quoted
Thanks,
Marcelo
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html