Re: [PATCH v5 26/58] perf mem-phys-addr: Port mem-phys-addr to use python module

sashiko-bot
2026-04-24T17:08:32Z

Sashiko AI review found 4 potential issue(s):
- [Medium] Python 3.10+ requirement breaks compatibility on older LTS distributions.
- [Medium] Per-sample print statements for unresolved physical addresses will severely spam stdout.
- [Medium] Missing validation of physical address presence risks silent output corruption.
- [Low] Logic flaw in binary search return value handling causes unintended negative list indexing.
--

commit 2fe83bab7027dd98239e7ac8be26c0436646b4a9
Author: Ian Rogers <irogers@google.com>

perf mem-phys-addr: Port mem-phys-addr to use python module

This commit adds an example script demonstrating the perf python API to
load a perf.data file and resolve physical address samples based on
/proc/iomem.

> diff --git a/tools/perf/python/mem-phys-addr.py b/tools/perf/python/mem-phys-addr.py
> new file mode 100755
> index 00000000000000..ba874d7a2011cf
> --- /dev/null
> +++ b/tools/perf/python/mem-phys-addr.py
> @@ -0,0 +1,117 @@

[ ... ]

> +# Physical memory layout from /proc/iomem. Key is the indent and then
> +# a list of ranges.
> +iomem: Dict[int, list[IomemEntry]] = collections.defaultdict(list)
> +# Child nodes from the iomem parent.
> +children: Dict[IomemEntry, set[IomemEntry]] = collections.defaultdict(set)

The built-in collection parameterization using list[IomemEntry] and
set[IomemEntry] was introduced in Python 3.9.

Will this script crash with a TypeError on systems running older Python
environments like Python 3.8?

[ ... ]

> +def find_memory_type(phys_addr) -> Optional[IomemEntry]:
> + """Search iomem for the range containing phys_addr with the maximum indent"""
> + for i in range(max_indent, -1, -1):
> + if i not in iomem:
> + continue
> + position = bisect.bisect_right(iomem[i], phys_addr,
> + key=lambda entry: entry.begin)

The key parameter for bisect.bisect_right() was added in Python 3.10.

Does this create a strict dependency on Python 3.10+?

> + if position is None:
> + continue

The bisect_right function always returns an integer, so checking for None
appears to be dead code.

> + iomem_entry = iomem[i][position-1]

If phys_addr is strictly less than the first element's begin address,
bisect_right returns 0.

Does this cause position - 1 to evaluate to -1, which then inadvertently
accesses the last element of the iomem[i] list?

> + if iomem_entry.begin <= phys_addr <= iomem_entry.end:
> + return iomem_entry
> + print(f"Didn't find {phys_addr}")

Since find_memory_type() is called for every single sample in the dataset,
won't this print statement heavily spam standard output if a dataset contains
many unmapped physical addresses?

> + return None

[ ... ]

> + def process_event(sample):
> + """Process a single sample event."""
> + phys_addr = sample.sample_phys_addr
> + entry = find_memory_type(phys_addr)

If a user runs the script on a perf dataset recorded without --phys-data,
sample_phys_addr may default to 0.

Without validating if physical addresses are actually present in the samples,
could this silently attribute all such samples to whatever memory range
contains address 0?

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260424164721.2229025-1-irogers@google.com?part=26