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

[Michal Hocko]

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? Sure you can save few bytes in the spaces but then I would just
argue to use \t rather than fixed string length.

That being said, I agree that there should be a common helper I just
really dislike the above.

-- 
Michal Hocko
SUSE Labs