Re: [PATCH v4] tools/mm: rewrite show_page_info.py with modular design and new features

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] tools/mm: _page_fields() reads head._mapcount on typed and compound pages
- [Medium] tools/mm: unhandled AttributeError in _scan_page_types() for slab pages
--

commit 6fb0388630d5dc0fef903fe1ea732b1f50562c48
Author: Ye Liu <liuye@kylinos.cn>

tools/mm: rewrite show_page_info.py with modular design and new features

This commit rewrites show_page_info.py into a modular field-collector
architecture, adds new input modes (--pfn, --page, --user, --selftest), and
introduces reverse mapping, page table walking, and additional page state
reporting.

> diff --git a/tools/mm/show_page_info.py b/tools/mm/show_page_info.py
> index c46d8ea283d7d..2202be56465c4 100644
> --- a/tools/mm/show_page_info.py
> +++ b/tools/mm/show_page_info.py
> @@ -4,166 +4,918 @@

[ ... ]

> +def _page_fields(head, fields):
> +    """Collect non-slab page fields (refcount, mapping, anon/file, etc.)."""
> +    mapping = head.mapping.value_()
> +    fields["Page Mapping"] = hex(mapping)
> +
> +    if mapping == 0:
> +        fields["Page Anon/File"] = "Unmapped"
> +        refcount = head._refcount.counter.value_()
> +        if refcount > 0:
> +            fields["Page Refcount"] = refcount
> +            # page_type and _mapcount share a union: for typed pages
> +            # (reported as "Page Type"), _mapcount holds the raw page
> +            # type value, not a mapcount.
> +            if _page_type(head) is None:
> +                fields["Page Mapcount"] = head._mapcount.counter.value_()
> +        lru = _lru_type(head)
> +        if lru:
> +            fields["LRU"] = lru
> +        swap = _swapcache_info(head)
> +        if swap:
> +            fields["Swap Entry"] = swap
> +        return
> +
> +    fields["Page Refcount"] = head._refcount.counter.value_()
> +    fields["Page Mapcount"] = head._mapcount.counter.value_()

[Severity: Medium]
Should this path also check whether _page_type(head) is None, and pass page
into _page_fields() for compound pages?

In struct page (include/linux/mm_types.h), page_type and _mapcount share a
union. While the mapping == 0 branch above skips Page Mapcount when
_page_type(head) is set, typed pages such as PGTY_hugetlb (which sets
head.mapping to the hugetlbfs address_space or anon_vma) and PGTY_zsmalloc
(where struct zpdesc overlays mapping with movable_ops) have mapping != 0.
When show_page_state() calls _page_fields(head, fields) on these pages, it
unconditionally prints the raw page_type union bits (such as a large negative
number around 0xf4000000) as Page Mapcount.

In addition, show_page_state() only passes head to _page_fields(head, fields)
and reads head._mapcount:

show_page_state()
    if PageSlab(head):
        _slab_fields(head, fields)
    else:
        _page_fields(head, fields)

For non-hugetlb compound pages (large folios), per-page PTE mapcounts are
stored in each subpage's page._mapcount, while folio-wide/PMD mapcounts live
on the first tail page (_large_mapcount and _entire_mapcount). Reading only
head._mapcount loses tail-subpage PTE mapcount reporting when inspecting a
tail page and reports -1 for PMD-mapped compound folios.

[ ... ]

> +def _scan_page_types():
> +    """Scan all pages and categorize by type. Returns {type: page} dict."""
> +    from drgn.helpers.linux.mm import for_each_page
> +
> +    objexts_flag = prog.constant("MEMCG_DATA_OBJEXTS").value_()
> +    mask = prog.constant('__NR_MEMCG_DATA_FLAGS').value_() - 1
> +
> +    found = {}
> +    needed = ["anon", "file", "compound_anon", "compound_file",
> +              "slab_multi", "slab_single", "slab_no_objcg", "uncharged"]
> +
> +    for page in for_each_page(prog):
> +        try:
> +            head = compound_head(page) if PageCompound(page) else page
> +
> +            if PageSlab(head):
> +                if head.memcg_data.read_() & objexts_flag:
> +                    try:
> +                        slab = cast("struct slab *", head)
> +                        if slab.obj_exts_needs_objcg.value_():
> +                            slab_stride = _slab_obj_ext_stride(slab)
> +                            n = _count_distinct_memcg(head, slab_stride, mask)
> +                            if n >= 2 and "slab_multi" not in found:
> +                                found["slab_multi"] = page
> +                            elif n == 1 and "slab_single" not in found:
> +                                found["slab_single"] = page
> +                        elif "slab_no_objcg" not in found:
> +                            found["slab_no_objcg"] = page
> +                    except (AttributeError, FaultError):
> +                        pass
> +                continue

[Severity: Medium]
What happens in _scan_page_types() during --selftest on 32-bit kernels or
kernels without slab.obj_exts_needs_objcg?

In mm/slab.h, obj_exts_needs_objcg in struct freelist_counters (embedded in
struct slab) is only defined under #ifdef CONFIG_64BIT (added in commit
7def2e8549e5 ("mm/slab: introduce crimson_flag and obj_exts_needs_objcg")).

Earlier in _slab_memcg_info(), the access to slab.obj_exts_needs_objcg is
wrapped in its own try/except AttributeError block so that if the field does
not exist, execution falls through to _collect_memcg_ptrs():

    slab = cast("struct slab *", page)
    try:
        if not slab.obj_exts_needs_objcg.value_():
            return "none", "/sys/fs/cgroup/memory/"
    except AttributeError:
        pass

Here in _scan_page_types(), slab.obj_exts_needs_objcg.value_() is at the top
of the single try/except block covering the whole slab classification. When
it raises AttributeError, the entire block is skipped, so none of slab_multi,
slab_single, or slab_no_objcg can be populated and the loop can never satisfy
all(k in found for k in needed) to terminate the full page scan early.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-show_page_info_refactor-v4-1-f28c8176e2d8@linux.dev?part=1