[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-13 Thread Joonsoo Kim
On Wed, Nov 12, 2014 at 10:39:24AM +, Mel Gorman wrote:
> On Wed, Nov 12, 2014 at 11:17:16AM +0900, Joonsoo Kim wrote:
> > On Wed, Nov 12, 2014 at 03:22:41AM +0200, Kirill A. Shutemov wrote:
> > > On Tue, Nov 11, 2014 at 04:49:13PM -0800, Andrew Morton wrote:
> > > > On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  > > > linux.com> wrote:
> > > > 
> > > > > On Tue, 11 Nov 2014, Andrew Morton wrote:
> > > > > 
> > > > > > 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)
> > > 
> > > 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.
> > 
> > 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.

Great explanation!

Thanks Mel.


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-13 Thread Vlastimil Babka
On 11/12/2014 12:31 AM, Andrew Morton wrote:
>
> (switched to email.  Please respond via emailed reply-to-all, not via the
> bugzilla web interface).
>
> On Thu, 06 Nov 2014 17:28:41 + bugzilla-daemon at bugzilla.kernel.org 
> 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: akpm at linux-foundation.org
>>Reporter: luke-jr+linuxbugs at utopios.org
>>  Regression: No
>
> Well this is interesting.
>
>
>> [359782.842112] kernel BUG at mm/slab.c:2625!
>> ...
>> [359782.843008] Call Trace:
>> [359782.843017]  [] __kmalloc+0xdf/0x200
>> [359782.843037]  [] ? ttm_page_pool_free+0x35/0x180 [ttm]
>> [359782.843060]  [] ttm_page_pool_free+0x35/0x180 [ttm]
>> [359782.843084]  [] ttm_pool_shrink_scan+0xae/0xd0 [ttm]
>> [359782.843108]  [] shrink_slab_node+0x12b/0x2e0
>> [359782.843129]  [] ? fragmentation_index+0x14/0x70
>> [359782.843150]  [] ? zone_watermark_ok+0x1a/0x20
>> [359782.843171]  [] shrink_slab+0xc8/0x110
>> [359782.843189]  [] do_try_to_free_pages+0x300/0x410
>> [359782.843210]  [] try_to_free_pages+0xbb/0x190
>> [359782.843230]  [] __alloc_pages_nodemask+0x696/0xa90
>> [359782.843253]  [] do_huge_pmd_anonymous_page+0xfa/0x3f0
>> [359782.843278]  [] ? debug_smp_processor_id+0x17/0x20
>> [359782.843300]  [] ? __lru_cache_add+0x57/0xa0
>> [359782.843321]  [] 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?

I think so. I noticed and fixed it in the RFC about reducing 
alloc_pages* parameters [1], but it's buried in patch 2/4 Guess I should 
have made it a separate non-RFC patch. Will do soon hopefully.

Vlastimil


[1] https://lkml.org/lkml/2014/8/6/249



[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Joonsoo Kim
On Wed, Nov 12, 2014 at 04:37:46AM +0200, Kirill A. Shutemov wrote:
> On Wed, Nov 12, 2014 at 11:17:16AM +0900, Joonsoo Kim wrote:
> > On Wed, Nov 12, 2014 at 03:22:41AM +0200, Kirill A. Shutemov wrote:
> > > On Tue, Nov 11, 2014 at 04:49:13PM -0800, Andrew Morton wrote:
> > > > On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  > > > linux.com> wrote:
> > > > 
> > > > > On Tue, 11 Nov 2014, Andrew Morton wrote:
> > > > > 
> > > > > > 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)
> > > 
> > > 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.
> > 
> > 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.

Hmm... AFAIK, without MOVABLE flag page allocator will try to move
pages to find space for THP page. Am I missing something?

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

Good to hear. :)

I think that we can go another way that breaks the hugepage. This
operation makes it movable and CMA would be succeed.

Thanks.


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Tetsuo Handa
Andrew Morton wrote:
> 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.

-   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

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


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Joonsoo Kim
On Wed, Nov 12, 2014 at 03:22:41AM +0200, Kirill A. Shutemov wrote:
> On Tue, Nov 11, 2014 at 04:49:13PM -0800, Andrew Morton wrote:
> > On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  > linux.com> wrote:
> > 
> > > On Tue, 11 Nov 2014, Andrew Morton wrote:
> > > 
> > > > 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)
> 
> 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.

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.

Thanks.


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Joonsoo Kim
On Tue, Nov 11, 2014 at 05:44:12PM -0800, Andrew Morton wrote:
> On Wed, 12 Nov 2014 10:22:45 +0900 Joonsoo Kim  
> wrote:
> 
> > On Tue, Nov 11, 2014 at 05:02:43PM -0800, Andrew Morton wrote:
> > > On Wed, 12 Nov 2014 00:54:01 + Luke Dashjr  wrote:
> > > 
> > > > On Wednesday, November 12, 2014 12:49:13 AM Andrew Morton wrote:
> > > > > 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.
> 
> OK.
> 
> So where's the bug?  I'm inclined to say that it's in ttm.  It's taking

I agree that.

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

Yes, avoiding would be the best.

If not possible, introducing new common helper for changing shrinker
control's gfp to valid allocation gfp is better than just open code.

Thanks.

> 
> 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 majordomo at kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"dont at kvack.org"> email at kvack.org 


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Mel Gorman
On Wed, Nov 12, 2014 at 11:17:16AM +0900, Joonsoo Kim wrote:
> On Wed, Nov 12, 2014 at 03:22:41AM +0200, Kirill A. Shutemov wrote:
> > On Tue, Nov 11, 2014 at 04:49:13PM -0800, Andrew Morton wrote:
> > > On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  > > linux.com> wrote:
> > > 
> > > > On Tue, 11 Nov 2014, Andrew Morton wrote:
> > > > 
> > > > > 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)
> > 
> > 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.
> 
> 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.

-- 
Mel Gorman
SUSE Labs


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Joonsoo Kim
On Tue, Nov 11, 2014 at 05:02:43PM -0800, Andrew Morton wrote:
> On Wed, 12 Nov 2014 00:54:01 + Luke Dashjr  wrote:
> 
> > On Wednesday, November 12, 2014 12:49:13 AM Andrew Morton wrote:
> > > 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.

Thanks.



[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Joonsoo Kim
On Tue, Nov 11, 2014 at 03:31:20PM -0800, Andrew Morton wrote:
> 
> (switched to email.  Please respond via emailed reply-to-all, not via the
> bugzilla web interface).
> 
> On Thu, 06 Nov 2014 17:28:41 + bugzilla-daemon at bugzilla.kernel.org 
> 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: akpm at linux-foundation.org
> >   Reporter: luke-jr+linuxbugs at utopios.org
> > Regression: No
> 
> Well this is interesting.
> 
> 
> > [359782.842112] kernel BUG at mm/slab.c:2625!
> > ...
> > [359782.843008] Call Trace:
> > [359782.843017]  [] __kmalloc+0xdf/0x200
> > [359782.843037]  [] ? ttm_page_pool_free+0x35/0x180 [ttm]
> > [359782.843060]  [] ttm_page_pool_free+0x35/0x180 [ttm]
> > [359782.843084]  [] ttm_pool_shrink_scan+0xae/0xd0 [ttm]
> > [359782.843108]  [] shrink_slab_node+0x12b/0x2e0
> > [359782.843129]  [] ? fragmentation_index+0x14/0x70
> > [359782.843150]  [] ? zone_watermark_ok+0x1a/0x20
> > [359782.843171]  [] shrink_slab+0xc8/0x110
> > [359782.843189]  [] do_try_to_free_pages+0x300/0x410
> > [359782.843210]  [] try_to_free_pages+0xbb/0x190
> > [359782.843230]  [] __alloc_pages_nodemask+0x696/0xa90
> > [359782.843253]  [] do_huge_pmd_anonymous_page+0xfa/0x3f0
> > [359782.843278]  [] ? debug_smp_processor_id+0x17/0x20
> > [359782.843300]  [] ? __lru_cache_add+0x57/0xa0
> > [359782.843321]  [] 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?

Hello, Andrew.

__GFP_DMA32 and __GFP_HIGHMEM isn't included in ~__GFP_BITS_MASK.
~__GFP_BITS_MASK means all the high bits excluding all gfp bits.

As you already know, HIGHMEM isn't appropriate for slab because there
is no direct mapping on this memory. And, if we want memory only from
the memory on DMA32 area, specific kmem_cache is needed. But, there is
no interface for it, so allocation for DMA32 is also restricted here.

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

> 
> And it's quite 

[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Kirill A. Shutemov
On Wed, Nov 12, 2014 at 11:17:16AM +0900, Joonsoo Kim wrote:
> On Wed, Nov 12, 2014 at 03:22:41AM +0200, Kirill A. Shutemov wrote:
> > On Tue, Nov 11, 2014 at 04:49:13PM -0800, Andrew Morton wrote:
> > > On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  > > linux.com> wrote:
> > > 
> > > > On Tue, 11 Nov 2014, Andrew Morton wrote:
> > > > 
> > > > > 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)
> > 
> > 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.
> 
> 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.

-- 
 Kirill A. Shutemov


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Kirill A. Shutemov
On Tue, Nov 11, 2014 at 05:56:03PM -0800, Andrew Morton wrote:
> On Wed, 12 Nov 2014 03:47:03 +0200 "Kirill A. Shutemov"  shutemov.name> wrote:
> 
> > On Wed, Nov 12, 2014 at 03:22:41AM +0200, Kirill A. Shutemov wrote:
> > > On Tue, Nov 11, 2014 at 04:49:13PM -0800, Andrew Morton wrote:
> > > > On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  > > > linux.com> wrote:
> > > > 
> > > > > On Tue, 11 Nov 2014, Andrew Morton wrote:
> > > > > 
> > > > > > 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)
> > > 
> > > 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:
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index dcb47074ae03..eb165d29c5e5 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -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(_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
with if-have-to-allocate gfp_mask: we want to respect __GFP_IO/FS on
alloc, but not nessesary both if there's no restriction from the context.

For shrink_slab(), __GFP_DMA32 and __GFP_HIGHMEM don't make sense in both
cases.

__GFP_IMMEDIATELY_GO_BUG would work too, but we also need to provide
macros to construct alloc-suitable mask from the given one for
yes-i-really-have-to-allocate case.

-- 
 Kirill A. Shutemov


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Kirill A. Shutemov
On Wed, Nov 12, 2014 at 03:22:41AM +0200, Kirill A. Shutemov wrote:
> On Tue, Nov 11, 2014 at 04:49:13PM -0800, Andrew Morton wrote:
> > On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  > linux.com> wrote:
> > 
> > > On Tue, 11 Nov 2014, Andrew Morton wrote:
> > > 
> > > > 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)
> 
> 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:

diff --git a/mm/vmscan.c b/mm/vmscan.c
index dcb47074ae03..eb165d29c5e5 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -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(_rwsem)) {
/*
 * If we would return 0, our callers would understand that we
-- 
 Kirill A. Shutemov


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Kirill A. Shutemov
On Tue, Nov 11, 2014 at 04:49:13PM -0800, Andrew Morton wrote:
> On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  
> wrote:
> 
> > On Tue, 11 Nov 2014, Andrew Morton wrote:
> > 
> > > 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)

do_huge_pmd_anonymous_page
 alloc_hugepage_vma
  alloc_pages_vma(GFP_TRANSHUGE)

GFP_TRANSHUGE contains GFP_HIGHUSER_MOVABLE, which has __GFP_HIGHMEM.

-- 
 Kirill A. Shutemov


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-12 Thread Luke Dashjr
On Wednesday, November 12, 2014 12:49:13 AM Andrew Morton wrote:
> 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

Luke


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-11 Thread Andrew Morton
On Wed, 12 Nov 2014 13:08:55 +0900 Tetsuo Handa  wrote:

> Andrew Morton wrote:
> > 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.
> 
> -   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
> 
> -   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)

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.

Apart from all the other things we're discussing here, it should do
this because kmalloc() isn't very reliable within a shrinker.


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

Yes, there are such problems.

Shrinkers do all sorts of surprising things - some of the filesystem
ones do disk writes!  And these involve all sorts of locking and memory
allocations.  But they won't be directly using scan_control.gfp_mask. 
They may be using open-coded __GFP_NOFS for the allocations.  The
complicated ones pass the IO over to kernel threads and wait for them
to complete, which addresses the stack consumption concerns (at least).




[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-11 Thread Christoph Lameter
On Tue, 11 Nov 2014, Andrew Morton wrote:

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

Could we change BUG_ON to diplay the value? This keeps on coming up.

If you want to add this to the slab allocators then please add to
mm/slab_common.c and refer to it from the other allocators.



[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-11 Thread Christoph Lameter
On Tue, 11 Nov 2014, Andrew Morton wrote:

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


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-11 Thread Andrew Morton
On Wed, 12 Nov 2014 03:47:03 +0200 "Kirill A. Shutemov"  wrote:

> On Wed, Nov 12, 2014 at 03:22:41AM +0200, Kirill A. Shutemov wrote:
> > On Tue, Nov 11, 2014 at 04:49:13PM -0800, Andrew Morton wrote:
> > > On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  > > linux.com> wrote:
> > > 
> > > > On Tue, 11 Nov 2014, Andrew Morton wrote:
> > > > 
> > > > > 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)
> > 
> > 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:
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index dcb47074ae03..eb165d29c5e5 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -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(_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;

?



[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-11 Thread Andrew Morton
On Wed, 12 Nov 2014 10:22:45 +0900 Joonsoo Kim  
wrote:

> On Tue, Nov 11, 2014 at 05:02:43PM -0800, Andrew Morton wrote:
> > On Wed, 12 Nov 2014 00:54:01 + Luke Dashjr  wrote:
> > 
> > > On Wednesday, November 12, 2014 12:49:13 AM Andrew Morton wrote:
> > > > 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.

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.


[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-11 Thread Andrew Morton
On Wed, 12 Nov 2014 00:54:01 + Luke Dashjr  wrote:

> On Wednesday, November 12, 2014 12:49:13 AM Andrew Morton wrote:
> > 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.

It might just have been a random memory bitflip or other corruption of
course.  Is it repeatable at all?

If it is, please add the below and retest?

--- a/mm/slab.c~slab-improve-checking-for-invalid-gfp_flags
+++ a/mm/slab.c
@@ -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 (unlikely(flags & GFP_SLAB_BUG_MASK)) {
+   pr_emerg("gfp: %u\n", flags & GFP_SLAB_BUG_MASK);
+   BUG();
+   }
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
--- a/mm/slub.c~slab-improve-checking-for-invalid-gfp_flags
+++ a/mm/slub.c
@@ -1377,7 +1377,10 @@ static struct page *new_slab(struct kmem
int order;
int idx;

-   BUG_ON(flags & GFP_SLAB_BUG_MASK);
+   if (unlikely(flags & GFP_SLAB_BUG_MASK)) {
+   pr_emerg("gfp: %u\n", flags & GFP_SLAB_BUG_MASK);
+   BUG();
+   }

page = allocate_slab(s,
flags & (GFP_RECLAIM_MASK | GFP_CONSTRAINT_MASK), node);
_



[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-11 Thread Andrew Morton
On Wed, 12 Nov 2014 09:44:19 +0900 Joonsoo Kim  
wrote:

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

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.

diff -puN mm/slab.c~slab-improve-checking-for-invalid-gfp_flags mm/slab.c
--- a/mm/slab.c~slab-improve-checking-for-invalid-gfp_flags
+++ a/mm/slab.c
@@ -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 (unlikely(flags & GFP_SLAB_BUG_MASK)) {
+   pr_emerg("gfp: %u\n", flags & GFP_SLAB_BUG_MASK);
+   BUG();
+   }
local_flags = flags & (GFP_CONSTRAINT_MASK|GFP_RECLAIM_MASK);

/* Take the node list lock to change the colour_next on this node */
--- a/mm/slub.c~slab-improve-checking-for-invalid-gfp_flags
+++ a/mm/slub.c
@@ -1377,7 +1377,10 @@ static struct page *new_slab(struct kmem
int order;
int idx;

-   BUG_ON(flags & GFP_SLAB_BUG_MASK);
+   if (unlikely(flags & GFP_SLAB_BUG_MASK)) {
+   pr_emerg("gfp: %u\n", flags & GFP_SLAB_BUG_MASK);
+   BUG();
+   }

page = allocate_slab(s,
flags & (GFP_RECLAIM_MASK | GFP_CONSTRAINT_MASK), node);
_



[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-11 Thread Andrew Morton
On Tue, 11 Nov 2014 18:36:28 -0600 (CST) Christoph Lameter  
wrote:

> On Tue, 11 Nov 2014, Andrew Morton wrote:
> 
> > 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)

but I haven't traced that through and that's 32-bit.

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



[Bug 87891] New: kernel BUG at mm/slab.c:2625!

2014-11-11 Thread Andrew Morton

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

On Thu, 06 Nov 2014 17:28:41 + bugzilla-daemon at bugzilla.kernel.org 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: akpm at linux-foundation.org
>   Reporter: luke-jr+linuxbugs at utopios.org
> Regression: No

Well this is interesting.


> [359782.842112] kernel BUG at mm/slab.c:2625!
> ...
> [359782.843008] Call Trace:
> [359782.843017]  [] __kmalloc+0xdf/0x200
> [359782.843037]  [] ? ttm_page_pool_free+0x35/0x180 [ttm]
> [359782.843060]  [] ttm_page_pool_free+0x35/0x180 [ttm]
> [359782.843084]  [] ttm_pool_shrink_scan+0xae/0xd0 [ttm]
> [359782.843108]  [] shrink_slab_node+0x12b/0x2e0
> [359782.843129]  [] ? fragmentation_index+0x14/0x70
> [359782.843150]  [] ? zone_watermark_ok+0x1a/0x20
> [359782.843171]  [] shrink_slab+0xc8/0x110
> [359782.843189]  [] do_try_to_free_pages+0x300/0x410
> [359782.843210]  [] try_to_free_pages+0xbb/0x190
> [359782.843230]  [] __alloc_pages_nodemask+0x696/0xa90
> [359782.843253]  [] do_huge_pmd_anonymous_page+0xfa/0x3f0
> [359782.843278]  [] ? debug_smp_processor_id+0x17/0x20
> [359782.843300]  [] ? __lru_cache_add+0x57/0xa0
> [359782.843321]  [] 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 
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 
Cc: Pekka Enberg 
Cc: David Rientjes 
Cc: Joonsoo Kim 
Signed-off-by: Andrew Morton 
---

 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
--- a/include/linux/gfp.h~slab-improve-checking-for-invalid-gfp_flags
+++ a/include/linux/gfp.h
@@ -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