Hi qiwu,
On Tue, Sep 10, 2024 at 3:20 PM <qiwu.chen@xxxxxxxxxxxxx> wrote:
>
> Hi Tao,
> Thanks for your review.
No problem.
> >
> > Thanks for the fix, however there are several issues:
> >
> > On Mon, Sep 2, 2024 at 2:30 AM <qiwu.chen(a)transsion.com> wrote:
> >
> > The new members for the offset_table should always be appended to the
> > end of the struct.
> >
> >
> > 1. The function naming better be page_slab() instead of PageSlab.
> >
> > 2. For the 1st "if", the page_type is not used, only used in the
> > "else" branch. But page_type will always be readmem() previously, also
> > page_type may not exist for older kernels. Can you rewrite the
> > function?
> >
> >
> > There is a problem for readmem(page_page_type), the page_type member
> > of struct page is not introduced for kernels such as v4.17 and prior.
> > E.g for 3.10:
> >
> > crash> kmem -s ffff880169b30000
> >
> > kmem: invalid structure member offset: page_page_type
> > FILE: memory.c LINE: 9780 FUNCTION: vaddr_to_kmem_cache()
> >
> > [/root/crash-dev/crash] error trace: 530063 => 55e269 => 53f5d8 => 63740e
> >
> > 63740e: OFFSET_verify+164
> > 53f5d8: vaddr_to_kmem_cache+299
> > 55e269: dump_kmem_cache_slub+688
> > 530063: cmd_kmem+3017
> >
> > kmem: invalid structure member offset: page_page_type
> > FILE: memory.c LINE: 9780 FUNCTION: vaddr_to_kmem_cache()
> >
> > Also see the comment for function PageSlab.
> >
> I make a improvement in the following change as your comments. The page_slab() function will resolve the problem for readmem(page_page_type), since it will test the page_type member of struct page exist before readmem(page_page_type).
>
One suggestion. Do not attach your updated patch within your comments
like this. Otherwise it will be very inconvenient for others to search
for one specific version of your patch within the mailing list,
because all versions of your patch and discussions will be piled into
one thread.
Send your patch with an updated tile like "[PATCH v2] kmem: fix the
determination for slab page" is preferred.
Thanks,
Tao Liu
> diff --git a/defs.h b/defs.h
> index 2231cb6..e2a9278 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -2243,6 +2243,7 @@ struct offset_table { /* stash of commonly-used offsets */
> long vmap_node_busy;
> long rb_list_head;
> long file_f_inode;
> + long page_page_type;
> };
>
> struct size_table { /* stash of commonly-used sizes */
> @@ -2651,6 +2652,7 @@ struct vm_table { /* kernel VM-related data */
> ulong max_mem_section_nr;
> ulong zero_paddr;
> ulong huge_zero_paddr;
> + uint page_type_base;
> };
>
> #define NODES (0x1)
> @@ -2684,6 +2686,11 @@ struct vm_table { /* kernel VM-related data */
> #define SLAB_CPU_CACHE (0x10000000)
> #define SLAB_ROOT_CACHES (0x20000000)
> #define USE_VMAP_NODES (0x40000000)
> +/*
> + * The SLAB_PAGEFLAGS flag is introduced to detect the change of
> + * PG_slab's type from a page flag to a page type.
> + */
> +#define SLAB_PAGEFLAGS (0x80000000)
>
> #define IS_FLATMEM() (vt->flags & FLATMEM)
> #define IS_DISCONTIGMEM() (vt->flags & DISCONTIGMEM)
> diff --git a/memory.c b/memory.c
> index 967a9cf..a2885bd 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -351,6 +351,43 @@ static ulong handle_each_vm_area(struct handle_each_vm_area_args *);
>
> static ulong DISPLAY_DEFAULT;
>
> +#define PAGE_TYPE_BASE 0xf0000000
> +#define PageType(page_type, flag) \
> + ((page_type & (vt->page_type_base | flag)) == vt->page_type_base)
> +
> +static void page_type_init(void)
> +{
> + /*
> + * The page type macros covert into enum since kernel commit ff202303c398e,
> + * use the default macro value if the vmcoreinfo without enum pagetype.
> + */
> + if (!enumerator_value("PAGE_TYPE_BASE", (long *)&vt->page_type_base))
> + vt->page_type_base = PAGE_TYPE_BASE;
> +}
> +
> +/*
> + * The PG_slab's type has changed from a page flag to a page type
> + * since kernel commit 46df8e73a4a3.
> + */
> +static bool page_slab(ulong pp, ulong flags)
> +{
> + if (vt->flags & SLAB_PAGEFLAGS) {
> + if ((flags >> vt->PG_slab) & 1)
> + return TRUE;
> + }
> +
> + if (VALID_MEMBER(page_page_type)) {
> + uint page_type;
> +
> + readmem(pp+OFFSET(page_page_type), KVADDR, &page_type,
> + sizeof(page_type), "page_type", FAULT_ON_ERROR);
> + if (PageType(page_type, (uint)vt->PG_slab))
> + return TRUE;
> + }
> +
> + return FALSE;
> +}
> +
> /*
> * Verify that the sizeof the primitive types are reasonable.
> */
> @@ -504,6 +541,10 @@ vm_init(void)
> ANON_MEMBER_OFFSET_INIT(page_compound_head, "page", "compound_head");
> MEMBER_OFFSET_INIT(page_private, "page", "private");
> MEMBER_OFFSET_INIT(page_freelist, "page", "freelist");
> + if (MEMBER_EXISTS("page", "page_type")) {
> + MEMBER_OFFSET_INIT(page_page_type, "page", "page_type");
> + page_type_init();
> + }
>
> MEMBER_OFFSET_INIT(mm_struct_pgd, "mm_struct", "pgd");
>
> @@ -5931,7 +5972,7 @@ dump_mem_map_SPARSEMEM(struct meminfo *mi)
> if ((flags >> v22_PG_Slab) & 1)
> slabs++;
> } else if (vt->PG_slab) {
> - if ((flags >> vt->PG_slab) & 1)
> + if (page_slab(pp, flags))
> slabs++;
> } else {
> if ((flags >> v24_PG_slab) & 1)
> @@ -6381,7 +6422,7 @@ dump_mem_map(struct meminfo *mi)
> if ((flags >> v22_PG_Slab) & 1)
> slabs++;
> } else if (vt->PG_slab) {
> - if ((flags >> vt->PG_slab) & 1)
> + if (page_slab(pp, flags))
> slabs++;
> } else {
> if ((flags >> v24_PG_slab) & 1)
> @@ -6775,6 +6816,9 @@ page_flags_init_from_pageflag_names(void)
> vt->pageflags_data[i].name = nameptr;
> vt->pageflags_data[i].mask = mask;
>
> + if (!strncmp(nameptr, "slab", 4))
> + vt->flags |= SLAB_PAGEFLAGS;
> +
> if (CRASHDEBUG(1)) {
> fprintf(fp, " %08lx %s\n",
> vt->pageflags_data[i].mask,
> @@ -6836,7 +6880,8 @@ page_flags_init_from_pageflags_enum(void)
> strcpy(nameptr, arglist[0] + strlen("PG_"));
> vt->pageflags_data[p].name = nameptr;
> vt->pageflags_data[p].mask = 1 << atoi(arglist[2]);
> -
> + if (!strncmp(nameptr, "slab", 4))
> + vt->flags |= SLAB_PAGEFLAGS;
> p++;
> }
> } else
> @@ -9736,14 +9781,14 @@ vaddr_to_kmem_cache(ulong vaddr, char *buf, int verbose)
> readmem(page+OFFSET(page_flags), KVADDR,
> &page_flags, sizeof(ulong), "page.flags",
> FAULT_ON_ERROR);
> - if (!(page_flags & (1 << vt->PG_slab))) {
> + if (!page_slab(page, page_flags)) {
> if (((vt->flags & KMALLOC_SLUB) || VALID_MEMBER(page_compound_head)) ||
> ((vt->flags & KMALLOC_COMMON) &&
> VALID_MEMBER(page_slab) && VALID_MEMBER(page_first_page))) {
> readmem(compound_head(page)+OFFSET(page_flags), KVADDR,
> &page_flags, sizeof(ulong), "page.flags",
> FAULT_ON_ERROR);
> - if (!(page_flags & (1 << vt->PG_slab)))
> + if (!page_slab(compound_head(page), page_flags))
> return NULL;
> } else
> return NULL;
> @@ -20195,7 +20240,7 @@ char *
> is_slab_page(struct meminfo *si, char *buf)
> {
> int i, cnt;
> - ulong page_slab, page_flags, name;
> + ulong pg_slab, page_flags, name;
> ulong *cache_list;
> char *retval;
>
> @@ -20210,11 +20255,11 @@ is_slab_page(struct meminfo *si, char *buf)
> RETURN_ON_ERROR|QUIET))
> return NULL;
>
> - if (!(page_flags & (1 << vt->PG_slab)))
> + if (!page_slab(si->spec_addr, page_flags))
> return NULL;
>
> if (!readmem(si->spec_addr + OFFSET(page_slab), KVADDR,
> - &page_slab, sizeof(ulong), "page.slab",
> + &pg_slab, sizeof(ulong), "page.slab",
> RETURN_ON_ERROR|QUIET))
> return NULL;
>
> @@ -20222,7 +20267,7 @@ is_slab_page(struct meminfo *si, char *buf)
> cnt = get_kmem_cache_list(&cache_list);
>
> for (i = 0; i < cnt; i++) {
> - if (page_slab == cache_list[i]) {
> + if (pg_slab == cache_list[i]) {
> if (!readmem(cache_list[i] + OFFSET(kmem_cache_name),
> KVADDR, &name, sizeof(char *),
> "kmem_cache.name", QUIET|RETURN_ON_ERROR))
> --
> Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
> To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> Contribution Guidelines: https://github.com/crash-utility/crash/wiki
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
