Hi Kasuhito,
Just as a follow-up review of this part of your original patch:
+#ifdef VMEMMAP_VADDR
+ nr = nr_mem_sections;
+ if (machdep->flags & VMEMMAP)
+ nr = pfn_to_section_nr((addr - VMEMMAP_VADDR) / SIZE(page));
+ else if (readmem(addr + OFFSET(page_flags), KVADDR, &flags,
+ sizeof(ulong), "page.flags", RETURN_ON_ERROR|QUIET))
+ nr = (flags >> (SIZE(page_flags)*8 - SECTIONS_SHIFT())
+ & ((1UL << SECTIONS_SHIFT()) - 1));
+
+ if (nr < nr_mem_sections) {
+#else
for (nr = 0; nr < nr_mem_sections ; nr++) {
+#endif
One of my original concerns was associated with the backwards-compatiblity
of the non-VMEMMAP page->flags usage, primarily because it has changed
over the years. Perhaps the SECTIONS_SHIFT part has remained the same,
but depending upon its future stability in this function still worries me.
But more importantly, the VMEMMAP section of your patch fails on
architectures like ARM64 which have a phys_offset that needs to be
recognized w/respect to physical addresses. For example, here's an
ARM64 system whose physical addressing starts at 0x4000000000:
crash> help -m | grep phys_offset
phys_offset: 4000000000
crash> kmem -p | head
PAGE PHYSICAL MAPPING INDEX CNT FLAGS
ffff7e0000000000 4000000000 0 0 0 0
ffff7e0000000040 4000001000 0 0 0 0
ffff7e0000000080 4000002000 0 0 0 0
ffff7e00000000c0 4000003000 0 0 0 0
ffff7e0000000100 4000004000 0 0 0 0
ffff7e0000000140 4000005000 0 0 0 0
ffff7e0000000180 4000006000 0 0 0 0
ffff7e00000001c0 4000007000 0 0 0 0
ffff7e0000000200 4000008000 0 0 0 0
crash>
Your patch presumes that the pfn can calculated by using
(addr - VMEMMAP_VADDR) / SIZE(page), which does not take the
phys_offset into account. Therefore, with your patch, any call
to is_page_ptr() will fail:
crash> kmem -S dentry
CACHE NAME OBJSIZE ALLOCATED TOTAL SLABS SSIZE
kmem: page_to_nid: invalid page: ffff7e0004821500
kmem: dentry: cannot gather relevant slab data
ffff8003ed151200 dentry 304 ? ? ? 8k
crash> kmem -p | grep ffff7e0004821500
ffff7e0004821500 4120854000 0 0 1 3fffe000008100 slab,head
crash>
Perhaps the existing nr_mem_sections iteration loop could be transformed
into a binary search loop? Just a thought...
Thanks,
Dave
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
