Re: + mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback.patch added to -mm tree

Re: + mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback.patch added to -mm tree

[Minchan Kim]

Hi Andrea,

I didn't follow up USB stick freeze issue but the patch's concept
looks good to me. But there are some comments about this patch.

1. __GFP_NO_KSWAPD

This patch is based on assumption that hugepage allocation have a good
fallback and now hugepage allocation uses __GFP_NO_KSWAPD.

__GFP_NO_KSWAPD's goal is just prevent unnecessary wakeup kswapd and
only user is just thp now so I can understand why you use it but how
about __GFP_NORETRY?

I think __GFP_NORETRY assume caller has a fallback mechanism(ex, SLUB)
and he think latency is important in such context.

2. LRU churn

By this patch, async migration can't migrate dirty page of normal fs.
It can move the victim page to head of LRU. I hope we can reduce LRU
churning as possible. For it, we can do it when we isolate the LRU
pages.
If compaction mode is async, we can exclude the dirty pages in
isolate_migratepages.


On Wed, Mar 23, 2011 at 6:53 AM,  <akpm@linux-foundation.org> wrote:
>
> The patch titled
>     mm: compaction: Use async migration for __GFP_NO_KSWAPD and enforce no writeback
> has been added to the -mm tree.  Its filename is
>     mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback.patch
>
> Before you just go and hit "reply", please:
>   a) Consider who else should be cc'ed
>   b) Prefer to cc a suitable mailing list as well
>   c) Ideally: find the original patch on the mailing list and do a
>      reply-to-all to that, adding suitable additional cc's
>
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>
> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> out what to do about this
>
> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
>
> ------------------------------------------------------
> Subject: mm: compaction: Use async migration for __GFP_NO_KSWAPD and enforce no writeback
> From: Andrea Arcangeli <aarcange@redhat.com>
>
> __GFP_NO_KSWAPD allocations are usually very expensive and not mandatory
> to succeed as they have graceful fallback.  Waiting for I/O in those,
> tends to be overkill in terms of latencies, so we can reduce their latency
> by disabling sync migrate.
>
> Unfortunately, even with async migration it's still possible for the
> process to be blocked waiting for a request slot (e.g.  get_request_wait
> in the block layer) when ->writepage is called.  To prevent
> __GFP_NO_KSWAPD blocking, this patch prevents ->writepage being called on
> dirty page cache for asynchronous migration.
>
> [mel@csn.ul.ie: Avoid writebacks for NFS, retry locked pages, use bool]
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Mel Gorman <mel@csn.ul.ie>
> Cc: Arthur Marsh <arthur.marsh@internode.on.net>
> Cc: Clemens Ladisch <cladisch@googlemail.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> Signed-off-by: Andrew Morton <>
> ---
>
>  mm/migrate.c    |   48 +++++++++++++++++++++++++++++++---------------
>  mm/page_alloc.c |    2 -
>  2 files changed, 34 insertions(+), 16 deletions(-)
>
> diff -puN mm/migrate.c~mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback mm/migrate.c
> --- a/mm/migrate.c~mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback
> +++ a/mm/migrate.c
> @@ -564,7 +564,7 @@ static int fallback_migrate_page(struct
>  *  == 0 - success
>  */
>  static int move_to_new_page(struct page *newpage, struct page *page,
> -                                               int remap_swapcache)
> +                                       int remap_swapcache, bool sync)
>  {
>        struct address_space *mapping;
>        int rc;
> @@ -586,18 +586,28 @@ static int move_to_new_page(struct page
>        mapping = page_mapping(page);
>        if (!mapping)
>                rc = migrate_page(mapping, newpage, page);
> -       else if (mapping->a_ops->migratepage)
> +       else {
>                /*
> -                * Most pages have a mapping and most filesystems
> -                * should provide a migration function. Anonymous
> -                * pages are part of swap space which also has its
> -                * own migration function. This is the most common
> -                * path for page migration.
> +                * Do not writeback pages if !sync and migratepage is
> +                * not pointing to migrate_page() which is nonblocking
> +                * (swapcache/tmpfs uses migratepage = migrate_page).
>                 */
> -               rc = mapping->a_ops->migratepage(mapping,
> -                                               newpage, page);
> -       else
> -               rc = fallback_migrate_page(mapping, newpage, page);
> +               if (PageDirty(page) && !sync &&
> +                   mapping->a_ops->migratepage != migrate_page)
> +                       rc = -EBUSY;
> +               else if (mapping->a_ops->migratepage)
> +                       /*
> +                        * Most pages have a mapping and most filesystems
> +                        * should provide a migration function. Anonymous
> +                        * pages are part of swap space which also has its
> +                        * own migration function. This is the most common
> +                        * path for page migration.
> +                        */
> +                       rc = mapping->a_ops->migratepage(mapping,
> +                                                       newpage, page);
> +               else
> +                       rc = fallback_migrate_page(mapping, newpage, page);
> +       }
>
>        if (rc) {
>                newpage->mapping = NULL;
> @@ -641,7 +651,7 @@ static int unmap_and_move(new_page_t get
>        rc = -EAGAIN;
>
>        if (!trylock_page(page)) {
> -               if (!force)
> +               if (!force || !sync)
>                        goto move_newpage;
>
>                /*
> @@ -686,7 +696,15 @@ static int unmap_and_move(new_page_t get
>        BUG_ON(charge);
>
>        if (PageWriteback(page)) {
> -               if (!force || !sync)
> +               /*
> +                * For !sync, there is no point retrying as the retry loop
> +                * is expected to be too short for PageWriteback to be cleared
> +                */
> +               if (!sync) {
> +                       rc = -EBUSY;
> +                       goto uncharge;
> +               }
> +               if (!force)
>                        goto uncharge;
>                wait_on_page_writeback(page);
>        }
> @@ -757,7 +775,7 @@ static int unmap_and_move(new_page_t get
>
>  skip_unmap:
>        if (!page_mapped(page))
> -               rc = move_to_new_page(newpage, page, remap_swapcache);
> +               rc = move_to_new_page(newpage, page, remap_swapcache, sync);
>
>        if (rc && remap_swapcache)
>                remove_migration_ptes(page, page);
> @@ -850,7 +868,7 @@ static int unmap_and_move_huge_page(new_
>        try_to_unmap(hpage, TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
>
>        if (!page_mapped(hpage))
> -               rc = move_to_new_page(new_hpage, hpage, 1);
> +               rc = move_to_new_page(new_hpage, hpage, 1, sync);
>
>        if (rc)
>                remove_migration_ptes(hpage, hpage);
> diff -puN mm/page_alloc.c~mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback mm/page_alloc.c
> --- a/mm/page_alloc.c~mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback
> +++ a/mm/page_alloc.c
> @@ -2103,7 +2103,7 @@ rebalance:
>                                        sync_migration);
>        if (page)
>                goto got_pg;
> -       sync_migration = true;
> +       sync_migration = !(gfp_mask & __GFP_NO_KSWAPD);
>
>        /* Try direct reclaim and then allocating */
>        page = __alloc_pages_direct_reclaim(gfp_mask, order,
> _
>
> Patches currently in -mm which might be from aarcange@redhat.com are
>
> origin.patch
> mm-compaction-prevent-kswapd-compacting-memory-to-reduce-cpu-usage.patch
> mm-compaction-check-migrate_pagess-return-value-instead-of-list_empty.patch
> mm-deactivate-invalidated-pages.patch
> memcg-move-memcg-reclaimable-page-into-tail-of-inactive-list.patch
> memcg-move-memcg-reclaimable-page-into-tail-of-inactive-list-fix.patch
> mm-reclaim-invalidated-page-asap.patch
> pagewalk-only-split-huge-pages-when-necessary.patch
> smaps-break-out-smaps_pte_entry-from-smaps_pte_range.patch
> smaps-pass-pte-size-argument-in-to-smaps_pte_entry.patch
> smaps-teach-smaps_pte_range-about-thp-pmds.patch
> smaps-have-smaps-show-transparent-huge-pages.patch
> mm-vmscan-kswapd-should-not-free-an-excessive-number-of-pages-when-balancing-small-zones.patch
> mm-compaction-minimise-the-time-irqs-are-disabled-while-isolating-free-pages.patch
> mm-compaction-minimise-the-time-irqs-are-disabled-while-isolating-pages-for-migration.patch
> mm-compaction-minimise-the-time-irqs-are-disabled-while-isolating-pages-for-migration-fix.patch
> mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback.patch
> mm-add-__gfp_other_node-flag.patch
> mm-use-__gfp_other_node-for-transparent-huge-pages.patch
> ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages.patch
> ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix.patch
> ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix-fix.patch
> ksm-add-vm_stat-and-meminfo-entry-to-reflect-pte-mapping-to-ksm-pages-fix-fix-fix.patch
> mm-add-vm-counters-for-transparent-hugepages.patch
> memcg-use-native-word-page-statistics-counters-fix-event-counter-breakage-with-thp.patch
>
>



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: + mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback.patch added to -mm tree

[Andrea Arcangeli]

Hello Minchan,

On Wed, Mar 23, 2011 at 07:58:24AM +0900, Minchan Kim wrote:
> Hi Andrea,
> 
> I didn't follow up USB stick freeze issue but the patch's concept
> looks good to me. But there are some comments about this patch.
> 
> 1. __GFP_NO_KSWAPD
> 
> This patch is based on assumption that hugepage allocation have a good
> fallback and now hugepage allocation uses __GFP_NO_KSWAPD.

Yes, the only goal was to bypass kswapd. We don't want an overwork to
try to generate those.

> __GFP_NO_KSWAPD's goal is just prevent unnecessary wakeup kswapd and
> only user is just thp now so I can understand why you use it but how
> about __GFP_NORETRY?
> 
> I think __GFP_NORETRY assume caller has a fallback mechanism(ex, SLUB)
> and he think latency is important in such context.

__GFP_NORETRY sounds the opposite of __GFP_REPEAT. So it gets a bit
confusing as one would expect if you don't pass __GFP_REPEAT you're
already in __GFP_NORETRY mode.

OTOH it's like __GFP_NORETRY -> normal -> __GFP_REPEAT, so it wouldn't
be wrong either. Where __GFP_REPEAT does it best at not failing even
when the kernel stack allocation would have failed.

Now before thinking further into this, we should probably ask Alex how
thing goes if we undo the change to page_alloc.c so that
__GFP_NO_KSWAPD will only affect kswapd like before (so without
requiring a rename).

It's possible that such change wasn't needed. The less things
__GFP_NO_KSWAPD does and the closer THP allocations are to "default"
high order allocations the better/simpler. Now that we solved the
problem we can more easily refine these bits. __GFP_NO_KSWAPD is
absolutely needed for frequent huge allocations especially with a
kswapd that doesn't use compaction (and we found compaction in kswapd
is detrimental even for small order allocations, so __GFP_NO_KSWAPD
isn't going away too soon, but if we can make it again only specific
to kswapd it's better). I ideally would like THP allocation not having
to specify anything and the allocator to work just fine by itself.

__GFP_REPEAT is a magic needed for hugetlbfs to insist forever to be
paranoid in trying to increase nr_hugepages with the highest possible
effort no matter how much swapping or slowdown/hang it generates
during the "echo". In the same way __GFP_NO_KSWAPD is to avoid
interference with kswapd that can't use compaction without leading to
too much wasted CPU yet. But the less stuff these bitflags do, the
better.

> 2. LRU churn
> 
> By this patch, async migration can't migrate dirty page of normal fs.
> It can move the victim page to head of LRU. I hope we can reduce LRU
> churning as possible. For it, we can do it when we isolate the LRU
> pages.
> If compaction mode is async, we can exclude the dirty pages in
> isolate_migratepages.

I've no trivial solution for the lru churning but, at least we're not
making the lru churning any worse with this patch.

I see your point that we could reduce the lru churning further after
this patch, and that is cleaner if done as an incremental change
considering it's a new improvement that become possible after the
patch and it doesn't fix any regression.

To reduce it, we'd need to expose the migrate internal details to the
compaction code. It's not good enough to just check PageDirty
considering that dirty pages are migrated perfectly even when they're
not anonymous with a page_mapping != 0, if ->migratepage is
migrate_page (like for tmpfs/swapcache). So to optimize that, I guess
we could add a can_migrate_async(page) to the migrate code, to call in
the compaction loop. It should work.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: + mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback.patch added to -mm tree

[Minchan Kim]

On Wed, Mar 23, 2011 at 9:25 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> Hello Minchan,
>
> On Wed, Mar 23, 2011 at 07:58:24AM +0900, Minchan Kim wrote:
>> Hi Andrea,
>>
>> I didn't follow up USB stick freeze issue but the patch's concept
>> looks good to me. But there are some comments about this patch.
>>
>> 1. __GFP_NO_KSWAPD
>>
>> This patch is based on assumption that hugepage allocation have a good
>> fallback and now hugepage allocation uses __GFP_NO_KSWAPD.
>
> Yes, the only goal was to bypass kswapd. We don't want an overwork to
> try to generate those.
>
>> __GFP_NO_KSWAPD's goal is just prevent unnecessary wakeup kswapd and
>> only user is just thp now so I can understand why you use it but how
>> about __GFP_NORETRY?
>>
>> I think __GFP_NORETRY assume caller has a fallback mechanism(ex, SLUB)
>> and he think latency is important in such context.
>
> __GFP_NORETRY sounds the opposite of __GFP_REPEAT. So it gets a bit
> confusing as one would expect if you don't pass __GFP_REPEAT you're
> already in __GFP_NORETRY mode.
>
> OTOH it's like __GFP_NORETRY -> normal -> __GFP_REPEAT, so it wouldn't
> be wrong either. Where __GFP_REPEAT does it best at not failing even
> when the kernel stack allocation would have failed.
>
> Now before thinking further into this, we should probably ask Alex how
> thing goes if we undo the change to page_alloc.c so that
> __GFP_NO_KSWAPD will only affect kswapd like before (so without
> requiring a rename).

Okay. I will look at result.
If the problem happens again with reverted patch of page_alloc.c,
Don't we have to investigate further the problem happens with SLUB or
some driver's big memory allocation which is lower than 2M? We didn't
see the problem allocation under 2M but async migration's history was
short so we can't make sure it.

>
> It's possible that such change wasn't needed. The less things
> __GFP_NO_KSWAPD does and the closer THP allocations are to "default"
> high order allocations the better/simpler. Now that we solved the
> problem we can more easily refine these bits. __GFP_NO_KSWAPD is
> absolutely needed for frequent huge allocations especially with a
> kswapd that doesn't use compaction (and we found compaction in kswapd
> is detrimental even for small order allocations, so __GFP_NO_KSWAPD
> isn't going away too soon, but if we can make it again only specific
> to kswapd it's better). I ideally would like THP allocation not having
> to specify anything and the allocator to work just fine by itself.

Absolutely.

>
> __GFP_REPEAT is a magic needed for hugetlbfs to insist forever to be
> paranoid in trying to increase nr_hugepages with the highest possible
> effort no matter how much swapping or slowdown/hang it generates
> during the "echo". In the same way __GFP_NO_KSWAPD is to avoid
> interference with kswapd that can't use compaction without leading to
> too much wasted CPU yet. But the less stuff these bitflags do, the
> better.

Don't you want to add async migration for low order allocation like SLUB?
If you don't want to do async migration low order allocation, we can
add the check if (gfp_flags & __GFP_RETRY) && (order >= 9 or some
threshold) for async migration?

My point is to avoid implicit hidden meaning of __GFP_NO_KSWAPD
although __GFP_REPEAT already does it.

>
>> 2. LRU churn
>>
>> By this patch, async migration can't migrate dirty page of normal fs.
>> It can move the victim page to head of LRU. I hope we can reduce LRU
>> churning as possible. For it, we can do it when we isolate the LRU
>> pages.
>> If compaction mode is async, we can exclude the dirty pages in
>> isolate_migratepages.
>
> I've no trivial solution for the lru churning but, at least we're not
> making the lru churning any worse with this patch.

If async migration is going on and meet the dirty page, the patch can
return the -EBUSY so the page could put back to head of LRU but the
old migration can be going on although the page is dirty.

>
> I see your point that we could reduce the lru churning further after
> this patch, and that is cleaner if done as an incremental change
> considering it's a new improvement that become possible after the
> patch and it doesn't fix any regression.

Okay.

>
> To reduce it, we'd need to expose the migrate internal details to the
> compaction code. It's not good enough to just check PageDirty
> considering that dirty pages are migrated perfectly even when they're
> not anonymous with a page_mapping != 0, if ->migratepage is
> migrate_page (like for tmpfs/swapcache). So to optimize that, I guess
> we could add a can_migrate_async(page) to the migrate code, to call in
> the compaction loop. It should work.
>

Totally agree with you.
It's another topic and goes next time.

Thanks, Andrea.
-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: + mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback.patch added to -mm tree

[Andrea Arcangeli]

On Wed, Mar 23, 2011 at 03:01:33PM +0900, Minchan Kim wrote:
> Okay. I will look at result.
> If the problem happens again with reverted patch of page_alloc.c,
> Don't we have to investigate further the problem happens with SLUB or
> some driver's big memory allocation which is lower than 2M? We didn't
> see the problem allocation under 2M but async migration's history was
> short so we can't make sure it.

Yes, probably. This is also why I hope the page_alloc.c part didn't
make a difference. We kept it to be sure to make any sign of sync
migration to go away from the stack traces, but I hope it's not so
important anymore now. Reclaim eventually also becomes synchronous.

> Don't you want to add async migration for low order allocation like SLUB?
> If you don't want to do async migration low order allocation, we can
> add the check if (gfp_flags & __GFP_RETRY) && (order >= 9 or some
> threshold) for async migration?
> 
> My point is to avoid implicit hidden meaning of __GFP_NO_KSWAPD
> although __GFP_REPEAT already does it.

I see your point, so let's think about it after testing of the
reversal of the page_alloc.c change. If that's not necessary we just
reverse it and it already solves these concerns.

> If async migration is going on and meet the dirty page, the patch can
> return the -EBUSY so the page could put back to head of LRU but the
> old migration can be going on although the page is dirty.

Ok, but in term of LRU it's not like we're going to help much in
skipping the page in compaction, it'd leave the sync pages there, and
only list_del the async pages. I think it's mostly a cpu saving
optimization, I doubt the lru ordering will be much more accurate by
not doing list_del on the sync pages considering we would list_del
the rest but not the sync part.

> Totally agree with you.
> It's another topic and goes next time.
> 
> Thanks, Andrea.

Thanks to you for pointing out these problems.

Andrea

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: + mm-compaction-use-async-migration-for-__gfp_no_kswapd-and-enforce-no-writeback.patch added to -mm tree

[Minchan Kim]

On Wed, Mar 23, 2011 at 11:36 PM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> On Wed, Mar 23, 2011 at 03:01:33PM +0900, Minchan Kim wrote:
>> Okay. I will look at result.
>> If the problem happens again with reverted patch of page_alloc.c,
>> Don't we have to investigate further the problem happens with SLUB or
>> some driver's big memory allocation which is lower than 2M? We didn't
>> see the problem allocation under 2M but async migration's history was
>> short so we can't make sure it.
>
> Yes, probably. This is also why I hope the page_alloc.c part didn't
> make a difference. We kept it to be sure to make any sign of sync
> migration to go away from the stack traces, but I hope it's not so
> important anymore now. Reclaim eventually also becomes synchronous.
>
>> Don't you want to add async migration for low order allocation like SLUB?
>> If you don't want to do async migration low order allocation, we can
>> add the check if (gfp_flags & __GFP_RETRY) && (order >= 9 or some
>> threshold) for async migration?
>>
>> My point is to avoid implicit hidden meaning of __GFP_NO_KSWAPD
>> although __GFP_REPEAT already does it.
>
> I see your point, so let's think about it after testing of the
> reversal of the page_alloc.c change. If that's not necessary we just
> reverse it and it already solves these concerns.

Absolutely.

>
>> If async migration is going on and meet the dirty page, the patch can
>> return the -EBUSY so the page could put back to head of LRU but the
>> old migration can be going on although the page is dirty.
>
> Ok, but in term of LRU it's not like we're going to help much in
> skipping the page in compaction, it'd leave the sync pages there, and
> only list_del the async pages. I think it's mostly a cpu saving
> optimization, I doubt the lru ordering will be much more accurate by
> not doing list_del on the sync pages considering we would list_del
> the rest but not the sync part.

Yes. In terms of all LRU pages, I doubt it but isn't it better than
current meaningless rotation if we can do it easily?

Anyway, It's not a urgent issue so I don't mind it. :)
Thanks.

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>