Re: [PATCH] proc, smaps: reduce printing overhead

[Joe Perches]

On Thu, 2016-08-18 at 16:26 +0200, Michal Hocko wrote:
> On Thu 18-08-16 06:26:05, Joe Perches wrote:
> > On Thu, 2016-08-18 at 13:31 +0200, Michal Hocko wrote:
> > []
> > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > []
> > > @@ -721,6 +721,13 @@ void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
> > >  {
> > >  }
> > >  
> > > +static void print_name_value_kb(struct seq_file *m, const char *name, unsigned long val)
> > > +{
> > > +	seq_puts(m, name);
> > > +	seq_put_decimal_ull(m, 0, val);
> > > +	seq_puts(m, " kB\n");
> > > +}
> > The seq_put_decimal_ull function has different arguments
> > in -next, the separator is changed to const char *.
> > 
> > $ git log --stat -p -1 49f87a2773000ced0c850639975f43134de48342 -- fs/seq_file.c
> OK, I haven't noticed that. I can rebase, although I wonder whether the
> change is a universal win. Not that the strlen on a short string would
> matter but just look at the usage:
>      76 " "
>       1 "/"
>       1 ""
>       1 "cpu  "
>       1 "intr "
>       1 g ? " " : "",
>       1 "\nFDSize:\t"
>       1 "\nGid:\t"
>       1 "\nNgid:\t"
>       1 "\nnonvoluntary_ctxt_switches:\t"
>       1 "\nPid:\t"
>       1 "\nPPid:\t"
>       1 "\nSigQ:\t"
>       1 "\nTgid:\t"
>       1 "\nTracerPid:\t"
>       1 "\nUid:\t"
>       1 "Seccomp:\t"
>       1 "softirq "
>      10 "\t"
>       1 "Threads:\t"
>       1 "voluntary_ctxt_switches:\t"
> 
> Most users simply need a single character. Those few could just seq_puts
> for the string followed by seq_put_decimal_ull.
>  
> > 
> > Maybe this change in fs/proc/meminfo.c should be
> > made into a public function.
> > 
> > $ git log --stat -p -1  5e27340c20516104c38668e597b3200f339fc64d
> > 
> > static void show_val_kb(struct seq_file *m, const char *s, unsigned long num)
> > +{
> > +       char v[32];
> > +       static const char blanks[7] = {' ', ' ', ' ', ' ',' ', ' ', ' '};
> > +       int len;
> > +
> > +       len = num_to_str(v, sizeof(v), num << (PAGE_SHIFT - 10));
> > +
> > +       seq_write(m, s, 16);
> > +
> > +       if (len > 0) {
> > +               if (len < 8)
> > +                       seq_write(m, blanks, 8 - len);
> > +
> > +               seq_write(m, v, len);
> > +       }
> > +       seq_write(m, " kB\n", 4);
> > +}
> Uff, this is just ugly as hell, seriously! a) why does it hardcode the
> name to be 16 characters max in such a subtle way and b) doesn't it try
> to be overly clever when doing that in the caller doesn't cost all that
> much?

It's optimized for the meminfo caller which had
16 byte fixed length string prefixes.

>  Sure you can save few bytes in the spaces but then I would just
> argue to use \t rather than fixed string length.

The output formatting can't be changed as it /proc

And your proposed patch is actually inappropriate
as it effectively changes %8lu to %lu