New: kernel BUG at mm/slab.c:2625!

https://bugzilla.kernel.org/show_bug.cgi?id=87891

            Bug ID: 87891
           Summary: kernel BUG at mm/slab.c:2625!
           Product: Memory Management
           Version: 2.5
    Kernel Version: 3.17.2
          Hardware: i386
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: blocking
          Priority: P1
         Component: Slab Allocator
          Assignee: [email protected]
          Reporter: [email protected]
        Regression: No

[359782.842112] kernel BUG at mm/slab.c:2625!
...
[359782.843008] Call Trace:
[359782.843017]  [<ffffffff8115181f>] __kmalloc+0xdf/0x200
[359782.843037]  [<ffffffffa0466285>] ? ttm_page_pool_free+0x35/0x180 [ttm]
[359782.843060]  [<ffffffffa0466285>] ttm_page_pool_free+0x35/0x180 [ttm]
[359782.843084]  [<ffffffffa046674e>] ttm_pool_shrink_scan+0xae/0xd0 [ttm]
[359782.843108]  [<ffffffff8111c2fb>] shrink_slab_node+0x12b/0x2e0
[359782.843129]  [<ffffffff81127ed4>] ? fragmentation_index+0x14/0x70
[359782.843150]  [<ffffffff8110fc3a>] ? zone_watermark_ok+0x1a/0x20

[359782.843171]  [<ffffffff8111ceb8>] shrink_slab+0xc8/0x110
[359782.843189]  [<ffffffff81120480>] do_try_to_free_pages+0x300/0x410
[359782.843210]  [<ffffffff8112084b>] try_to_free_pages+0xbb/0x190
[359782.843230]  [<ffffffff81113136>] __alloc_pages_nodemask+0x696/0xa90
[359782.843253]  [<ffffffff8115810a>] do_huge_pmd_anonymous_page+0xfa/0x3f0
[359782.843278]  [<ffffffff812dffe7>] ? debug_smp_processor_id+0x17/0x20
[359782.843300]  [<ffffffff81118dc7>] ? __lru_cache_add+0x57/0xa0
[359782.843321]  [<ffffffff811385ce>] handle_mm_fault+0x37e/0xdd0

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 17 lines · On Thu, 06 Nov 2014 17:28:41 +0000 [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=87891

            Bug ID: 87891
           Summary: kernel BUG at mm/slab.c:2625!
           Product: Memory Management
           Version: 2.5
    Kernel Version: 3.17.2
          Hardware: i386
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: blocking
          Priority: P1
         Component: Slab Allocator
          Assignee: [email protected]
          Reporter: [email protected]
        Regression: No

Well this is interesting.

Quoted 10 lines

[359782.842112] kernel BUG at mm/slab.c:2625!
...
[359782.843008] Call Trace:
[359782.843017]  [<ffffffff8115181f>] __kmalloc+0xdf/0x200
[359782.843037]  [<ffffffffa0466285>] ? ttm_page_pool_free+0x35/0x180 [ttm]
[359782.843060]  [<ffffffffa0466285>] ttm_page_pool_free+0x35/0x180 [ttm]
[359782.843084]  [<ffffffffa046674e>] ttm_pool_shrink_scan+0xae/0xd0 [ttm]
[359782.843108]  [<ffffffff8111c2fb>] shrink_slab_node+0x12b/0x2e0
[359782.843129]  [<ffffffff81127ed4>] ? fragmentation_index+0x14/0x70
[359782.843150]  [<ffffffff8110fc3a>] ? zone_watermark_ok+0x1a/0x20

Quoted 8 lines

[359782.843171]  [<ffffffff8111ceb8>] shrink_slab+0xc8/0x110
[359782.843189]  [<ffffffff81120480>] do_try_to_free_pages+0x300/0x410
[359782.843210]  [<ffffffff8112084b>] try_to_free_pages+0xbb/0x190
[359782.843230]  [<ffffffff81113136>] __alloc_pages_nodemask+0x696/0xa90
[359782.843253]  [<ffffffff8115810a>] do_huge_pmd_anonymous_page+0xfa/0x3f0
[359782.843278]  [<ffffffff812dffe7>] ? debug_smp_processor_id+0x17/0x20
[359782.843300]  [<ffffffff81118dc7>] ? __lru_cache_add+0x57/0xa0
[359782.843321]  [<ffffffff811385ce>] handle_mm_fault+0x37e/0xdd0

It went pagefault

        ->__alloc_pages_nodemask
          ->shrink_slab
            ->ttm_pool_shrink_scan
              ->ttm_page_pool_free
                ->kmalloc
                  ->cache_grow
                    ->BUG_ON(flags & GFP_SLAB_BUG_MASK);

And I don't really know why - I'm not seeing anything in there which can set a GFP flag which is outside GFP_SLAB_BUG_MASK. However I see lots of nits.

Core MM:

__alloc_pages_nodemask() does

	if (unlikely(!page)) {
		/*
		 * Runtime PM, block IO and its error handling path
		 * can deadlock because I/O on the device might not
		 * complete.
		 */
		gfp_mask = memalloc_noio_flags(gfp_mask);
		page = __alloc_pages_slowpath(gfp_mask, order,
				zonelist, high_zoneidx, nodemask,
				preferred_zone, classzone_idx, migratetype);
	}

so it permanently alters the value of incoming arg gfp_mask. This means that the following trace_mm_page_alloc() will print the wrong value of gfp_mask, and if we later do the `goto retry_cpuset', we retry with a possibly different gfp_mask. Isn't this a bug?

Also, why are we even passing a gfp_t down to the shrinkers? So they can work out the allocation context - things like __GFP_IO, __GFP_FS, etc? Is it even appropriate to use that mask for a new allocation attempt within a particular shrinker?

ttm:

I think it's a bad idea to be calling kmalloc() in the slab shrinker function. We *know* that the system is low on memory and is trying to free things up. Trying to allocate *more* memory at this time is asking for trouble. ttm_page_pool_free() could easily be tweaked to use a fixed-size local array of page*'s t avoid that allocation. Could someone implement this please?

slab:

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK. What's it trying to do here?

And it's quite infuriating to go BUG when the code could easily warn and fix it up.

And it's quite infuriating to go BUG because one of the bits was set, but not tell us which bit it was!

Could the slab guys please review this?

From: Andrew Morton <[email protected]>
Subject: slab: improve checking for invalid gfp_flags

- The code goes BUG, but doesn't tell us which bits were unexpectedly set. Print that out.

- The code goes BUG when it could jsut fix things up and proceed. Do that.

- ~__GFP_BITS_MASK already includes __GFP_DMA32 and __GFP_HIGHMEM, so remove those from the GFP_SLAB_BUG_MASK definition.

Cc Christoph Lameter <[email protected]>

Cc Pekka Enberg <[email protected]>

Cc David Rientjes <[email protected]>

Cc Joonsoo Kim <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 include/linux/gfp.h |    2 +-
 mm/slab.c           |    5 ++++-
 mm/slub.c           |    5 ++++-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff -puN include/linux/gfp.h~slab-improve-checking-for-invalid-gfp_flags include/linux/gfp.h

include/linux/gfp.h renamed from include/linux/gfp.h~slab-improve-checking-for-invalid-gfp_flags +1 -1

@@ -145,7 +145,7 @@ struct vm_area_struct;
 #define GFP_CONSTRAINT_MASK (__GFP_HARDWALL|__GFP_THISNODE)
 
 /* Do not use these with a slab allocator */
-#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)
+#define GFP_SLAB_BUG_MASK (~__GFP_BITS_MASK)
 
 /* Flag - indicates that the buffer will be suitable for DMA.  Ignored on some
    platforms, used as appropriate on others */

diff -puN mm/slab.c~slab-improve-checking-for-invalid-gfp_flags mm/slab.c

mm/slab.c renamed from mm/slab.c~slab-improve-checking-for-invalid-gfp_flags +4 -1

@@ -2590,7 +2590,10 @@ static int cache_grow(struct kmem_cache
 	 * Be lazy and only check for valid flags here,  keeping it out of the
 	 * critical path in kmem_cache_alloc().
 	 */
-	BUG_ON(flags & GFP_SLAB_BUG_MASK);
+	if (WARN_ON(flags & GFP_SLAB_BUG_MASK)) {
+		pr_emerg("gfp: %u\n", flags & GFP_SLAB_BUG_MASK);
+		flags &= ~GFP_SLAB_BUG_MASK;
+	}
 	local_flags = flags & (GFP_CONSTRAINT_MASK|GFP_RECLAIM_MASK);
 
 	/* Take the node list lock to change the colour_next on this node */

diff -puN mm/slub.c~slab-improve-checking-for-invalid-gfp_flags mm/slub.c

mm/slub.c renamed from mm/slub.c~slab-improve-checking-for-invalid-gfp_flags +4 -1

@@ -1377,7 +1377,10 @@ static struct page *new_slab(struct kmem
 	int order;
 	int idx;
 
-	BUG_ON(flags & GFP_SLAB_BUG_MASK);
+	if (WARN_ON(flags & GFP_SLAB_BUG_MASK)) {
+		pr_emerg("gfp: %u\n", flags & GFP_SLAB_BUG_MASK);
+		flags &= ~GFP_SLAB_BUG_MASK;
+	}
 
 	page = allocate_slab(s,
 		flags & (GFP_RECLAIM_MASK | GFP_CONSTRAINT_MASK), node);

_

--
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>

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Quoted 3 lines

And it's quite infuriating to go BUG when the code could easily warn and fix it up.

If user wants memory on HIGHMEM, it can be easily fixed by following change because all memory is compatible for HIGHMEM. But, if user wants memory on DMA32, it's not easy to fix because memory on NORMAL isn't compatible with DMA32. slab could return object from another slab page even if cache_grow() is successfully called. So BUG_ON() here looks right thing to me. We cannot know in advance whether ignoring this flag cause more serious result or not.

But anyway - Luke, please attach your .config to
https://bugzilla.kernel.org/show_bug.cgi?id=87891?

Quoted 2 lines · Andrew Morton

But anyway - Luke, please attach your .config to
https://bugzilla.kernel.org/show_bug.cgi?id=87891?

Done: https://bugzilla.kernel.org/attachment.cgi?id=157381

Well, attempting to fix it up and continue is nice, but we can live with the BUG.

Not knowing which bit was set is bad.

Quoted 6 lines · On Wed, 12 Nov 2014 00:54:01 +0000 Luke Dashjr <[email protected]> wrote:

Quoted 2 lines · Andrew Morton

But anyway - Luke, please attach your .config to
https://bugzilla.kernel.org/show_bug.cgi?id=87891?

Done: https://bugzilla.kernel.org/attachment.cgi?id=157381

OK, thanks. No CONFIG_HIGHMEM of course. I'm stumped.

Quoted 11 lines · On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Ah, yes, OK.

I suppose it's possible that __GFP_HIGHMEM was set.

do_huge_pmd_anonymous_page
->pte_alloc_one
->alloc_pages(__userpte_alloc_gfp==__GFP_HIGHMEM)

Quoted 10 lines · Andrew Morton

Quoted 6 lines · On Wed, 12 Nov 2014 00:54:01 +0000 Luke Dashjr <[email protected]> wrote:

Quoted 2 lines · Andrew Morton

But anyway - Luke, please attach your .config to
https://bugzilla.kernel.org/show_bug.cgi?id=87891?

Done: https://bugzilla.kernel.org/attachment.cgi?id=157381

OK, thanks. No CONFIG_HIGHMEM of course. I'm stumped.

Hello, Andrew.

I think that the cause is GFP_HIGHMEM.
GFP_HIGHMEM is always defined regardless CONFIG_HIGHMEM. Please look at the do_huge_pmd_anonymous_page().
It calls alloc_hugepage_vma() and then alloc_pages_vma() is called with alloc_hugepage_gfpmask(). This gfpmask includes GFP_TRANSHUGE and then GFP_HIGHUSER_MOVABLE.

Quoted 14 lines · Andrew Morton

Quoted 11 lines · On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Quoted 7 lines

Ah, yes, OK.

I suppose it's possible that __GFP_HIGHMEM was set.

do_huge_pmd_anonymous_page
->pte_alloc_one
->alloc_pages(__userpte_alloc_gfp==__GFP_HIGHMEM)

do_huge_pmd_anonymous_page
alloc_hugepage_vma
alloc_pages_vma(GFP_TRANSHUGE)

GFP_TRANSHUGE contains GFP_HIGHUSER_MOVABLE, which has __GFP_HIGHMEM.

Quoted 23 lines · Kirill A. Shutemov

Quoted 14 lines · Andrew Morton

Quoted 11 lines · On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Quoted 7 lines

Ah, yes, OK.

I suppose it's possible that __GFP_HIGHMEM was set.

do_huge_pmd_anonymous_page
->pte_alloc_one
->alloc_pages(__userpte_alloc_gfp==__GFP_HIGHMEM)

Quoted 5 lines

do_huge_pmd_anonymous_page
alloc_hugepage_vma
alloc_pages_vma(GFP_TRANSHUGE)

GFP_TRANSHUGE contains GFP_HIGHUSER_MOVABLE, which has __GFP_HIGHMEM.

Looks like it's reasonable to sanitize flags in shrink_slab() by dropping flags incompatible with slab expectation. Like this:

mm/vmscan.c +0 -0

index dcb47074ae03..eb165d29c5e5 100644

Quoted 38 lines · On Wed, 12 Nov 2014 03:47:03 +0200 "Kirill A. Shutemov" <[email protected]>…

Quoted 23 lines · Kirill A. Shutemov

Quoted 14 lines · Andrew Morton

Quoted 11 lines · On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Quoted 7 lines

Ah, yes, OK.

I suppose it's possible that __GFP_HIGHMEM was set.

do_huge_pmd_anonymous_page
->pte_alloc_one
->alloc_pages(__userpte_alloc_gfp==__GFP_HIGHMEM)

Quoted 5 lines

do_huge_pmd_anonymous_page
alloc_hugepage_vma
alloc_pages_vma(GFP_TRANSHUGE)

GFP_TRANSHUGE contains GFP_HIGHUSER_MOVABLE, which has __GFP_HIGHMEM.

Looks like it's reasonable to sanitize flags in shrink_slab() by dropping flags incompatible with slab expectation. Like this:

mm/vmscan.c +0 -0

index dcb47074ae03..eb165d29c5e5 100644

mm/vmscan.c unsigned long shrink_slab(struct shrink_control *shrinkctl, · line 376

Quoted 8 lines

mm/vmscan.c +2 -0

@@ -369,6 +369,8 @@ unsigned long shrink_slab(struct shrink_control *shrinkctl,
        if (nr_pages_scanned == 0)
                nr_pages_scanned = SWAP_CLUSTER_MAX;
 
+       shrinkctl->gfp_mask &= ~(__GFP_DMA32 | __GFP_HIGHMEM);
+
        if (!down_read_trylock(&shrinker_rwsem)) {
                /*
                 * If we would return 0, our callers would understand that we

Well no, because nobody is supposed to be passing this gfp_mask back into a new allocation attempt anyway. It would be better to do

	shrinkctl->gfp_mask |= __GFP_IMMEDIATELY_GO_BUG;

?

From my POV, the problem is that we combine what-need-to-be-freed gfp_mask

Quoted 12 lines · On Wed, 12 Nov 2014 10:22:45 +0900 Joonsoo Kim <[email protected]> wrote:

Quoted 10 lines · Andrew Morton

Quoted 6 lines · On Wed, 12 Nov 2014 00:54:01 +0000 Luke Dashjr <[email protected]> wrote:

Quoted 2 lines · Andrew Morton

But anyway - Luke, please attach your .config to
https://bugzilla.kernel.org/show_bug.cgi?id=87891?

Done: https://bugzilla.kernel.org/attachment.cgi?id=157381

OK, thanks. No CONFIG_HIGHMEM of course. I'm stumped.

Quoted 8 lines

Hello, Andrew.

I think that the cause is GFP_HIGHMEM.
GFP_HIGHMEM is always defined regardless CONFIG_HIGHMEM. Please look at the do_huge_pmd_anonymous_page().
It calls alloc_hugepage_vma() and then alloc_pages_vma() is called with alloc_hugepage_gfpmask(). This gfpmask includes GFP_TRANSHUGE and then GFP_HIGHUSER_MOVABLE.

OK.

So where's the bug? I'm inclined to say that it's in ttm. It's taking

a gfp_mask which means "this is the allocation attempt which we are attempting to satisfy" and uses that for its own allocation.

But ttm has no business using that gfp_mask for its own allocation attempt. If anything it should use something like, err,

	GFP_KERNEL & ~__GFP_IO & ~__GFP_FS | __GFP_HIGH

although as I mentioned earlier, it would be better to avoid allocation altogether.

Poor ttm guys - this is a bit of a trap we set for them.

--
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>

Quoted 14 lines · Andrew Morton

Quoted 11 lines · On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Quoted 7 lines

Ah, yes, OK.

I suppose it's possible that __GFP_HIGHMEM was set.

do_huge_pmd_anonymous_page
->pte_alloc_one
->alloc_pages(__userpte_alloc_gfp==__GFP_HIGHMEM)

do_huge_pmd_anonymous_page
alloc_hugepage_vma
alloc_pages_vma(GFP_TRANSHUGE)

GFP_TRANSHUGE contains GFP_HIGHUSER_MOVABLE, which has __GFP_HIGHMEM.

Quoted 27 lines · Kirill A. Shutemov

Quoted 14 lines · Andrew Morton

Quoted 11 lines · On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Quoted 7 lines

Ah, yes, OK.

I suppose it's possible that __GFP_HIGHMEM was set.

do_huge_pmd_anonymous_page
->pte_alloc_one
->alloc_pages(__userpte_alloc_gfp==__GFP_HIGHMEM)

do_huge_pmd_anonymous_page
alloc_hugepage_vma
alloc_pages_vma(GFP_TRANSHUGE)

Quoted 1 line

GFP_TRANSHUGE contains GFP_HIGHUSER_MOVABLE, which has __GFP_HIGHMEM.

Hello, Kirill.

BTW, why does GFP_TRANSHUGE have MOVABLE flag despite it isn't movable? After breaking hugepage, it could be movable, but, it may prevent CMA from working correctly until break.

Poor ttm guys - this is a bit of a trap we set for them.

Quoted 1 line · Andrew Morton

Poor ttm guys - this is a bit of a trap we set for them.

Commit a91576d7916f6cce (\"drm/ttm: Pass GFP flags in order to avoid deadlock.\") changed to use sc->gfp_mask rather than GFP_KERNEL.

(quoted diff) +1 -2

-       pages_to_free = kmalloc(npages_to_free * sizeof(struct page *),
-                       GFP_KERNEL);
+       pages_to_free = kmalloc(npages_to_free * sizeof(struct page *), gfp);

But this bug is caused by sc->gfp_mask containing some flags which are not in GFP_KERNEL, right? Then, I think

(quoted diff) +1 -1

-       pages_to_free = kmalloc(npages_to_free * sizeof(struct page *), gfp);
+       pages_to_free = kmalloc(npages_to_free * sizeof(struct page *), gfp & GFP_KERNEL);

would hide this bug.

But I think we should use GFP_ATOMIC (or drop __GFP_WAIT flag)

for
two reasons when __alloc_pages_nodemask() is called from shrinker functions.

(1) Stack usage by __alloc_pages_nodemask() is large. If we unlimitedly allow recursive __alloc_pages_nodemask() calls, kernel stack could overflow under extreme memory pressure.

(2) Some shrinker functions are using sleepable locks which could make kswapd sleep for unpredictable duration. If kswapd is unexpectedly blocked inside shrinker functions and somebody is expecting that kswapd is running for reclaiming memory, it is a memory allocation deadlock.

Speak of ttm module, commit 22e71691fd54c637 (\"drm/ttm: Use mutex_trylock() to avoid deadlock inside shrinker functions.\") prevents unlimited recursive __alloc_pages_nodemask() calls.

Quoted 32 lines · Joonsoo Kim

Quoted 23 lines · Kirill A. Shutemov

Quoted 14 lines · Andrew Morton

Quoted 11 lines · On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Quoted 7 lines

Ah, yes, OK.

I suppose it's possible that __GFP_HIGHMEM was set.

do_huge_pmd_anonymous_page
->pte_alloc_one
->alloc_pages(__userpte_alloc_gfp==__GFP_HIGHMEM)

Quoted 5 lines

do_huge_pmd_anonymous_page
alloc_hugepage_vma
alloc_pages_vma(GFP_TRANSHUGE)

GFP_TRANSHUGE contains GFP_HIGHUSER_MOVABLE, which has __GFP_HIGHMEM.

Hello, Kirill.

Quoted 3 lines

BTW, why does GFP_TRANSHUGE have MOVABLE flag despite it isn't movable? After breaking hugepage, it could be movable, but, it may prevent CMA from working correctly until break.

Again, the same alloc vs. free gfp_mask: we want page allocator to move pages around to find space from THP, but resulting page is no really movable.

I've tried to look into making THP movable: it requires quite a bit of infrastructure changes around rmap: try_to_unmap*(), remove_migration_pmd(), migration entries for PMDs, etc. I gets ugly pretty fast :-/
I probably need to give it second try. No promises.

Quoted 27 lines · Kirill A. Shutemov

Quoted 14 lines · Andrew Morton

Quoted 11 lines · On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Quoted 7 lines

Ah, yes, OK.

I suppose it's possible that __GFP_HIGHMEM was set.

do_huge_pmd_anonymous_page
->pte_alloc_one
->alloc_pages(__userpte_alloc_gfp==__GFP_HIGHMEM)

do_huge_pmd_anonymous_page
alloc_hugepage_vma
alloc_pages_vma(GFP_TRANSHUGE)

Quoted 1 line

GFP_TRANSHUGE contains GFP_HIGHUSER_MOVABLE, which has __GFP_HIGHMEM.

Hello, Kirill.

BTW, why does GFP_TRANSHUGE have MOVABLE flag despite it isn't movable? After breaking hugepage, it could be movable, but, it may prevent CMA from working correctly until break.

Quoted 32 lines · Joonsoo Kim

Quoted 23 lines · Kirill A. Shutemov

Quoted 14 lines · Andrew Morton

Quoted 11 lines · On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter <[email protected]> wrote:

Quoted 5 lines · Andrew Morton

There's no point in doing

	#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

because __GFP_DMA32|__GFP_HIGHMEM are already part of ~__GFP_BITS_MASK.

?? ~__GFP_BITS_MASK means bits 25 to 31 are set.

__GFP_DMA32 is bit 2 and __GFP_HIGHMEM is bit 1.

Quoted 7 lines

Ah, yes, OK.

I suppose it's possible that __GFP_HIGHMEM was set.

do_huge_pmd_anonymous_page
->pte_alloc_one
->alloc_pages(__userpte_alloc_gfp==__GFP_HIGHMEM)

Quoted 5 lines

do_huge_pmd_anonymous_page
alloc_hugepage_vma
alloc_pages_vma(GFP_TRANSHUGE)

GFP_TRANSHUGE contains GFP_HIGHUSER_MOVABLE, which has __GFP_HIGHMEM.

Hello, Kirill.

Quoted 4 lines

BTW, why does GFP_TRANSHUGE have MOVABLE flag despite it isn't movable? After breaking hugepage, it could be movable, but, it may prevent CMA from working correctly until break.

Because THP can use the Movable zone if it's allocated. When movable was introduced it did not just mean migratable. It meant it could also be moved to swap. THP can be broken up and swapped so it tagged as movable.

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 17 lines · On Thu, 06 Nov 2014 17:28:41 +0000 [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=87891

             Bug ID: 87891
            Summary: kernel BUG at mm/slab.c:2625!
            Product: Memory Management
            Version: 2.5
     Kernel Version: 3.17.2
           Hardware: i386
                 OS: Linux
               Tree: Mainline
             Status: NEW
           Severity: blocking
           Priority: P1
          Component: Slab Allocator
           Assignee: [email protected]
           Reporter: [email protected]
         Regression: No

Well this is interesting.

Quoted 10 lines

[359782.842112] kernel BUG at mm/slab.c:2625!
...
[359782.843008] Call Trace:
[359782.843017]  [<ffffffff8115181f>] __kmalloc+0xdf/0x200
[359782.843037]  [<ffffffffa0466285>] ? ttm_page_pool_free+0x35/0x180 [ttm]
[359782.843060]  [<ffffffffa0466285>] ttm_page_pool_free+0x35/0x180 [ttm]
[359782.843084]  [<ffffffffa046674e>] ttm_pool_shrink_scan+0xae/0xd0 [ttm]
[359782.843108]  [<ffffffff8111c2fb>] shrink_slab_node+0x12b/0x2e0
[359782.843129]  [<ffffffff81127ed4>] ? fragmentation_index+0x14/0x70
[359782.843150]  [<ffffffff8110fc3a>] ? zone_watermark_ok+0x1a/0x20

Quoted 8 lines

[359782.843171]  [<ffffffff8111ceb8>] shrink_slab+0xc8/0x110
[359782.843189]  [<ffffffff81120480>] do_try_to_free_pages+0x300/0x410
[359782.843210]  [<ffffffff8112084b>] try_to_free_pages+0xbb/0x190
[359782.843230]  [<ffffffff81113136>] __alloc_pages_nodemask+0x696/0xa90
[359782.843253]  [<ffffffff8115810a>] do_huge_pmd_anonymous_page+0xfa/0x3f0
[359782.843278]  [<ffffffff812dffe7>] ? debug_smp_processor_id+0x17/0x20
[359782.843300]  [<ffffffff81118dc7>] ? __lru_cache_add+0x57/0xa0
[359782.843321]  [<ffffffff811385ce>] handle_mm_fault+0x37e/0xdd0

It went pagefault

         ->__alloc_pages_nodemask
           ->shrink_slab
             ->ttm_pool_shrink_scan
               ->ttm_page_pool_free
                 ->kmalloc
                   ->cache_grow
                     ->BUG_ON(flags & GFP_SLAB_BUG_MASK);

And I don't really know why - I'm not seeing anything in there which can set a GFP flag which is outside GFP_SLAB_BUG_MASK. However I see lots of nits.

Core MM:

__alloc_pages_nodemask() does

	if (unlikely(!page)) {
		/*
		 * Runtime PM, block IO and its error handling path
		 * can deadlock because I/O on the device might not
		 * complete.
		 */
		gfp_mask = memalloc_noio_flags(gfp_mask);
		page = __alloc_pages_slowpath(gfp_mask, order,
				zonelist, high_zoneidx, nodemask,
				preferred_zone, classzone_idx, migratetype);
	}

so it permanently alters the value of incoming arg gfp_mask. This means that the following trace_mm_page_alloc() will print the wrong value of gfp_mask, and if we later do the `goto retry_cpuset', we retry with a possibly different gfp_mask. Isn't this a bug?

Quoted 18 lines · On Wed, 12 Nov 2014 13:08:55 +0900 Tetsuo Handa…

Quoted 1 line · Andrew Morton

Poor ttm guys - this is a bit of a trap we set for them.

Commit a91576d7916f6cce ("drm/ttm: Pass GFP flags in order to avoid deadlock.") changed to use sc->gfp_mask rather than GFP_KERNEL.

(quoted diff) +1 -2

-       pages_to_free = kmalloc(npages_to_free * sizeof(struct page *),
-                       GFP_KERNEL);
+       pages_to_free = kmalloc(npages_to_free * sizeof(struct page *), gfp);

But this bug is caused by sc->gfp_mask containing some flags which are not in GFP_KERNEL, right? Then, I think

(quoted diff) +1 -1

-       pages_to_free = kmalloc(npages_to_free * sizeof(struct page *), gfp);
+       pages_to_free = kmalloc(npages_to_free * sizeof(struct page *), gfp & GFP_KERNEL);

would hide this bug.

Quoted 1 line

But I think we should use GFP_ATOMIC (or drop __GFP_WAIT flag)

Well no - ttm_page_pool_free() should stop calling kmalloc altogether. Just do

	struct page *pages_to_free[16];

and rework the code to free 16 pages at a time. Easy.

From d3cb5393c9c8099d6b37e769f78c31af1541fe8c Mon Sep 17 00:00:00 2001