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

[sashiko-bot@kernel.org]

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