[PATCH v7 00/12] Support non-lru page migration
On Thu, Jun 16, 2016 at 07:09:32PM +0900, Minchan Kim wrote: > On Thu, Jun 16, 2016 at 05:42:11PM +0900, Sergey Senozhatsky wrote: > > On (06/16/16 15:47), Minchan Kim wrote: > > > > [..] > > > > > > this is what I'm getting with the [zsmalloc: keep first object > > > > > > offset in struct page] > > > > > > applied: "count:0 mapcount:-127". which may be not related to > > > > > > zsmalloc at this point. > > > > > > > > > > > > kernel: BUG: Bad page state in process khugepaged pfn:101db8 > > > > > > kernel: page:ea0004076e00 count:0 mapcount:-127 mapping: > > > > > > (null) index:0x1 > > > > > > > > > > Hm, it seems double free. > > > > > > > > > > It doen't happen if you disable zram? IOW, it seems to be related > > > > > zsmalloc migration? > > > > > > > > need to test more, can't confidently answer now. > > > > > > > > > How easy can you reprodcue it? Could you bisect it? > > > > > > > > it takes some (um.. random) time to trigger the bug. > > > > I'll try to come up with more details. > > > > > > Could you revert [1] and retest? > > > > > > [1] mm/compaction: split freepages without holding the zone lock > > > > ok, so this is not related to zsmalloc. finally manged to reproduce > > it. will fork a separate thread. > > The reason I mentioned [1] is that it seems to have a bug. > > isolate_freepages_block > __isolate_free_page > if(!zone_watermark_ok()) > return 0; > list_add_tail(>lru, freelist); > > However, the page is not isolated. > Joonsoo? Good job! I will fix it soon. Thanks.
[PATCH v2 13/18] mm/compaction: support non-lru movable page migration
On Tue, Mar 22, 2016 at 11:55:45PM +0900, Minchan Kim wrote: > On Tue, Mar 22, 2016 at 02:50:37PM +0900, Joonsoo Kim wrote: > > On Mon, Mar 21, 2016 at 03:31:02PM +0900, Minchan Kim wrote: > > > We have allowed migration for only LRU pages until now and it was > > > enough to make high-order pages. But recently, embedded system(e.g., > > > webOS, android) uses lots of non-movable pages(e.g., zram, GPU memory) > > > so we have seen several reports about troubles of small high-order > > > allocation. For fixing the problem, there were several efforts > > > (e,g,. enhance compaction algorithm, SLUB fallback to 0-order page, > > > reserved memory, vmalloc and so on) but if there are lots of > > > non-movable pages in system, their solutions are void in the long run. > > > > > > So, this patch is to support facility to change non-movable pages > > > with movable. For the feature, this patch introduces functions related > > > to migration to address_space_operations as well as some page flags. > > > > > > Basically, this patch supports two page-flags and two functions related > > > to page migration. The flag and page->mapping stability are protected > > > by PG_lock. > > > > > > PG_movable > > > PG_isolated > > > > > > bool (*isolate_page) (struct page *, isolate_mode_t); > > > void (*putback_page) (struct page *); > > > > > > Duty of subsystem want to make their pages as migratable are > > > as follows: > > > > > > 1. It should register address_space to page->mapping then mark > > > the page as PG_movable via __SetPageMovable. > > > > > > 2. It should mark the page as PG_isolated via SetPageIsolated > > > if isolation is sucessful and return true. > > > > > > 3. If migration is successful, it should clear PG_isolated and > > > PG_movable of the page for free preparation then release the > > > reference of the page to free. > > > > > > 4. If migration fails, putback function of subsystem should > > > clear PG_isolated via ClearPageIsolated. > > > > I think that this feature needs a separate document to describe > > requirement of each step in more detail. For example, #1 can be > > possible without holding a lock? I'm not sure because you lock > > the page when implementing zsmalloc page migration in 15th patch. > > Yes, we needs PG_lock because install page->mapping and PG_movable > should be atomic and PG_lock protects it. > > Better interface might be > > void __SetPageMovable(struct page *page, sruct address_space *mapping); > > > > > #3 also need more explanation. Before release, we need to > > unregister address_space. I guess that it needs to be done > > in migratepage() but there is no explanation. > > Okay, we can unregister address_space in __ClearPageMovable. > I will change it. > > > > > > > > > Cc: Vlastimil Babka > > > Cc: Mel Gorman > > > Cc: Hugh Dickins > > > Cc: dri-devel at lists.freedesktop.org > > > Cc: virtualization at lists.linux-foundation.org > > > Signed-off-by: Gioh Kim > > > Signed-off-by: Minchan Kim > > > --- > > > Documentation/filesystems/Locking | 4 + > > > Documentation/filesystems/vfs.txt | 5 ++ > > > fs/proc/page.c | 3 + > > > include/linux/fs.h | 2 + > > > include/linux/migrate.h| 2 + > > > include/linux/page-flags.h | 29 > > > include/uapi/linux/kernel-page-flags.h | 1 + > > > mm/compaction.c| 14 +++- > > > mm/migrate.c | 132 > > > + > > > 9 files changed, 177 insertions(+), 15 deletions(-) > > > > > > diff --git a/Documentation/filesystems/Locking > > > b/Documentation/filesystems/Locking > > > index 619af9bfdcb3..0bb79560abb3 100644 > > > --- a/Documentation/filesystems/Locking > > > +++ b/Documentation/filesystems/Locking > > > @@ -195,7 +195,9 @@ unlocks and drops the reference. > > > int (*releasepage) (struct page *, int); > > > void (*freepage)(struct page *); > > > int (*direct_IO)(struct kiocb *, struct iov_iter *iter, loff_t offset); > > > + bool (*isolate_page) (struct page *, isolate_mode_t); > > > int (*migratepage)(struct address_space *, struct page *, struct page > > > *); > &g
[PATCH v2 13/18] mm/compaction: support non-lru movable page migration
On Mon, Mar 21, 2016 at 03:31:02PM +0900, Minchan Kim wrote:
> We have allowed migration for only LRU pages until now and it was
> enough to make high-order pages. But recently, embedded system(e.g.,
> webOS, android) uses lots of non-movable pages(e.g., zram, GPU memory)
> so we have seen several reports about troubles of small high-order
> allocation. For fixing the problem, there were several efforts
> (e,g,. enhance compaction algorithm, SLUB fallback to 0-order page,
> reserved memory, vmalloc and so on) but if there are lots of
> non-movable pages in system, their solutions are void in the long run.
>
> So, this patch is to support facility to change non-movable pages
> with movable. For the feature, this patch introduces functions related
> to migration to address_space_operations as well as some page flags.
>
> Basically, this patch supports two page-flags and two functions related
> to page migration. The flag and page->mapping stability are protected
> by PG_lock.
>
> PG_movable
> PG_isolated
>
> bool (*isolate_page) (struct page *, isolate_mode_t);
> void (*putback_page) (struct page *);
>
> Duty of subsystem want to make their pages as migratable are
> as follows:
>
> 1. It should register address_space to page->mapping then mark
> the page as PG_movable via __SetPageMovable.
>
> 2. It should mark the page as PG_isolated via SetPageIsolated
> if isolation is sucessful and return true.
>
> 3. If migration is successful, it should clear PG_isolated and
> PG_movable of the page for free preparation then release the
> reference of the page to free.
>
> 4. If migration fails, putback function of subsystem should
> clear PG_isolated via ClearPageIsolated.
I think that this feature needs a separate document to describe
requirement of each step in more detail. For example, #1 can be
possible without holding a lock? I'm not sure because you lock
the page when implementing zsmalloc page migration in 15th patch.
#3 also need more explanation. Before release, we need to
unregister address_space. I guess that it needs to be done
in migratepage() but there is no explanation.
>
> Cc: Vlastimil Babka
> Cc: Mel Gorman
> Cc: Hugh Dickins
> Cc: dri-devel at lists.freedesktop.org
> Cc: virtualization at lists.linux-foundation.org
> Signed-off-by: Gioh Kim
> Signed-off-by: Minchan Kim
> ---
> Documentation/filesystems/Locking | 4 +
> Documentation/filesystems/vfs.txt | 5 ++
> fs/proc/page.c | 3 +
> include/linux/fs.h | 2 +
> include/linux/migrate.h| 2 +
> include/linux/page-flags.h | 29
> include/uapi/linux/kernel-page-flags.h | 1 +
> mm/compaction.c| 14 +++-
> mm/migrate.c | 132
> +
> 9 files changed, 177 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/filesystems/Locking
> b/Documentation/filesystems/Locking
> index 619af9bfdcb3..0bb79560abb3 100644
> --- a/Documentation/filesystems/Locking
> +++ b/Documentation/filesystems/Locking
> @@ -195,7 +195,9 @@ unlocks and drops the reference.
> int (*releasepage) (struct page *, int);
> void (*freepage)(struct page *);
> int (*direct_IO)(struct kiocb *, struct iov_iter *iter, loff_t offset);
> + bool (*isolate_page) (struct page *, isolate_mode_t);
> int (*migratepage)(struct address_space *, struct page *, struct page
> *);
> + void (*putback_page) (struct page *);
> int (*launder_page)(struct page *);
> int (*is_partially_uptodate)(struct page *, unsigned long, unsigned
> long);
> int (*error_remove_page)(struct address_space *, struct page *);
> @@ -219,7 +221,9 @@ invalidatepage: yes
> releasepage: yes
> freepage:yes
> direct_IO:
> +isolate_page:yes
> migratepage: yes (both)
> +putback_page:yes
> launder_page:yes
> is_partially_uptodate: yes
> error_remove_page: yes
> diff --git a/Documentation/filesystems/vfs.txt
> b/Documentation/filesystems/vfs.txt
> index b02a7d598258..4c1b6c3b4bc8 100644
> --- a/Documentation/filesystems/vfs.txt
> +++ b/Documentation/filesystems/vfs.txt
> @@ -592,9 +592,14 @@ struct address_space_operations {
> int (*releasepage) (struct page *, int);
> void (*freepage)(struct page *);
> ssize_t (*direct_IO)(struct kiocb *, struct iov_iter *iter, loff_t
> offset);
> + /* isolate a page for migration */
> + bool (*isolate_page) (struct page *, isolate_mode_t);
> /* migrate the contents of a page to the specified target */
> int (*migratepage) (struct page *, struct page *);
> + /* put the page back to right list */
> + void (*putback_page) (struct page *);
> int (*launder_page) (struct page *);
> +
> int (*is_partially_uptodate) (struct page *, unsigned long,
>
[Bug 87891] New: kernel BUG at mm/slab.c:2625!
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!
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!
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!
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!
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!
ry 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 infuriating to go BUG because one of the bits was set,
> but not tell us which bit it was!
Agreed. Let's fix it.
Thanks.
>
> 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 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
> --- 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 (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
> --- 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 (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 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
