Skip to contentSkip to conversation

vfs.git bits and pieces

GIT PULL Al Viro 6 participants 8 messages Nov 20, 2013 – Nov 21, 2013

Subsystems (5)
Memory management x86 VFS MM core +1 more subsystems
lore
Share & tools
lore

Al Viro

author maintainer
#0 lore raw

[Apologies for delay; I'd spent the last day hunting down something that turned out to be a VM leak completely unrelated to this stuff - it's present in mainline, for starters. Unreliable reproducers make for fun bisects ;-/ Anyway, by now I'm absolutely sure that this is a VM bug and not something I had somehow managed to break, so...]

Assorted bits that got missed in the first pull request + fixes for a couple of coredump regressions. Please, pull from the usual place -
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

Shortlog:
Al Viro (7):
dump_align(): fix the dumb braino dump_emit(): use __kernel_write(), not vfs_write()
gfs2: endianness misannotations
consolidate simple ->d_delete() instances take read_seqbegin_or_lock() and friends to seqlock.h dcache.c: get rid of pointless macros
fold try_to_ascend() into the sole remaining caller

Diffstat:

 arch/ia64/kernel/perfmon.c |    8 +----
 fs/9p/vfs_dentry.c         |   19 +---------
 fs/configfs/dir.c          |   12 +------
 fs/coredump.c              |    6 ++--
 fs/dcache.c                |   84 ++++++++++---------------------------------
 fs/efivarfs/super.c        |   11 +-----
 fs/gfs2/lock_dlm.c         |    8 ++--
 fs/gfs2/quota.c            |   23 +++++-------
 fs/gfs2/rgrp.c             |    4 +-
 fs/hostfs/hostfs_kern.c    |   11 +-----
 fs/libfs.c                 |   12 ++++---
 fs/proc/generic.c          |   18 +---------
 fs/proc/namespaces.c       |    8 +----
 include/linux/fs.h         |    2 +
 include/linux/seqlock.h    |   29 +++++++++++++++
 kernel/cgroup.c            |    7 +---
 net/sunrpc/rpc_pipe.c      |   11 +-----
 17 files changed, 86 insertions(+), 187 deletions(-)
author maintainer
#1 lore raw
Quoted 5 lines · Al Viro

[Apologies for delay; I'd spent the last day hunting down something that turned out to be a VM leak completely unrelated to this stuff - it's present in mainline, for starters. Unreliable reproducers make for fun bisects ;-/ Anyway, by now I'm absolutely sure that this is a VM bug and not something I had somehow managed to break, so...]

BTW, something odd happened to mm/memory.c - either a mangled patch or a lost followup. Take a look at the last commit in there:
commit ea1e7ed33708c7a760419ff9ded0a6cb90586a50

Author: Kirill A. Shutemov <[email protected]>
Date:   Thu Nov 14 14:31:53 2013 -0800

mm: create a separate slab for page->ptl allocation

If DEBUG_SPINLOCK and DEBUG_LOCK_ALLOC are enabled spinlock_t on x86_64 is 72 bytes. For page->ptl they will be allocated from kmalloc-96 slab, so we loose 24 on each. An average system can easily allocate few tens thousands of page->ptl and overhead is significant.

Let's create a separate slab for page->ptl allocation to solve this.

Fair enough, and yes, it does create that separate slab. The problem is, it's still using kmalloc/kfree for those beasts - page_ptl_cachep isn't used at all...

author maintainer
#2 lore raw
Quoted 28 lines · Al Viro
Quoted 5 lines · Al Viro

[Apologies for delay; I'd spent the last day hunting down something that turned out to be a VM leak completely unrelated to this stuff - it's present in mainline, for starters. Unreliable reproducers make for fun bisects ;-/ Anyway, by now I'm absolutely sure that this is a VM bug and not something I had somehow managed to break, so...]

BTW, something odd happened to mm/memory.c - either a mangled patch or a lost followup. Take a look at the last commit in there:
commit ea1e7ed33708c7a760419ff9ded0a6cb90586a50

Author: Kirill A. Shutemov <[email protected]>
Date:   Thu Nov 14 14:31:53 2013 -0800

mm: create a separate slab for page->ptl allocation

If DEBUG_SPINLOCK and DEBUG_LOCK_ALLOC are enabled spinlock_t on x86_64 is 72 bytes. For page->ptl they will be allocated from kmalloc-96 slab, so we loose 24 on each. An average system can easily allocate few tens thousands of page->ptl and overhead is significant.

Let's create a separate slab for page->ptl allocation to solve this.

Quoted 3 lines

Fair enough, and yes, it does create that separate slab. The problem is, it's still using kmalloc/kfree for those beasts - page_ptl_cachep isn't used at all...

While digging in the same area:

Wrong page freed on preallocate_pmds() failure exit

Note that pmds[i] is simply uninitialized at that point... Granted, it's very hard to hit (you need split page locks *and* kmalloc(sizeof(spinlock_t), GFP_KERNEL) failing), but the code is obviously bogus.

---

arch/x86/mm/pgtable.c +1 -1
index a7cccb6d..36aa999 100644
@@ -209,7 +209,7 @@ static int preallocate_pmds(pmd_t *pmds[])
 		if (!pmd)
 			failed = true;
 		if (pmd && !pgtable_pmd_page_ctor(virt_to_page(pmd))) {
-			free_page((unsigned long)pmds[i]);
+			free_page((unsigned long)pmd);
 			pmd = NULL;
 			failed = true;
 		}
#3 lore raw
Quoted 16 lines · Al Viro

While digging in the same area:

Wrong page freed on preallocate_pmds() failure exit

Note that pmds[i] is simply uninitialized at that point... Granted, it's very hard to hit (you need split page locks *and* kmalloc(sizeof(spinlock_t), GFP_KERNEL) failing), but the code is obviously bogus.

---

arch/x86/mm/pgtable.c +0 -0
index a7cccb6d..36aa999 100644

arch/x86/mm/pgtable.c static int preallocate_pmds(pmd_t *pmds[]) · line 215

Quoted 8 lines
arch/x86/mm/pgtable.c +1 -1
@@ -209,7 +209,7 @@ static int preallocate_pmds(pmd_t *pmds[])
 		if (!pmd)
 			failed = true;
 		if (pmd && !pgtable_pmd_page_ctor(virt_to_page(pmd))) {
-			free_page((unsigned long)pmds[i]);
+			free_page((unsigned long)pmd);
 			pmd = NULL;
 			failed = true;
 		}

trivia: It'd probably read better with an else too

		if (!pmd) {
			failed = true;
		} else if (!pgtable_pmd_page_ctor(virt_to_page(pmd))) {
			etc...
Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#4 lore raw
In reply to Al Viro
Quoted 10 lines · Al Viro

BTW, something odd happened to mm/memory.c - either a mangled patch or a lost followup:

    commit ea1e7ed33708
    mm: create a separate slab for page->ptl allocation

Fair enough, and yes, it does create that separate slab. The problem is, it's still using kmalloc/kfree for those beasts - page_ptl_cachep isn't used at all...

Ok, it looks straightforward enough to just replace the kmalloc/kfree with using a slab allocation using the page_ptl_cachep pointer. I'd do it myself, but I would like to know how it got lost? Also, much testing to make sure the cachep is initialized early enough.

Or should we just revert the commit that added the pointless/unused slab pointer?

Andrew, Kirill, comments?

Also note the other issue Al found: see commit 2a46eed54a28 ("Wrong page freed on preallocate_pmds() failure exit") that I just pushed out.

Linus

maintainer
#5 lore raw
Quoted 8 lines · On Wed, 20 Nov 2013 14:33:35 -0800 Linus Torvalds…
Quoted 7 lines · Al Viro

BTW, something odd happened to mm/memory.c - either a mangled patch or a lost followup:

    commit ea1e7ed33708
    mm: create a separate slab for page->ptl allocation
Quoted 8 lines
Quoted 3 lines

Fair enough, and yes, it does create that separate slab. The problem is, it's still using kmalloc/kfree for those beasts - page_ptl_cachep isn't used at all...

Ok, it looks straightforward enough to just replace the kmalloc/kfree with using a slab allocation using the page_ptl_cachep pointer. I'd do it myself, but I would like to know how it got lost? Also, much testing to make sure the cachep is initialized early enough.

agh, I went through hell keeping that patch alive and it appears I lost some of it.

Quoted 4 lines

Or should we just revert the commit that added the pointless/unused slab pointer?

Andrew, Kirill, comments?

Let's just kill it please. We can try again for 3.14.

Quoted 3 lines

Also note the other issue Al found: see commit 2a46eed54a28 ("Wrong page freed on preallocate_pmds() failure exit") that I just pushed out.

lgtm, thanks.

#6 lore raw
Quoted 14 lines · Andrew Morton
Quoted 12 lines · On Wed, 20 Nov 2013 14:33:35 -0800 Linus Torvalds…
Quoted 10 lines · Al Viro

BTW, something odd happened to mm/memory.c - either a mangled patch or a lost followup:

    commit ea1e7ed33708
    mm: create a separate slab for page->ptl allocation

Fair enough, and yes, it does create that separate slab. The problem is, it's still using kmalloc/kfree for those beasts - page_ptl_cachep isn't used at all...

Quoted 7 lines
Quoted 4 lines

Ok, it looks straightforward enough to just replace the kmalloc/kfree with using a slab allocation using the page_ptl_cachep pointer. I'd do it myself, but I would like to know how it got lost? Also, much testing to make sure the cachep is initialized early enough.

agh, I went through hell keeping that patch alive and it appears I lost some of it.

Actually, I've lost it while adding BLOATED_SPINLOCKS :(

Quoted 6 lines
Quoted 4 lines

Or should we just revert the commit that added the pointless/unused slab pointer?

Andrew, Kirill, comments?

Let's just kill it please. We can try again for 3.14.

I'm okay with that.
Only side note: it's useful not only for debug case, but also for PREEMPT_RT where spinlock_t is always bloated.

Fixed patch:

Quoted 1 line

From e624075b47caa2a15998225df7cec953d271b9ac Mon Sep 17 00:00:00 2001

From: "Kirill A. Shutemov" <[email protected]>
Date: Thu, 14 Nov 2013 14:31:53 -0800
Subject: [PATCH] mm: create a separate slab for page->ptl allocation, try two

If DEBUG_SPINLOCK and DEBUG_LOCK_ALLOC are enabled spinlock_t on x86_64 is 72 bytes. For page->ptl they will be allocated from kmalloc-96 slab, so we loose 24 on each. An average system can easily allocate few tens thousands of page->ptl and overhead is significant.

Let's create a separate slab for page->ptl allocation to solve this.

To make sure that it really works this time, some numbers from my test machine (just booted, no load):

Before:

  # grep '^\(kmalloc-96\|page->ptl\)' /proc/slabinfo
  kmalloc-96         31987  32190    128   30    1 : tunables  120   60    8 : slabdata   1073   1073     92
After:
  # grep '^\(kmalloc-96\|page->ptl\)' /proc/slabinfo
  page->ptl          27516  28143     72   53    1 : tunables  120   60    8 : slabdata    531    531      9
  kmalloc-96          3853   5280    128   30    1 : tunables  120   60    8 : slabdata    176    176      0

Note that the patch is useful not only for debug case, but also for PREEMPT_RT, where spinlock_t is always bloated.

---
 include/linux/mm.h |  9 +++++++++
 init/main.c        |  2 +-
 mm/memory.c        | 11 +++++++++--
 3 files changed, 19 insertions(+), 3 deletions(-)
include/linux/mm.h +9 -0
index 1cedd000cf29..0548eb201e05 100644
@@ -1318,6 +1318,7 @@ static inline pmd_t *pmd_alloc(struct mm_struct *mm, pud_t *pud, unsigned long a
 
 #if USE_SPLIT_PTE_PTLOCKS
 #if BLOATED_SPINLOCKS
+void __init ptlock_cache_init(void);
 extern bool ptlock_alloc(struct page *page);
 extern void ptlock_free(struct page *page);
 
@@ -1326,6 +1327,7 @@ static inline spinlock_t *ptlock_ptr(struct page *page)
 	return page->ptl;
 }
 #else /* BLOATED_SPINLOCKS */
+static inline void ptlock_cache_init(void) {}
 static inline bool ptlock_alloc(struct page *page)
 {
 	return true;
@@ -1378,10 +1380,17 @@ static inline spinlock_t *pte_lockptr(struct mm_struct *mm, pmd_t *pmd)
 {
 	return &mm->page_table_lock;
 }
+static inline void ptlock_cache_init(void) {}
 static inline bool ptlock_init(struct page *page) { return true; }
 static inline void pte_lock_deinit(struct page *page) {}
 #endif /* USE_SPLIT_PTE_PTLOCKS */
 
+static inline void pgtable_init(void)
+{
+	ptlock_cache_init();
+	pgtable_cache_init();
+}
+
 static inline bool pgtable_page_ctor(struct page *page)
 {
 	inc_zone_page_state(page, NR_PAGETABLE);
init/main.c +1 -1
index febc511e078a..01573fdfa186 100644
@@ -476,7 +476,7 @@ static void __init mm_init(void)
 	mem_init();
 	kmem_cache_init();
 	percpu_init_late();
-	pgtable_cache_init();
+	pgtable_init();
 	vmalloc_init();
 }
 
mm/memory.c +9 -2
index 5d9025f3b3e1..cf6098c10084 100644
@@ -4272,11 +4272,18 @@ void copy_user_huge_page(struct page *dst, struct page *src,
 #endif /* CONFIG_TRANSPARENT_HUGEPAGE || CONFIG_HUGETLBFS */
 
 #if USE_SPLIT_PTE_PTLOCKS && BLOATED_SPINLOCKS
+static struct kmem_cache *page_ptl_cachep;
+void __init ptlock_cache_init(void)
+{
+	page_ptl_cachep = kmem_cache_create("page->ptl", sizeof(spinlock_t), 0,
+			SLAB_PANIC, NULL);
+}
+
 bool ptlock_alloc(struct page *page)
 {
 	spinlock_t *ptl;
 
-	ptl = kmalloc(sizeof(spinlock_t), GFP_KERNEL);
+	ptl = kmem_cache_alloc(page_ptl_cachep, GFP_KERNEL);
 	if (!ptl)
 		return false;
 	page->ptl = ptl;
@@ -4285,6 +4292,6 @@ bool ptlock_alloc(struct page *page)
 
 void ptlock_free(struct page *page)
 {
-	kfree(page->ptl);
+	kmem_cache_free(page_ptl_cachep, page->ptl);
 }
 #endif
Signature · 7 lines
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#7 lore raw
In reply to Linus Torvalds
Quoted 9 lines
Quoted 9 lines

BTW, something odd happened to mm/memory.c - either a mangled patch or a lost followup:

    commit ea1e7ed33708
    mm: create a separate slab for page->ptl allocation

Fair enough, and yes, it does create that separate slab. The problem is, it's still using kmalloc/kfree for those beasts - page_ptl_cachep isn't used at all...

Quoted 4 lines · Linus Torvalds

Ok, it looks straightforward enough to just replace the kmalloc/kfree with using a slab allocation using the page_ptl_cachep pointer. I'd do it myself, but I would like to know how it got lost? Also, much testing to make sure the cachep is initialized early enough.

The initial sending had the proper hunks at the end, so something really got lost afterwards...
https://lkml.org/lkml/2013/10/22/129

Signature · 1 line
Damien Wyart