On 20.02.25 12:58, Balbir Singh wrote:
On 2/20/25 22:48, David Hildenbrand wrote:
On 20.02.25 00:13, Balbir Singh wrote:
page_pgmap() is referenced before checking if the page is a zone
device page and this triggers the warning in page_pgmap(). Refactor
the code to use the helper function after relevant checks.
Cc: Alistair Popple <apopple@xxxxxxxxxx>
Cc: Jason Gunthorpe <jgg@xxxxxxxx>
Cc: David Hildenbrand <david@xxxxxxxxxx>
Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
Signed-off-by: Balbir Singh <balbirs@xxxxxxxxxx>
---
Fixes: 7f1cfd71153b ("mm: allow compound zone device pages") on
mm-unstable
Is there actually something broken? At least for now, reading folio->pgmap should just work, although it might be garbage.
It triggers the VM_WARN_ON_ONCE_PAGE
static inline struct dev_pagemap *page_pgmap(const struct page *page)
{
VM_WARN_ON_ONCE_PAGE(!is_zone_device_page(page), page);
return page_folio(page)->pgmap;
}
Nothing is broken, because the code below has checks for is_device_coherent_page(),
but in general I think the WARN_ON is correct because it warns us against garbage
and it's propagation if the correct checks are not in place.
Ah! Now I read your "triggers the warning in page_pgmap()" in the
description.
It's usually a good idea to just include the splat you observed, if you
did, and call it "triggers the VM_WARN_ON_ONCE_PAGE".
The "Fixes:" should go above the "---" in that case.
mm/migrate_device.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/mm/migrate_device.c b/mm/migrate_device.c
index 6771893d4601..e0bf771edb6f 100644
--- a/mm/migrate_device.c
+++ b/mm/migrate_device.c
@@ -153,14 +153,17 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
goto next;
}
page = vm_normal_page(migrate->vma, addr, pte);
- pgmap = page_pgmap(page);
if (page && !is_zone_device_page(page) &&
!(migrate->flags & MIGRATE_VMA_SELECT_SYSTEM))
goto next;
- else if (page && is_device_coherent_page(page) &&
- (!(migrate->flags & MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
- pgmap->owner != migrate->pgmap_owner))
- goto next;
+ else if (page && is_device_coherent_page(page)) {
+ pgmap = page_pgmap(page);
+
+ if (!(migrate->flags &
+ MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
+ pgmap->owner != migrate->pgmap_owner)
+ goto next;
+ }
Coding style wants you to use
if () {
} else if {
}
Not
if ()
else if {
}
Ack, checkpatch.pl missed it, but agreed
Something simpler might be
page_pgmap(page)->owner != migrate->pgmap_owner
Yep, I had that and dropped it, the four clauses made it feel that it might
benefit from a split.
Right, the mixture of && and || is confusing.
So with the {}
Acked-by: David Hildenbrand <david@xxxxxxxxxx>
Thanks!
--
Cheers,
David / dhildenb
