Re: [RFC PATCH v3 2/7] proc: Reduce cache miss in {snmp,netstat}_seq_show

[hejianet <hejianet@gmail.com>]

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:
>> 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;
  }

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