Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1465190 > unrolled thread

[PATCH] proc, smaps: reduce printing overhead

Started byMichal Hocko <mhocko@kernel.org>
First post2016-08-18 13:40 +0200
Last post2016-08-23 18:20 +0200
Articles 20 on this page of 24 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] proc, smaps: reduce printing overhead Michal Hocko <mhocko@kernel.org> - 2016-08-18 13:40 +0200
    Re: [PATCH] proc, smaps: reduce printing overhead Joe Perches <joe@perches.com> - 2016-08-18 15:30 +0200
      Re: [PATCH] proc, smaps: reduce printing overhead Joe Perches <joe@perches.com> - 2016-08-18 16:50 +0200
      Re: [PATCH] proc, smaps: reduce printing overhead Joe Perches <joe@perches.com> - 2016-08-18 16:50 +0200
        Re: [PATCH] proc, smaps: reduce printing overhead Joe Perches <joe@perches.com> - 2016-08-19 03:00 +0200
          Re: [PATCH] proc, smaps: reduce printing overhead Michal Hocko <mhocko@kernel.org> - 2016-08-19 03:40 +0200
        Re: [PATCH] proc, smaps: reduce printing overhead Michal Hocko <mhocko@kernel.org> - 2016-08-19 06:00 +0200
      Re: [PATCH] proc, smaps: reduce printing overhead Michal Hocko <mhocko@kernel.org> - 2016-08-18 16:50 +0200
        Re: [PATCH] proc, smaps: reduce printing overhead Michal Hocko <mhocko@kernel.org> - 2016-08-18 16:50 +0200
    [PATCH 2/2] proc, smaps: reduce printing overhead Michal Hocko <mhocko@kernel.org> - 2016-08-19 12:20 +0200
    [PATCH 1/2] proc, meminfo: abstract show_val_kb Michal Hocko <mhocko@kernel.org> - 2016-08-19 12:20 +0200
    [PATCH 0/2] fs, proc: optimize smaps output formatting Michal Hocko <mhocko@kernel.org> - 2016-08-19 12:20 +0200
      Re: [PATCH 0/2] fs, proc: optimize smaps output formatting Joe Perches <joe@perches.com> - 2016-08-19 19:50 +0200
        Re: [PATCH 0/2] fs, proc: optimize smaps output formatting Joe Perches <joe@perches.com> - 2016-08-19 22:20 +0200
        Re: [PATCH 0/2] fs, proc: optimize smaps output formatting Michal Hocko <mhocko@kernel.org> - 2016-08-20 09:30 +0200
          Re: [PATCH 0/2] fs, proc: optimize smaps output formatting Joe Perches <joe@perches.com> - 2016-08-20 10:00 +0200
          [PATCH 0/2] seq: Speed up /proc/<pid>/smaps Joe Perches <joe@perches.com> - 2016-08-20 10:10 +0200
          [PATCH 1/2] seq_file: Add __seq_open_private_bufsize for seq file_operation sizes Joe Perches <joe@perches.com> - 2016-08-20 10:10 +0200
          [PATCH 2/2] proc: task_mmu: Reduce output processing cpu time Joe Perches <joe@perches.com> - 2016-08-20 10:10 +0200
            Re: [PATCH 2/2] proc: task_mmu: Reduce output processing cpu time Michal Hocko <mhocko@kernel.org> - 2016-08-22 09:30 +0200
              Re: [PATCH 2/2] proc: task_mmu: Reduce output processing cpu time Joe Perches <joe@perches.com> - 2016-08-22 10:10 +0200
                Re: [PATCH 2/2] proc: task_mmu: Reduce output processing cpu time Joe Perches <joe@perches.com> - 2016-08-22 10:40 +0200
                  Re: [PATCH 2/2] proc: task_mmu: Reduce output processing cpu time Michal Hocko <mhocko@kernel.org> - 2016-08-22 14:10 +0200
    Re: [PATCH] proc, smaps: reduce printing overhead Michal Hocko <mhocko@kernel.org> - 2016-08-23 18:20 +0200

Page 1 of 2  [1] 2  Next page →

#1465190 — [PATCH] proc, smaps: reduce printing overhead

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-18 13:40 +0200
Subject[PATCH] proc, smaps: reduce printing overhead
Message-ID<s7tPX-7AZ-1@gated-at.bofh.it>
From: Michal Hocko <mhocko@suse.com>

seq_printf (used by show_smap) can be pretty expensive when dumping a
lot of numbers.  Say we would like to get Rss and Pss from a particular
process.  In order to measure a pathological case let's generate as many
mappings as possible:

$ cat max_mmap.c
int main()
{
	while (mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED|MAP_POPULATE, -1, 0) != MAP_FAILED)
		;

	printf("pid:%d\n", getpid());
	pause();
	return 0;
}

$ awk '/^Rss/{rss+=$2} /^Pss/{pss+=$2} END {printf "rss:%d pss:%d\n", rss, pss}' /proc/$pid/smaps

would do a trick. The whole runtime is in the kernel space which is not
that that unexpected because smaps is not the cheapest one (we have to
do rmap walk etc.).

        Command being timed: "awk /^Rss/{rss+=$2} /^Pss/{pss+=$2} END {printf "rss:%d pss:%d\n", rss, pss} /proc/3050/smaps"
        User time (seconds): 0.01
        System time (seconds): 0.44
        Percent of CPU this job got: 99%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.47

But the perf says:
    22.55%  awk      [kernel.kallsyms]  [k] format_decode
    14.65%  awk      [kernel.kallsyms]  [k] vsnprintf
     6.40%  awk      [kernel.kallsyms]  [k] number
     2.53%  awk      [kernel.kallsyms]  [k] shmem_mapping
     2.53%  awk      [kernel.kallsyms]  [k] show_smap
     1.81%  awk      [kernel.kallsyms]  [k] lock_acquire

we are spending most of the time actually generating the output which is
quite lame. Let's replace seq_printf by seq_puts and seq_put_decimal_ull.
This will give us:
        Command being timed: "awk /^Rss/{rss+=$2} /^Pss/{pss+=$2} END {printf "rss:%d pss:%d\n", rss, pss} /proc/3067/smaps"
        User time (seconds): 0.00
        System time (seconds): 0.41
        Percent of CPU this job got: 99%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.42

which will give us ~7% improvement. Perf says:
    28.87%  awk      [kernel.kallsyms]  [k] seq_puts
     5.30%  awk      [kernel.kallsyms]  [k] vsnprintf
     4.54%  awk      [kernel.kallsyms]  [k] format_decode
     3.73%  awk      [kernel.kallsyms]  [k] show_smap
     2.56%  awk      [kernel.kallsyms]  [k] shmem_mapping
     1.92%  awk      [kernel.kallsyms]  [k] number
     1.80%  awk      [kernel.kallsyms]  [k] lock_acquire
     1.75%  awk      [kernel.kallsyms]  [k] print_name_value_kb

Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/proc/task_mmu.c | 63 ++++++++++++++++++++++--------------------------------
 1 file changed, 25 insertions(+), 38 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 187d84ef9de9..41c24c0811da 100644
--- 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");
+}
+
 static int show_smap(struct seq_file *m, void *v, int is_pid)
 {
 	struct vm_area_struct *vma = v;
@@ -765,45 +772,25 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
 
 	show_map_vma(m, vma, is_pid);
 
-	seq_printf(m,
-		   "Size:           %8lu kB\n"
-		   "Rss:            %8lu kB\n"
-		   "Pss:            %8lu kB\n"
-		   "Shared_Clean:   %8lu kB\n"
-		   "Shared_Dirty:   %8lu kB\n"
-		   "Private_Clean:  %8lu kB\n"
-		   "Private_Dirty:  %8lu kB\n"
-		   "Referenced:     %8lu kB\n"
-		   "Anonymous:      %8lu kB\n"
-		   "AnonHugePages:  %8lu kB\n"
-		   "ShmemPmdMapped: %8lu kB\n"
-		   "Shared_Hugetlb: %8lu kB\n"
-		   "Private_Hugetlb: %7lu kB\n"
-		   "Swap:           %8lu kB\n"
-		   "SwapPss:        %8lu kB\n"
-		   "KernelPageSize: %8lu kB\n"
-		   "MMUPageSize:    %8lu kB\n"
-		   "Locked:         %8lu kB\n",
-		   (vma->vm_end - vma->vm_start) >> 10,
-		   mss.resident >> 10,
-		   (unsigned long)(mss.pss >> (10 + PSS_SHIFT)),
-		   mss.shared_clean  >> 10,
-		   mss.shared_dirty  >> 10,
-		   mss.private_clean >> 10,
-		   mss.private_dirty >> 10,
-		   mss.referenced >> 10,
-		   mss.anonymous >> 10,
-		   mss.anonymous_thp >> 10,
-		   mss.shmem_thp >> 10,
-		   mss.shared_hugetlb >> 10,
-		   mss.private_hugetlb >> 10,
-		   mss.swap >> 10,
-		   (unsigned long)(mss.swap_pss >> (10 + PSS_SHIFT)),
-		   vma_kernel_pagesize(vma) >> 10,
-		   vma_mmu_pagesize(vma) >> 10,
-		   (vma->vm_flags & VM_LOCKED) ?
+	print_name_value_kb(m, "Size:           ", (vma->vm_end - vma->vm_start) >> 10);
+	print_name_value_kb(m, "Rss:            ", mss.resident >> 10);
+	print_name_value_kb(m, "Pss:            ", (unsigned long)(mss.pss >> (10 + PSS_SHIFT)));
+	print_name_value_kb(m, "Shared_Clean:   ", mss.shared_clean  >> 10);
+	print_name_value_kb(m, "Shared_Dirty:   ", mss.shared_dirty  >> 10);
+	print_name_value_kb(m, "Private_Clean:  ", mss.private_clean >> 10);
+	print_name_value_kb(m, "Private_Dirty:  ", mss.private_dirty >> 10);
+	print_name_value_kb(m, "Referenced:     ", mss.referenced >> 10);
+	print_name_value_kb(m, "Anonymous:      ", mss.anonymous >> 10);
+	print_name_value_kb(m, "AnonHugePages:  ", mss.anonymous_thp >> 10);
+	print_name_value_kb(m, "ShmemPmdMapped: ", mss.shmem_thp >> 10);
+	print_name_value_kb(m, "Shared_Hugetlb: ", mss.shared_hugetlb >> 10);
+	print_name_value_kb(m, "Private_Hugetlb: ", mss.private_hugetlb >> 10);
+	print_name_value_kb(m, "Swap:           ", mss.swap >> 10);
+	print_name_value_kb(m, "SwapPss:        ", (unsigned long)(mss.swap_pss >> (10 + PSS_SHIFT)));
+	print_name_value_kb(m, "KernelPageSize: ", vma_kernel_pagesize(vma) >> 10);
+	print_name_value_kb(m, "MMUPageSize:    ", vma_mmu_pagesize(vma) >> 10);
+	print_name_value_kb(m, "Locked:         ", (vma->vm_flags & VM_LOCKED) ?
 			(unsigned long)(mss.pss >> (10 + PSS_SHIFT)) : 0);
-
 	arch_show_smap(m, vma);
 	show_smap_vma_flags(m, vma);
 	m_cache_vma(m, vma);
-- 
2.8.1

[toc] | [next] | [standalone]

#1465311

FromJoe Perches <joe@perches.com>
Date2016-08-18 15:30 +0200
Message-ID<s7vyp-kW-11@gated-at.bofh.it>
In reply to#1465190
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

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);
+}

[toc] | [prev] | [next] | [standalone]

#1465583

FromJoe Perches <joe@perches.com>
Date2016-08-18 16:50 +0200
Message-ID<s7wNP-16E-1@gated-at.bofh.it>
In reply to#1465311
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

[toc] | [prev] | [next] | [standalone]

#1465587

FromJoe Perches <joe@perches.com>
Date2016-08-18 16:50 +0200
Message-ID<s7wNP-16E-11@gated-at.bofh.it>
In reply to#1465311
On Thu, 2016-08-18 at 16:41 +0200, Michal Hocko wrote:
> On Thu 18-08-16 16:26:16, Michal Hocko wrote:
> > 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.
> ohh, I misread the code. It tries to emulate the width formater. But is
> this really necessary? Do we know about any tools doing a fixed string
> parsing?

I don't, but it's proc and all the output formatting
shouldn't be changed.

Appended to is generally OK, but whitespace changed is
not good.

[toc] | [prev] | [next] | [standalone]

#1465672

FromJoe Perches <joe@perches.com>
Date2016-08-19 03:00 +0200
Message-ID<s7Gka-6XF-3@gated-at.bofh.it>
In reply to#1465587
On Thu, 2016-08-18 at 16:58 +0200, Michal Hocko wrote:
> On Thu 18-08-16 07:46:03, Joe Perches wrote:
> > 
> > On Thu, 2016-08-18 at 16:41 +0200, Michal Hocko wrote:
> > > 
> > > On Thu 18-08-16 16:26:16, Michal Hocko wrote:
> > > > 
> > > > 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.
> > > ohh, I misread the code. It tries to emulate the width formater. But is
> > > this really necessary? Do we know about any tools doing a fixed string
> > > parsing?
> > I don't, but it's proc and all the output formatting
> > shouldn't be changed.
> > 
> > Appended to is generally OK, but whitespace changed is
> > not good.
> OK fair enough, I will
> -       seq_write(m, s, 16);
> +       seq_puts(m, s);
> 
> because smaps needs more than 16 chars and export it in
> fs/proc/internal.h
> 
> will retest and repost.

The shift in the meminfo case uses PAGE_SHIFT too.

I suggest you make a local static instead and for
that one 17 byte line do

	seq_printf(m, "Private_Hugetlb: %7lu kB\n",  mss.private_hugetlb >> 10);

Another possible thing is to speed up all seq_puts 
uses with fixed chars strings by avoiding the runtime
strlen and use the compiler known string length:

https://lkml.org/lkml/2016/8/11/607

[toc] | [prev] | [next] | [standalone]

#1465813

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-19 03:40 +0200
Message-ID<s7GWS-7sc-41@gated-at.bofh.it>
In reply to#1465672
On Thu 18-08-16 08:23:30, Joe Perches wrote:
> On Thu, 2016-08-18 at 16:58 +0200, Michal Hocko wrote:
> > On Thu 18-08-16 07:46:03, Joe Perches wrote:
> > > 
> > > On Thu, 2016-08-18 at 16:41 +0200, Michal Hocko wrote:
> > > > 
> > > > On Thu 18-08-16 16:26:16, Michal Hocko wrote:
> > > > > 
> > > > > 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.
> > > > ohh, I misread the code. It tries to emulate the width formater. But is
> > > > this really necessary? Do we know about any tools doing a fixed string
> > > > parsing?
> > > I don't, but it's proc and all the output formatting
> > > shouldn't be changed.
> > > 
> > > Appended to is generally OK, but whitespace changed is
> > > not good.
> > OK fair enough, I will
> > -       seq_write(m, s, 16);
> > +       seq_puts(m, s);
> > 
> > because smaps needs more than 16 chars and export it in
> > fs/proc/internal.h
> > 
> > will retest and repost.
> 
> The shift in the meminfo case uses PAGE_SHIFT too.

OK, I have missed that part as well. So I have to do turn all the values
into page units from bytes just to let the function turn them into kB.
Sigh...

But anyway, I have done basically a copy of your show_val_kb and run
on top of the current linux-next and while the base is giving me the
comparable results to my mmomt based testing:
        Command being timed: "awk /^Rss/{rss+=$2} /^Pss/{pss+=$2} END {printf "rss:%d pss:%d\n", rss, pss} /proc/3021/smaps"
        User time (seconds): 0.00
        System time (seconds): 0.44
        Percent of CPU this job got: 99%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.45

The patch on top (below) is eating actually more system time which is
more than unexpected to me:
        Command being timed: "awk /^Rss/{rss+=$2} /^Pss/{pss+=$2} END {printf "rss:%d pss:%d\n", rss, pss} /proc/3048/smaps"
        User time (seconds): 0.00
        System time (seconds): 0.50
        Percent of CPU this job got: 99%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.50

and perf says
    21.65%  awk      [kernel.kallsyms]  [k] seq_puts
     8.41%  awk      [kernel.kallsyms]  [k] seq_write
     4.64%  awk      [kernel.kallsyms]  [k] vsnprintf
     4.20%  awk      [kernel.kallsyms]  [k] format_decode
     3.37%  awk      [kernel.kallsyms]  [k] show_smap
     2.15%  awk      [kernel.kallsyms]  [k] lock_acquire
     2.05%  awk      [kernel.kallsyms]  [k] num_to_str
     2.05%  awk      [kernel.kallsyms]  [k] print_name_value_kb
     1.76%  awk      [kernel.kallsyms]  [k] shmem_mapping
     1.61%  awk      [kernel.kallsyms]  [k] number

The results were slightly better when I dropped the alignment thingy
and returned back to seq_put_decimal_ull but it was still sys in range
0.46-0.48. So I though I just made some mistake in my previous measuring
but getting back to my testing kernel based on the mmotm tree it all
gets back to sys 0.40-0.41 while the base mmotm was 0.44-0.48.

I didn't get to compare perf profiles closely but I do not see anything
really outstanding there at the first glance. I will probably not pursue
this anymore as I do not have enough time to debug this any further
and the results do not seem so convincing with the linux-next anymore.
Maybe measuring this on the bare metal will lead to different results
(I was using kvm virt. machine). Or maybe I just made a stupid mistake
somewhere...
---
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 187d84ef9de9..eebebbc12c67 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -721,6 +721,23 @@ 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)
+{
+	static const char blanks[7] = {' ', ' ', ' ', ' ',' ', ' ', ' '};
+	char v[32];
+	int len;
+
+	seq_puts(m, name);
+	len = num_to_str(v, sizeof(v), val >> 10);
+	if (len > 0) {
+		if (len < 8)
+			seq_write(m, blanks, 8 - len);
+
+		seq_write(m, v, len);
+	}
+	seq_puts(m, " kB\n");
+}
+
 static int show_smap(struct seq_file *m, void *v, int is_pid)
 {
 	struct vm_area_struct *vma = v;
@@ -765,45 +782,25 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
 
 	show_map_vma(m, vma, is_pid);
 
-	seq_printf(m,
-		   "Size:           %8lu kB\n"
-		   "Rss:            %8lu kB\n"
-		   "Pss:            %8lu kB\n"
-		   "Shared_Clean:   %8lu kB\n"
-		   "Shared_Dirty:   %8lu kB\n"
-		   "Private_Clean:  %8lu kB\n"
-		   "Private_Dirty:  %8lu kB\n"
-		   "Referenced:     %8lu kB\n"
-		   "Anonymous:      %8lu kB\n"
-		   "AnonHugePages:  %8lu kB\n"
-		   "ShmemPmdMapped: %8lu kB\n"
-		   "Shared_Hugetlb: %8lu kB\n"
-		   "Private_Hugetlb: %7lu kB\n"
-		   "Swap:           %8lu kB\n"
-		   "SwapPss:        %8lu kB\n"
-		   "KernelPageSize: %8lu kB\n"
-		   "MMUPageSize:    %8lu kB\n"
-		   "Locked:         %8lu kB\n",
-		   (vma->vm_end - vma->vm_start) >> 10,
-		   mss.resident >> 10,
-		   (unsigned long)(mss.pss >> (10 + PSS_SHIFT)),
-		   mss.shared_clean  >> 10,
-		   mss.shared_dirty  >> 10,
-		   mss.private_clean >> 10,
-		   mss.private_dirty >> 10,
-		   mss.referenced >> 10,
-		   mss.anonymous >> 10,
-		   mss.anonymous_thp >> 10,
-		   mss.shmem_thp >> 10,
-		   mss.shared_hugetlb >> 10,
-		   mss.private_hugetlb >> 10,
-		   mss.swap >> 10,
-		   (unsigned long)(mss.swap_pss >> (10 + PSS_SHIFT)),
-		   vma_kernel_pagesize(vma) >> 10,
-		   vma_mmu_pagesize(vma) >> 10,
-		   (vma->vm_flags & VM_LOCKED) ?
-			(unsigned long)(mss.pss >> (10 + PSS_SHIFT)) : 0);
-
+	print_name_value_kb(m, "Size:           ", vma->vm_end - vma->vm_start);
+	print_name_value_kb(m, "Rss:            ", mss.resident);
+	print_name_value_kb(m, "Pss:            ", (unsigned long)(mss.pss >> PSS_SHIFT));
+	print_name_value_kb(m, "Shared_Clean:   ", mss.shared_clean);
+	print_name_value_kb(m, "Shared_Dirty:   ", mss.shared_dirty);
+	print_name_value_kb(m, "Private_Clean:  ", mss.private_clean);
+	print_name_value_kb(m, "Private_Dirty:  ", mss.private_dirty);
+	print_name_value_kb(m, "Referenced:     ", mss.referenced);
+	print_name_value_kb(m, "Anonymous:      ", mss.anonymous);
+	print_name_value_kb(m, "AnonHugePages:  ", mss.anonymous_thp);
+	print_name_value_kb(m, "ShmemPmdMapped: ", mss.shmem_thp);
+	print_name_value_kb(m, "Shared_Hugetlb: ", mss.shared_hugetlb);
+	print_name_value_kb(m, "Private_Hugetlb: ", mss.private_hugetlb);
+	print_name_value_kb(m, "Swap:           ", mss.swap);
+	print_name_value_kb(m, "SwapPss:        ", (unsigned long)(mss.swap_pss >> PSS_SHIFT));
+	print_name_value_kb(m, "KernelPageSize: ", vma_kernel_pagesize(vma));
+	print_name_value_kb(m, "MMUPageSize:    ", vma_mmu_pagesize(vma));
+	print_name_value_kb(m, "Locked:         ", (vma->vm_flags & VM_LOCKED) ?
+			(unsigned long)(mss.pss >> PSS_SHIFT) : 0);
 	arch_show_smap(m, vma);
 	show_smap_vma_flags(m, vma);
 	m_cache_vma(m, vma);
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1465971

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-19 06:00 +0200
Message-ID<s7Gka-6XF-5@gated-at.bofh.it>
In reply to#1465587
On Thu 18-08-16 07:46:03, Joe Perches wrote:
> On Thu, 2016-08-18 at 16:41 +0200, Michal Hocko wrote:
> > On Thu 18-08-16 16:26:16, Michal Hocko wrote:
> > > 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.
> > ohh, I misread the code. It tries to emulate the width formater. But is
> > this really necessary? Do we know about any tools doing a fixed string
> > parsing?
> 
> I don't, but it's proc and all the output formatting
> shouldn't be changed.
> 
> Appended to is generally OK, but whitespace changed is
> not good.

OK fair enough, I will
-       seq_write(m, s, 16);
+       seq_puts(m, s);

because smaps needs more than 16 chars and export it in
fs/proc/internal.h

will retest and repost.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1465597

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-18 16:50 +0200
Message-ID<s7wNP-16E-3@gated-at.bofh.it>
In reply to#1465311
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

[toc] | [prev] | [next] | [standalone]

#1465623

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-18 16:50 +0200
Message-ID<s7wNP-16E-13@gated-at.bofh.it>
In reply to#1465597
On Thu 18-08-16 16:26:16, Michal Hocko wrote:
> 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.

ohh, I misread the code. It tries to emulate the width formater. But is
this really necessary? Do we know about any tools doing a fixed string
parsing?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1466266 — [PATCH 2/2] proc, smaps: reduce printing overhead

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-19 12:20 +0200
Subject[PATCH 2/2] proc, smaps: reduce printing overhead
Message-ID<s7P46-4jX-9@gated-at.bofh.it>
In reply to#1465190
From: Michal Hocko <mhocko@suse.com>

seq_printf (used by show_smap) can be pretty expensive when dumping a
lot of numbers.  Say we would like to get Rss and Pss from a particular
process.  In order to measure a pathological case let's generate as many
mappings as possible:

$ cat max_mmap.c
int main()
{
	while (mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED|MAP_POPULATE, -1, 0) != MAP_FAILED)
		;

	printf("pid:%d\n", getpid());
	pause();
	return 0;
}

$ awk '/^Rss/{rss+=$2} /^Pss/{pss+=$2} END {printf "rss:%d pss:%d\n", rss, pss}' /proc/$pid/smaps

would do a trick. The whole runtime is in the kernel space which is not
that that unexpected because smaps is not the cheapest one (we have to
do pte walk etc.).

        Command being timed: "awk /^Rss/{rss+=$2} /^Pss/{pss+=$2} END {printf "rss:%d pss:%d\n", rss, pss} /proc/3050/smaps"
        User time (seconds): 0.01
        System time (seconds): 0.44
        Percent of CPU this job got: 99%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.47

This seems to be quite consistent (10 runs)
min: 0.44 max: 0.46 avg: 0.45 std: 0.01

perf says:
    22.55%  awk      [kernel.kallsyms]  [k] format_decode
    14.65%  awk      [kernel.kallsyms]  [k] vsnprintf
     6.40%  awk      [kernel.kallsyms]  [k] number
     2.53%  awk      [kernel.kallsyms]  [k] shmem_mapping
     2.53%  awk      [kernel.kallsyms]  [k] show_smap
     1.81%  awk      [kernel.kallsyms]  [k] lock_acquire

we are spending most of the time actually generating the output which
is quite lame. Let's replace seq_printf by a cheaper seq_write and
show_val_kb which are much cheaper because they are doing the bare
minimum. show_name_pages_kb already does that so mimic it and define a
helper for values given in bytes. This will give us (10 runs):
min: 0.31 max: 0.32 avg: 0.31 std: 0.00

Reported-by: Jann Horn <jann@thejh.net>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/proc/internal.h |  7 +++++++
 fs/proc/task_mmu.c | 58 ++++++++++++++++++------------------------------------
 2 files changed, 26 insertions(+), 39 deletions(-)

diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 10492701f4c1..6a369fc1949d 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -314,3 +314,10 @@ extern void show_val_kb(struct seq_file *m, unsigned long num);
  	seq_write(seq, name, sizeof(name));	\
  	show_val_kb(seq, (pages) << (PAGE_SHIFT - 10));\
  })
+
+#define show_name_bytes_kb(seq, name, val)	\
+({						\
+ 	BUILD_BUG_ON(!__builtin_constant_p(name));\
+ 	seq_write(seq, name, sizeof(name));	\
+ 	show_val_kb(seq, (val) >> 10);		\
+})
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 187d84ef9de9..eebb91d44a58 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -765,45 +765,25 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
 
 	show_map_vma(m, vma, is_pid);
 
-	seq_printf(m,
-		   "Size:           %8lu kB\n"
-		   "Rss:            %8lu kB\n"
-		   "Pss:            %8lu kB\n"
-		   "Shared_Clean:   %8lu kB\n"
-		   "Shared_Dirty:   %8lu kB\n"
-		   "Private_Clean:  %8lu kB\n"
-		   "Private_Dirty:  %8lu kB\n"
-		   "Referenced:     %8lu kB\n"
-		   "Anonymous:      %8lu kB\n"
-		   "AnonHugePages:  %8lu kB\n"
-		   "ShmemPmdMapped: %8lu kB\n"
-		   "Shared_Hugetlb: %8lu kB\n"
-		   "Private_Hugetlb: %7lu kB\n"
-		   "Swap:           %8lu kB\n"
-		   "SwapPss:        %8lu kB\n"
-		   "KernelPageSize: %8lu kB\n"
-		   "MMUPageSize:    %8lu kB\n"
-		   "Locked:         %8lu kB\n",
-		   (vma->vm_end - vma->vm_start) >> 10,
-		   mss.resident >> 10,
-		   (unsigned long)(mss.pss >> (10 + PSS_SHIFT)),
-		   mss.shared_clean  >> 10,
-		   mss.shared_dirty  >> 10,
-		   mss.private_clean >> 10,
-		   mss.private_dirty >> 10,
-		   mss.referenced >> 10,
-		   mss.anonymous >> 10,
-		   mss.anonymous_thp >> 10,
-		   mss.shmem_thp >> 10,
-		   mss.shared_hugetlb >> 10,
-		   mss.private_hugetlb >> 10,
-		   mss.swap >> 10,
-		   (unsigned long)(mss.swap_pss >> (10 + PSS_SHIFT)),
-		   vma_kernel_pagesize(vma) >> 10,
-		   vma_mmu_pagesize(vma) >> 10,
-		   (vma->vm_flags & VM_LOCKED) ?
-			(unsigned long)(mss.pss >> (10 + PSS_SHIFT)) : 0);
-
+	show_name_bytes_kb(m, "Size:           ", vma->vm_end - vma->vm_start);
+	show_name_bytes_kb(m, "Rss:            ", mss.resident);
+	show_name_bytes_kb(m, "Pss:            ", (unsigned long)(mss.pss >> PSS_SHIFT));
+	show_name_bytes_kb(m, "Shared_Clean:   ", mss.shared_clean);
+	show_name_bytes_kb(m, "Shared_Dirty:   ", mss.shared_dirty);
+	show_name_bytes_kb(m, "Private_Clean:  ", mss.private_clean);
+	show_name_bytes_kb(m, "Private_Dirty:  ", mss.private_dirty);
+	show_name_bytes_kb(m, "Referenced:     ", mss.referenced);
+	show_name_bytes_kb(m, "Anonymous:      ", mss.anonymous);
+	show_name_bytes_kb(m, "AnonHugePages:  ", mss.anonymous_thp);
+	show_name_bytes_kb(m, "ShmemPmdMapped: ", mss.shmem_thp);
+	show_name_bytes_kb(m, "Shared_Hugetlb: ", mss.shared_hugetlb);
+	show_name_bytes_kb(m, "Private_Hugetlb: ", mss.private_hugetlb);
+	show_name_bytes_kb(m, "Swap:           ", mss.swap);
+	show_name_bytes_kb(m, "SwapPss:        ", (unsigned long)(mss.swap_pss >> PSS_SHIFT));
+	show_name_bytes_kb(m, "KernelPageSize: ", vma_kernel_pagesize(vma));
+	show_name_bytes_kb(m, "MMUPageSize:    ", vma_mmu_pagesize(vma));
+	show_name_bytes_kb(m, "Locked:         ", (vma->vm_flags & VM_LOCKED) ?
+			(unsigned long)(mss.pss >> PSS_SHIFT) : 0);
 	arch_show_smap(m, vma);
 	show_smap_vma_flags(m, vma);
 	m_cache_vma(m, vma);
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1466267 — [PATCH 1/2] proc, meminfo: abstract show_val_kb

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-19 12:20 +0200
Subject[PATCH 1/2] proc, meminfo: abstract show_val_kb
Message-ID<s7P46-4jX-5@gated-at.bofh.it>
In reply to#1465190
From: Michal Hocko <mhocko@suse.com>

show_val_kb is currently tight meminfo usage but we would like to reuse
it for other proc files. Let's pull it out to proc internal header,
rename to show_name_pages_kb to be explicit that it operates on page
units and change show_val_kb to only print the value.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/proc/internal.h | 10 ++++++
 fs/proc/meminfo.c  | 93 ++++++++++++++++++++++++++----------------------------
 2 files changed, 55 insertions(+), 48 deletions(-)

diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 7931c558c192..10492701f4c1 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -304,3 +304,13 @@ extern unsigned long task_statm(struct mm_struct *,
 				unsigned long *, unsigned long *,
 				unsigned long *, unsigned long *);
 extern void task_mem(struct seq_file *, struct mm_struct *);
+
+/* prints given value (in kB) padded properly to 8 spaces */
+extern void show_val_kb(struct seq_file *m, unsigned long num);
+
+#define show_name_pages_kb(seq, name, pages)	\
+({						\
+ 	BUILD_BUG_ON(!__builtin_constant_p(name));\
+ 	seq_write(seq, name, sizeof(name));	\
+ 	show_val_kb(seq, (pages) << (PAGE_SHIFT - 10));\
+ })
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
index 8a428498d6b2..65e0bc6213e2 100644
--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -23,16 +23,13 @@ void __attribute__((weak)) arch_report_meminfo(struct seq_file *m)
 {
 }
 
-static void show_val_kb(struct seq_file *m, const char *s, unsigned long num)
+void show_val_kb(struct seq_file *m, 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);
-
+	len = num_to_str(v, sizeof(v), num);
 	if (len > 0) {
 		if (len < 8)
 			seq_write(m, blanks, 8 - len);
@@ -65,74 +62,74 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
 
 	available = si_mem_available();
 
-	show_val_kb(m, "MemTotal:       ", i.totalram);
-	show_val_kb(m, "MemFree:        ", i.freeram);
-	show_val_kb(m, "MemAvailable:   ", available);
-	show_val_kb(m, "Buffers:        ", i.bufferram);
-	show_val_kb(m, "Cached:         ", cached);
-	show_val_kb(m, "SwapCached:     ", total_swapcache_pages());
-	show_val_kb(m, "Active:         ", pages[LRU_ACTIVE_ANON] +
+	show_name_pages_kb(m, "MemTotal:       ", i.totalram);
+	show_name_pages_kb(m, "MemFree:        ", i.freeram);
+	show_name_pages_kb(m, "MemAvailable:   ", available);
+	show_name_pages_kb(m, "Buffers:        ", i.bufferram);
+	show_name_pages_kb(m, "Cached:         ", cached);
+	show_name_pages_kb(m, "SwapCached:     ", total_swapcache_pages());
+	show_name_pages_kb(m, "Active:         ", pages[LRU_ACTIVE_ANON] +
 					   pages[LRU_ACTIVE_FILE]);
-	show_val_kb(m, "Inactive:       ", pages[LRU_INACTIVE_ANON] +
+	show_name_pages_kb(m, "Inactive:       ", pages[LRU_INACTIVE_ANON] +
 					   pages[LRU_INACTIVE_FILE]);
-	show_val_kb(m, "Active(anon):   ", pages[LRU_ACTIVE_ANON]);
-	show_val_kb(m, "Inactive(anon): ", pages[LRU_INACTIVE_ANON]);
-	show_val_kb(m, "Active(file):   ", pages[LRU_ACTIVE_FILE]);
-	show_val_kb(m, "Inactive(file): ", pages[LRU_INACTIVE_FILE]);
-	show_val_kb(m, "Unevictable:    ", pages[LRU_UNEVICTABLE]);
-	show_val_kb(m, "Mlocked:        ", global_page_state(NR_MLOCK));
+	show_name_pages_kb(m, "Active(anon):   ", pages[LRU_ACTIVE_ANON]);
+	show_name_pages_kb(m, "Inactive(anon): ", pages[LRU_INACTIVE_ANON]);
+	show_name_pages_kb(m, "Active(file):   ", pages[LRU_ACTIVE_FILE]);
+	show_name_pages_kb(m, "Inactive(file): ", pages[LRU_INACTIVE_FILE]);
+	show_name_pages_kb(m, "Unevictable:    ", pages[LRU_UNEVICTABLE]);
+	show_name_pages_kb(m, "Mlocked:        ", global_page_state(NR_MLOCK));
 
 #ifdef CONFIG_HIGHMEM
-	show_val_kb(m, "HighTotal:      ", i.totalhigh);
-	show_val_kb(m, "HighFree:       ", i.freehigh);
-	show_val_kb(m, "LowTotal:       ", i.totalram - i.totalhigh);
-	show_val_kb(m, "LowFree:        ", i.freeram - i.freehigh);
+	show_name_pages_kb(m, "HighTotal:      ", i.totalhigh);
+	show_name_pages_kb(m, "HighFree:       ", i.freehigh);
+	show_name_pages_kb(m, "LowTotal:       ", i.totalram - i.totalhigh);
+	show_name_pages_kb(m, "LowFree:        ", i.freeram - i.freehigh);
 #endif
 
 #ifndef CONFIG_MMU
-	show_val_kb(m, "MmapCopy:       ",
+	show_name_pages_kb(m, "MmapCopy:       ",
 		    (unsigned long)atomic_long_read(&mmap_pages_allocated));
 #endif
 
-	show_val_kb(m, "SwapTotal:      ", i.totalswap);
-	show_val_kb(m, "SwapFree:       ", i.freeswap);
-	show_val_kb(m, "Dirty:          ",
+	show_name_pages_kb(m, "SwapTotal:      ", i.totalswap);
+	show_name_pages_kb(m, "SwapFree:       ", i.freeswap);
+	show_name_pages_kb(m, "Dirty:          ",
 		    global_node_page_state(NR_FILE_DIRTY));
-	show_val_kb(m, "Writeback:      ",
+	show_name_pages_kb(m, "Writeback:      ",
 		    global_node_page_state(NR_WRITEBACK));
-	show_val_kb(m, "AnonPages:      ",
+	show_name_pages_kb(m, "AnonPages:      ",
 		    global_node_page_state(NR_ANON_MAPPED));
-	show_val_kb(m, "Mapped:         ",
+	show_name_pages_kb(m, "Mapped:         ",
 		    global_node_page_state(NR_FILE_MAPPED));
-	show_val_kb(m, "Shmem:          ", i.sharedram);
-	show_val_kb(m, "Slab:           ",
+	show_name_pages_kb(m, "Shmem:          ", i.sharedram);
+	show_name_pages_kb(m, "Slab:           ",
 		    global_page_state(NR_SLAB_RECLAIMABLE) +
 		    global_page_state(NR_SLAB_UNRECLAIMABLE));
 
-	show_val_kb(m, "SReclaimable:   ",
+	show_name_pages_kb(m, "SReclaimable:   ",
 		    global_page_state(NR_SLAB_RECLAIMABLE));
-	show_val_kb(m, "SUnreclaim:     ",
+	show_name_pages_kb(m, "SUnreclaim:     ",
 		    global_page_state(NR_SLAB_UNRECLAIMABLE));
 	seq_printf(m, "KernelStack:    %8lu kB\n",
 		   global_page_state(NR_KERNEL_STACK_KB));
-	show_val_kb(m, "PageTables:     ",
+	show_name_pages_kb(m, "PageTables:     ",
 		    global_page_state(NR_PAGETABLE));
 #ifdef CONFIG_QUICKLIST
-	show_val_kb(m, "Quicklists:     ", quicklist_total_size());
+	show_name_pages_kb(m, "Quicklists:     ", quicklist_total_size());
 #endif
 
-	show_val_kb(m, "NFS_Unstable:   ",
+	show_name_pages_kb(m, "NFS_Unstable:   ",
 		    global_node_page_state(NR_UNSTABLE_NFS));
-	show_val_kb(m, "Bounce:         ",
+	show_name_pages_kb(m, "Bounce:         ",
 		    global_page_state(NR_BOUNCE));
-	show_val_kb(m, "WritebackTmp:   ",
+	show_name_pages_kb(m, "WritebackTmp:   ",
 		    global_node_page_state(NR_WRITEBACK_TEMP));
-	show_val_kb(m, "CommitLimit:    ", vm_commit_limit());
-	show_val_kb(m, "Committed_AS:   ", committed);
+	show_name_pages_kb(m, "CommitLimit:    ", vm_commit_limit());
+	show_name_pages_kb(m, "Committed_AS:   ", committed);
 	seq_printf(m, "VmallocTotal:   %8lu kB\n",
 		   (unsigned long)VMALLOC_TOTAL >> 10);
-	show_val_kb(m, "VmallocUsed:    ", 0ul);
-	show_val_kb(m, "VmallocChunk:   ", 0ul);
+	show_name_pages_kb(m, "VmallocUsed:    ", 0ul);
+	show_name_pages_kb(m, "VmallocChunk:   ", 0ul);
 
 #ifdef CONFIG_MEMORY_FAILURE
 	seq_printf(m, "HardwareCorrupted: %5lu kB\n",
@@ -140,17 +137,17 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
 #endif
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
-	show_val_kb(m, "AnonHugePages:  ",
+	show_name_pages_kb(m, "AnonHugePages:  ",
 		    global_node_page_state(NR_ANON_THPS) * HPAGE_PMD_NR);
-	show_val_kb(m, "ShmemHugePages: ",
+	show_name_pages_kb(m, "ShmemHugePages: ",
 		    global_node_page_state(NR_SHMEM_THPS) * HPAGE_PMD_NR);
-	show_val_kb(m, "ShmemPmdMapped: ",
+	show_name_pages_kb(m, "ShmemPmdMapped: ",
 		    global_node_page_state(NR_SHMEM_PMDMAPPED) * HPAGE_PMD_NR);
 #endif
 
 #ifdef CONFIG_CMA
-	show_val_kb(m, "CmaTotal:       ", totalcma_pages);
-	show_val_kb(m, "CmaFree:        ",
+	show_name_pages_kb(m, "CmaTotal:       ", totalcma_pages);
+	show_name_pages_kb(m, "CmaFree:        ",
 		    global_page_state(NR_FREE_CMA_PAGES));
 #endif
 
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1466269 — [PATCH 0/2] fs, proc: optimize smaps output formatting

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-19 12:20 +0200
Subject[PATCH 0/2] fs, proc: optimize smaps output formatting
Message-ID<s7P46-4jX-7@gated-at.bofh.it>
In reply to#1465190
Hi,
this is rebased on top of next-20160818. Joe has pointed out that
meminfo is using a similar trick so I have extracted guts of what we
have already and made it more generic to be usable for smaps as well
(patch 1). The second patch then replaces seq_printf with seq_write
and show_val_kb which should have smaller overhead and my measuring (in
kvm) shows quite a nice improvements. I hope kvm is not playing tricks
on me but I didn't get to test on a real HW.

Michal Hocko (2):
      proc, meminfo: abstract show_val_kb
      proc, smaps: reduce printing overhead

 fs/proc/internal.h | 17 ++++++++++
 fs/proc/meminfo.c  | 93 ++++++++++++++++++++++++++----------------------------
 fs/proc/task_mmu.c | 58 +++++++++++-----------------------
 3 files changed, 81 insertions(+), 87 deletions(-)

[toc] | [prev] | [next] | [standalone]

#1466584 — Re: [PATCH 0/2] fs, proc: optimize smaps output formatting

FromJoe Perches <joe@perches.com>
Date2016-08-19 19:50 +0200
SubjectRe: [PATCH 0/2] fs, proc: optimize smaps output formatting
Message-ID<s7W5A-aY-9@gated-at.bofh.it>
In reply to#1466269
On Fri, 2016-08-19 at 12:12 +0200, Michal Hocko wrote:
> Hi,
> this is rebased on top of next-20160818. Joe has pointed out that
> meminfo is using a similar trick so I have extracted guts of what we
> have already and made it more generic to be usable for smaps as well
> (patch 1). The second patch then replaces seq_printf with seq_write
> and show_val_kb which should have smaller overhead and my measuring (in
> kvm) shows quite a nice improvements. I hope kvm is not playing tricks
> on me but I didn't get to test on a real HW.


Hi Michal.

A few comments:

For the first patch:

I think this isn't worth the expansion in object size (x86-64 defconfig)

$ size fs/proc/meminfo.o*
   text	   data	    bss	    dec	    hex	filename
   2698	      8	      0	   2706	    a92	fs/proc/meminfo.o.new
   2142	      8	      0	   2150	    866	fs/proc/meminfo.o.old

Creating a new static in task_mmu would be smaller and faster code.

There are only 3 other uses of %8lu in fs/proc/task_nommu.c and
those use bytes not kB.

There are a few other likely not performance sensitive similar
uses in <arch>/mm

$ git grep -E "seq_printf.*%8lu kB" arch
arch/x86/mm/pageattr.c:	seq_printf(m, "DirectMap4k:    %8lu kB\n",
arch/x86/mm/pageattr.c:	seq_printf(m, "DirectMap2M:    %8lu kB\n",
arch/x86/mm/pageattr.c:	seq_printf(m, "DirectMap4M:    %8lu kB\n",
arch/x86/mm/pageattr.c:		seq_printf(m, "DirectMap1G:    %8lu kB\n",
arch/s390/mm/pageattr.c:	seq_printf(m, "DirectMap4k:    %8lu kB\n",
arch/s390/mm/pageattr.c:	seq_printf(m, "DirectMap1M:    %8lu kB\n",
arch/s390/mm/pageattr.c:	seq_printf(m, "DirectMap2G:    %8lu kB\n",

For the second patch:

seq_show starts with a PAGE_SIZE buffer and if that buffer isn't
big enough, seq_show redoes the entire output done to that point
into a new buffer << 1 until the buffer is big enough to hold
the output.

So I expect this case of multiple pages / megabytes worth of smap
output (40MB in your pathological case) would be rather faster if
single_open_size was used appropriately for expected output size.

And this would definitely be faster if seq_has_overflowed() was
used somewhere in the iteration loop.

[toc] | [prev] | [next] | [standalone]

#1466657 — Re: [PATCH 0/2] fs, proc: optimize smaps output formatting

FromJoe Perches <joe@perches.com>
Date2016-08-19 22:20 +0200
SubjectRe: [PATCH 0/2] fs, proc: optimize smaps output formatting
Message-ID<s7YqJ-1O3-23@gated-at.bofh.it>
In reply to#1466584
On Fri, 2016-08-19 at 10:43 -0700, Joe Perches wrote:
> And this would definitely be faster if seq_has_overflowed() was
> used somewhere in the iteration loop.

Adding a seq_has_overflowed() test seems unnecessary as the
fs/seq_file.c traverse() static function already does a
seq_has_overflowed().

And I get:

$ t_mm (your allocate all vma program modified to show count)
count: 65514 pid:2051
$ wc -c /proc/2051/smaps 
39515615 /proc/2051/smaps

smap vma output is a little more than 600 bytes per vma.

I'll look around to see how best go use single_open_size
assuming 768 bytes/vma rounded up to the next PAGE_SIZE.

[toc] | [prev] | [next] | [standalone]

#1466791 — Re: [PATCH 0/2] fs, proc: optimize smaps output formatting

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-20 09:30 +0200
SubjectRe: [PATCH 0/2] fs, proc: optimize smaps output formatting
Message-ID<s88T7-8qJ-5@gated-at.bofh.it>
In reply to#1466584
On Fri 19-08-16 10:43:15, Joe Perches wrote:
> On Fri, 2016-08-19 at 12:12 +0200, Michal Hocko wrote:
> > Hi,
> > this is rebased on top of next-20160818. Joe has pointed out that
> > meminfo is using a similar trick so I have extracted guts of what we
> > have already and made it more generic to be usable for smaps as well
> > (patch 1). The second patch then replaces seq_printf with seq_write
> > and show_val_kb which should have smaller overhead and my measuring (in
> > kvm) shows quite a nice improvements. I hope kvm is not playing tricks
> > on me but I didn't get to test on a real HW.
> 
> 
> Hi Michal.
> 
> A few comments:
> 
> For the first patch:
> 
> I think this isn't worth the expansion in object size (x86-64 defconfig)
> 
> $ size fs/proc/meminfo.o*
>    text	   data	    bss	    dec	    hex	filename
>    2698	      8	      0	   2706	    a92	fs/proc/meminfo.o.new
>    2142	      8	      0	   2150	    866	fs/proc/meminfo.o.old
> 
> Creating a new static in task_mmu would be smaller and faster code.

Hmm, nasty...
add/remove: 0/0 grow/shrink: 2/1 up/down: 1081/-24 (1057)
function                                     old     new   delta
meminfo_proc_show                           1134    1745    +611
show_smap                                    560    1030    +470
show_val_kb                                  140     116     -24
Total: Before=91716, After=92773, chg +1.15%

it seems to be calls to seq_write which blown up the size. So I've tried
to put seq_write back to show_val_kb and did only sizeof() inside those
macros and that reduced the size but not fully back to the original code
size. So it seems the value shifts consumed some portion of that as well.
I've ended up with the following incremental diff which leads to
   text    data     bss     dec     hex filename
 100728    1443     400  102571   190ab fs/proc/built-in.o.next
 101658    1443     400  103501   1944d fs/proc/built-in.o.patched
 100951    1443     400  102794   1918a fs/proc/built-in.o.incremental

There is still some increase wrt. the baseline but I guess that can be
explained by single seq_printf -> many show_name_val_kb calls.

If that looks acceptable I will respin both patches. I would really
like to prefer to not duplicate show_val_kb into task_mmu as much as
possible, though.

---
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 6a369fc1949d..de9c561f83b4 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -307,17 +307,3 @@ extern void task_mem(struct seq_file *, struct mm_struct *);
 
 /* prints given value (in kB) padded properly to 8 spaces */
 extern void show_val_kb(struct seq_file *m, unsigned long num);
-
-#define show_name_pages_kb(seq, name, pages)	\
-({						\
- 	BUILD_BUG_ON(!__builtin_constant_p(name));\
- 	seq_write(seq, name, sizeof(name));	\
- 	show_val_kb(seq, (pages) << (PAGE_SHIFT - 10));\
- })
-
-#define show_name_bytes_kb(seq, name, val)	\
-({						\
- 	BUILD_BUG_ON(!__builtin_constant_p(name));\
- 	seq_write(seq, name, sizeof(name));	\
- 	show_val_kb(seq, (val) >> 10);		\
-})
diff --git a/fs/proc/meminfo.c b/fs/proc/meminfo.c
index 65e0bc6213e2..7f2937cd231c 100644
--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -39,6 +39,14 @@ void show_val_kb(struct seq_file *m, unsigned long num)
 	seq_write(m, " kB\n", 4);
 }
 
+static void show_name_pages_kb(struct seq_file *m, const char *name,
+		unsigned long pages)
+{
+ 	seq_write(m, name, 16);
+ 	show_val_kb(m, pages << (PAGE_SHIFT - 10));
+}
+
+
 static int meminfo_proc_show(struct seq_file *m, void *v)
 {
 	struct sysinfo i;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index eebb91d44a58..a92898f20a1f 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -721,6 +721,19 @@ void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
 {
 }
 
+static void show_name_val_kb(struct seq_file *m, const char *name, size_t len,
+		unsigned long val)
+{
+	seq_write(m, name, len);
+ 	show_val_kb(m, val >> 10);
+}
+
+#define show_name_bytes_kb(seq, name, val)	\
+({						\
+ 	BUILD_BUG_ON(!__builtin_constant_p(name));\
+ 	show_name_val_kb(seq, name, sizeof(name), val);\
+})
+
 static int show_smap(struct seq_file *m, void *v, int is_pid)
 {
 	struct vm_area_struct *vma = v;
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1466794 — Re: [PATCH 0/2] fs, proc: optimize smaps output formatting

FromJoe Perches <joe@perches.com>
Date2016-08-20 10:00 +0200
SubjectRe: [PATCH 0/2] fs, proc: optimize smaps output formatting
Message-ID<s89m9-89-5@gated-at.bofh.it>
In reply to#1466791
On Sat, 2016-08-20 at 09:29 +0200, Michal Hocko wrote:
> On Fri 19-08-16 10:43:15, Joe Perches wrote:
> > 
> > On Fri, 2016-08-19 at 12:12 +0200, Michal Hocko wrote:
> > > 
> > > Hi,
> > > this is rebased on top of next-20160818. Joe has pointed out that
> > > meminfo is using a similar trick so I have extracted guts of what we
> > > have already and made it more generic to be usable for smaps as well
> > > (patch 1). The second patch then replaces seq_printf with seq_write
> > > and show_val_kb which should have smaller overhead and my measuring (in
> > > kvm) shows quite a nice improvements. I hope kvm is not playing tricks
> > > on me but I didn't get to test on a real HW.
> > 
> > Hi Michal.
> > 
> > A few comments:
> > 
> > For the first patch:
> > 
> > I think this isn't worth the expansion in object size (x86-64 defconfig)
> > 
> > $ size fs/proc/meminfo.o*
> >    text	   data	    bss	    dec	    hex	filename
> >    2698	      8	      0	   2706	    a92	fs/proc/meminfo.o.new
> >    2142	      8	      0	   2150	    866	fs/proc/meminfo.o.old
> > 
> > Creating a new static in task_mmu would be smaller and faster code.
> Hmm, nasty...
> add/remove: 0/0 grow/shrink: 2/1 up/down: 1081/-24 (1057)
> function                                     old     new   delta
> meminfo_proc_show                           1134    1745    +611
> show_smap                                    560    1030    +470
> show_val_kb                                  140     116     -24
> Total: Before=91716, After=92773, chg +1.15%
> 
> it seems to be calls to seq_write which blown up the size. So I've tried
> to put seq_write back to show_val_kb and did only sizeof() inside those
> macros and that reduced the size but not fully back to the original code
> size. So it seems the value shifts consumed some portion of that as well.
> I've ended up with the following incremental diff which leads to
>    text    data     bss     dec     hex filename
>  100728    1443     400  102571   190ab fs/proc/built-in.o.next
>  101658    1443     400  103501   1944d fs/proc/built-in.o.patched
>  100951    1443     400  102794   1918a fs/proc/built-in.o.incremental
> 
> There is still some increase wrt. the baseline but I guess that can be
> explained by single seq_printf -> many show_name_val_kb calls.
> 
> If that looks acceptable I will respin both patches. I would really
> like to prefer to not duplicate show_val_kb into task_mmu as much as
> possible, though.

I think the patch set I'll send you in a few minutes
will speed up /proc/<pid>/smaps a whole lot more.

Please test it using your little test bench.

cheers, Joe

[toc] | [prev] | [next] | [standalone]

#1466796 — [PATCH 0/2] seq: Speed up /proc/<pid>/smaps

FromJoe Perches <joe@perches.com>
Date2016-08-20 10:10 +0200
Subject[PATCH 0/2] seq: Speed up /proc/<pid>/smaps
Message-ID<s89vP-qD-3@gated-at.bofh.it>
In reply to#1466791
Doing a simple cat of these files can take a lot more cpu than
it should.  Optimize it a bit.

Joe Perches (2):
  seq_file: Add __seq_open_private_bufsize for seq file_operation sizes
  proc: task_mmu: Reduce output processing cpu time

 fs/proc/task_mmu.c       | 94 ++++++++++++++++++++++++------------------------
 fs/seq_file.c            | 31 ++++++++++++++++
 include/linux/seq_file.h |  3 ++
 3 files changed, 82 insertions(+), 46 deletions(-)

-- 
2.8.0.rc4.16.g56331f8

[toc] | [prev] | [next] | [standalone]

#1466797 — [PATCH 1/2] seq_file: Add __seq_open_private_bufsize for seq file_operation sizes

FromJoe Perches <joe@perches.com>
Date2016-08-20 10:10 +0200
Subject[PATCH 1/2] seq_file: Add __seq_open_private_bufsize for seq file_operation sizes
Message-ID<s89vP-qD-5@gated-at.bofh.it>
In reply to#1466791
Specifying an initial output buffer size can reduce the
number of regenerations of the seq_<output> buffers when
the buffer overflows.

Add another version of __seq_open_private that takes an
initial buffer size.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/seq_file.c            | 31 +++++++++++++++++++++++++++++++
 include/linux/seq_file.h |  3 +++
 2 files changed, 34 insertions(+)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index b8ac757e..d98fa77 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -652,6 +652,37 @@ int seq_open_private(struct file *filp, const struct seq_operations *ops,
 }
 EXPORT_SYMBOL(seq_open_private);
 
+void *__seq_open_private_bufsize(struct file *f,
+				 const struct seq_operations *ops,
+				 int psize, size_t bufsize)
+{
+	int rc;
+	void *private;
+	struct seq_file *seq;
+
+	private = kzalloc(psize, GFP_KERNEL);
+	if (private == NULL)
+		goto out;
+
+	rc = seq_open(f, ops);
+	if (rc < 0)
+		goto out_free;
+
+	seq = f->private_data;
+	seq->private = private;
+
+	kfree(seq->buf);
+	seq->buf = seq_buf_alloc(seq->size = round_up(bufsize, PAGE_SIZE));
+
+	return private;
+
+out_free:
+	kfree(private);
+out:
+	return NULL;
+}
+EXPORT_SYMBOL(__seq_open_private_bufsize);
+
 void seq_putc(struct seq_file *m, char c)
 {
 	if (m->count >= m->size)
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index e305b66..719f1b8 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -136,6 +136,9 @@ int single_open(struct file *, int (*)(struct seq_file *, void *), void *);
 int single_open_size(struct file *, int (*)(struct seq_file *, void *), void *, size_t);
 int single_release(struct inode *, struct file *);
 void *__seq_open_private(struct file *, const struct seq_operations *, int);
+void *__seq_open_private_bufsize(struct file *f,
+				 const struct seq_operations *ops,
+				 int psize, size_t bufsize);
 int seq_open_private(struct file *, const struct seq_operations *, int);
 int seq_release_private(struct inode *, struct file *);
 
-- 
2.8.0.rc4.16.g56331f8

[toc] | [prev] | [next] | [standalone]

#1466798 — [PATCH 2/2] proc: task_mmu: Reduce output processing cpu time

FromJoe Perches <joe@perches.com>
Date2016-08-20 10:10 +0200
Subject[PATCH 2/2] proc: task_mmu: Reduce output processing cpu time
Message-ID<s89vQ-qD-7@gated-at.bofh.it>
In reply to#1466791
Use the new __seq_open_private_buffer to estimate the final
output /proc/<pid>/smaps filesize to reduce the number of
reallocations of overflowed buffers.

Use a simpler single-line function to emit various values in kB.

Signed-off-by: Joe Perches <joe@perches.com>
---
 fs/proc/task_mmu.c | 94 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 46 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 187d84e..170509b 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -224,19 +224,21 @@ static void m_stop(struct seq_file *m, void *v)
 static int proc_maps_open(struct inode *inode, struct file *file,
 			const struct seq_operations *ops, int psize)
 {
-	struct proc_maps_private *priv = __seq_open_private(file, ops, psize);
+	struct proc_maps_private *priv;
+	struct mm_struct *mm;
+
+	mm = proc_mem_open(inode, PTRACE_MODE_READ);
+	if (IS_ERR(mm))
+		return PTR_ERR(mm);
 
+	priv = __seq_open_private_bufsize(file, ops, psize,
+					  mm && mm->map_count ?
+					  mm->map_count * 0x300 : PAGE_SIZE);
 	if (!priv)
 		return -ENOMEM;
 
 	priv->inode = inode;
-	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
-	if (IS_ERR(priv->mm)) {
-		int err = PTR_ERR(priv->mm);
-
-		seq_release_private(inode, file);
-		return err;
-	}
+	priv->mm = mm;
 
 	return 0;
 }
@@ -721,6 +723,25 @@ void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
 {
 }
 
+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 >> 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);
+}
+
 static int show_smap(struct seq_file *m, void *v, int is_pid)
 {
 	struct vm_area_struct *vma = v;
@@ -765,44 +786,25 @@ static int show_smap(struct seq_file *m, void *v, int is_pid)
 
 	show_map_vma(m, vma, is_pid);
 
-	seq_printf(m,
-		   "Size:           %8lu kB\n"
-		   "Rss:            %8lu kB\n"
-		   "Pss:            %8lu kB\n"
-		   "Shared_Clean:   %8lu kB\n"
-		   "Shared_Dirty:   %8lu kB\n"
-		   "Private_Clean:  %8lu kB\n"
-		   "Private_Dirty:  %8lu kB\n"
-		   "Referenced:     %8lu kB\n"
-		   "Anonymous:      %8lu kB\n"
-		   "AnonHugePages:  %8lu kB\n"
-		   "ShmemPmdMapped: %8lu kB\n"
-		   "Shared_Hugetlb: %8lu kB\n"
-		   "Private_Hugetlb: %7lu kB\n"
-		   "Swap:           %8lu kB\n"
-		   "SwapPss:        %8lu kB\n"
-		   "KernelPageSize: %8lu kB\n"
-		   "MMUPageSize:    %8lu kB\n"
-		   "Locked:         %8lu kB\n",
-		   (vma->vm_end - vma->vm_start) >> 10,
-		   mss.resident >> 10,
-		   (unsigned long)(mss.pss >> (10 + PSS_SHIFT)),
-		   mss.shared_clean  >> 10,
-		   mss.shared_dirty  >> 10,
-		   mss.private_clean >> 10,
-		   mss.private_dirty >> 10,
-		   mss.referenced >> 10,
-		   mss.anonymous >> 10,
-		   mss.anonymous_thp >> 10,
-		   mss.shmem_thp >> 10,
-		   mss.shared_hugetlb >> 10,
-		   mss.private_hugetlb >> 10,
-		   mss.swap >> 10,
-		   (unsigned long)(mss.swap_pss >> (10 + PSS_SHIFT)),
-		   vma_kernel_pagesize(vma) >> 10,
-		   vma_mmu_pagesize(vma) >> 10,
-		   (vma->vm_flags & VM_LOCKED) ?
-			(unsigned long)(mss.pss >> (10 + PSS_SHIFT)) : 0);
+	show_val_kb(m, "Size:           ", vma->vm_end - vma->vm_start);
+	show_val_kb(m, "Rss:            ", mss.resident);
+	show_val_kb(m, "Pss:            ", mss.pss >> PSS_SHIFT);
+	show_val_kb(m, "Shared_Clean:   ", mss.shared_clean);
+	show_val_kb(m, "Shared_Dirty:   ", mss.shared_dirty);
+	show_val_kb(m, "Private_Clean:  ", mss.private_clean);
+	show_val_kb(m, "Private_Dirty:  ", mss.private_dirty);
+	show_val_kb(m, "Referenced:     ", mss.referenced);
+	show_val_kb(m, "Anonymous:      ", mss.anonymous);
+	show_val_kb(m, "AnonHugePages:  ", mss.anonymous_thp);
+	show_val_kb(m, "ShmemPmdMapped: ", mss.shmem_thp);
+	show_val_kb(m, "Shared_Hugetlb: ", mss.shared_hugetlb);
+	seq_printf(m, "Private_Hugetlb: %7lu kB\n",  mss.private_hugetlb >> 10);
+	show_val_kb(m, "Swap:           ", mss.swap);
+	show_val_kb(m, "SwapPss:        ", mss.swap_pss >> PSS_SHIFT);
+	show_val_kb(m, "KernelPageSize: ", vma_kernel_pagesize(vma));
+	show_val_kb(m, "MMUPageSize:    ", vma_mmu_pagesize(vma));
+	show_val_kb(m, "Locked:         ",
+		    vma->vm_flags & VM_LOCKED ? mss.pss >> PSS_SHIFT : 0);
 
 	arch_show_smap(m, vma);
 	show_smap_vma_flags(m, vma);
-- 
2.8.0.rc4.16.g56331f8

[toc] | [prev] | [next] | [standalone]

#1467417 — Re: [PATCH 2/2] proc: task_mmu: Reduce output processing cpu time

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-22 09:30 +0200
SubjectRe: [PATCH 2/2] proc: task_mmu: Reduce output processing cpu time
Message-ID<s8RQd-3fx-17@gated-at.bofh.it>
In reply to#1466798
On Sat 20-08-16 01:00:17, Joe Perches wrote:
[...]
>  static int proc_maps_open(struct inode *inode, struct file *file,
>  			const struct seq_operations *ops, int psize)
>  {
> -	struct proc_maps_private *priv = __seq_open_private(file, ops, psize);
> +	struct proc_maps_private *priv;
> +	struct mm_struct *mm;
> +
> +	mm = proc_mem_open(inode, PTRACE_MODE_READ);
> +	if (IS_ERR(mm))
> +		return PTR_ERR(mm);
>  
> +	priv = __seq_open_private_bufsize(file, ops, psize,
> +					  mm && mm->map_count ?
> +					  mm->map_count * 0x300 : PAGE_SIZE);

NAK to this! Seriously, this just gives any random user access to user
defined amount of memory which not accounted, not reclaimable and a
potential consumer of any higher order blocks.

Besides that, at least one show_smap output will always fit inside the
single page and AFAIR (it's been quite a while since I've looked into
seq_file internals) the buffer grows only when the single show doesn't
fit in.

>  	if (!priv)
>  		return -ENOMEM;
>  
>  	priv->inode = inode;
> -	priv->mm = proc_mem_open(inode, PTRACE_MODE_READ);
> -	if (IS_ERR(priv->mm)) {
> -		int err = PTR_ERR(priv->mm);
> -
> -		seq_release_private(inode, file);
> -		return err;
> -	}
> +	priv->mm = mm;
>  
>  	return 0;
>  }
> @@ -721,6 +723,25 @@ void __weak arch_show_smap(struct seq_file *m, struct vm_area_struct *vma)
>  {
>  }
>  
> +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 >> 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);
> +}
> +

I really do not understand why you insist on code duplication rather
than reuse but if you really insist then just make this (without the
above __seq_open_private_bufsize, re-measure and add the results to the
changelog and repost.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web