memcg uncharge page counter mismatch

DISCUSSION Minchan Kim 3 participants 16 messages Dec 1, 2015 – Dec 4, 2015

Subsystems (4)

Memory management MGLRU MM reclaim memcg

lore

Share & tools

lore

Minchan Kim

author

2015-12-01 13:34 UTC #0 lore raw

With new test on mmotm-2015-11-25-17-08, I saw below WARNING message several times. I couldn't see it with reverting new THP refcount redesign.

I will try to make reproducer when I have a time but not sure. Before that, I hope someone catches it up.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1340 at mm/page_counter.c:26 page_counter_cancel+0x34/0x40()
Modules linked in:
CPU: 0 PID: 1340 Comm: madvise_test Not tainted 4.4.0-rc2-mm1-kirill+ #12
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 ffffffff81782eeb ffff880072b97be8 ffffffff8126f476 0000000000000000
 ffff880072b97c20 ffffffff8103e476 ffff88006b35d0b0 00000000000001fe
 0000000000000000 00000000000001fe ffff88006b35d000 ffff880072b97c30
Call Trace:
 [<ffffffff8126f476>] dump_stack+0x44/0x5e
 [<ffffffff8103e476>] warn_slowpath_common+0x86/0xc0
 [<ffffffff8103e56a>] warn_slowpath_null+0x1a/0x20
 [<ffffffff8114c754>] page_counter_cancel+0x34/0x40
 [<ffffffff8114c852>] page_counter_uncharge+0x22/0x30
 [<ffffffff8114fe17>] uncharge_batch+0x47/0x140
 [<ffffffff81150033>] uncharge_list+0x123/0x190
 [<ffffffff8115222b>] mem_cgroup_uncharge_list+0x1b/0x20
 [<ffffffff810fe9bb>] release_pages+0xdb/0x350
 [<ffffffff8113044d>] free_pages_and_swap_cache+0x9d/0x120
 [<ffffffff8111a546>] tlb_flush_mmu_free+0x36/0x60
 [<ffffffff8111b63c>] tlb_finish_mmu+0x1c/0x50
 [<ffffffff81125f38>] exit_mmap+0xd8/0x130
 [<ffffffff8103bd56>] mmput+0x56/0xe0
 [<ffffffff8103ff4d>] do_exit+0x1fd/0xaa0
 [<ffffffff8104086f>] do_group_exit+0x3f/0xb0
 [<ffffffff810408f4>] SyS_exit_group+0x14/0x20
 [<ffffffff8142b617>] entry_SYSCALL_64_fastpath+0x12/0x6a
---[ end trace 7864cf719fb83e12 ]---

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

Michal Hocko

maintainer

2015-12-02 10:16 UTC #1 lore raw

Quoted 3 lines · Minchan Kim

With new test on mmotm-2015-11-25-17-08, I saw below WARNING message several times. I couldn't see it with reverting new THP refcount redesign.

Just a wild guess. What prevents migration/compaction from calling split_huge_page on thp zero page? There is VM_BUG_ON but it is not clear whether you run with CONFIG_DEBUG_VM enabled.

Also, how big is the underflow?

Quoted 31 lines

I will try to make reproducer when I have a time but not sure. Before that, I hope someone catches it up.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1340 at mm/page_counter.c:26 page_counter_cancel+0x34/0x40()
Modules linked in:
CPU: 0 PID: 1340 Comm: madvise_test Not tainted 4.4.0-rc2-mm1-kirill+ #12
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 ffffffff81782eeb ffff880072b97be8 ffffffff8126f476 0000000000000000
 ffff880072b97c20 ffffffff8103e476 ffff88006b35d0b0 00000000000001fe
 0000000000000000 00000000000001fe ffff88006b35d000 ffff880072b97c30
Call Trace:
 [<ffffffff8126f476>] dump_stack+0x44/0x5e
 [<ffffffff8103e476>] warn_slowpath_common+0x86/0xc0
 [<ffffffff8103e56a>] warn_slowpath_null+0x1a/0x20
 [<ffffffff8114c754>] page_counter_cancel+0x34/0x40
 [<ffffffff8114c852>] page_counter_uncharge+0x22/0x30
 [<ffffffff8114fe17>] uncharge_batch+0x47/0x140
 [<ffffffff81150033>] uncharge_list+0x123/0x190
 [<ffffffff8115222b>] mem_cgroup_uncharge_list+0x1b/0x20
 [<ffffffff810fe9bb>] release_pages+0xdb/0x350
 [<ffffffff8113044d>] free_pages_and_swap_cache+0x9d/0x120
 [<ffffffff8111a546>] tlb_flush_mmu_free+0x36/0x60
 [<ffffffff8111b63c>] tlb_finish_mmu+0x1c/0x50
 [<ffffffff81125f38>] exit_mmap+0xd8/0x130
 [<ffffffff8103bd56>] mmput+0x56/0xe0
 [<ffffffff8103ff4d>] do_exit+0x1fd/0xaa0
 [<ffffffff8104086f>] do_group_exit+0x3f/0xb0
 [<ffffffff810408f4>] SyS_exit_group+0x14/0x20
 [<ffffffff8142b617>] entry_SYSCALL_64_fastpath+0x12/0x6a
---[ end trace 7864cf719fb83e12 ]---

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Minchan Kim

author

2015-12-03 01:39 UTC #2 lore raw

Quoted 7 lines · Michal Hocko

Quoted 3 lines · Minchan Kim
With new test on mmotm-2015-11-25-17-08, I saw below WARNING message several times. I couldn't see it with reverting new THP refcount redesign.
Just a wild guess. What prevents migration/compaction from calling split_huge_page on thp zero page? There is VM_BUG_ON but it is not clear

I guess migration should work with LRU pages now but zero page couldn't stay there.

Quoted 1 line

whether you run with CONFIG_DEBUG_VM enabled.

I enabled VM_DEBUG_VM.

Quoted 2 lines

Also, how big is the underflow?

mm/page_counter.c +2 -0

index 7c6a63d..adc27c3 100644
@@ -23,6 +23,8 @@ void page_counter_cancel(struct page_counter *counter, unsigned long nr_pages)
 
        new = atomic_long_sub_return(nr_pages, &counter->count);
        /* More uncharges than charges? */
+       if (new < 0)
+               printk("nr_pages %lu new %ld\n", nr_pages, new);
        WARN_ON_ONCE(new < 0);
 }

nr_pages 512 new -31

------------[ cut here ]------------
WARNING: CPU: 3 PID: 1145 at mm/page_counter.c:28 page_counter_cancel+0x44/0x50()
Modules linked in:
CPU: 3 PID: 1145 Comm: madvise_test Not tainted 4.4.0-rc2-mm1-kirill+ #17
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 ffffffff81782f09 ffff8800598c3b90 ffffffff8126f476 0000000000000000
 ffff8800598c3bc8 ffffffff8103e476 ffff88007f14f8b0 0000000000000200
 0000000000000000 0000000000000200 ffff88007f14f800 ffff8800598c3bd8
Call Trace:
 [<ffffffff8126f476>] dump_stack+0x44/0x5e
 [<ffffffff8103e476>] warn_slowpath_common+0x86/0xc0
 [<ffffffff8103e56a>] warn_slowpath_null+0x1a/0x20
 [<ffffffff8114c744>] page_counter_cancel+0x44/0x50
 [<ffffffff8114c842>] page_counter_uncharge+0x22/0x30
 [<ffffffff8114fe07>] uncharge_batch+0x47/0x140
 [<ffffffff81150023>] uncharge_list+0x123/0x190
 [<ffffffff811521f9>] mem_cgroup_uncharge+0x29/0x30
 [<ffffffff810fe3be>] __page_cache_release+0x15e/0x200
 [<ffffffff810fe47e>] __put_compound_page+0x1e/0x50
 [<ffffffff810fe960>] release_pages+0xd0/0x370
 [<ffffffff8113042d>] free_pages_and_swap_cache+0x9d/0x120
 [<ffffffff8111a516>] tlb_flush_mmu_free+0x36/0x60
 [<ffffffff8111b60c>] tlb_finish_mmu+0x1c/0x50
 [<ffffffff81125f1c>] exit_mmap+0xec/0x140
 [<ffffffff8103bd56>] mmput+0x56/0xe0
 [<ffffffff8103ff4d>] do_exit+0x1fd/0xaa0
 [<ffffffff8104086f>] do_group_exit+0x3f/0xb0
 [<ffffffff810408f4>] SyS_exit_group+0x14/0x20
 [<ffffffff8142b617>] entry_SYSCALL_64_fastpath+0x12/0x6a
---[ end trace 872ed93351e964c0 ]---

nr_pages 293 new -324
nr_pages 16 new -340
nr_pages 342 new -91
nr_pages 246 new -337
nr_pages 15 new -352
nr_pages 15 new -367

Minchan Kim

author

2015-12-03 02:09 UTC #3 lore raw

Quoted 63 lines · Minchan Kim

Quoted 7 lines · Michal Hocko

Quoted 3 lines · Minchan Kim
With new test on mmotm-2015-11-25-17-08, I saw below WARNING message several times. I couldn't see it with reverting new THP refcount redesign.
Just a wild guess. What prevents migration/compaction from calling split_huge_page on thp zero page? There is VM_BUG_ON but it is not clear

I guess migration should work with LRU pages now but zero page couldn't stay there.

Quoted 1 line

whether you run with CONFIG_DEBUG_VM enabled.

I enabled VM_DEBUG_VM.

Quoted 2 lines

Also, how big is the underflow?

mm/page_counter.c +2 -0

index 7c6a63d..adc27c3 100644
@@ -23,6 +23,8 @@ void page_counter_cancel(struct page_counter *counter, unsigned long nr_pages)
 
        new = atomic_long_sub_return(nr_pages, &counter->count);
        /* More uncharges than charges? */
+       if (new < 0)
+               printk("nr_pages %lu new %ld\n", nr_pages, new);
        WARN_ON_ONCE(new < 0);
 }

nr_pages 512 new -31

------------[ cut here ]------------
WARNING: CPU: 3 PID: 1145 at mm/page_counter.c:28 page_counter_cancel+0x44/0x50()
Modules linked in:
CPU: 3 PID: 1145 Comm: madvise_test Not tainted 4.4.0-rc2-mm1-kirill+ #17
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 ffffffff81782f09 ffff8800598c3b90 ffffffff8126f476 0000000000000000
 ffff8800598c3bc8 ffffffff8103e476 ffff88007f14f8b0 0000000000000200
 0000000000000000 0000000000000200 ffff88007f14f800 ffff8800598c3bd8
Call Trace:
 [<ffffffff8126f476>] dump_stack+0x44/0x5e
 [<ffffffff8103e476>] warn_slowpath_common+0x86/0xc0
 [<ffffffff8103e56a>] warn_slowpath_null+0x1a/0x20
 [<ffffffff8114c744>] page_counter_cancel+0x44/0x50
 [<ffffffff8114c842>] page_counter_uncharge+0x22/0x30
 [<ffffffff8114fe07>] uncharge_batch+0x47/0x140
 [<ffffffff81150023>] uncharge_list+0x123/0x190
 [<ffffffff811521f9>] mem_cgroup_uncharge+0x29/0x30
 [<ffffffff810fe3be>] __page_cache_release+0x15e/0x200
 [<ffffffff810fe47e>] __put_compound_page+0x1e/0x50
 [<ffffffff810fe960>] release_pages+0xd0/0x370
 [<ffffffff8113042d>] free_pages_and_swap_cache+0x9d/0x120
 [<ffffffff8111a516>] tlb_flush_mmu_free+0x36/0x60
 [<ffffffff8111b60c>] tlb_finish_mmu+0x1c/0x50
 [<ffffffff81125f1c>] exit_mmap+0xec/0x140
 [<ffffffff8103bd56>] mmput+0x56/0xe0
 [<ffffffff8103ff4d>] do_exit+0x1fd/0xaa0
 [<ffffffff8104086f>] do_group_exit+0x3f/0xb0
 [<ffffffff810408f4>] SyS_exit_group+0x14/0x20
 [<ffffffff8142b617>] entry_SYSCALL_64_fastpath+0x12/0x6a
---[ end trace 872ed93351e964c0 ]---

Quoted 6 lines

nr_pages 293 new -324
nr_pages 16 new -340
nr_pages 342 new -91
nr_pages 246 new -337
nr_pages 15 new -352
nr_pages 15 new -367

My guess is that it's related to new feature of Kirill's THP 'PageDoubleMap' so a THP page could be mapped a pte but !pmd_trans_huge(*pmd) so memcg precharge in move_charge should handle it?

Michal Hocko

maintainer

2015-12-03 08:54 UTC #4 lore raw

Quoted 7 lines · Minchan Kim

Quoted 6 lines · Minchan Kim
Quoted 5 lines · Michal Hocko
Quoted 3 lines · Minchan Kim
With new test on mmotm-2015-11-25-17-08, I saw below WARNING message several times. I couldn't see it with reverting new THP refcount redesign.

Quoted 5 lines

Quoted 5 lines
Quoted 2 lines
Just a wild guess. What prevents migration/compaction from calling split_huge_page on thp zero page? There is VM_BUG_ON but it is not clear
I guess migration should work with LRU pages now but zero page couldn't stay there.

Ahh, you are right. I have missed PageLRU check in isolate_migratepages_block pfn walker.

Quoted 6 lines

Quoted 6 lines
Quoted 1 line
whether you run with CONFIG_DEBUG_VM enabled.
I enabled VM_DEBUG_VM.
Quoted 2 lines
Also, how big is the underflow?

[...]

Quoted 6 lines

Quoted 6 lines
nr_pages 293 new -324
nr_pages 16 new -340
nr_pages 342 new -91
nr_pages 246 new -337
nr_pages 15 new -352
nr_pages 15 new -367

They are quite large but that is not that surprising if we consider that we are batching many uncharges at once.

Quoted 3 lines

My guess is that it's related to new feature of Kirill's THP 'PageDoubleMap' so a THP page could be mapped a pte but !pmd_trans_huge(*pmd) so memcg precharge in move_charge should handle it?

I am not familiar with the current state of THP after the rework unfortunately. So if I got you right then you are saying that pmd_trans_huge_lock fails to notice a THP so we will not charge it as THP and only charge one head page and then the tear down path will correctly recognize it as a THP and uncharge the full size, right?

I have to admit I have no idea how PageDoubleMap works and how can pmd_trans_huge fail.

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Minchan Kim

author

2015-12-03 12:59 UTC #5 lore raw

Quoted 38 lines · Michal Hocko

Quoted 7 lines · Minchan Kim
Quoted 6 lines · Minchan Kim
Quoted 5 lines · Michal Hocko
Quoted 3 lines · Minchan Kim
With new test on mmotm-2015-11-25-17-08, I saw below WARNING message several times. I couldn't see it with reverting new THP refcount redesign.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Just a wild guess. What prevents migration/compaction from calling split_huge_page on thp zero page? There is VM_BUG_ON but it is not clear
I guess migration should work with LRU pages now but zero page couldn't stay there.
Ahh, you are right. I have missed PageLRU check in isolate_migratepages_block pfn walker.
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
whether you run with CONFIG_DEBUG_VM enabled.
I enabled VM_DEBUG_VM.
Quoted 2 lines
Also, how big is the underflow?
[...]
Quoted 6 lines
Quoted 6 lines
nr_pages 293 new -324
nr_pages 16 new -340
nr_pages 342 new -91
nr_pages 246 new -337
nr_pages 15 new -352
nr_pages 15 new -367
They are quite large but that is not that surprising if we consider that we are batching many uncharges at once.
Quoted 3 lines
My guess is that it's related to new feature of Kirill's THP 'PageDoubleMap' so a THP page could be mapped a pte but !pmd_trans_huge(*pmd) so memcg precharge in move_charge should handle it?

Quoted 5 lines

I am not familiar with the current state of THP after the rework unfortunately. So if I got you right then you are saying that pmd_trans_huge_lock fails to notice a THP so we will not charge it as THP and only charge one head page and then the tear down path will correctly recognize it as a THP and uncharge the full size, right?

Exactly.

Michal Hocko

maintainer

2015-12-03 13:37 UTC #6 lore raw

Quoted 4 lines · Minchan Kim

Quoted 3 lines · Michal Hocko
Quoted 2 lines · Minchan Kim
Quoted 1 line · Minchan Kim
On Wed, Dec 02, 2015 at 11:16:43AM +0100, Michal Hocko wrote:

[...]

Quoted 16 lines

Quoted 16 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Also, how big is the underflow?
[...]
Quoted 6 lines
Quoted 6 lines
nr_pages 293 new -324
nr_pages 16 new -340
nr_pages 342 new -91
nr_pages 246 new -337
nr_pages 15 new -352
nr_pages 15 new -367
They are quite large but that is not that surprising if we consider that we are batching many uncharges at once.
Quoted 3 lines
My guess is that it's related to new feature of Kirill's THP 'PageDoubleMap' so a THP page could be mapped a pte but !pmd_trans_huge(*pmd) so memcg precharge in move_charge should handle it?

Quoted 7 lines

Quoted 5 lines
I am not familiar with the current state of THP after the rework unfortunately. So if I got you right then you are saying that pmd_trans_huge_lock fails to notice a THP so we will not charge it as THP and only charge one head page and then the tear down path will correctly recognize it as a THP and uncharge the full size, right?
Exactly.

Hmm, but are pages represented by those ptes on the LRU list? __split_huge_pmd_locked doesn't seem to do any lru care. If they are not on any LRU then mem_cgroup_move_charge_pte_range should ignore such a pte and the THP (which the pte is part of) should stay in the original memcg.

How do you trigger this issue btw?

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Michal Hocko

maintainer

2015-12-03 13:43 UTC #7 lore raw

Quoted 28 lines · Michal Hocko

Quoted 4 lines · Minchan Kim
Quoted 3 lines · Michal Hocko
Quoted 2 lines · Minchan Kim
Quoted 1 line · Minchan Kim
On Wed, Dec 02, 2015 at 11:16:43AM +0100, Michal Hocko wrote:
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Also, how big is the underflow?
[...]
Quoted 6 lines
Quoted 6 lines
nr_pages 293 new -324
nr_pages 16 new -340
nr_pages 342 new -91
nr_pages 246 new -337
nr_pages 15 new -352
nr_pages 15 new -367
They are quite large but that is not that surprising if we consider that we are batching many uncharges at once.
Quoted 3 lines
My guess is that it's related to new feature of Kirill's THP 'PageDoubleMap' so a THP page could be mapped a pte but !pmd_trans_huge(*pmd) so memcg precharge in move_charge should handle it?
I am not familiar with the current state of THP after the rework unfortunately. So if I got you right then you are saying that pmd_trans_huge_lock fails to notice a THP so we will not charge it as THP and only charge one head page and then the tear down path will correctly recognize it as a THP and uncharge the full size, right?

Quoted 7 lines

Quoted 1 line
Exactly.
Hmm, but are pages represented by those ptes on the LRU list? __split_huge_pmd_locked doesn't seem to do any lru care. If they are not on any LRU then mem_cgroup_move_charge_pte_range should ignore such a pte and the THP (which the pte is part of) should stay in the original memcg.

Ohh, PageLRU is
PAGEFLAG(LRU, lru, PF_HEAD)

So we are checking the head and it is on LRU. Now I can see how this might happen. Let me think about a fix...

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Michal Hocko

maintainer

2015-12-03 14:58 UTC #8 lore raw

Quoted 37 lines · Michal Hocko

Quoted 28 lines · Michal Hocko
Quoted 4 lines · Minchan Kim
Quoted 3 lines · Michal Hocko
Quoted 2 lines · Minchan Kim
Quoted 1 line · Minchan Kim
On Wed, Dec 02, 2015 at 11:16:43AM +0100, Michal Hocko wrote:
[...]
Quoted 22 lines
Quoted 21 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Also, how big is the underflow?
[...]
Quoted 6 lines
Quoted 6 lines
nr_pages 293 new -324
nr_pages 16 new -340
nr_pages 342 new -91
nr_pages 246 new -337
nr_pages 15 new -352
nr_pages 15 new -367
They are quite large but that is not that surprising if we consider that we are batching many uncharges at once.
Quoted 3 lines
My guess is that it's related to new feature of Kirill's THP 'PageDoubleMap' so a THP page could be mapped a pte but !pmd_trans_huge(*pmd) so memcg precharge in move_charge should handle it?
I am not familiar with the current state of THP after the rework unfortunately. So if I got you right then you are saying that pmd_trans_huge_lock fails to notice a THP so we will not charge it as THP and only charge one head page and then the tear down path will correctly recognize it as a THP and uncharge the full size, right?
Quoted 7 lines
Quoted 1 line
Exactly.
Hmm, but are pages represented by those ptes on the LRU list? __split_huge_pmd_locked doesn't seem to do any lru care. If they are not on any LRU then mem_cgroup_move_charge_pte_range should ignore such a pte and the THP (which the pte is part of) should stay in the original memcg.

Quoted 5 lines

Ohh, PageLRU is
PAGEFLAG(LRU, lru, PF_HEAD)
So we are checking the head and it is on LRU. Now I can see how this might happen. Let me think about a fix...

Perhaps something like the following? I wouldn't be surprised if it blown up magnificently. I am still pretty confused about all the consequences of the thp rework so there are some open questions. I also do not see isolate_lru_page can work properly. It doesn't seem to operate on the head page but who knows maybe there is some other trickery because this sounds like something to blow up from other places as well. So I must be missing something here.

Warning, this looks ugly as hell.
---

mm/memcontrol.c +43 ~~-20~~

index 79a29d564bff..f5c3af0b74b0 100644
@@ -4501,7 +4501,6 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
 /**
  * mem_cgroup_move_account - move account of the page
  * @page: the page
- * @nr_pages: number of regular pages (>1 for huge pages)
  * @from: mem_cgroup which the page is moved from.
  * @to:	mem_cgroup which the page is moved to. @from != @to.
  *

@@ -4509,30 +4508,43 @@ static struct page *mc_handle_file_pte(struct vm_area_struct *vma,
  *
  * This function doesn't do "charge" to new cgroup and doesn't do "uncharge"
  * from old cgroup.
+ *
+ * Returns the number of moved pages.
  */
-static int mem_cgroup_move_account(struct page *page,
-				   bool compound,
+static unsigned mem_cgroup_move_account(struct page *page,
 				   struct mem_cgroup *from,
 				   struct mem_cgroup *to)
 {
 	unsigned long flags;
-	unsigned int nr_pages = compound ? hpage_nr_pages(page) : 1;
-	int ret;
+	bool compound = PageCompound(page);
+	unsigned nr_pages = 1;
+	unsigned ret = 0;
 	bool anon;
 
 	VM_BUG_ON(from == to);
 	VM_BUG_ON_PAGE(PageLRU(page), page);
-	VM_BUG_ON(compound && !PageTransHuge(page));
+
+	if (compound) {
+		/*
+		 * We might see a split huge pmd and a tail page in a regular
+		 * pte. Make sure to work on the whole THP.
+		 *
+		 * TODO: what about pmd_trans_huge_lock for tail page mapped via
+		 * pte? That means we are already split up so we cannot race?
+		 * TODO: reference on the tail page will keep the head alive,
+		 * right?
+		 */
+		page = compound_head(page);
+		nr_pages = hpage_nr_pages(page);
+	}
 
 	/*
 	 * Prevent mem_cgroup_replace_page() from looking at
 	 * page->mem_cgroup of its source page while we change it.
 	 */
-	ret = -EBUSY;
 	if (!trylock_page(page))
 		goto out;
 
-	ret = -EINVAL;
 	if (page->mem_cgroup != from)
 		goto out_unlock;

@@ -4580,7 +4592,7 @@ static int mem_cgroup_move_account(struct page *page,
 	page->mem_cgroup = to;
 	spin_unlock_irqrestore(&from->move_lock, flags);
 
-	ret = 0;
+	ret = nr_pages;
 
 	local_irq_disable();
 	mem_cgroup_charge_statistics(to, page, compound, nr_pages);

@@ -4858,6 +4870,7 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
 	enum mc_target_type target_type;
 	union mc_target target;
 	struct page *page;
+	struct page *huge_page = NULL;
 
 	if (pmd_trans_huge_lock(pmd, vma, &ptl)) {
 		if (mc.precharge < HPAGE_PMD_NR) {

@@ -4868,11 +4881,12 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
 		if (target_type == MC_TARGET_PAGE) {
 			page = target.page;
 			if (!isolate_lru_page(page)) {
-				if (!mem_cgroup_move_account(page, true,
-							     mc.from, mc.to)) {
-					mc.precharge -= HPAGE_PMD_NR;
-					mc.moved_charge += HPAGE_PMD_NR;
-				}
+				unsigned moved;
+
+				moved = mem_cgroup_move_account(page,
+							     mc.from, mc.to);
+				mc.precharge -= moved;
+				mc.moved_charge += moved;
 				putback_lru_page(page);
 			}
 			put_page(page);

@@ -4895,15 +4909,24 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
 		switch (get_mctgt_type(vma, addr, ptent, &target)) {
 		case MC_TARGET_PAGE:
 			page = target.page;
-			if (isolate_lru_page(page))
+			/* We have already handled this THP */
+			if (compound_head(page) == huge_page)
 				goto put;
-			if (!mem_cgroup_move_account(page, false,
-						mc.from, mc.to)) {
-				mc.precharge--;
+
+			if (!isolate_lru_page(page)) {
+				unsigned move;
+
+				move = mem_cgroup_move_account(page,
+								mc.from, mc.to);
+				if (move > 1)
+					huge_page = compound_head(page);
+
+				mc.precharge -= move;
 				/* we uncharge from mc.from later. */
-				mc.moved_charge++;
+				mc.moved_charge += move;
+				/* LRU always operates on the head page */
+				putback_lru_page(compound_head(page));
 			}
-			putback_lru_page(page);
 put:			/* get_mctgt_type() gets the page */
 			put_page(page);
 			break;

mm/vmscan.c +4 -1

index 4589cfdbe405..14b65949b7d0 100644
@@ -1430,9 +1430,12 @@ int isolate_lru_page(struct page *page)
 	VM_BUG_ON_PAGE(!page_count(page), page);
 
 	if (PageLRU(page)) {
-		struct zone *zone = page_zone(page);
+		struct zone *zone;
 		struct lruvec *lruvec;
 
+		/* TODO: is this correct? */
+		page = compound_head(page);
+		zone = page_zone(page);
 		spin_lock_irq(&zone->lru_lock);
 		lruvec = mem_cgroup_page_lruvec(page, zone);
 		if (PageLRU(page)) {

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Michal Hocko

maintainer

2015-12-03 15:47 UTC #9 lore raw

On Thu 03-12-15 15:58:50, Michal Hocko wrote:
[....]

Quoted 1 line

Warning, this looks ugly as hell.

I was thinking about it some more and it seems that we should rather not bother with partial thp at all and keep it in the original memcg instead. It is way much less code and I do not think this will be too disruptive. Somebody should be holding the thp head, right?

Minchan, does this fix the issue you are seeing.
---

mm/memcontrol.c +8 -0

index 79a29d564bff..143c933f0b81 100644
@@ -4895,6 +4895,14 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
 		switch (get_mctgt_type(vma, addr, ptent, &target)) {
 		case MC_TARGET_PAGE:
 			page = target.page;
+			/*
+			 * We can have a part of the split pmd here. Moving it
+			 * can be done but it would be too convoluted so simply
+			 * ignore such a partial THP and keep it in original
+			 * memcg. There should be somebody mapping the head.
+			 */
+			if (PageCompound(page))
+				goto put;
 			if (isolate_lru_page(page))
 				goto put;
 			if (!mem_cgroup_move_account(page, false,

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Minchan Kim

author

2015-12-04 05:35 UTC #10 lore raw

Quoted 10 lines · Michal Hocko

On Thu 03-12-15 15:58:50, Michal Hocko wrote:
[....]
Quoted 1 line
Warning, this looks ugly as hell.
I was thinking about it some more and it seems that we should rather not bother with partial thp at all and keep it in the original memcg instead. It is way much less code and I do not think this will be too disruptive. Somebody should be holding the thp head, right?
Minchan, does this fix the issue you are seeing.

This patch solves the issue but not sure it's right approach. I think it could make regression that in old, we could charge a THP page but we can't now. Whether it's trivial or not, it depends on memcg guys.

Thanks.

Quoted 23 lines

---

mm/memcontrol.c +8 -0

index 79a29d564bff..143c933f0b81 100644
@@ -4895,6 +4895,14 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
 		switch (get_mctgt_type(vma, addr, ptent, &target)) {
 		case MC_TARGET_PAGE:
 			page = target.page;
+			/*
+			 * We can have a part of the split pmd here. Moving it
+			 * can be done but it would be too convoluted so simply
+			 * ignore such a partial THP and keep it in original
+			 * memcg. There should be somebody mapping the head.
+			 */
+			if (PageCompound(page))
+				goto put;
 			if (isolate_lru_page(page))
 				goto put;
 			if (!mem_cgroup_move_account(page, false,

Signature · 2 lines

Michal Hocko
SUSE Labs

Signature · 8 lines

Kind regards,
Minchan Kim

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

Michal Hocko

maintainer

2015-12-04 08:52 UTC #11 lore raw

Quoted 10 lines · Minchan Kim

Quoted 9 lines · Michal Hocko
On Thu 03-12-15 15:58:50, Michal Hocko wrote:
[....]
Quoted 1 line
Warning, this looks ugly as hell.
I was thinking about it some more and it seems that we should rather not bother with partial thp at all and keep it in the original memcg instead. It is way much less code and I do not think this will be too disruptive. Somebody should be holding the thp head, right?

Quoted 5 lines

Quoted 1 line
Minchan, does this fix the issue you are seeing.
This patch solves the issue but not sure it's right approach. I think it could make regression that in old, we could charge a THP page but we can't now.

The page would still get charged when allocated. It just wouldn't get moved when mapped only partially. IIUC there will be still somebody mapping the THP head via pmd, right? That process will move the page to the new memcg when moved. Or is it possible that we will end up only with pte mapped THP from all processes? Kirill?

If not then I think it is reasonable to expect that partially mapped THP is not moved during task migration. I will post an official patch after Kirill confirms my understanding.

Anyway thanks for the testing and pointing me to right direction Minchan!

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Minchan Kim

author

2015-12-04 09:16 UTC #12 lore raw

Quoted 13 lines · Michal Hocko

Quoted 12 lines · Minchan Kim
Quoted 10 lines · Michal Hocko
On Thu 03-12-15 15:58:50, Michal Hocko wrote:
[....]
Quoted 1 line
Warning, this looks ugly as hell.
I was thinking about it some more and it seems that we should rather not bother with partial thp at all and keep it in the original memcg instead. It is way much less code and I do not think this will be too disruptive. Somebody should be holding the thp head, right?
Minchan, does this fix the issue you are seeing.

Quoted 7 lines

Quoted 3 lines
This patch solves the issue but not sure it's right approach. I think it could make regression that in old, we could charge a THP page but we can't now.
The page would still get charged when allocated. It just wouldn't get moved when mapped only partially. IIUC there will be still somebody mapping the THP head via pmd, right? That process will move the page to

If I read code correctly, No. The split_huge_pmd splits just pmd, not page itself. IOW, it could be possible !pmd_trans_huge(pmd) && PageTransHuge although there is only process owns the page.

Quoted 2 lines

the new memcg when moved. Or is it possible that we will end up only with pte mapped THP from all processes? Kirill?

I'm not Kirill but I think it's possible.
If so, a thing we can use is page_mapcount(page) == 1. With that, it could gaurantee only a process owns the page so charge 512 instead of 1?

Quoted 7 lines

If not then I think it is reasonable to expect that partially mapped THP is not moved during task migration. I will post an official patch after Kirill confirms my understanding.
Anyway thanks for the testing and pointing me to right direction Minchan!

Thanks for the quick patch and feedback, Michal.

Quoted 4 lines

Signature · 3 lines
-- 
Michal Hocko
SUSE Labs

Signature · 8 lines

Kind regards,
Minchan Kim

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

Michal Hocko

maintainer

2015-12-04 09:58 UTC #13 lore raw

Quoted 18 lines · Minchan Kim

Quoted 17 lines · Michal Hocko
Quoted 15 lines · Minchan Kim
Quoted 10 lines · Michal Hocko
On Thu 03-12-15 15:58:50, Michal Hocko wrote:
[....]
Quoted 1 line
Warning, this looks ugly as hell.
I was thinking about it some more and it seems that we should rather not bother with partial thp at all and keep it in the original memcg instead. It is way much less code and I do not think this will be too disruptive. Somebody should be holding the thp head, right?
Minchan, does this fix the issue you are seeing.
This patch solves the issue but not sure it's right approach. I think it could make regression that in old, we could charge a THP page but we can't now.

Quoted 7 lines

Quoted 3 lines
The page would still get charged when allocated. It just wouldn't get moved when mapped only partially. IIUC there will be still somebody mapping the THP head via pmd, right? That process will move the page to
If I read code correctly, No. The split_huge_pmd splits just pmd, not page itself. IOW, it could be possible !pmd_trans_huge(pmd) && PageTransHuge although there is only process owns the page.

I am not sure I follow you. I thought there would still be other pmd which will hold the THP. Why should we keep the page as huge when all processes which map it have already split it up?

On the other hand it is true that the last process which maps the whole thp might have exited and leave others to map it partially.

Quoted 6 lines

Quoted 2 lines
the new memcg when moved. Or is it possible that we will end up only with pte mapped THP from all processes? Kirill?
I'm not Kirill but I think it's possible.
If so, a thing we can use is page_mapcount(page) == 1. With that, it could gaurantee only a process owns the page so charge 512 instead of 1?

Alright the exclusive holder should indeed move it. I will think how to simplify the previous patch (has it helped in your testing btw.?).

Signature · 8 lines

Michal Hocko
SUSE Labs

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

Minchan Kim

author

2015-12-04 13:35 UTC #14 lore raw

Quoted 23 lines · Michal Hocko

Quoted 22 lines · Minchan Kim
Quoted 20 lines · Michal Hocko
Quoted 10 lines · Minchan Kim
Quoted 9 lines · Michal Hocko
On Thu 03-12-15 15:58:50, Michal Hocko wrote:
[....]
Quoted 1 line
Warning, this looks ugly as hell.
I was thinking about it some more and it seems that we should rather not bother with partial thp at all and keep it in the original memcg instead. It is way much less code and I do not think this will be too disruptive. Somebody should be holding the thp head, right?
Quoted 5 lines
Quoted 1 line
Minchan, does this fix the issue you are seeing.
This patch solves the issue but not sure it's right approach. I think it could make regression that in old, we could charge a THP page but we can't now.
The page would still get charged when allocated. It just wouldn't get moved when mapped only partially. IIUC there will be still somebody mapping the THP head via pmd, right? That process will move the page to

Quoted 7 lines

Quoted 3 lines
If I read code correctly, No. The split_huge_pmd splits just pmd, not page itself. IOW, it could be possible !pmd_trans_huge(pmd) && PageTransHuge although there is only process owns the page.
I am not sure I follow you. I thought there would still be other pmd which will hold the THP. Why should we keep the page as huge when all processes which map it have already split it up?

I didn't follow Kirill's work but just read part of code to implement MADV_FREE so I just guess. (high-order-alloc-and-compaction/split/collapse) are costly operations so new work tried to avoid split page as far as possible. For example, if it works with splitting pmd, not THP page, it doesn't split the THP page where in mprotect path.
Even, it could do delay split-page via deferred _split_huge_page even if THP page is freed.

Quoted 7 lines

On the other hand it is true that the last process which maps the whole thp might have exited and leave others to map it partially.
Quoted 3 lines
Quoted 2 lines
the new memcg when moved. Or is it possible that we will end up only with pte mapped THP from all processes? Kirill?

Quoted 6 lines

Quoted 3 lines
I'm not Kirill but I think it's possible.
If so, a thing we can use is page_mapcount(page) == 1. With that, it could gaurantee only a process owns the page so charge 512 instead of 1?
Alright the exclusive holder should indeed move it. I will think how to simplify the previous patch (has it helped in your testing btw.?).

At least, your patch doesn't make the WARNING but I didn't check the accouting was right.

Thanks.

Quoted 4 lines

Signature · 3 lines
-- 
Michal Hocko
SUSE Labs

Signature · 8 lines

Kind regards,
Minchan Kim

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

Johannes Weiner

maintainer Acked-by

2015-12-04 16:01 UTC #15 lore raw

In reply to Michal Hocko

Quoted 9 lines · Michal Hocko

mm/memcontrol.c +1 -0

index 79a29d564bff..143c933f0b81 100644
@@ -4895,6 +4895,14 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
 		switch (get_mctgt_type(vma, addr, ptent, &target)) {
 		case MC_TARGET_PAGE:
 			page = target.page;
+			/*

mm/memcontrol.c static int mem_cgroup_move_charge_pte_range(pmd_t *pmd, · line 4908

Quoted 10 lines

mm/memcontrol.c +7 -0

+			 * We can have a part of the split pmd here. Moving it
+			 * can be done but it would be too convoluted so simply
+			 * ignore such a partial THP and keep it in original
+			 * memcg. There should be somebody mapping the head.
+			 */
+			if (PageCompound(page))
+				goto put;
 			if (isolate_lru_page(page))
 				goto put;
 			if (!mem_cgroup_move_account(page, false,

Acked-by Johannes Weiner <[email protected]>

The charge moving concept is fundamentally flawed and its implementation here is incomplete and races with reclaim.

Really, nobody should be using this. Absent any actual regression reports, a minimal fix to stop this code from generating warnings should be enough.