----- Original Message -----
> 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.
It's true that crash should follow the upstream values, but in the past
there have been periods of times when the MAX_PHYSMEM_BITS and SECTION_SIZE_BITS
for different architectures changed upstream, but were not immediately updated
in
the crash utility. And that was because crash still worked OK because most
systems did not have large enough memory for the changes to cause things to
fail.
>
> > 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
So this goes to the question as to whether is_page_ptr() should return TRUE
if the incoming page address is accessible(), but it references a physical
address
that does not exist. With the current code, it verifies that it's a page
address,
and that the page address points to an actual physical memory page. I suggested
using accessible() on the page structure address, but that would not necessarily
be accurate because theoretically a vmemmap'd/instantiated page of page
structures
could contain page structure addresses that do not reference physical memory.
(e.g., if a hole started at a page address that's in the middle of a vmemmap'd
page of page structures)
So if we don't want to change the functionality of is_page_ptr(), then maybe
the binary search would be a suitable compromise for both accuracy and speed
on extremely large systems?
Dave
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
