linux-kernel - Re: [PATCH] proc, smaps: reduce printing overhead

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1471531306.4319.38.camel@perches.com>
Date:	Thu, 18 Aug 2016 07:41:46 -0700
From:	Joe Perches <joe@...ches.com>
To:	Michal Hocko <mhocko@...nel.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	LKML <linux-kernel@...r.kernel.org>, Jann Horn <jann@...jh.net>
Subject: Re: [PATCH] proc, smaps: reduce printing overhead

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