mm: Use a folio in the softleaf_is_device_private path
Subsystems (2)
PATCH v4 1/1 mm: Use a folio in the softleaf_is_device_private path
From: Hongfu Li <[email protected]>
Use the folio APIs in the device_private migration path of do_swap_page(), replacing four calls to compound_head() with one.
---
v4:
- Drop the do_non_swap_page() extraction (patch 2/2 from v2)
- Added Reviewed-by: Lorenzo Stoakes
- Link to v2: https://lore.kernel.org/all/[email protected]/
v3:
- Please ignore v3.
v2:
- Drop fault_folio, reuse the existing folio variable instead
- Refresh commit messages
- Update comment to use "folio" instead of "page" in the
device_private path
- Add Acked-by: David Hildenbrand
- Add patch 2/2 to extract do_non_swap_page() from do_swap_page()
- Link to v1: https://lore.kernel.org/all/[email protected]/
--- mm/memory.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
mm/memory.c +6 -5
@@ -4873,18 +4873,19 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) goto unlock;/* -* Get a page reference while we know the page can't be +* Get a folio reference while we know the folio can't be * freed. */ -if (trylock_page (vmf->page)) { + folio =page_folio (vmf->page); +if (folio_trylock (folio)) {struct dev_pagemap *pgmap; -get_page (vmf->page); +folio_get (folio);pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); -unlock_page (vmf->page); -put_page (vmf->page); +folio_unlock (folio); +folio_put (folio); }else {pte_unmap (vmf->pte);softleaf_entry_wait_on_locked (entry, vmf->ptl);
Signature · 1 line
2.54.0
Quoted 8 lines · Hongfu Li
From: Hongfu Li <[email protected]>
Use the folio APIs in the
device_privatemigration path ofdo_swap_page(), replacing four calls tocompound_head()with one.Signed-off-by Hongfu Li <[email protected]>Reviewed-by Lorenzo Stoakes (ARM) <[email protected]>
Quoted 51 lines
---
v4:
- Drop thedo_non_swap_page()extraction (patch 2/2 from v2)
- Added Reviewed-by: Lorenzo Stoakes
- Link to v2: https://lore.kernel.org/all/[email protected]/
v3:
- Please ignore v3.
v2:
- Dropfault_folio, reuse the existing folio variable instead
- Refresh commit messages
- Update comment to use "folio" instead of "page" in the
device_privatepath
- Add Acked-by: David Hildenbrand
- Add patch 2/2 to extractdo_non_swap_page()fromdo_swap_page()
- Link to v1: https://lore.kernel.org/all/[email protected]/--- mm/memory.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)mm/memory.c +6
-5@@ -4873,18 +4873,19 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) goto unlock;/* -* Get a page reference while we know the page can't be +* Get a folio reference while we know the folio can't be * freed. */ -if (trylock_page (vmf->page)) { + folio =page_folio (vmf->page); +if (folio_trylock (folio)) {struct dev_pagemap *pgmap; -get_page (vmf->page); +folio_get (folio);pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); -unlock_page (vmf->page); -put_page (vmf->page); +folio_unlock (folio); +folio_put (folio); }else {pte_unmap (vmf->pte);softleaf_entry_wait_on_locked (entry, vmf->ptl);Signature · 1 line
2.54.0
David Hildenbrand (Arm)
maintainerQuoted 46 lines · Hongfu Li
From: Hongfu Li <[email protected]>
Use the folio APIs in the
device_privatemigration path ofdo_swap_page(), replacing four calls tocompound_head()with one.Signed-off-by Hongfu Li <[email protected]>Reviewed-by Lorenzo Stoakes (ARM) <[email protected]>---
v4:
- Drop thedo_non_swap_page()extraction (patch 2/2 from v2)
- Added Reviewed-by: Lorenzo Stoakes
- Link to v2: https://lore.kernel.org/all/[email protected]/
v3:
- Please ignore v3.
v2:
- Dropfault_folio, reuse the existing folio variable instead
- Refresh commit messages
- Update comment to use "folio" instead of "page" in the
device_privatepath
- Add Acked-by: David Hildenbrand
- Add patch 2/2 to extractdo_non_swap_page()fromdo_swap_page()
- Link to v1: https://lore.kernel.org/all/[email protected]/--- mm/memory.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)mm/memory.c +4
-3@@ -4873,18 +4873,19 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) goto unlock;/* -* Get a page reference while we know the page can't be +* Get a folio reference while we know the folio can't be * freed. */ -if (trylock_page (vmf->page)) { + folio =page_folio (vmf->page); +if (folio_trylock (folio)) {struct dev_pagemap *pgmap; -get_page (vmf->page); +folio_get (folio);
mm/memory.c vm_fault_t do_swap_page(struct vm_fault *vmf) · line 4891
Quoted 10 lines
mm/memory.c +2
-2pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); -unlock_page (vmf->page); -put_page (vmf->page); +folio_unlock (folio); +folio_put (folio); }else {pte_unmap (vmf->pte);softleaf_entry_wait_on_locked (entry, vmf->ptl);
Okay, so this breaks the HMM selftests:
# ok 30 hmm.hmm_device_private.migrate_anon_huge_fault # # RUN hmm.hmm_device_private.migrate_partial_unmap_fault ... [ 14.589125] Internal error: Oops - BUG: 00000000f2000800 [#1] SMP [ 14.589178] Modules linked in: test_hmm [ 14.589203] CPU: 25 UID: 0 PID: 653 Comm: hmm-tests Not tainted 7.2.0-rc5+ #14 PREEMPT [ 14.589243] Hardware name: linux,dummy-virt (DT) [ 14.589268] pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--) [ 14.589300] pc : folio_unlock+0x78/0x88 [ 14.589326] lr : folio_unlock+0x78/0x88 [ 14.589349] sp : ffff80008284bb90 [ 14.589368] x29: ffff80008284bb90 x28: ffff0000cc8ce180 x27: 0000000000000000 [ 14.589406] x26: 0000000000000000 x25: ffff0000ca395dc0 x24: 0000000000000000 [ 14.589447] x23: 0000000020100073 x22: ffff0000c23d35c0 x21: ffffffffbfff8000 [ 14.589485] x20: ffffffffbfff8000 x19: ffff80008284bca0 x18: 0000000000000010 [ 14.589523] x17: 3030303030303030 x16: 2066666666666666 x15: 6630303030303030 [ 14.589562] x14: 0000000000000000 x13: 29296f696c6f6628 x12: 64656b636f6c5f74 [ 14.589603] x11: fffffffffffc7100 x10: fffffffffffc70b0 x9 : ffffb0504ff88f70 [ 14.589640] x8 : 3fffffffffffdfff x7 : ffff000c79b24bc0 x6 : 00000000000001a7 [ 14.589680] x5 : ffff000c79b24bc0 x4 : 0000000000000000 x3 : 0000000000000000 [ 14.589718] x2 : 0000000000000000 x1 : ffff0000cc8ce180 x0 : 000000000000003f [ 14.589756] Call trace: [ 14.589769] folio_unlock+0x78/0x88 (P) [ 14.589792] do_swap_page+0xf40/0x1580 [ 14.589815] __handle_mm_fault+0x648/0xe68 [ 14.589838] handle_mm_fault+0x9c/0x330 [ 14.589858] do_page_fault+0x268/0x7e0 [ 14.589882] do_translation_fault+0x5c/0x80 [ 14.589904] do_mem_abort+0x48/0xa0 [ 14.589927] el0_da+0x38/0xd0 [ 14.589948] el0t_64_sync_handler+0xd0/0xe8 [ 14.589970] el0t_64_sync+0x198/0x1a0 [ 14.589990] Code: d65f03c0 d000eec1 913c4021 94015993 (d4210000)
I can only speculate that we need another
folio = page_folio(vmf->page);
after the migrate_to_ram() call.
Let me try that real quick.
Signature · 3 lines
Cheers, David
David Hildenbrand (Arm)
maintainerQuoted 93 lines · David Hildenbrand (Arm)
Quoted 46 lines · Hongfu Li
From: Hongfu Li <[email protected]>
Use the folio APIs in the
device_privatemigration path ofdo_swap_page(), replacing four calls tocompound_head()with one.Signed-off-by Hongfu Li <[email protected]>Reviewed-by Lorenzo Stoakes (ARM) <[email protected]>---
v4:
- Drop thedo_non_swap_page()extraction (patch 2/2 from v2)
- Added Reviewed-by: Lorenzo Stoakes
- Link to v2: https://lore.kernel.org/all/[email protected]/
v3:
- Please ignore v3.
v2:
- Dropfault_folio, reuse the existing folio variable instead
- Refresh commit messages
- Update comment to use "folio" instead of "page" in the
device_privatepath
- Add Acked-by: David Hildenbrand
- Add patch 2/2 to extractdo_non_swap_page()fromdo_swap_page()
- Link to v1: https://lore.kernel.org/all/[email protected]/--- mm/memory.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)mm/memory.c +4
-3@@ -4873,18 +4873,19 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) goto unlock;/* -* Get a page reference while we know the page can't be +* Get a folio reference while we know the folio can't be * freed. */ -if (trylock_page (vmf->page)) { + folio =page_folio (vmf->page); +if (folio_trylock (folio)) {struct dev_pagemap *pgmap; -get_page (vmf->page); +folio_get (folio);mm/memory.c vm_fault_t do_swap_page(struct vm_fault *vmf) · line 4891
Quoted 10 lines
mm/memory.c +2
-2pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); -unlock_page (vmf->page); -put_page (vmf->page); +folio_unlock (folio); +folio_put (folio); }else {pte_unmap (vmf->pte);softleaf_entry_wait_on_locked (entry, vmf->ptl);Okay, so this breaks the HMM selftests:
# ok 30 hmm.hmm_device_private.migrate_anon_huge_fault # # RUN hmm.hmm_device_private.migrate_partial_unmap_fault ... [ 14.589125] Internal error: Oops - BUG: 00000000f2000800 [#1] SMP [ 14.589178] Modules linked in: test_hmm [ 14.589203] CPU: 25 UID: 0 PID: 653 Comm: hmm-tests Not tainted 7.2.0-rc5+ #14 PREEMPT [ 14.589243] Hardware name: linux,dummy-virt (DT) [ 14.589268] pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--) [ 14.589300] pc : folio_unlock+0x78/0x88 [ 14.589326] lr : folio_unlock+0x78/0x88 [ 14.589349] sp : ffff80008284bb90 [ 14.589368] x29: ffff80008284bb90 x28: ffff0000cc8ce180 x27: 0000000000000000 [ 14.589406] x26: 0000000000000000 x25: ffff0000ca395dc0 x24: 0000000000000000 [ 14.589447] x23: 0000000020100073 x22: ffff0000c23d35c0 x21: ffffffffbfff8000 [ 14.589485] x20: ffffffffbfff8000 x19: ffff80008284bca0 x18: 0000000000000010 [ 14.589523] x17: 3030303030303030 x16: 2066666666666666 x15: 6630303030303030 [ 14.589562] x14: 0000000000000000 x13: 29296f696c6f6628 x12: 64656b636f6c5f74 [ 14.589603] x11: fffffffffffc7100 x10: fffffffffffc70b0 x9 : ffffb0504ff88f70 [ 14.589640] x8 : 3fffffffffffdfff x7 : ffff000c79b24bc0 x6 : 00000000000001a7 [ 14.589680] x5 : ffff000c79b24bc0 x4 : 0000000000000000 x3 : 0000000000000000 [ 14.589718] x2 : 0000000000000000 x1 : ffff0000cc8ce180 x0 : 000000000000003f [ 14.589756] Call trace: [ 14.589769] folio_unlock+0x78/0x88 (P) [ 14.589792] do_swap_page+0xf40/0x1580 [ 14.589815] __handle_mm_fault+0x648/0xe68 [ 14.589838] handle_mm_fault+0x9c/0x330 [ 14.589858] do_page_fault+0x268/0x7e0 [ 14.589882] do_translation_fault+0x5c/0x80 [ 14.589904] do_mem_abort+0x48/0xa0 [ 14.589927] el0_da+0x38/0xd0 [ 14.589948] el0t_64_sync_handler+0xd0/0xe8 [ 14.589970] el0t_64_sync+0x198/0x1a0 [ 14.589990] Code: d65f03c0 d000eec1 913c4021 94015993 (d4210000)
Quoted 8 lines
I can only speculate that we need another
folio = page_folio(vmf->page);after the
migrate_to_ram()call.Let me try that real quick.
Yeah, that fixes it:
mm/memory.c +1 -0
@@ -4937,6 +4937,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); + folio =page_folio (vmf->page);folio_unlock (folio);folio_put (folio); }else {
Signature · 3 lines
Cheers, David
Lorenzo Stoakes (ARM)
reviewerQuoted 110 lines · David Hildenbrand (Arm)
Quoted 93 lines · David Hildenbrand (Arm)
Quoted 46 lines · Hongfu Li
From: Hongfu Li <[email protected]>
Use the folio APIs in the
device_privatemigration path ofdo_swap_page(), replacing four calls tocompound_head()with one.Signed-off-by Hongfu Li <[email protected]>Reviewed-by Lorenzo Stoakes (ARM) <[email protected]>---
v4:
- Drop thedo_non_swap_page()extraction (patch 2/2 from v2)
- Added Reviewed-by: Lorenzo Stoakes
- Link to v2: https://lore.kernel.org/all/[email protected]/
v3:
- Please ignore v3.
v2:
- Dropfault_folio, reuse the existing folio variable instead
- Refresh commit messages
- Update comment to use "folio" instead of "page" in the
device_privatepath
- Add Acked-by: David Hildenbrand
- Add patch 2/2 to extractdo_non_swap_page()fromdo_swap_page()
- Link to v1: https://lore.kernel.org/all/[email protected]/--- mm/memory.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)mm/memory.c +4
-3@@ -4873,18 +4873,19 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) goto unlock;/* -* Get a page reference while we know the page can't be +* Get a folio reference while we know the folio can't be * freed. */ -if (trylock_page (vmf->page)) { + folio =page_folio (vmf->page); +if (folio_trylock (folio)) {struct dev_pagemap *pgmap; -get_page (vmf->page); +folio_get (folio);mm/memory.c vm_fault_t do_swap_page(struct vm_fault *vmf) · line 4891
Quoted 10 lines
mm/memory.c +2
-2pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); -unlock_page (vmf->page); -put_page (vmf->page); +folio_unlock (folio); +folio_put (folio); }else {pte_unmap (vmf->pte);softleaf_entry_wait_on_locked (entry, vmf->ptl);Okay, so this breaks the HMM selftests:
# ok 30 hmm.hmm_device_private.migrate_anon_huge_fault # # RUN hmm.hmm_device_private.migrate_partial_unmap_fault ... [ 14.589125] Internal error: Oops - BUG: 00000000f2000800 [#1] SMP [ 14.589178] Modules linked in: test_hmm [ 14.589203] CPU: 25 UID: 0 PID: 653 Comm: hmm-tests Not tainted 7.2.0-rc5+ #14 PREEMPT [ 14.589243] Hardware name: linux,dummy-virt (DT) [ 14.589268] pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--) [ 14.589300] pc : folio_unlock+0x78/0x88 [ 14.589326] lr : folio_unlock+0x78/0x88 [ 14.589349] sp : ffff80008284bb90 [ 14.589368] x29: ffff80008284bb90 x28: ffff0000cc8ce180 x27: 0000000000000000 [ 14.589406] x26: 0000000000000000 x25: ffff0000ca395dc0 x24: 0000000000000000 [ 14.589447] x23: 0000000020100073 x22: ffff0000c23d35c0 x21: ffffffffbfff8000 [ 14.589485] x20: ffffffffbfff8000 x19: ffff80008284bca0 x18: 0000000000000010 [ 14.589523] x17: 3030303030303030 x16: 2066666666666666 x15: 6630303030303030 [ 14.589562] x14: 0000000000000000 x13: 29296f696c6f6628 x12: 64656b636f6c5f74 [ 14.589603] x11: fffffffffffc7100 x10: fffffffffffc70b0 x9 : ffffb0504ff88f70 [ 14.589640] x8 : 3fffffffffffdfff x7 : ffff000c79b24bc0 x6 : 00000000000001a7 [ 14.589680] x5 : ffff000c79b24bc0 x4 : 0000000000000000 x3 : 0000000000000000 [ 14.589718] x2 : 0000000000000000 x1 : ffff0000cc8ce180 x0 : 000000000000003f [ 14.589756] Call trace: [ 14.589769] folio_unlock+0x78/0x88 (P) [ 14.589792] do_swap_page+0xf40/0x1580 [ 14.589815] __handle_mm_fault+0x648/0xe68 [ 14.589838] handle_mm_fault+0x9c/0x330 [ 14.589858] do_page_fault+0x268/0x7e0 [ 14.589882] do_translation_fault+0x5c/0x80 [ 14.589904] do_mem_abort+0x48/0xa0 [ 14.589927] el0_da+0x38/0xd0 [ 14.589948] el0t_64_sync_handler+0xd0/0xe8 [ 14.589970] el0t_64_sync+0x198/0x1a0 [ 14.589990] Code: d65f03c0 d000eec1 913c4021 94015993 (d4210000)Quoted 8 lines
I can only speculate that we need another
folio = page_folio(vmf->page);after the
migrate_to_ram()call.Let me try that real quick.
Yeah, that fixes it:
mm/memory.c +0
-0
mm/memory.c vm_fault_t do_swap_page(struct vm_fault *vmf) · line 4940
Quoted 4 lines
mm/memory.c +1
-0@@ -4937,6 +4937,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); + folio =page_folio (vmf->page);
So after migrate_to_ram() the folio might have been split or otherwise somehow the page doesn't belong to the same locked, refcount-incremented folio it did before?
That's kinda a footgun... but this documents it at least.
I think a comment explaining how this can happen would be helpful though as this doesn't seem intuitive.
Quoted 9 lines
mm/memory.c +0
-1folio_unlock (folio);folio_put (folio); }else { --Cheers,
David
--
Cheers, Lorenzo
David Hildenbrand (Arm)
maintainerQuoted 49 lines · Lorenzo Stoakes (ARM)
Quoted 48 lines · David Hildenbrand (Arm)
Quoted 36 lines · David Hildenbrand (Arm)
Okay, so this breaks the HMM selftests:
# ok 30 hmm.hmm_device_private.migrate_anon_huge_fault # # RUN hmm.hmm_device_private.migrate_partial_unmap_fault ... [ 14.589125] Internal error: Oops - BUG: 00000000f2000800 [#1] SMP [ 14.589178] Modules linked in: test_hmm [ 14.589203] CPU: 25 UID: 0 PID: 653 Comm: hmm-tests Not tainted 7.2.0-rc5+ #14 PREEMPT [ 14.589243] Hardware name: linux,dummy-virt (DT) [ 14.589268] pstate: 63400005 (nZCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--) [ 14.589300] pc : folio_unlock+0x78/0x88 [ 14.589326] lr : folio_unlock+0x78/0x88 [ 14.589349] sp : ffff80008284bb90 [ 14.589368] x29: ffff80008284bb90 x28: ffff0000cc8ce180 x27: 0000000000000000 [ 14.589406] x26: 0000000000000000 x25: ffff0000ca395dc0 x24: 0000000000000000 [ 14.589447] x23: 0000000020100073 x22: ffff0000c23d35c0 x21: ffffffffbfff8000 [ 14.589485] x20: ffffffffbfff8000 x19: ffff80008284bca0 x18: 0000000000000010 [ 14.589523] x17: 3030303030303030 x16: 2066666666666666 x15: 6630303030303030 [ 14.589562] x14: 0000000000000000 x13: 29296f696c6f6628 x12: 64656b636f6c5f74 [ 14.589603] x11: fffffffffffc7100 x10: fffffffffffc70b0 x9 : ffffb0504ff88f70 [ 14.589640] x8 : 3fffffffffffdfff x7 : ffff000c79b24bc0 x6 : 00000000000001a7 [ 14.589680] x5 : ffff000c79b24bc0 x4 : 0000000000000000 x3 : 0000000000000000 [ 14.589718] x2 : 0000000000000000 x1 : ffff0000cc8ce180 x0 : 000000000000003f [ 14.589756] Call trace: [ 14.589769] folio_unlock+0x78/0x88 (P) [ 14.589792] do_swap_page+0xf40/0x1580 [ 14.589815] __handle_mm_fault+0x648/0xe68 [ 14.589838] handle_mm_fault+0x9c/0x330 [ 14.589858] do_page_fault+0x268/0x7e0 [ 14.589882] do_translation_fault+0x5c/0x80 [ 14.589904] do_mem_abort+0x48/0xa0 [ 14.589927] el0_da+0x38/0xd0 [ 14.589948] el0t_64_sync_handler+0xd0/0xe8 [ 14.589970] el0t_64_sync+0x198/0x1a0 [ 14.589990] Code: d65f03c0 d000eec1 913c4021 94015993 (d4210000)Quoted 8 lines
I can only speculate that we need another
folio = page_folio(vmf->page);after the
migrate_to_ram()call.Let me try that real quick.
Yeah, that fixes it:
Quoted 13 lines
mm/memory.c vm_fault_t do_swap_page(struct vm_fault *vmf) · line 4940
Quoted 9 lines
mm/memory.c +1
-0@@ -4937,6 +4937,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); + folio =page_folio (vmf->page);So after
migrate_to_ram()the folio might have been split or otherwise somehow the page doesn't belong to the same locked, refcount-incremented folio it did before?
I suspect a split.
Quoted 5 lines
That's kinda a footgun... but this documents it at least.
I think a comment explaining how this can happen would be helpful though as this doesn't seem intuitive.
I think, conceptually, calling into something that consumes a page (vmf->page) always needs care when operating on folios.
Passing the vmf to some callback might be the odd thing here, because the vmf->page contract is not really clear.
Signature · 3 lines
Cheers, David
Quoted 9 lines
Quoted 3 lines
So after
migrate_to_ram()the folio might have been split or otherwise somehow the page doesn't belong to the same locked, refcount-incremented folio it did before?I suspect a split.
Quoted 3 lines
That's kinda a footgun... but this documents it at least.
Quoted 8 lines
Quoted 2 lines
I think a comment explaining how this can happen would be helpful though as this doesn't seem intuitive.
I think, conceptually, calling into something that consumes a page (
vmf->page) always needs care when operating on folios.Passing the vmf to some callback might be the odd thing here, because the
vmf->pagecontract is not really clear.
I'm very sorry for introducing this regression.
Thank you for identifying and testing this issue.
I will double-check this code path and apply your fix to run further tests locally.
Would it make sense to add a comment here like the below, to make this subtle behavior clearer for future readers?
mm/memory.c +6 -0
@@ -4937,6 +4937,12 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); +/* +* migrate_to_ram() can split a large folio, updating +* vmf->page to a different folio. Re-fetch folio for +* correct unlock/put. +*/ + folio =page_folio (vmf->page);folio_unlock (folio);folio_put (folio); }else {
David Hildenbrand (Arm)
maintainerQuoted 32 lines · Hongfu Li
Quoted 9 lines
Quoted 3 lines
So after
migrate_to_ram()the folio might have been split or otherwise somehow the page doesn't belong to the same locked, refcount-incremented folio it did before?I suspect a split.
Quoted 3 lines
That's kinda a footgun... but this documents it at least.
Quoted 8 lines
Quoted 2 lines
I think a comment explaining how this can happen would be helpful though as this doesn't seem intuitive.
I think, conceptually, calling into something that consumes a page (
vmf->page) always needs care when operating on folios.Passing the vmf to some callback might be the odd thing here, because the
vmf->pagecontract is not really clear.I'm very sorry for introducing this regression.
Thank you for identifying and testing this issue.I will double-check this code path and apply your fix to run further tests locally.
Would it make sense to add a comment here like the below, to make this subtle behavior clearer for future readers?
mm/memory.c +0
-0
mm/memory.c vm_fault_t do_swap_page(struct vm_fault *vmf) · line 4944
Quoted 8 lines
mm/memory.c +5
-0@@ -4937,6 +4937,12 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); +/* +* migrate_to_ram() can split a large folio, updating +* vmf->page to a different folio. Re-fetch folio for +* correct unlock/put. +*/
"migrate_to_ram() might have split the folio."
Should be sufficient I guess.
Signature · 3 lines
Cheers, David
Lorenzo Stoakes (ARM)
reviewerQuoted 44 lines · David Hildenbrand (Arm)
Quoted 32 lines · Hongfu Li
Quoted 9 lines
Quoted 3 lines
So after
migrate_to_ram()the folio might have been split or otherwise somehow the page doesn't belong to the same locked, refcount-incremented folio it did before?I suspect a split.
Quoted 3 lines
That's kinda a footgun... but this documents it at least.
Quoted 8 lines
Quoted 2 lines
I think a comment explaining how this can happen would be helpful though as this doesn't seem intuitive.
I think, conceptually, calling into something that consumes a page (
vmf->page) always needs care when operating on folios.Passing the vmf to some callback might be the odd thing here, because the
vmf->pagecontract is not really clear.I'm very sorry for introducing this regression.
Thank you for identifying and testing this issue.I will double-check this code path and apply your fix to run further tests locally.
Would it make sense to add a comment here like the below, to make this subtle behavior clearer for future readers?
mm/memory.c +0
-0mm/memory.c vm_fault_t do_swap_page(struct vm_fault *vmf) · line 4944
Quoted 8 lines
mm/memory.c +5
-0@@ -4937,6 +4937,12 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); +/* +* migrate_to_ram() can split a large folio, updating +* vmf->page to a different folio. Re-fetch folio for +* correct unlock/put. +*/ "
migrate_to_ram()might have split the folio."
Quoted 1 line
Should be sufficient I guess.
Agreed, no need for a schloppy long comment here!
/* migrate_to_ram() might have split the folio. */ folio = page_folio(vmf->page);
Should be fine.
The commit message should be updated to reflect it too and mention that we are going from 4 -> 2 compound_head()'s now.
Quoted 5 lines
--
Cheers,David
--
Cheers, Lorenzo
Quoted 43 lines · Lorenzo Stoakes (ARM)
Quoted 39 lines · David Hildenbrand (Arm)
Quoted 28 lines · Hongfu Li
Quoted 7 lines
Quoted 3 lines
So after
migrate_to_ram()the folio might have been split or otherwise somehow the page doesn't belong to the same locked, refcount-incremented folio it did before?I suspect a split.
Quoted 2 lines
That's kinda a footgun... but this documents it at least.
Quoted 7 lines
Quoted 2 lines
I think a comment explaining how this can happen would be helpful though as this doesn't seem intuitive.
I think, conceptually, calling into something that consumes a page (
vmf->page) always needs care when operating on folios.Passing the vmf to some callback might be the odd thing here, because the
vmf->pagecontract is not really clear.I'm very sorry for introducing this regression.
Thank you for identifying and testing this issue.I will double-check this code path and apply your fix to run further tests locally.
Would it make sense to add a comment here like the below, to make this subtle behavior clearer for future readers?
mm/memory.c +0
-0mm/memory.c vm_fault_t do_swap_page(struct vm_fault *vmf) · line 4944
Quoted 8 lines
mm/memory.c +5
-0@@ -4937,6 +4937,12 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)pte_unmap_unlock (vmf->pte, vmf->ptl); pgmap =page_pgmap (vmf->page); ret = pgmap->ops->migrate_to_ram (vmf); +/* +* migrate_to_ram() can split a large folio, updating +* vmf->page to a different folio. Re-fetch folio for +* correct unlock/put. +*/ "
migrate_to_ram()might have split the folio."Quoted 1 line
Should be sufficient I guess.
Agreed, no need for a schloppy long comment here!
Quoted 7 lines
/* migrate_to_ram() might have split the folio. */ folio = page_folio(vmf->page);Should be fine.
The commit message should be updated to reflect it too and mention that we are going from 4 -> 2 compound_head()'s now.
Thanks a lot for the suggestion. A shorter comment is indeed better.
Will use your suggested one‑line comment, add the folio re‑fetch, and update commit message for v5.
Quoted 6 lines
Quoted 4 lines
--
Cheers,David
--
Cheers, Lorenzo
Signature · 2 lines
Best regards, Hongfu