proc: Make inline name size calculation automatic

PATCH In review Alexey Dobriyan 3 participants 6 messages Jun 14, 2018 – Aug 17, 2018

Subsystems (2)

VFS Proc filesystem

lore

Share & tools

lore

Alexey Dobriyan

author

2018-06-14 20:10 UTC #0 lore raw

Quoted 2 lines

Require a minimum inline name size of 33+1 to allow for names that look like two hex numbers with a dash between.

Hi, David.

Why 34? /proc will fallback to separate allocation for name anyway.

I sent nearly identical patch earlier.
https://marc.info/?l=linux-kernel&m=152667374404900&w=4

If you compare, the differences are:

* no BUILD_BUG_ON,
* 64 bytes is too litle even on 32-bit,
* 512 bytes is probably too much even on 64-bit.

Review

Quoted 3 lines

(quoted diff) +1 -2

-	.name		= proc_root.inline_name,
-	.inline_name	= "/proc",
+	.name		= "/proc",

This will "uninline" 5 bytes wasting space.

David Howells

2018-06-14 20:30 UTC #1 lore raw

Quoted 4 lines · Alexey Dobriyan

Quoted 2 lines
Require a minimum inline name size of 33+1 to allow for names that look like two hex numbers with a dash between.
Why 34? /proc will fallback to separate allocation for name anyway.

See above comment. I ran find on /proc and there were a bunch of files whose names were "<hex>-<hex>". Allow for 16-char hex addresses and add a NUL char to that and you get 34.

David

Alexey Dobriyan

author

2018-06-15 17:15 UTC #2 lore raw

Quoted 10 lines · David Howells

Quoted 4 lines · Alexey Dobriyan
Quoted 2 lines
Require a minimum inline name size of 33+1 to allow for names that look like two hex numbers with a dash between.
Why 34? /proc will fallback to separate allocation for name anyway.
See above comment. I ran find on /proc and there were a bunch of files whose names were "<hex>-<hex>". Allow for 16-char hex addresses and add a NUL char to that and you get 34.

Those must be /proc/*/map_files symlinks. If that's the case, they don't have PDEs allocated.

Alexey Dobriyan

author

2018-06-17 21:57 UTC #3 lore raw

PATCH 1/1 proc: fixup PDE allocation bloat

In reply to David Howells

commit 24074a35c5c975c94cd9691ae962855333aac47f
("proc: Make inline name size calculation automatic") started to put PDE allocations into kmalloc-256 which is unnecessary as ~40 character names are very rare.

Put allocation back into kmalloc-192 cache for 64-bit non-debug builds.

Put BUILD_BUG_ON to know when PDE size is gotten out of control.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/proc/inode.c    |    6 ++++--
 fs/proc/internal.h |   17 +++++++----------
 2 files changed, 11 insertions(+), 12 deletions(-)

fs/proc/inode.c +4 -2

@@ -105,8 +105,10 @@ void __init proc_init_kmemcache(void)
 		kmem_cache_create("pde_opener", sizeof(struct pde_opener), 0,
 				  SLAB_ACCOUNT|SLAB_PANIC, NULL);
 	proc_dir_entry_cache = kmem_cache_create_usercopy(
-		"proc_dir_entry", SIZEOF_PDE_SLOT, 0, SLAB_PANIC,
-		OFFSETOF_PDE_NAME, SIZEOF_PDE_INLINE_NAME, NULL);
+		"proc_dir_entry", SIZEOF_PDE, 0, SLAB_PANIC,
+		offsetof(struct proc_dir_entry, inline_name),
+		SIZEOF_PDE_INLINE_NAME, NULL);
+	BUILD_BUG_ON(sizeof(struct proc_dir_entry) >= SIZEOF_PDE);
 }
 
 static int proc_show_options(struct seq_file *seq, struct dentry *root)

fs/proc/internal.h +7 ~~-10~~

@@ -65,16 +65,13 @@ struct proc_dir_entry {
 	char inline_name[];
 } __randomize_layout;
 
-#define OFFSETOF_PDE_NAME offsetof(struct proc_dir_entry, inline_name)
-#define SIZEOF_PDE_SLOT					\
-	(OFFSETOF_PDE_NAME + 34 <= 64 ? 64 :		\
-	 OFFSETOF_PDE_NAME + 34 <= 128 ? 128 :		\
-	 OFFSETOF_PDE_NAME + 34 <= 192 ? 192 :		\
-	 OFFSETOF_PDE_NAME + 34 <= 256 ? 256 :		\
-	 OFFSETOF_PDE_NAME + 34 <= 512 ? 512 :		\
-	 0)
-
-#define SIZEOF_PDE_INLINE_NAME (SIZEOF_PDE_SLOT - OFFSETOF_PDE_NAME)
+#ifdef CONFIG_64BIT
+#define SIZEOF_PDE	(sizeof(spinlock_t) <= 4 ? 192 : 256)
+#else
+#define SIZEOF_PDE	(sizeof(spinlock_t) <= 4 ? 128 : 192)
+#endif
+
+#define SIZEOF_PDE_INLINE_NAME (SIZEOF_PDE - sizeof(struct proc_dir_entry))
 
 extern struct kmem_cache *proc_dir_entry_cache;
 void pde_free(struct proc_dir_entry *pde);

Shakeel Butt

2018-07-20 00:07 UTC #4 lore raw

Re: [PATCH] proc: fixup PDE allocation bloat

Quoted 20 lines · Alexey Dobriyan

commit 24074a35c5c975c94cd9691ae962855333aac47f
("proc: Make inline name size calculation automatic") started to put PDE allocations into kmalloc-256 which is unnecessary as ~40 character names are very rare.
Put allocation back into kmalloc-192 cache for 64-bit non-debug builds.
Put BUILD_BUG_ON to know when PDE size is gotten out of control.
Signed-off-by Alexey Dobriyan <[email protected]>
---
 fs/proc/inode.c    |    6 ++++--
 fs/proc/internal.h |   17 +++++++----------
 2 files changed, 11 insertions(+), 12 deletions(-)

fs/proc/inode.c void __init proc_init_kmemcache(void) · line 108

Quoted 6 lines

fs/proc/inode.c +1 -2

@@ -105,8 +105,10 @@ void __init proc_init_kmemcache(void)
                kmem_cache_create("pde_opener", sizeof(struct pde_opener), 0,
                                  SLAB_ACCOUNT|SLAB_PANIC, NULL);
        proc_dir_entry_cache = kmem_cache_create_usercopy(
-               "proc_dir_entry", SIZEOF_PDE_SLOT, 0, SLAB_PANIC,
-               OFFSETOF_PDE_NAME, SIZEOF_PDE_INLINE_NAME, NULL);
+               "proc_dir_entry", SIZEOF_PDE, 0, SLAB_PANIC,

Hi Alexey, can you comment if proc_dir_entry_cache should or shouldn't have SLAB_ACCOUNT flag?

Shakeel

Alexey Dobriyan

author

2018-08-17 13:28 UTC #5 lore raw

Re: [PATCH] proc: fixup PDE allocation bloat

Quoted 18 lines · Shakeel Butt

Quoted 17 lines · Alexey Dobriyan
commit 24074a35c5c975c94cd9691ae962855333aac47f
("proc: Make inline name size calculation automatic") started to put PDE allocations into kmalloc-256 which is unnecessary as ~40 character names are very rare.
Put allocation back into kmalloc-192 cache for 64-bit non-debug builds.
Put BUILD_BUG_ON to know when PDE size is gotten out of control.
Signed-off-by Alexey Dobriyan <[email protected]>
---
 fs/proc/inode.c    |    6 ++++--
 fs/proc/internal.h |   17 +++++++----------
 2 files changed, 11 insertions(+), 12 deletions(-)

Quoted 12 lines

fs/proc/inode.c void __init proc_init_kmemcache(void) · line 108

Quoted 9 lines

fs/proc/inode.c +1 -2

@@ -105,8 +105,10 @@ void __init proc_init_kmemcache(void)
                kmem_cache_create("pde_opener", sizeof(struct pde_opener), 0,
                                  SLAB_ACCOUNT|SLAB_PANIC, NULL);
        proc_dir_entry_cache = kmem_cache_create_usercopy(
-               "proc_dir_entry", SIZEOF_PDE_SLOT, 0, SLAB_PANIC,
-               OFFSETOF_PDE_NAME, SIZEOF_PDE_INLINE_NAME, NULL);
+               "proc_dir_entry", SIZEOF_PDE, 0, SLAB_PANIC,

Hi Alexey, can you comment if proc_dir_entry_cache should or shouldn't have SLAB_ACCOUNT flag?

It should not (but see below):

SLAB_ACCOUNT is for allocations which can be done by userspace directly:
open(2) directly allocates "struct file".

But /proc entries aren't like that: say, /proc/cpuinfo is created by kernel and userspace can't do anything about it.

Some subsystems create /proc entries based on userspace actions and those aren't related to hardware (example: xt_hashlimit.c) but those are few so kernel doesn't bother accounting those.

Or in other words: user can't mkdir(1) and touch(1) and ln(1) inside /proc at will and therefore PDEs aren't accounted.