Hi, The "kmem -[sS]" commands can take several minutes to complete with the following conditions: * The system has a lot of memory sections with CONFIG_SPARSEMEM. * The kernel uses SLUB and it has a very long partial slab list. crash> kmem -s dentry | awk '{print strftime("%T"), $0}' 10:18:34 CACHE NAME OBJSIZE ALLOCATED TOTAL SLABS SSIZE 10:19:41 ffff88017fc78a00 dentry 192 9038949 10045728 239184 8k crash> kmem -S dentry | bash -c 'cat >/dev/null ; echo $SECONDS' 334 One of the causes is that is_page_ptr() in count_partial() checks if a given slub page address is a page struct by searching all memory sections linearly for the one which includes it. nr_mem_sections = NR_MEM_SECTIONS(); for (nr = 0; nr < nr_mem_sections ; nr++) { if ((sec_addr = valid_section_nr(nr))) { ... With CONFIG_SPARSEMEM{_VMEMMAP}, we can calculate the memory section which includes a page struct with its page.flags, or its address and VMEMMAP_VADDR. With this patch doing so, the computation amount can be significantly reduced in that case. crash> kmem -s dentry | awk '{print strftime("%T"), $0}' 10:34:55 CACHE NAME OBJSIZE ALLOCATED TOTAL SLABS SSIZE 10:34:55 ffff88017fc78a00 dentry 192 9038949 10045728 239184 8k crash> kmem -S dentry | bash -c 'cat >/dev/null ; echo $SECONDS' 2 This patch uses VMEMMAP_VADDR. It is not defined on PPC64, but it looks like PPC64 supports VMEMMAP flag and machdep->machspec->vmemmap_base is it, so this patch also defines it for PPC64. This might need some help from PPC folks. Signed-off-by: Kazuhito Hagio <k-hagio(a)ab.jp.nec.com&gt; --- defs.h | 2 ++ memory.c | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/defs.h b/defs.h index aa17792..84e68ca 100644 --- a/defs.h +++ b/defs.h @@ -3861,6 +3861,8 @@ struct efi_memory_desc_t { #define IS_VMALLOC_ADDR(X) machdep->machspec->is_vmaddr(X) #define KERNELBASE machdep->pageoffset +#define VMEMMAP_VADDR (machdep->machspec->vmemmap_base) + #define PGDIR_SHIFT (machdep->pageshift + (machdep->pageshift -3) + (machdep->pageshift - 2)) #define PMD_SHIFT (machdep->pageshift + (machdep->pageshift - 3)) diff --git a/memory.c b/memory.c index 0df8ecc..0696763 100644 --- a/memory.c +++ b/memory.c @@ -13348,10 +13348,25 @@ is_page_ptr(ulong addr, physaddr_t *phys) ulong nr_mem_sections; ulong coded_mem_map, mem_map, end_mem_map; physaddr_t section_paddr; +#ifdef VMEMMAP_VADDR + ulong flags; +#endif if (IS_SPARSEMEM()) { nr_mem_sections = NR_MEM_SECTIONS(); +#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 if ((sec_addr = valid_section_nr(nr))) { coded_mem_map = section_mem_map_addr(sec_addr); mem_map = sparse_decode_mem_map(coded_mem_map, nr); -- 1.8.3.1 -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility

Hi Dave, Thank you for your comment! On 2/13/2018 2:33 PM, Dave Anderson wrote: > > Hi Kasuhito, > > I am planning on releasing crash-7.2.1 today, so this will have to be deferred > to 7.2.2. > > Because of your questions about ppc64, possible backwards-compatibility issues, > or potential future changes to page.flags usage, this permanent change to the > is_page_ptr() function solely for the purposes of SLUB's count_partial() function > makes me nervous. Sorry, my explanation was not enough. Please let me supplement a little. As far as I know, the count_partial() is the function which could receive the effect of this patch most. But is_page_ptr() could be called many times also through the other functions, so this will improve them, too. Moreover, the is_page_ptr()'s loop could loop NR_MEM_SECTIONS() times, which is 33554432 on x86_64(5level) if I understand correctly, in the worst case. So I thought that ideally we should improve the is_page_ptr() itself if possible, rather than find the callers which could call it many times and change them. Also, I am willing to drop the definition of VMEMMAP_VADDR for ppc64 this time.

> > There is really no compelling reason that count_partial() absolutely *must* use > is_page_ptr(), and so I'm thinking that perhaps you could come up with a less > heavy-handed method for simply testing whether a page.lru entry points to another > vmemmap'd page. Something along the lines of adding this for > vmemmap-enabled kernels: > > #define IN_VMEMMAP_RANGE(page) ((page >= VMEMMAP_VADDR) && (page <= > VMEMMAP_END)) > > and then have count_partial() replace the is_page_ptr() call with another > slub function that does something like this for vmemmap-enabled kernels: > > (IN_VMMEMAP_RANGE(next) && accessible(next)) > > Or instead of accessible(), it could read "next" as a list_head with RETURN_ON_ERROR, > and verify that next->prev points back to the current list_head. > > Non-vmemmap-enabled kernels could still use is_page_ptr(). > > What do you think of doing something like that? Given possible compatibility issues you said, I think that the way you suggested might well be enough for now. I'll try a method like the above. Thanks, Kazuhito Hagio > > Dave > > > > > ----- Original Message ----- >> Hi, >> >> The "kmem -[sS]" commands can take several minutes to complete with >> the following conditions: >> * The system has a lot of memory sections with CONFIG_SPARSEMEM. >> * The kernel uses SLUB and it has a very long partial slab list. >> >> crash> kmem -s dentry | awk '{print strftime("%T"), $0}' >> 10:18:34 CACHE NAME OBJSIZE ALLOCATED >> TOTAL >> SLABS SSIZE >> 10:19:41 ffff88017fc78a00 dentry 192 9038949 >> 10045728 >> 239184 8k >> crash> kmem -S dentry | bash -c 'cat >/dev/null ; echo $SECONDS' >> 334 >> >> One of the causes is that is_page_ptr() in count_partial() checks if >> a given slub page address is a page struct by searching all memory >> sections linearly for the one which includes it. >> >> nr_mem_sections = NR_MEM_SECTIONS(); >> for (nr = 0; nr < nr_mem_sections ; nr++) { >> if ((sec_addr = valid_section_nr(nr))) { >> ... >> >> With CONFIG_SPARSEMEM{_VMEMMAP}, we can calculate the memory section >> which includes a page struct with its page.flags, or its address and >> VMEMMAP_VADDR. With this patch doing so, the computation amount can be >> significantly reduced in that case. >> >> crash> kmem -s dentry | awk '{print strftime("%T"), $0}' >> 10:34:55 CACHE NAME OBJSIZE ALLOCATED >> TOTAL >> SLABS SSIZE >> 10:34:55 ffff88017fc78a00 dentry 192 9038949 >> 10045728 >> 239184 8k >> crash> kmem -S dentry | bash -c 'cat >/dev/null ; echo $SECONDS' >> 2 >> >> This patch uses VMEMMAP_VADDR. It is not defined on PPC64, but it looks >> like PPC64 supports VMEMMAP flag and machdep->machspec->vmemmap_base is >> it, so this patch also defines it for PPC64. This might need some help >> from PPC folks. >> >> Signed-off-by: Kazuhito Hagio <k-hagio(a)ab.jp.nec.com&gt; >> --- >> defs.h | 2 ++ >> memory.c | 15 +++++++++++++++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/defs.h b/defs.h >> index aa17792..84e68ca 100644 >> --- a/defs.h >> +++ b/defs.h >> @@ -3861,6 +3861,8 @@ struct efi_memory_desc_t { >> #define IS_VMALLOC_ADDR(X) machdep->machspec->is_vmaddr(X) >> #define KERNELBASE machdep->pageoffset >> >> +#define VMEMMAP_VADDR (machdep->machspec->vmemmap_base) >> + >> #define PGDIR_SHIFT (machdep->pageshift + (machdep->pageshift -3) + >> (machdep->pageshift - 2)) >> #define PMD_SHIFT (machdep->pageshift + (machdep->pageshift - 3)) >> >> diff --git a/memory.c b/memory.c >> index 0df8ecc..0696763 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -13348,10 +13348,25 @@ is_page_ptr(ulong addr, physaddr_t *phys) >> ulong nr_mem_sections; >> ulong coded_mem_map, mem_map, end_mem_map; >> physaddr_t section_paddr; >> +#ifdef VMEMMAP_VADDR >> + ulong flags; >> +#endif >> >> if (IS_SPARSEMEM()) { >> nr_mem_sections = NR_MEM_SECTIONS(); >> +#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 >> if ((sec_addr = valid_section_nr(nr))) { >> coded_mem_map = section_mem_map_addr(sec_addr); >> mem_map = sparse_decode_mem_map(coded_mem_map, >> nr); >> -- >> 1.8.3.1 >> >> -- >> Crash-utility mailing list >> Crash-utility(a)redhat.com >> https://www.redhat.com/mailman/listinfo/crash-utility >> > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility > -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility

>>> Because of your questions about ppc64, possible backwards-compatibility issues, >>> or potential future changes to page.flags usage, this permanent change to the >>> is_page_ptr() function solely for the purposes of SLUB's count_partial() function >>> makes me nervous. >> >> Sorry, my explanation was not enough. Please let me supplement a little. >> As far as I know, the count_partial() is the function which could receive the >> effect of this patch most. But is_page_ptr() could be called many times also >> through the other functions, so this will improve them, too. Moreover, the >> is_page_ptr()'s loop could loop NR_MEM_SECTIONS() times, which is 33554432 >> on x86_64(5level) if I understand correctly, in the worst case. >> >> So I thought that ideally we should improve the is_page_ptr() itself if possible, >> rather than find the callers which could call it many times and change them. >> Also, I am willing to drop the definition of VMEMMAP_VADDR for ppc64 this >> time. > > OK, I understand your point. But what concerns me is that the function's > purpose is to absolutely identify whether the incoming page structure address > is a correct page structure address. But if an invalid address gets passed > into is_page_ptr(), your patch would take the invalid address, calculate an > invalid "nr", and continue from there, right?

Another suggestion/question -- if is_page_ptr() is called with a NULL phys argument (as is done most of the time), could it skip the "if IS_SPARSEMEM()" section at the top, and still utilize the part at the bottom, where it walks through the vt->node_table[x] array? I'm not sure about the "ppend" calculation though -- even if there are holes in the node's address space, is it still a contiguous chunk of page structure addresses per-node?

> > Dave > >> >>> >>> There is really no compelling reason that count_partial() absolutely >>> *must* use >>> is_page_ptr(), and so I'm thinking that perhaps you could come up with a >>> less >>> heavy-handed method for simply testing whether a page.lru entry points to >>> another >>> vmemmap'd page. Something along the lines of adding this for >>> vmemmap-enabled kernels: >>> >>> #define IN_VMEMMAP_RANGE(page) ((page >= VMEMMAP_VADDR) && (page <= >>> VMEMMAP_END)) >>> >>> and then have count_partial() replace the is_page_ptr() call with another >>> slub function that does something like this for vmemmap-enabled kernels: >>> >>> (IN_VMMEMAP_RANGE(next) && accessible(next)) >>> >>> Or instead of accessible(), it could read "next" as a list_head with >>> RETURN_ON_ERROR, >>> and verify that next->prev points back to the current list_head. >>> >>> Non-vmemmap-enabled kernels could still use is_page_ptr(). >>> >>> What do you think of doing something like that? >> >> Given possible compatibility issues you said, I think that the way you >> suggested >> might well be enough for now. I'll try a method like the above. >> >> Thanks, >> Kazuhito Hagio >> >>> >>> Dave >>> >>> >>> >>> >>> ----- Original Message ----- >>>> Hi, >>>> >>>> The "kmem -[sS]" commands can take several minutes to complete with >>>> the following conditions: >>>> * The system has a lot of memory sections with CONFIG_SPARSEMEM. >>>> * The kernel uses SLUB and it has a very long partial slab list. >>>> >>>> crash> kmem -s dentry | awk '{print strftime("%T"), $0}' >>>> 10:18:34 CACHE NAME OBJSIZE ALLOCATED >>>> TOTAL >>>> SLABS SSIZE >>>> 10:19:41 ffff88017fc78a00 dentry 192 9038949 >>>> 10045728 >>>> 239184 8k >>>> crash> kmem -S dentry | bash -c 'cat >/dev/null ; echo $SECONDS' >>>> 334 >>>> >>>> One of the causes is that is_page_ptr() in count_partial() checks if >>>> a given slub page address is a page struct by searching all memory >>>> sections linearly for the one which includes it. >>>> >>>> nr_mem_sections = NR_MEM_SECTIONS(); >>>> for (nr = 0; nr < nr_mem_sections ; nr++) { >>>> if ((sec_addr = valid_section_nr(nr))) { >>>> ... >>>> >>>> With CONFIG_SPARSEMEM{_VMEMMAP}, we can calculate the memory section >>>> which includes a page struct with its page.flags, or its address and >>>> VMEMMAP_VADDR. With this patch doing so, the computation amount can be >>>> significantly reduced in that case. >>>> >>>> crash> kmem -s dentry | awk '{print strftime("%T"), $0}' >>>> 10:34:55 CACHE NAME OBJSIZE ALLOCATED >>>> TOTAL >>>> SLABS SSIZE >>>> 10:34:55 ffff88017fc78a00 dentry 192 9038949 >>>> 10045728 >>>> 239184 8k >>>> crash> kmem -S dentry | bash -c 'cat >/dev/null ; echo $SECONDS' >>>> 2 >>>> >>>> This patch uses VMEMMAP_VADDR. It is not defined on PPC64, but it looks >>>> like PPC64 supports VMEMMAP flag and machdep->machspec->vmemmap_base is >>>> it, so this patch also defines it for PPC64. This might need some help >>>> from PPC folks. >>>> >>>> Signed-off-by: Kazuhito Hagio <k-hagio(a)ab.jp.nec.com&gt; >>>> --- >>>> defs.h | 2 ++ >>>> memory.c | 15 +++++++++++++++ >>>> 2 files changed, 17 insertions(+) >>>> >>>> diff --git a/defs.h b/defs.h >>>> index aa17792..84e68ca 100644 >>>> --- a/defs.h >>>> +++ b/defs.h >>>> @@ -3861,6 +3861,8 @@ struct efi_memory_desc_t { >>>> #define IS_VMALLOC_ADDR(X) machdep->machspec->is_vmaddr(X) >>>> #define KERNELBASE machdep->pageoffset >>>> >>>> +#define VMEMMAP_VADDR (machdep->machspec->vmemmap_base) >>>> + >>>> #define PGDIR_SHIFT (machdep->pageshift + (machdep->pageshift -3) + >>>> (machdep->pageshift - 2)) >>>> #define PMD_SHIFT (machdep->pageshift + (machdep->pageshift - 3)) >>>> >>>> diff --git a/memory.c b/memory.c >>>> index 0df8ecc..0696763 100644 >>>> --- a/memory.c >>>> +++ b/memory.c >>>> @@ -13348,10 +13348,25 @@ is_page_ptr(ulong addr, physaddr_t *phys) >>>> ulong nr_mem_sections; >>>> ulong coded_mem_map, mem_map, end_mem_map; >>>> physaddr_t section_paddr; >>>> +#ifdef VMEMMAP_VADDR >>>> + ulong flags; >>>> +#endif >>>> >>>> if (IS_SPARSEMEM()) { >>>> nr_mem_sections = NR_MEM_SECTIONS(); >>>> +#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 >>>> if ((sec_addr = valid_section_nr(nr))) { >>>> coded_mem_map = >>>> section_mem_map_addr(sec_addr); >>>> mem_map = sparse_decode_mem_map(coded_mem_map, >>>> nr); >>>> -- >>>> 1.8.3.1 >>>> >>>> -- >>>> Crash-utility mailing list >>>> Crash-utility(a)redhat.com >>>> https://www.redhat.com/mailman/listinfo/crash-utility >>>> >>> >>> -- >>> Crash-utility mailing list >>> Crash-utility(a)redhat.com >>> https://www.redhat.com/mailman/listinfo/crash-utility >>> >> >> -- >> Crash-utility mailing list >> Crash-utility(a)redhat.com >> https://www.redhat.com/mailman/listinfo/crash-utility >> > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility > -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility

>>> OK, I understand your point. But what concerns me is that the function's >>> purpose is to absolutely identify whether the incoming page structure >>> address >>> is a correct page structure address. But if an invalid address gets >>> passed >>> into is_page_ptr(), your patch would take the invalid address, calculate >>> an >>> invalid "nr", and continue from there, right? > > Yes, if an invalid "nr" is the number where section does not exist, > valid_section_nr() would return 0. Even if it is the number where section > exists by accident, the invalid "addr" is not between mem_map and > end_mem_map, > or not page-aligned, because if so, it is a page structure address. > > Also without this patch, when an invalid address comes, the loop could tries > many invalid "nr"s less than NR_MEM_SECTIONS(). > > I hope this answers your concern.. > >> >> Another suggestion/question -- if is_page_ptr() is called with a NULL phys >> argument (as is done most of the time), could it skip the "if >> IS_SPARSEMEM()" >> section at the top, and still utilize the part at the bottom, where it >> walks >> through the vt->node_table[x] array? I'm not sure about the "ppend" >> calculation >> though -- even if there are holes in the node's address space, is it still >> a >> contiguous chunk of page structure addresses per-node? > > I'm still investigating and not sure yet, but I think that SPASEMEM uses > mem_section instead of node_mem_map means page structures could be > non-contignuous per-node according to architecture or condition. > > typedef struct pglist_data { > ... > #ifdef CONFIG_FLAT_NODE_MEM_MAP /* means !SPARSEMEM */ > struct page *node_mem_map; > > I'll continue to check it. You are right, but in the case where pglist_data.node_mem_map does *not* exist, the crash utility initializes each vt->node_table[node].mem_map with the node's starting mem_map address by using the return value from phys_to_page() of the node's starting physical address -- which uses the sparsemem functions. The question is whether the current "ppend" calculation is correct for the last physical page in a node. If it is not correct, then perhaps an "mem_map_end" value can be added to the node_table structure, initialized by using phys_to_page() to get the page address of the last physical address in the node. And then in that case, the question is whether the mem_map range of virtual addresses are contiguous -- even if there are holes in the mem_map virtual address range.

Thanks, Dave > > Thanks, > Kazuhito Hagio > >> >>> >>> Dave >>> >>>> >>>>> >>>>> There is really no compelling reason that count_partial() absolutely >>>>> *must* use >>>>> is_page_ptr(), and so I'm thinking that perhaps you could come up with a >>>>> less >>>>> heavy-handed method for simply testing whether a page.lru entry points >>>>> to >>>>> another >>>>> vmemmap'd page. Something along the lines of adding this for >>>>> vmemmap-enabled kernels: >>>>> >>>>> #define IN_VMEMMAP_RANGE(page) ((page >= VMEMMAP_VADDR) && (page <= >>>>> VMEMMAP_END)) >>>>> >>>>> and then have count_partial() replace the is_page_ptr() call with >>>>> another >>>>> slub function that does something like this for vmemmap-enabled kernels: >>>>> >>>>> (IN_VMMEMAP_RANGE(next) && accessible(next)) >>>>> >>>>> Or instead of accessible(), it could read "next" as a list_head with >>>>> RETURN_ON_ERROR, >>>>> and verify that next->prev points back to the current list_head. >>>>> >>>>> Non-vmemmap-enabled kernels could still use is_page_ptr(). >>>>> >>>>> What do you think of doing something like that? >>>> >>>> Given possible compatibility issues you said, I think that the way you >>>> suggested >>>> might well be enough for now. I'll try a method like the above. >>>> >>>> Thanks, >>>> Kazuhito Hagio >>>> >>>>> >>>>> Dave >>>>> >>>>> >>>>> >>>>> >>>>> ----- Original Message ----- >>>>>> Hi, >>>>>> >>>>>> The "kmem -[sS]" commands can take several minutes to complete with >>>>>> the following conditions: >>>>>> * The system has a lot of memory sections with CONFIG_SPARSEMEM. >>>>>> * The kernel uses SLUB and it has a very long partial slab list. >>>>>> >>>>>> crash> kmem -s dentry | awk '{print strftime("%T"), $0}' >>>>>> 10:18:34 CACHE NAME OBJSIZE ALLOCATED >>>>>> TOTAL >>>>>> SLABS SSIZE >>>>>> 10:19:41 ffff88017fc78a00 dentry 192 9038949 >>>>>> 10045728 >>>>>> 239184 8k >>>>>> crash> kmem -S dentry | bash -c 'cat >/dev/null ; echo $SECONDS' >>>>>> 334 >>>>>> >>>>>> One of the causes is that is_page_ptr() in count_partial() checks if >>>>>> a given slub page address is a page struct by searching all memory >>>>>> sections linearly for the one which includes it. >>>>>> >>>>>> nr_mem_sections = NR_MEM_SECTIONS(); >>>>>> for (nr = 0; nr < nr_mem_sections ; nr++) { >>>>>> if ((sec_addr = valid_section_nr(nr))) { >>>>>> ... >>>>>> >>>>>> With CONFIG_SPARSEMEM{_VMEMMAP}, we can calculate the memory section >>>>>> which includes a page struct with its page.flags, or its address and >>>>>> VMEMMAP_VADDR. With this patch doing so, the computation amount can be >>>>>> significantly reduced in that case. >>>>>> >>>>>> crash> kmem -s dentry | awk '{print strftime("%T"), $0}' >>>>>> 10:34:55 CACHE NAME OBJSIZE ALLOCATED >>>>>> TOTAL >>>>>> SLABS SSIZE >>>>>> 10:34:55 ffff88017fc78a00 dentry 192 9038949 >>>>>> 10045728 >>>>>> 239184 8k >>>>>> crash> kmem -S dentry | bash -c 'cat >/dev/null ; echo $SECONDS' >>>>>> 2 >>>>>> >>>>>> This patch uses VMEMMAP_VADDR. It is not defined on PPC64, but it looks >>>>>> like PPC64 supports VMEMMAP flag and machdep->machspec->vmemmap_base is >>>>>> it, so this patch also defines it for PPC64. This might need some help >>>>>> from PPC folks. >>>>>> >>>>>> Signed-off-by: Kazuhito Hagio <k-hagio(a)ab.jp.nec.com&gt; >>>>>> --- >>>>>> defs.h | 2 ++ >>>>>> memory.c | 15 +++++++++++++++ >>>>>> 2 files changed, 17 insertions(+) >>>>>> >>>>>> diff --git a/defs.h b/defs.h >>>>>> index aa17792..84e68ca 100644 >>>>>> --- a/defs.h >>>>>> +++ b/defs.h >>>>>> @@ -3861,6 +3861,8 @@ struct efi_memory_desc_t { >>>>>> #define IS_VMALLOC_ADDR(X) machdep->machspec->is_vmaddr(X) >>>>>> #define KERNELBASE machdep->pageoffset >>>>>> >>>>>> +#define VMEMMAP_VADDR (machdep->machspec->vmemmap_base) >>>>>> + >>>>>> #define PGDIR_SHIFT (machdep->pageshift + (machdep->pageshift -3) >>>>>> + >>>>>> (machdep->pageshift - 2)) >>>>>> #define PMD_SHIFT (machdep->pageshift + (machdep->pageshift - >>>>>> 3)) >>>>>> >>>>>> diff --git a/memory.c b/memory.c >>>>>> index 0df8ecc..0696763 100644 >>>>>> --- a/memory.c >>>>>> +++ b/memory.c >>>>>> @@ -13348,10 +13348,25 @@ is_page_ptr(ulong addr, physaddr_t *phys) >>>>>> ulong nr_mem_sections; >>>>>> ulong coded_mem_map, mem_map, end_mem_map; >>>>>> physaddr_t section_paddr; >>>>>> +#ifdef VMEMMAP_VADDR >>>>>> + ulong flags; >>>>>> +#endif >>>>>> >>>>>> if (IS_SPARSEMEM()) { >>>>>> nr_mem_sections = NR_MEM_SECTIONS(); >>>>>> +#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 >>>>>> if ((sec_addr = valid_section_nr(nr))) { >>>>>> coded_mem_map = >>>>>> section_mem_map_addr(sec_addr); >>>>>> mem_map = >>>>>> sparse_decode_mem_map(coded_mem_map, >>>>>> nr); >>>>>> -- >>>>>> 1.8.3.1 >>>>>> >>>>>> -- >>>>>> Crash-utility mailing list >>>>>> Crash-utility(a)redhat.com >>>>>> https://www.redhat.com/mailman/listinfo/crash-utility >>>>>> >>>>> >>>>> -- >>>>> Crash-utility mailing list >>>>> Crash-utility(a)redhat.com >>>>> https://www.redhat.com/mailman/listinfo/crash-utility >>>>> >>>> >>>> -- >>>> Crash-utility mailing list >>>> Crash-utility(a)redhat.com >>>> https://www.redhat.com/mailman/listinfo/crash-utility >>>> >>> >>> -- >>> Crash-utility mailing list >>> Crash-utility(a)redhat.com >>> https://www.redhat.com/mailman/listinfo/crash-utility >>> >> >> -- >> Crash-utility mailing list >> Crash-utility(a)redhat.com >> https://www.redhat.com/mailman/listinfo/crash-utility >> > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility > -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility

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

Hi Dave, On 2/21/2018 4:14 PM, Dave Anderson wrote: > > > ----- 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. Thank you, I understood it more precisely. [...] > 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) As to the last example (IIUC), I had confirmed that accessible() returned FALSE if a page address was in a hole like below on x86_64/RHEL7, so I was writing the prototype patch. crash> kmem -n ... NR SECTION CODED_MEM_MAP MEM_MAP PFN ... 23 ffff88043ffd82e0 ffffea0000000000 ffffea0002e00000 753664 ## There is a 32 ffff88043ffd8400 ffffea0000000000 ffffea0004000000 1048576 ## hole here. ... crash> kmem ffffea0002ffffc0 ## The last page struct of NR=23 DEBUG: addr=0xffffea0002ffffc0 nr=23 accessible=1 DEBUG: addr=0xffffea0002ffffc0 nr=23 accessible=1 ## in is_page_ptr() DEBUG: addr=0xffffea0002ffffc0 nr=23 accessible=1 DEBUG: addr=0xffffea0002ffffc0 nr=23 accessible=1 PAGE PHYSICAL MAPPING INDEX CNT FLAGS ffffea0002ffffc0 bffff000 0 0 1 1fffff00000000 crash> kmem ffffea0003000000 ## A page address in a hole. kmem: WARNING: cannot make virtual-to-physical translation: ffffea0003000000 DEBUG: addr=0xffffea0003000000 nr=24 accessible=0 DEBUG: addr=0xffffea0003000000 nr=24 accessible=0 ## returned 0 DEBUG: addr=0xffffea0003000000 nr=24 accessible=0 ffffea0003000000: kernel virtual address not found in mem map I'm not sure whether there is the case that a page address does not reference physical memory except for the above. But originally, since accessible() is readmem(), which actually reads a dump file, it could return FALSE also due to some errors quietly, and this could leads to a wrong judgement. And I had thought that if accessible() was valid for the page validity test, there was no need to calculate its section. So could it remove the accessible() and continue to utilize the valid_section_nr() section for the test like this?: --- diff --git a/defs.h b/defs.h index aa17792..ab98cb7 100644 --- a/defs.h +++ b/defs.h @@ -3335,6 +3335,9 @@ struct arm64_stackframe { #define VTOP(X) x86_64_VTOP((ulong)(X)) #define IS_VMALLOC_ADDR(X) x86_64_IS_VMALLOC_ADDR((ulong)(X)) +#define IS_VMEMMAP_PAGE_ADDR(X) x86_64_IS_VMEMMAP_PAGE_ADDR((ulong)(X)) +#define VMEMMAP_PAGE_TO_PFN(X) (((ulong)(X) - VMEMMAP_VADDR) / SIZE(page)) + /* * the default page table level for x86_64: * 4 level page tables @@ -5646,6 +5649,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_ADDR(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..928c3c2 100644 --- a/memory.c +++ b/memory.c @@ -13350,6 +13350,30 @@ is_page_ptr(ulong addr, physaddr_t *phys) physaddr_t section_paddr; if (IS_SPARSEMEM()) { +#ifdef IS_VMEMMAP_PAGE_ADDR + if (machdep->flags & VMEMMAP) { + if (IS_VMEMMAP_PAGE_ADDR(addr)) { + nr = pfn_to_section_nr(VMEMMAP_PAGE_TO_PFN(addr)); + if ((sec_addr = valid_section_nr(nr))) { + coded_mem_map = section_mem_map_addr(sec_addr); + mem_map = sparse_decode_mem_map(coded_mem_map, nr); + end_mem_map = mem_map + (PAGES_PER_SECTION() * SIZE(page)); + + if ((addr >= mem_map) && (addr < end_mem_map)) { + if ((addr - mem_map) % SIZE(page)) + return FALSE; + if (phys) { + section_paddr = PTOB(section_nr_to_pfn(nr)); + pgnum = (addr - mem_map) / SIZE(page); + *phys = section_paddr + ((physaddr_t)pgnum * PAGESIZE()); + } + 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..7240c5d 100644 --- a/x86_64.c +++ b/x86_64.c @@ -1570,6 +1570,14 @@ x86_64_IS_VMALLOC_ADDR(ulong vaddr) (vaddr >= VSYSCALL_START && vaddr < VSYSCALL_END)); } +int +x86_64_IS_VMEMMAP_PAGE_ADDR(ulong vaddr) +{ + return ((machdep->flags & VMEMMAP) && + (vaddr >= VMEMMAP_VADDR && vaddr <= VMEMMAP_END) && + !((vaddr - VMEMMAP_VADDR) % SIZE(page))); +} + static int x86_64_is_module_addr(ulong vaddr) { --- > 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? I have not considered it enough yet, but if all ranges of mem_sections are ascending for section numbers and vmemmap holes like above are to be managed well, it might be good. (I'm guessing that the binary search might need an auxiliary array or something due to the vmemmap holes..) Thanks, Kazuhito Hagio

> First, the mem_section numbers are ascending. They may not necessarily start > with 0, and there may be holes, but they are ascending. That being the case, > there is no need for is_page_ptr() to walk through NR_MEM_SECTIONS() worth > of entries, because there will be an ending number that's typically much > smaller. Even on a 256GB dumpfile I have on hand, which has a NR_MEM_SECTIONS() > value of 524288, the largest valid section number is 2055. (What is the smallest > and largest number that you see on whatever large-memory system that you are > testing with?) > > In any case, let's store the largest section number during initialization in > the vm_table, and use it as a delimeter in is_page_ptr(). I agree with you. This will improve the worst case of the loop. Also, if the binary search is implemented in the future, it could be utilized. (The largest valid section numbers of each architecture in my test logs are 1543 on a 192GB x86_64 and 2047 on a 32GB ppc64.)

> diff --git a/defs.h b/defs.h > index aa17792..8768fd5 100644 > --- a/defs.h > +++ b/defs.h > @@ -2369,6 +2369,7 @@ struct vm_table { /* kernel VM-related data */ > ulong mask; > char *name; > } *pageflags_data; > + ulong max_mem_section_nr; > }; > > #define NODES (0x1) > diff --git a/memory.c b/memory.c > index 0df8ecc..6770937 100644 > --- a/memory.c > +++ b/memory.c > @@ -255,7 +255,7 @@ static void PG_reserved_flag_init(void); > static void PG_slab_flag_init(void); > static ulong nr_blockdev_pages(void); > void sparse_mem_init(void); > -void dump_mem_sections(void); > +void dump_mem_sections(int); > void list_mem_sections(void); > ulong sparse_decode_mem_map(ulong, ulong); > char *read_mem_section(ulong); > @@ -13350,7 +13350,7 @@ is_page_ptr(ulong addr, physaddr_t *phys) > physaddr_t section_paddr; > > if (IS_SPARSEMEM()) { > - nr_mem_sections = NR_MEM_SECTIONS(); > + nr_mem_sections = vt->max_mem_section_nr+1; > for (nr = 0; nr < nr_mem_sections ; nr++) { > if ((sec_addr = valid_section_nr(nr))) { > coded_mem_map = section_mem_map_addr(sec_addr); > @@ -13668,6 +13668,7 @@ dump_vm_table(int verbose) > fprintf(fp, " swap_info_struct: %lx\n", (ulong)vt->swap_info_struct); > fprintf(fp, " mem_sec: %lx\n", (ulong)vt->mem_sec); > fprintf(fp, " mem_section: %lx\n", (ulong)vt->mem_section); > + fprintf(fp, " max_mem_section_nr: %ld\n", vt->max_mem_section_nr); > fprintf(fp, " ZONE_HIGHMEM: %d\n", vt->ZONE_HIGHMEM); > fprintf(fp, "node_online_map_len: %d\n", vt->node_online_map_len); > if (vt->node_online_map_len) { > @@ -16295,8 +16296,8 @@ dump_memory_nodes(int initialize) > vt->numnodes = n; > } > > - if (!initialize && IS_SPARSEMEM()) > - dump_mem_sections(); > + if (IS_SPARSEMEM()) > + dump_mem_sections(initialize); > } > > /* > @@ -17128,9 +17129,9 @@ pfn_to_map(ulong pfn) > } > > void > -dump_mem_sections(void) > +dump_mem_sections(int initialize) > { > - ulong nr,addr; > + ulong nr, max, addr; > ulong nr_mem_sections; > ulong coded_mem_map, mem_map, pfn; > char buf1[BUFSIZE]; > @@ -17140,6 +17141,15 @@ dump_mem_sections(void) > > nr_mem_sections = NR_MEM_SECTIONS(); > > + if (initialize) { > + for (nr = max = 0; nr < nr_mem_sections ; nr++) { > + if (valid_section_nr(nr)) > + max = nr; > + } > + vt->max_mem_section_nr = max; > + return; > + } > + > fprintf(fp, "\n"); > pad_line(fp, BITS32() ? 59 : 67, '-'); > fprintf(fp, "\n\nNR %s %s %s PFN\n", > > > Now, with respect to the architecture-specific, VMEMMAP-only, part > that is of most interest to you, let's do it with an architecture > specific callback. You can post it for x86_64, and the other architecture > maintainers can write their own version. For example, add a new > callback function to the machdep_table structure, i.e., like this: > > struct machdep_table { > ulong flags; > ulong kvbase; > ... > void (*get_irq_affinity)(int); > void (*show_interrupts)(int, ulong *); > + int is_page_ptr(ulong, physaddr_t *); > }; > > Write the x86_64_is_page_ptr() function that works for VMEMMAP > kernels, and returns FALSE otherwise. And add the call to the top > of is_page_ptr() like this: > > int > is_page_ptr(ulong addr, physaddr_t *phys) > { > int n; > ulong ppstart, ppend; > struct node_table *nt; > ulong pgnum, node_size; > ulong nr, sec_addr; > ulong nr_mem_sections; > ulong coded_mem_map, mem_map, end_mem_map; > physaddr_t section_paddr; > > + if (machdep->is_page_ptr(addr, phys)) > + return TRUE; > > if (IS_SPARSEMEM()) { > ... > > To avoid having to check whether the machdep->is_page_ptr function > exists, write a generic_is_page_ptr() function that just returns > FALSE, and initialize machdep->is_page_ptr to generic_is_page_ptr in > the setup_environment() function. Later on, individual architectures > can overwrite it when machdep_init(SETUP_ENV) is called. > > How's that sound? It looks readable and refined. If an incoming address is not a page address, the IS_SPARSEMEM() section is also executed, but I think that it does not matter because it is rare that the situation occurs many times at once and it is likely that the code will become ugly to avoid it. So I'll prepare the x86_64 part based on the above.

Thanks, Kazuhito Hagio > > Dave > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility > -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility

Hi Dave, On 2/27/2018 4:45 PM, Kazuhito Hagio wrote: [...] >> First, the mem_section numbers are ascending. They may not necessarily >> start >> with 0, and there may be holes, but they are ascending. That being the >> case, >> there is no need for is_page_ptr() to walk through NR_MEM_SECTIONS() worth >> of entries, because there will be an ending number that's typically much >> smaller. Even on a 256GB dumpfile I have on hand, which has a >> NR_MEM_SECTIONS() >> value of 524288, the largest valid section number is 2055. (What is the >> smallest >> and largest number that you see on whatever large-memory system that you >> are >> testing with?) >> >> In any case, let's store the largest section number during initialization >> in >> the vm_table, and use it as a delimeter in is_page_ptr(). > > I agree with you. This will improve the worst case of the loop. Also, > if the binary search is implemented in the future, it could be utilized. > (The largest valid section numbers of each architecture in my test logs > are 1543 on a 192GB x86_64 and 2047 on a 32GB ppc64.) I checked and tested the former patch you proposed below as it is and I didn't find any problem. Could you merge this? (or is there anything I should do?) > >> diff --git a/defs.h b/defs.h >> index aa17792..8768fd5 100644 >> --- a/defs.h >> +++ b/defs.h >> @@ -2369,6 +2369,7 @@ struct vm_table { /* kernel >> VM-related data */ >> ulong mask; >> char *name; >> } *pageflags_data; >> + ulong max_mem_section_nr; >> }; >> >> #define NODES (0x1) >> diff --git a/memory.c b/memory.c >> index 0df8ecc..6770937 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -255,7 +255,7 @@ static void PG_reserved_flag_init(void); >> static void PG_slab_flag_init(void); >> static ulong nr_blockdev_pages(void); >> void sparse_mem_init(void); >> -void dump_mem_sections(void); >> +void dump_mem_sections(int); >> void list_mem_sections(void); >> ulong sparse_decode_mem_map(ulong, ulong); >> char *read_mem_section(ulong); >> @@ -13350,7 +13350,7 @@ is_page_ptr(ulong addr, physaddr_t *phys) >> physaddr_t section_paddr; >> >> if (IS_SPARSEMEM()) { >> - nr_mem_sections = NR_MEM_SECTIONS(); >> + nr_mem_sections = vt->max_mem_section_nr+1; >> for (nr = 0; nr < nr_mem_sections ; nr++) { >> if ((sec_addr = valid_section_nr(nr))) { >> coded_mem_map = section_mem_map_addr(sec_addr); >> @@ -13668,6 +13668,7 @@ dump_vm_table(int verbose) >> fprintf(fp, " swap_info_struct: %lx\n", (ulong)vt->swap_info_struct); >> fprintf(fp, " mem_sec: %lx\n", (ulong)vt->mem_sec); >> fprintf(fp, " mem_section: %lx\n", (ulong)vt->mem_section); >> + fprintf(fp, " max_mem_section_nr: %ld\n", vt->max_mem_section_nr); >> fprintf(fp, " ZONE_HIGHMEM: %d\n", vt->ZONE_HIGHMEM); >> fprintf(fp, "node_online_map_len: %d\n", vt->node_online_map_len); >> if (vt->node_online_map_len) { >> @@ -16295,8 +16296,8 @@ dump_memory_nodes(int initialize) >> vt->numnodes = n; >> } >> >> - if (!initialize && IS_SPARSEMEM()) >> - dump_mem_sections(); >> + if (IS_SPARSEMEM()) >> + dump_mem_sections(initialize); >> } >> >> /* >> @@ -17128,9 +17129,9 @@ pfn_to_map(ulong pfn) >> } >> >> void >> -dump_mem_sections(void) >> +dump_mem_sections(int initialize) >> { >> - ulong nr,addr; >> + ulong nr, max, addr; >> ulong nr_mem_sections; >> ulong coded_mem_map, mem_map, pfn; >> char buf1[BUFSIZE]; >> @@ -17140,6 +17141,15 @@ dump_mem_sections(void) >> >> nr_mem_sections = NR_MEM_SECTIONS(); >> >> + if (initialize) { >> + for (nr = max = 0; nr < nr_mem_sections ; nr++) { >> + if (valid_section_nr(nr)) >> + max = nr; >> + } >> + vt->max_mem_section_nr = max; >> + return; >> + } >> + >> fprintf(fp, "\n"); >> pad_line(fp, BITS32() ? 59 : 67, '-'); >> fprintf(fp, "\n\nNR %s %s %s PFN\n", >> >> >> Now, with respect to the architecture-specific, VMEMMAP-only, part >> that is of most interest to you, let's do it with an architecture >> specific callback. You can post it for x86_64, and the other architecture >> maintainers can write their own version. For example, add a new >> callback function to the machdep_table structure, i.e., like this: >> >> struct machdep_table { >> ulong flags; >> ulong kvbase; >> ... >> void (*get_irq_affinity)(int); >> void (*show_interrupts)(int, ulong *); >> + int is_page_ptr(ulong, physaddr_t *); >> }; >> >> Write the x86_64_is_page_ptr() function that works for VMEMMAP >> kernels, and returns FALSE otherwise. And add the call to the top >> of is_page_ptr() like this: >> >> int >> is_page_ptr(ulong addr, physaddr_t *phys) >> { >> int n; >> ulong ppstart, ppend; >> struct node_table *nt; >> ulong pgnum, node_size; >> ulong nr, sec_addr; >> ulong nr_mem_sections; >> ulong coded_mem_map, mem_map, end_mem_map; >> physaddr_t section_paddr; >> >> + if (machdep->is_page_ptr(addr, phys)) >> + return TRUE; >> >> if (IS_SPARSEMEM()) { >> ... >> >> To avoid having to check whether the machdep->is_page_ptr function >> exists, write a generic_is_page_ptr() function that just returns >> FALSE, and initialize machdep->is_page_ptr to generic_is_page_ptr in >> the setup_environment() function. Later on, individual architectures >> can overwrite it when machdep_init(SETUP_ENV) is called. >> >> How's that sound? > > It looks readable and refined. > > If an incoming address is not a page address, the IS_SPARSEMEM() section > is also executed, but I think that it does not matter because it is rare > that the situation occurs many times at once and it is likely that the code > will become ugly to avoid it. > > So I'll prepare the x86_64 part based on the above. I thought that you would merge the common part, but is it wrong? Could I post it with the x86_64 part? Sorry I didn't understand well how to proceed with this. And thank you very much for your help with this issue! Kazuhito Hagio > > Thanks, > Kazuhito Hagio > >> >> Dave >> >> -- >> Crash-utility mailing list >> Crash-utility(a)redhat.com >> https://www.redhat.com/mailman/listinfo/crash-utility >> > > -- > Crash-utility mailing list > Crash-utility(a)redhat.com > https://www.redhat.com/mailman/listinfo/crash-utility > -- Crash-utility mailing list Crash-utility(a)redhat.com https://www.redhat.com/mailman/listinfo/crash-utility