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

[hejianet]

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