Re: [PATCH v7 41/59] perf netdev-times: Port netdev-times to use python module

sashiko-bot
2026-04-25T23:07:41Z

Sashiko AI review found 6 potential issue(s):
- [High] Unhandled exception when calling `.comm()` on an unknown thread causes the script to crash.
- [Medium] RX `kfree_skb` events are incorrectly misidentified and printed as `skb_copy_datagram_iovec`.
- [Medium] Brittle event name matching can bypass attribute extraction and cause KeyErrors.
- [Medium] `handle_consume_skb` completely omits RX tracking, leaving successfully delivered packets untracked.
- [Low] Manual iteration over `sys.argv` ignores `argparse` validation and introduces brittle parsing behavior.
- [Low] Incorrect attribute extraction for `skb:skb_copy_datagram_iovec` requests `skblen` instead of `len`.
--

commit 38c100f5ac87ac6f769261a5e074d27809005ec6
Author: Ian Rogers <irogers@google.com>
perf netdev-times: Port netdev-times to use python module

This commit ports the netdev-times script to a Python module format.
It uses a class structure to encapsulate state and processes events using
perf.session in time order. Tracepoint fields are directly extracted
from sample attributes.

> diff --git a/tools/perf/python/netdev-times.py b/tools/perf/python/netdev-times.py
> new file mode 100755
> index 000000000000..3fe46b4e7f21
> --- /dev/null
> +++ b/tools/perf/python/netdev-times.py
[ ... ]
> + def print_receive(self, hunk: dict) -> None:
[ ... ]
> + if 'comm' in event:
> + print(PF_WJOINT)
> + print(PF_CPY_DGRAM %
> + (self.diff_msec(base_t, event['comm_t']),
> + event['pid'], event['comm']))
> + elif 'handle' in event:
> + print(PF_WJOINT)
> + if event['handle'] == "kfree_skb":
> + print(PF_KFREE_SKB %
> + (self.diff_msec(base_t, event['comm_t']),
> + event['location']))

Because handle_kfree_skb() updates rec_data with a 'comm' key, will the
check here for 'comm' match first? This appears to cause kfree_skb events
to be misidentified and printed using the PF_CPY_DGRAM format instead of
PF_KFREE_SKB, making the kfree_skb block dead code.

[ ... ]
> + def handle_consume_skb(self, event: dict) -> None:
> + """Handle skb:consume_skb event."""
> + time = event['time']
> + skbaddr = event['skbaddr']
> + for i, skb in enumerate(self.tx_xmit_list):
> + if skb['skbaddr'] == skbaddr:
> + skb['free_t'] = time
> + self.tx_free_list.append(skb)
> + del self.tx_xmit_list[i]
> + return

Does this omit receive tracking for consumed SKBs? The handler only iterates
over tx_xmit_list and returns, failing to check rx_skb_list. This would
leave successfully delivered packets untracked on the receive path,
leaving the consume_skb format logic in print_receive() unused.

[ ... ]
> + def process_event(self, sample: perf.sample_event) -> None:
> + """Process events directly on-the-fly."""
> + name = str(sample.evsel)
> + pid = sample.sample_pid
> + if hasattr(self, 'session') and self.session:
> + comm = self.session.find_thread(pid).comm()
> + else:
> + comm = "Unknown"

Can this cause an unhandled exception if find_thread(pid) returns None?
For idle threads or PIDs missing PERF_RECORD_COMM, find_thread() can fail,
and calling .comm() directly on the result could crash the script.

> + event_data = {
> + 'name': name[6:-1] if name.startswith("evsel(") else name,
> + 'time': sample.sample_time,
> + 'cpu': sample.sample_cpu,
> + 'pid': pid,
> + 'comm': comm,
> + }
> +
> + # Extract specific fields based on event type
> + if name.startswith("evsel(irq:softirq_"):

Does the event name extraction safely handle cases where the event
name does not start with "evsel("? The event_data 'name' handles this,
but the subsequent if/elif chain uses the raw 'name' string and hardcodes
the "evsel(" wrapper. If the string representation lacks this prefix, the
fields might not be extracted, leading to a KeyError later.

[ ... ]
> + elif name == "evsel(skb:skb_copy_datagram_iovec)":
> + event_data['skbaddr'] = getattr(sample, "skbaddr", 0)
> + event_data['skblen'] = getattr(sample, "skblen", 0)

Is skblen the correct attribute here? The kernel tracepoint for
skb:skb_copy_datagram_iovec exports this field as "len", so this might
silently default to 0.

[ ... ]
> +if __name__ == "__main__":
> + ap = argparse.ArgumentParser(description="Display a process of packets and processed time.")
> + ap.add_argument("-i", "--input", default="perf.data", help="Input file name")
> + ap.add_argument("tx", nargs="?", help="show only tx chart")
> + ap.add_argument("rx", nargs="?", help="show only rx chart")
> + ap.add_argument("dev", nargs="?", help="show only specified device")
> + ap.add_argument("debug", nargs="?", help="work with debug mode. It shows buffer status.")
> + args = ap.parse_args()
> +
> + parsed_args = argparse.Namespace(tx=False, rx=False, dev=None, debug=False, input=args.input)
> +
> + for arg in sys.argv[1:]:
> + if arg == 'tx':

Is it expected to iterate directly over sys.argv[1:] here? If an argparse
option like "-i tx" is passed, the manual loop might see 'tx' and
erroneously enable the transmit chart mode instead of respecting the
argparse validation.

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