Re: [net-next,v6,11/15] ibmveth: Add per-queue RX and TX statistics collection

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 6 · Low: 0

- [Medium] The patch bundles an independently applicable use-after-free
  fix for the reset worker into a statistics feature patch and…
- [Medium] The new ibmveth_get_stats64() populates rx_packets/rx_bytes
  and tx_packets/tx_bytes/tx_dropped but never sets rx_missed_errors or…
- [Medium] ibmveth_get_queue_stats_tx() (and the retired-queue part of
  ibmveth_get_base_stats()) exports the driver's software TX drop…
- [Medium] The new per-queue ethtool -S key tables add driver-private
  strings for counters that already have standard uAPI fields, while the…
- [Medium] ibmveth_update_rx_no_buffer() detects a buffer-list page
  replacement only by the heuristic `if (drops <…
- [Medium] ibmveth_close() calls ibmveth_update_rx_no_buffer() for every
  live queue without holding that queue's replenish_lock, while netpoll…

--

Below is an automated review report for this commit.

ibmveth: Add per-queue RX and TX statistics collection

This moves the ibmveth RX and TX counters out of struct ibmveth_adapter
into per-queue arrays allocated at probe and freed at remove, adds
netdev_stat_ops and ndo_get_stats64, and makes the ethtool -S block
variable length with new per-queue keys.

[ ... ]

[ ... ]

+/*
+ * Only counters with no home in the standard interfaces belong here.
+ * packets, bytes and drops are reported through netdev_stat_ops.
+ */
+static const struct ibmveth_qstat ibmveth_rx_qstat_keys[] = {
+	{ "rx%d_interrupts", IBMVETH_RXQ_OFF(interrupts) },
+	{ "rx%d_polls", IBMVETH_RXQ_OFF(polls) },
+	{ "rx%d_large_packets", IBMVETH_RXQ_OFF(large_packets) },
+	{ "rx%d_invalid_buffers", IBMVETH_RXQ_OFF(invalid_buffers) },
+	{ "rx%d_no_buffer_drops", IBMVETH_RXQ_OFF(no_buffer_drops) },
+};
+
+static const struct ibmveth_qstat ibmveth_tx_qstat_keys[] = {
+	{ "tx%d_large_packets", IBMVETH_TXQ_OFF(large_packets) },
+	{ "tx%d_send_failures", IBMVETH_TXQ_OFF(send_failures) },
+	{ "tx%d_checksum_offload", IBMVETH_TXQ_OFF(checksum_offload) },
+};

[Severity: Medium]
Do some of these new ethtool -S keys duplicate counters that already have
a standard netlink representation?

The comment above the tables says only counters with no home in the
standard interfaces belong here, and the changelog says ethtool -S keeps
"only the driver-specific keys that have no standard equivalent". Looking
at Documentation/netlink/specs/netdev.yaml:

  tx-hw-gso-packets: Number of packets that necessitated segmentation
                     into smaller packets by the device
  tx-needs-csum:     Number of packets that required the device to
                     calculate the checksum
  rx-hw-drop-overruns: Number of packets dropped due to transient lack of
                     resources, such as buffer space, host descriptors etc.

tx%d_large_packets is the large-send/GSO packet count, tx%d_checksum_offload
is incremented exactly where the driver asks the device to compute the
checksum, and rx%d_no_buffer_drops is a buffer-exhaustion drop count.

Meanwhile the new callbacks added in this same patch fill only packets,
bytes, hw_drops and alloc_fail, leaving hw_gso_packets, needs_csum and
hw_drop_overruns unset. Could those standard fields be filled in
ibmveth_get_queue_stats_rx()/ibmveth_get_queue_stats_tx() instead of
adding private strings, so the values are not exported through two
different interfaces?

rx%d_invalid_buffers is also summed into hw_drops by
ibmveth_get_queue_stats_rx(), so it appears in both places as well.

[ ... ]

[Severity: Medium]
Can the retired page's count be lost when the new page is already ahead
of the old value at the first sample?

The only signal that the buffer-list page was replaced is the numeric
decrease. ibmveth_close() samples the counter and leaves it in
no_buffer_drops:

	for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++)
		ibmveth_update_rx_no_buffer(adapter, i);
	ibmveth_free_all_queues(adapter);

and the next ibmveth_open() gets a fresh zeroed page from
ibmveth_alloc_rx_queues():

		adapter->buffer_list_addr[i] =
			(void *)get_zeroed_page(GFP_KERNEL);

Between ibmveth_register_rx_queues() and the first per-queue
ibmveth_replenish_task() in ibmveth_open() the LAN is live with no buffers
posted, so PHYP can push the new page's counter up before the driver reads
it. If the previous session ended at 50 and the new page reads 500 on the
first sample, the "drops < qstats->no_buffer_drops" test is false and those
50 drops disappear from the adapter-wide rx_no_buffer key, from per-queue
rx-hw-drops and from ibmveth_get_base_stats().

Would recording the retirement at the point the page is actually released
(close and queue reuse) be more reliable than inferring it from a decrease?

[Severity: Medium]
This turns a single idempotent store into a two-field accumulating
read-modify-write. Is every caller holding that queue's replenish_lock?

ibmveth_close() calls it without the lock:

	for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++)
		ibmveth_update_rx_no_buffer(adapter, i);

while netpoll can run the same update concurrently under the lock:

  netpoll_poll_dev() -> ndo_poll_controller
    ibmveth_poll_controller()
      ibmveth_replenish_task(adapter, i)
        spin_lock_irqsave(&rxq->replenish_lock, flags)
        ...
        ibmveth_update_rx_no_buffer(adapter, queue_index)

ibmveth_close() is also called directly from ibmveth_set_csum_offload(),
ibmveth_set_tso(), ibmveth_change_mtu() and veth_pool_store() while
netif_running() is still true, so netpoll_poll_disable() has not run.
netpoll_poll_dev() only bails on a missing netpoll info, a held dev_lock,
!netif_running() or local xmit activity, and ndo_poll_controller is not
gated by NAPI state.

With the old single store the worst case was a stale snapshot; with the
new "no_buffer_retired += no_buffer_drops; no_buffer_drops = drops"
sequence an interleaving can double the carry or lose it permanently.

The RX shrink path later in the series wraps the identical call:

	spin_lock_irqsave(&rxq->replenish_lock, flags);
	ibmveth_update_rx_no_buffer(adapter, i);
	spin_unlock_irqrestore(&rxq->replenish_lock, flags);

with the comment that netpoll still reaches these queues. Should the close
path take the same lock?

[ ... ]

[Severity: Medium]
Could this reg_state gate, together with the ibmveth_remove() reorder
further down, be split out as its own patch with a Fixes: tag?

The changelog acknowledges it: "That reorder is a use-after-free fix in
its own right; it is carried here because this patch depends on it. No
Fixes: tag".

At the baseline the ordering defect is real. ibmveth_remove() has:

	cancel_work_sync(&adapter->work);

	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
		kobject_put(&adapter->rx_buff_pool[i].kobj);

	unregister_netdev(netdev);

and the RX path can re-arm the work after that cancel:

  ibmveth_poll_skip_bad_correlator() -> schedule_work(&adapter->work)
  ibmveth_replenish_task()           -> schedule_work(&adapter->work)

so the worker can run after free_netdev(). Both halves of that fix are
here inside a roughly 500 line feature commit with no Fixes: tag, which
makes the fix hard to identify or backport on its own.

[ ... ]

[Severity: Medium]
Should this also fill rx_missed_errors or rx_dropped?

The RX loop reports only packets and bytes, while the TX loop does report
tx_dropped, so the RX drops the driver already tracks are not visible in
rtnl_link_stats64 at all.

include/uapi/linux/if_link.h describes the matching field:

 * @rx_missed_errors: Count of packets missed by the host.
 *   Counts number of packets dropped by the device due to lack
 *   of buffer space.

which is exactly what PHYP's buffer-list page counter records and what
ibmveth_get_queue_stats_rx() below exports as rx-hw-drops.

Documentation/networking/statistics.rst says:

  Drivers should report all statistics which have a matching member in
  struct rtnl_link_stats64 exclusively via `.ndo_get_stats64`.

dev_get_stats() does not derive rtnl stats from netdev_stat_ops, so
"ip -s -s link" shows zero RX drops on a device whose qstats and
ethtool -S rx_no_buffer key are both climbing. Is that divergence
intended?

+
+static void ibmveth_get_queue_stats_rx(struct net_device *dev, int idx,
+				       struct netdev_queue_stats_rx *stats)
+{

[ ... ]

+static void ibmveth_get_queue_stats_tx(struct net_device *dev, int idx,
+				       struct netdev_queue_stats_tx *stats)
+{
+	struct ibmveth_adapter *adapter = netdev_priv(dev);
+
+	stats->packets = adapter->tx_qstats[idx].packets;
+	stats->bytes = adapter->tx_qstats[idx].bytes;
+	stats->hw_drops = adapter->tx_qstats[idx].dropped_packets;
+}

[Severity: Medium]
Does dropped_packets match what tx-hw-drops is specified to count?

Documentation/netlink/specs/netdev.yaml:

        name: tx-hw-drops
        doc: |
          Number of packets that arrived at the device but never left it,

In ibmveth_start_xmit() the counter is bumped for several conditions that
happen entirely in the driver, before ibmveth_send() issues any hypercall:

	if (unlikely(!adapter->tx_ltb_ptr[queue_num])) {
		adapter->tx_qstats[queue_num].dropped_packets++;
		goto out;
	}

plus the ibmveth_is_packet_unsupported() loopback rejection, the
skb_checksum_help() failure, skb->len > adapter->tx_ltb_size, and
total_bytes != skb->len. None of those packets reached the device.

The loopback case is reachable by any local sender addressing the
device's own MAC. Would rtnl_link_stats64::tx_dropped alone (which this
patch already populates) be the right home for these, keeping hw_drops
for the ibmveth_send() failure only?

The same mapping is used for the retired queues in
ibmveth_get_base_stats():

	tx->hw_drops += adapter->tx_qstats[i].dropped_packets;

[ ... ]

This is the second half of the reordering discussed above at
ibmveth_reset(); the same question about lifting it into its own
Fixes:-tagged patch applies here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788102125.git.mmc%40linux.ibm.com