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

[Michal Hocko]

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