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

sashiko-bot
2026-04-25T18:12:33Z

Sashiko AI review found 5 potential issue(s):
- [High] Inconsistent event name matching logic leads to KeyError crashes on environments lacking the 'evsel(' wrapper.
- [High] Dropped packets (kfree_skb) are incorrectly printed as successfully copied datagrams.
- [High] Missing 'consume_skb' handling for received SKBs leads to missed consumption events.
- [Medium] Argument parsing logic is disconnected from argparse definitions.
- [Medium] Softirq chunks that process zero packets are silently dropped.
--

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

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

This patch ports the legacy Python script netdev-times.py to a Python 3
module using a class-based structure. It utilizes the perf.session API to
process events and collect traces.

> 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
> @@ -0,0 +1,472 @@
[ ... ]
> + 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:

When handle_kfree_skb() processes an event, it updates rec_data with both
'handle' and 'comm':

rec_data.update({'handle': "kfree_skb",
'comm': comm, ...})

Will this cause kfree_skb events to match the "if 'comm' in event"
condition and mistakenly print as successfully copied datagrams?

[ ... ]
> + def handle_irq_softirq_exit(self, event: dict) -> None:
> + """Handle irq:softirq_exit event."""
> + time = event['time']
> + cpu = event['cpu']
> + irq_list = []
> + event_list = []
> + sirq_ent_t = None
> +
> + if cpu in self.irq_dic:
> + irq_list = self.irq_dic[cpu]
> + del self.irq_dic[cpu]
> + if cpu in self.net_rx_dic:
> + sirq_ent_t = self.net_rx_dic[cpu]['sirq_ent_t']
> + event_list = self.net_rx_dic[cpu]['event_list']
> + del self.net_rx_dic[cpu]
> + if not irq_list or not event_list or sirq_ent_t is None:
> + return

If a softirq execution processes no packets, event_list will be an empty
list. Since an empty list evaluates to false in Python, will this cause
chunks that process zero packets to be silently dropped?

[ ... ]
> + 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 logic miss consumption events for received SKBs?

Unlike handle_kfree_skb(), which searches rx_skb_list for a matching
skbaddr, this function only iterates over tx_xmit_list. Could this leave
received SKBs in rx_skb_list without marking them as consumed?

[ ... ]
> + 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"
> + 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)

Is it possible for the environment's perf bindings to omit the "evsel("
wrapper?

If name does not start with "evsel(", event_data['name'] will remain
unwrapped, so it can still successfully route via handle_single_event().

However, the "if name.startswith('evsel(')" branches here will be skipped,
meaning fields like 'irq' or 'skbaddr' won't be extracted.

Will this lead to KeyError crashes in handler functions like
handle_irq_handler_entry() when they try to access those missing fields?

[ ... ]
> +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
> + elif arg == 'rx':
> + parsed_args.rx = True
> + elif arg.startswith('dev='):
> + parsed_args.dev = arg[4:]
> + elif arg == 'debug':
> + parsed_args.debug = True

Is it intentional to manually iterate over sys.argv[1:] after setting up
and calling ap.parse_args()?

This approach seems to disconnect the argument parsing from the argparse
definitions. Valid argparse invocations, such as passing a device name
positionally, appear to be ignored.

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