2:41 p.m.
Hi Dave,
Thank you so much for your review!
On 2/21/2018 11:41 AM, Dave Anderson wrote:
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.
Yes, I understand the concern.
The SECTIONS_SHIFT part is the same as the function page_to_section() in
include/linux/mm.h. I thought that if the values related to SECTIONS_SHIFT
in kernel change, the crash utility will have to follow it regardless of
this patch, because they are used commonly in crash. But possible changes
could not be limited to such values.
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>
Oh, it's my big mistake. I had to limit it to the architectures on which
I could test it from the beginning... I'm thinking about a patch like below
which targets only on x86_64. But I'll learn more around that area.
Prototype:
diff --git a/defs.h b/defs.h
index aa17792..106f03e 100644
--- a/defs.h
+++ b/defs.h
@@ -3334,6 +3334,7 @@ struct arm64_stackframe {
#define PTOV(X) ((unsigned long)(X)+(machdep->kvbase))
#define VTOP(X) x86_64_VTOP((ulong)(X))
#define IS_VMALLOC_ADDR(X) x86_64_IS_VMALLOC_ADDR((ulong)(X))
+#define IS_VMEMMAP_PAGE(X) x86_64_IS_VMEMMAP_PAGE((ulong)(X))
/*
* the default page table level for x86_64:
@@ -5646,6 +5647,7 @@ void x86_64_dump_machdep_table(ulong);
ulong x86_64_PTOV(ulong);
ulong x86_64_VTOP(ulong);
int x86_64_IS_VMALLOC_ADDR(ulong);
+int x86_64_IS_VMEMMAP_PAGE(ulong);
void x86_64_display_idt_table(void);
#define display_idt_table() x86_64_display_idt_table()
long x86_64_exception_frame(ulong, ulong, char *, struct bt_info *, FILE *);
diff --git a/memory.c b/memory.c
index 0df8ecc..5d98ec0 100644
--- a/memory.c
+++ b/memory.c
@@ -13350,6 +13350,16 @@ is_page_ptr(ulong addr, physaddr_t *phys)
physaddr_t section_paddr;
if (IS_SPARSEMEM()) {
+#ifdef IS_VMEMMAP_PAGE
+ if (machdep->flags & VMEMMAP) {
+ if (IS_VMEMMAP_PAGE(addr)) {
+ if (phys)
+ *phys = PTOB((addr - VMEMMAP_VADDR) / SIZE(page));
+ return TRUE;
+ }
+ return FALSE;
+ }
+#endif
nr_mem_sections = NR_MEM_SECTIONS();
for (nr = 0; nr < nr_mem_sections ; nr++) {
if ((sec_addr = valid_section_nr(nr))) {
diff --git a/x86_64.c b/x86_64.c
index 7449571..b86d71f 100644
--- a/x86_64.c
+++ b/x86_64.c
@@ -1570,6 +1570,15 @@ x86_64_IS_VMALLOC_ADDR(ulong vaddr)
(vaddr >= VSYSCALL_START && vaddr < VSYSCALL_END));
}
+int
+x86_64_IS_VMEMMAP_PAGE(ulong vaddr)
+{
+ return ((machdep->flags & VMEMMAP) &&
+ (vaddr >= VMEMMAP_VADDR && vaddr < VMEMMAP_END) &&
+ !((vaddr - VMEMMAP_VADDR) % SIZE(page)) &&
+ accessible(vaddr));
+}
+
static int
x86_64_is_module_addr(ulong vaddr)
{
Perhaps the existing nr_mem_sections iteration loop could be
transformed
into a binary search loop? Just a thought...
That also sounds fine. I'll consider it for non-VMEMMAP after VMEMMAP.
Thanks,
Kazuhito Hagio
Thanks,
Dave
--
Crash-utility mailing list
Crash-utility(a)redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility