Re: [PATCH v3] 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 3 potential issue(s) to consider:
- [Medium] tools/mm: _page_node_zone() aborts NUMA loop on NULL node_data entry
- [Medium] tools/mm: _vma_anon_pgoff() skips __vm_anon_pgoff_lo on 32-bit kernels
- [Medium] tools/mm: _page_fields() reads overlaid _mapcount on typed pages
--

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

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

This commit rewrites tools/mm/show_page_info.py into a modular field-collector
architecture, adds --pfn, --page, --user, and --selftest modes, fixes slab,
memcg, and compound page handling, and adds reverse mapping, PTE walk, and
zone/node lookups.

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

[ ... ]

> +# ---------------------------------------------------------------------------
> +# Reverse mapping (rmap)
> +# ---------------------------------------------------------------------------
> +
> +def _vma_anon_pgoff(vma):
> +    """Get anon pgoff for a VMA (vma_start_anon_pgoff)."""
>      try:
> -        print(f'PID: {pid} Comm: {task.comm.string_().decode()} mm: {hex(mm)}')
> +        hi = vma.__vm_anon_pgoff_hi.value_()
> +        lo = vma.__vm_anon_pgoff_lo.value_()
> +        return (hi << 32) | lo
> +    except AttributeError:
> +        return vma.vm_pgoff.value_()

[Severity: Medium]
On 32-bit kernels (!CONFIG_64BIT), will _vma_anon_pgoff() skip reading
__vm_anon_pgoff_lo and fall back to vma.vm_pgoff instead?

In include/linux/mm.h:vma_start_anon_pgoff(), __vm_anon_pgoff_lo is always
present in struct vm_area_struct while __vm_anon_pgoff_hi is only defined
under CONFIG_64BIT:

    static inline pgoff_t vma_start_anon_pgoff(const struct vm_area_struct *vma)
    {
        pgoff_t pgoff = 0;

    #ifdef CONFIG_64BIT
        pgoff += vma->__vm_anon_pgoff_hi;
        pgoff <<= 32;
    #endif
        pgoff += vma->__vm_anon_pgoff_lo;
        return pgoff;
    }

When show_page_state() -> _rmap_fields() -> get_rmap_info() calls
_vma_anon_pgoff() on a 32-bit kernel, accessing vma.__vm_anon_pgoff_hi raises
AttributeError before vma.__vm_anon_pgoff_lo is read. For MAP_PRIVATE
file-backed VMAs with COW anonymous pages, returning vma.vm_pgoff uses the
file offset rather than the anonymous page offset, causing get_rmap_info() and
_vma_owner() to miss the VMA in the range check or compute the wrong virtual
address.

[ ... ]

> +def _page_node_zone(page):
> +    """Find node ID and zone name via PFN range lookup.
> +
> +    Uses prog["node_data"] which is x86-specific. Falls back to
> +    contig_page_data for UMA (single-node) systems. May return
> +    (-1, "Unknown") on architectures where neither symbol exists.
> +    """
> +    pfn = page_to_pfn(page).value_()
> +    max_zones = prog.constant("__MAX_NR_ZONES").value_()
> +    try:
> +        nr_nodes = prog["nr_node_ids"].value_()
> +        node_data = prog["node_data"]
> +        for nid in range(nr_nodes):
> +            try:
> +                pgdat = node_data[nid].read_()
> +            except (IndexError, FaultError):
> +                continue
> +            for zid in range(max_zones):
> +                zone = pgdat.node_zones[zid]
> +                start = zone.zone_start_pfn.value_()
> +                spanned = zone.spanned_pages.value_()
> +                if spanned and start <= pfn < start + spanned:
> +                    return nid, zone.name.string_().decode()
> +    except (KeyError, FaultError):
> +        pass

[Severity: Medium]
What happens in _page_node_zone() on a NUMA system when an earlier slot in
node_data[nid] is NULL (for example, an offline or unpopulated node)?

node_data[nid].read_() reads the pointer element from the node_data array and
returns a valid drgn pointer Object with value 0x0 without raising FaultError.
When the inner loop then evaluates zone.zone_start_pfn.value_(), dereferencing
the NULL pgdat raises FaultError outside the inner try block.

That FaultError is caught by the outer try/except block, which aborts
iteration over all remaining NUMA nodes and causes _page_node_zone() to return
(-1, "Unknown") for pages on higher-numbered nodes. Should this check if
not pgdat: continue after reading node_data[nid]?

[ ... ]

> +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
> +            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]
In _page_fields(), can reading head._mapcount.counter.value_() report bogus
negative values for non-slab typed pages?

In struct page, page_type and _mapcount share a union. When show_page_state()
inspects a non-slab page that has a page_type (where _page_type(head) is not
None, such as PGTY_table, PGTY_hugetlb, PGTY_zsmalloc, or PGTY_large_kmalloc),
_page_fields() still reads head._mapcount.counter.value_() and prints the raw
PGTY_* bit pattern as a large negative integer (for example, -218103809 for
0xf2ffffff on PGTY_table).

Should _page_fields() skip "Page Mapcount" when _page_type(head) is not None?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-show_page_info_refactor-v3-1-cc74947cbb8b@linux.dev?part=1