| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 22 Sep 2016 13:38:52 +0800
From: hejianet <hejianet@...il.com>
To: Marcelo <marcelo.leitner@...il.com>
Cc: netdev@...r.kernel.org, linux-sctp@...r.kernel.org,
linux-kernel@...r.kernel.org, davem@...emloft.net,
Alexey Kuznetsov <kuznet@....inr.ac.ru>,
James Morris <jmorris@...ei.org>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
Patrick McHardy <kaber@...sh.net>,
Vlad Yasevich <vyasevich@...il.com>,
Neil Horman <nhorman@...driver.com>,
Steffen Klassert <steffen.klassert@...unet.com>,
Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [RFC PATCH v3 2/7] proc: Reduce cache miss in
{snmp,netstat}_seq_show
On 9/22/16 2:24 AM, Marcelo wrote:
> 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 :-)
>
>> On 9/14/16 7:55 PM, Marcelo wrote:
>>> 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:
>>>>> 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..
>>>> 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
>>
>> diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
>> index c6ee8a2..cc41590 100644
>> --- a/net/ipv4/proc.c
>> +++ b/net/ipv4/proc.c
>> @@ -486,22 +486,37 @@ static const struct file_operations snmp_seq_fops = {
>> */
>> static int netstat_seq_show_tcpext(struct seq_file *seq, void *v)
>> {
>> - int i;
>> - unsigned long buff[LINUX_MIB_MAX];
>> + int i, c;
>> + unsigned long buff[LINUX_MIB_MAX/2 + 1];
>> struct net *net = seq->private;
>>
>> - memset(buff, 0, sizeof(unsigned long) * LINUX_MIB_MAX);
>> + memset(buff, 0, sizeof(unsigned long) * (LINUX_MIB_MAX/2 + 1));
>>
>> seq_puts(seq, "TcpExt:");
>> for (i = 0; snmp4_net_list[i].name; i++)
>> seq_printf(seq, " %s", snmp4_net_list[i].name);
>>
>> seq_puts(seq, "\nTcpExt:");
>> - snmp_get_cpu_field_batch(buff, snmp4_net_list,
>> - net->mib.net_statistics);
>> - for (i = 0; snmp4_net_list[i].name; i++)
>> + for_each_possible_cpu(c) {
>> + for (i = 0; i < LINUX_MIB_MAX/2; i++)
>> + buff[i] += snmp_get_cpu_field(
>> + net->mib.net_statistics,
>> + c, snmp4_net_list[i].entry);
>> + }
>> + for (i = 0; i < LINUX_MIB_MAX/2; i++)
>> seq_printf(seq, " %lu", buff[i]);
>>
>> + memset(buff, 0, sizeof(unsigned long) * (LINUX_MIB_MAX/2 + 1));
>> + for_each_possible_cpu(c) {
>> + for (i = LINUX_MIB_MAX/2; snmp4_net_list[i].name; i++)
>> + buff[i - LINUX_MIB_MAX/2] += snmp_get_cpu_field(
>> + net->mib.net_statistics,
>> + c,
>> + snmp4_net_list[i].entry);
>> + }
>> + for (i = LINUX_MIB_MAX/2; snmp4_net_list[i].name; i++)
>> + seq_printf(seq, " %lu", buff[i - LINUX_MIB_MAX/2]);
>> +
>> return 0;
>> }
> 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
>
>>>>>> +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
>>> Thanks,
>>> Marcelo
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
Powered by blists - more mailing lists