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

sashiko-bot
2026-04-23T04:11:04Z

Sashiko AI review found 6 potential issue(s):
- [High] Brittle string matching for the `evsel(` prefix will cause unhandled `KeyError` exceptions if the prefix is missing.
- [High] `consume_skb` events for received packets are ignored because `handle_consume_skb` skips the receive list.
- [High] `print_receive()` falsely prints `kfree_skb` events as `skb_copy_datagram_iovec` due to shadowed dictionary keys.
- [Medium] The commit message contradicts the actual code execution regarding event processing order.
- [Medium] Manual iteration over `sys.argv[1:]` conflicts with `argparse`, treating arbitrary arguments or input filenames as command flags.
- [Medium] The use of `list.insert(0, ...)` on large bounded arrays creates an O(N^2) performance bottleneck.
--

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

perf netdev-times: Port netdev-times to use python module

This commit ports the netdev-times script to Python 3, refactoring it
to use a class structure and the perf.session module to collect and
process events.

> perf netdev-times: Port netdev-times to use python module
>
> Ported from tools/perf/scripts/python/.
> - Refactored the script to use a class structure (NetDevTimesAnalyzer)
> to encapsulate state.
> - Used perf.session for event collection and processed them in time
> order at the end to match legacy behavior.

Does the commit message accurately reflect the implementation? The code in
process_event() appears to call handle_single_event() immediately, processing
events on the fly as they are received rather than collecting them for
end-of-run time ordering.

> - Extracted tracepoint fields directly from sample attributes.
> - Moved format string constants to module level.
> - Cleaned up Python 2 compatibility artifacts (like cmp_to_key).
>
> Assisted-by: Gemini:gemini-3.1-pro-preview
> Signed-off-by: Ian Rogers <irogers@google.com>

diff --git a/tools/perf/python/netdev-times.py b/tools/perf/python/netdev-times.py
new file mode 100755
index 0000000000000..568986e2d4924
--- /dev/null
+++ b/tools/perf/python/netdev-times.py
[ ... ]
> + def print_receive(self, hunk: dict) -> None:
[ ... ]
> + else:
> + print(PF_NET_RECV %
> + (self.diff_msec(base_t, event['event_t']),
> + event['skbaddr'],
> + event['len']))
> + 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']))

Will kfree_skb events be misreported as skb_copy_datagram_iovec here?
Since handle_kfree_skb() also injects the 'comm' key into the event, the
first condition ('comm' in event) will evaluate to true, preventing the
'handle' in event branch from executing.

[ ... ]
> + def handle_netif_receive_skb(self, event: dict) -> None:
> + """Handle net:netif_receive_skb event."""
> + time = event['time']
> + cpu = event['cpu']
> + skbaddr = event['skbaddr']
> + skblen = event['skblen']
> + if cpu in self.net_rx_dic:
> + rec_data = {'event_name': 'netif_receive_skb',
> + 'event_t': time, 'skbaddr': skbaddr, 'len': skblen}
> + event_list = self.net_rx_dic[cpu]['event_list']
> + event_list.append(rec_data)
> + self.rx_skb_list.insert(0, rec_data)

Could inserting at the beginning of the list cause a performance bottleneck?
In Python, inserting at index 0 requires shifting all subsequent elements.
With the list bounded to 65,536 elements, this could degrade execution time
when processing many events.

[ ... ]
> + 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 function also need to loop through self.rx_skb_list, similarly
to handle_kfree_skb()? By skipping the receive list, incoming packet events
will never be matched with their consume_skb counterpart, and they will not
be reported properly in print_receive().

[ ... ]
> + 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.process(pid).comm()
> + else:
> + comm = "Unknown"
> + 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_"):
> + event_data['vec'] = getattr(sample, "vec", 0)
> + # Filter for NET_RX
> + try:
> + if perf.symbol_str("irq:softirq_entry", "vec", # type: ignore
> + event_data['vec']) != "NET_RX":
> + return
> + except AttributeError:
> + # Fallback if symbol_str not available or fails
> + if event_data['vec'] != 3: # NET_RX_SOFTIRQ is usually 3
> + return
> + elif name == "evsel(irq:irq_handler_entry)":

What happens if str(sample.evsel) lacks the "evsel(" prefix?
The condition for extracting tracepoint-specific fields would fail.
Since the event name in event_data has the prefix stripped, it would still
be dispatched to handle_irq_handler_entry(), which would then try to access
event['irq'] and result in a KeyError crash.

[ ... ]
> +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':
> + parsed_args.tx = True

Is there a chance this manual loop over sys.argv conflicts with argparse?
If a user passes an input file named "tx" via "-i tx" or "--input tx", the
manual loop will see the string "tx" and incorrectly activate parsed_args.tx.

[ ... ]

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