This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ibmveth: Add per-queue RX and TX statistics collection
This adds per-queue RX and TX counter arrays that are allocated at probe
and freed at remove, updates them from the poll/IRQ/xmit paths, and
exposes the values through ethtool -S plus a new ndo_get_stats64
implementation. It also adds the hcall_* counters to the ethtool string
table and reworks the probe error paths and ibmveth_remove() teardown
order.
A few questions below.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index 36f0926711e60..3202b657c9b8f 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -98,7 +98,17 @@ static struct ibmveth_stat ibmveth_stats[] = {
> { "fw_enabled_ipv6_csum", IBMVETH_STAT_OFF(fw_ipv6_csum_support) },
> { "tx_large_packets", IBMVETH_STAT_OFF(tx_large_packets) },
> { "rx_large_packets", IBMVETH_STAT_OFF(rx_large_packets) },
> - { "fw_enabled_large_send", IBMVETH_STAT_OFF(fw_large_send_support) }
> + { "fw_enabled_large_send", IBMVETH_STAT_OFF(fw_large_send_support) },
> + { "hcall_reg_lan_queue", IBMVETH_STAT_OFF(hcall_stats.reg_lan_queue) },
> + { "hcall_reg_lan", IBMVETH_STAT_OFF(hcall_stats.reg_lan) },
> + { "hcall_add_bufs_queue",
> + IBMVETH_STAT_OFF(hcall_stats.add_bufs_queue) },
> + { "hcall_add_bufs", IBMVETH_STAT_OFF(hcall_stats.add_bufs) },
> + { "hcall_add_buf", IBMVETH_STAT_OFF(hcall_stats.add_buf) },
> + { "hcall_free_lan_queue",
> + IBMVETH_STAT_OFF(hcall_stats.free_lan_queue) },
> + { "hcall_free_lan", IBMVETH_STAT_OFF(hcall_stats.free_lan) },
> + { "hcall_send_lan", IBMVETH_STAT_OFF(hcall_stats.send_lan) },
> };
[Severity: Low]
Can these hcall_* values lose increments once they become user visible?
The fields in adapter->hcall_stats are plain u64 bumped with an
unserialized read-modify-write. ibmveth_replenish_task() takes only the
queue-local lock:
spin_lock_irqsave(&rxq->replenish_lock, flags);
so two NAPI instances replenishing different RX queues both reach
adapter->hcall_stats.add_bufs_queue++;
with no shared serialization, and separate TX queues both reach
adapter->hcall_stats.send_lan++;
in ibmveth_send(). Two CPUs can read N and both store N+1.
Would per-queue counters summed on read (the same approach used for the
new rx_qstats/tx_qstats), or atomic64_t, be preferable here?
[ ... ]
> @@ -1011,7 +1075,23 @@ static void ibmveth_update_rx_no_buffer(struct
> ibmveth_adapter *adapter,
> p = adapter->buffer_list_addr[queue_index] + 4096 - 8;
> drops = be64_to_cpup(p);
>
> - adapter->rx_no_buffer = drops;
> + /*
> + * PHYP's buffer-list page counter is absolute for that page. A new
> + * page (reopen / queue reuse after -L) starts near zero; fold the
> + * previous absolute into retired so adapter-level sums stay
> + * monotonic.
> + */
> + if (adapter->rx_qstats) {
> + u64 *slot = &adapter->rx_qstats[queue_index].no_buffer_drops;
> +
> + if (drops < *slot)
> + adapter->rx_no_buffer_retired += *slot;
> + *slot = drops;
> + } else {
> + if (drops < adapter->rx_no_buffer)
> + adapter->rx_no_buffer_retired += adapter->rx_no_buffer;
> + adapter->rx_no_buffer = drops;
> + }
> }
[Severity: Low]
Does the no_buffer_drops slot lose the final PHYP value when a queue is
retired by a channel shrink?
This makes rx_qstats[i].no_buffer_drops a mirror of the page-absolute
counter, and folding into rx_no_buffer_retired only happens when a later
read observes a decrease. ibmveth_update_rx_no_buffer() has only two call
sites, the replenish path and ibmveth_close(), and it early-returns for
queue_index >= ibmveth_get_num_rx_queues(adapter)
Once the incremental resize patch later in this series lands, scale-down
does:
ibmveth_publish_num_rx_queues(adapter, new_count);
...
for (i = new_count; i < old_count; i++)
ibmveth_destroy_subordinate_rx_queue(adapter, i);
which reaches ibmveth_free_single_rx_queue() and dma_unmap_single() plus
free_page() on buffer_list_addr[i]. The live count is already lowered, so
a late harvest is rejected by the guard above, and everything PHYP
recorded in that page since the last replenish read is dropped from both
the per-queue slot and the adapter sum.
Would harvesting the absolute before lowering the published queue count
and freeing the page fix this?
> @@ -2239,22 +2319,158 @@ static int ibmveth_set_features(struct net_device
> *dev,
[ ... ]
> static void ibmveth_get_strings(struct net_device *dev, u32 stringset, u8
> *data)
> {
> + struct ibmveth_adapter *adapter = netdev_priv(dev);
> + u8 *p = data;
> int i;
>
> if (stringset != ETH_SS_STATS)
> return;
>
> - for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++, data += ETH_GSTRING_LEN)
> - memcpy(data, ibmveth_stats[i].name, ETH_GSTRING_LEN);
> + for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++) {
> + memcpy(p, ibmveth_stats[i].name, ETH_GSTRING_LEN);
> + p += ETH_GSTRING_LEN;
> + }
> +
> + for (i = 0; i < ibmveth_get_num_rx_queues(adapter); i++) {
> + ethtool_sprintf(&p, "rx%d_packets", i);
> + ethtool_sprintf(&p, "rx%d_bytes", i);
> + ethtool_sprintf(&p, "rx%d_interrupts", i);
> + ethtool_sprintf(&p, "rx%d_polls", i);
> + ethtool_sprintf(&p, "rx%d_large_packets", i);
> + ethtool_sprintf(&p, "rx%d_invalid_buffers", i);
> + ethtool_sprintf(&p, "rx%d_no_buffer_drops", i);
> + }
> +
> + for (i = 0; i < dev->real_num_tx_queues; i++) {
> + ethtool_sprintf(&p, "tx%d_packets", i);
> + ethtool_sprintf(&p, "tx%d_bytes", i);
> + ethtool_sprintf(&p, "tx%d_large_packets", i);
> + ethtool_sprintf(&p, "tx%d_dropped_packets", i);
> + ethtool_sprintf(&p, "tx%d_send_failures", i);
> + ethtool_sprintf(&p, "tx%d_checksum_offload", i);
> + }
[Severity: Medium]
Should the per-queue packet, byte and drop counters go through the
standard per-queue statistics interface instead of private ethtool
strings?
rx%d_packets, rx%d_bytes, tx%d_packets, tx%d_bytes and
tx%d_dropped_packets map directly onto existing fields:
include/net/netdev_queues.h
struct netdev_stat_ops {
void (*get_queue_stats_rx)(struct net_device *dev, int idx,
struct netdev_queue_stats_rx *stats);
...
The driver adds only .ndo_get_stats64 (device-wide) and never sets
netdev->stat_ops, so the newly collected per-queue values are reachable
only through the private ethtool blob, which cannot be removed once
shipped. The genuinely driver-specific counters (interrupts, polls,
invalid_buffers, no_buffer_drops, send_failures, checksum_offload) look
fine in ethtool -S.
Could the packets/bytes/dropped set be exposed via netdev_stat_ops
qstats instead?
> +
> + for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
> + ethtool_sprintf(&p, "pool%d_size", i);
> + ethtool_sprintf(&p, "pool%d_active", i);
> + ethtool_sprintf(&p, "pool%d_available", i);
> + }
> }
>
> static int ibmveth_get_sset_count(struct net_device *dev, int sset)
> {
> + struct ibmveth_adapter *adapter = netdev_priv(dev);
> +
> switch (sset) {
> case ETH_SS_STATS:
> - return ARRAY_SIZE(ibmveth_stats);
> + return ARRAY_SIZE(ibmveth_stats) +
> + ibmveth_get_num_rx_queues(adapter) *
> + IBMVETH_NUM_RX_QSTATS +
> + dev->real_num_tx_queues * IBMVETH_NUM_TX_QSTATS +
> + IBMVETH_NUM_BUFF_POOLS * 3;
> default:
> return -EOPNOTSUPP;
> }
> @@ -2263,11 +2479,44 @@ static int ibmveth_get_sset_count(struct net_device
> *dev, int sset)
> static void ibmveth_get_ethtool_stats(struct net_device *dev,
> struct ethtool_stats *stats, u64 *data)
> {
[ ... ]
> + for (j = 0; j < IBMVETH_NUM_BUFF_POOLS; j++) {
> + data[i++] = adapter->rx_buff_pool[0][j].size;
> + data[i++] = adapter->rx_buff_pool[0][j].active;
> + data[i++] = atomic_read(&adapter->rx_buff_pool[0][j].available);
> + }
> }
[Severity: Medium]
This is a pre-existing issue rather than something this patch creates for
the lifetime part below, but the commit message describes only per-queue
RX/TX statistics and the hcall_* strings, while the patch also carries two
other changes.
First, three new ethtool -S entries per buffer pool are added:
ethtool_sprintf(&p, "pool%d_size", i);
ethtool_sprintf(&p, "pool%d_active", i);
ethtool_sprintf(&p, "pool%d_available", i);
and counted as IBMVETH_NUM_BUFF_POOLS * 3 in ibmveth_get_sset_count().
The values are read only from adapter->rx_buff_pool[0][j], but
rx_buff_pool is declared as
struct ibmveth_buff_pool
rx_buff_pool[IBMVETH_MAX_RX_QUEUES][IBMVETH_NUM_BUFF_POOLS];
so in multi-queue mode queues 1..N-1 are not represented and the string
names carry no queue index. pool size and active are also already
available through the per-pool sysfs attributes (veth_size_attr,
veth_num_attr, veth_active_attr). Should these strings be dropped, or at
least given a queue index and mentioned in the commit message?
Second, ibmveth_remove() reorders unregister_netdev() ahead of
cancel_work_sync(), and the new ibmveth_probe_cleanup() clears vio
drvdata before free_netdev(). The in-code comments describe both as
fixing existing problems (a worker running after free_netdev(), and CMO
get_desired_dma() seeing a freed netdev on rebind), neither of which the
commit message body mentions and neither of which has a Fixes: tag.
Could these be split into their own patches?
[ ... ]
> @@ -3107,6 +3410,7 @@ static const struct net_device_ops ibmveth_netdev_ops =
> {
> .ndo_validate_addr = eth_validate_addr,
> .ndo_set_mac_address = ibmveth_set_mac_addr,
> .ndo_features_check = ibmveth_features_check,
> + .ndo_get_stats64 = ibmveth_get_stats64,
> #ifdef CONFIG_NET_POLL_CONTROLLER
> .ndo_poll_controller = ibmveth_poll_controller,
> #endif
> @@ -3121,6 +3425,23 @@ static void ibmveth_put_pool_kobjs(struct
> ibmveth_adapter *adapter,
> kobject_put(&adapter->rx_buff_pool[0][i].kobj);
> }
>
> +static void ibmveth_probe_cleanup(struct ibmveth_adapter *adapter,
> + int pools_ready)
> +{
> + struct net_device *netdev = adapter->netdev;
> +
> + cancel_work_sync(&adapter->work);
> + ibmveth_put_pool_kobjs(adapter, pools_ready);
> +
> + ibmveth_free_tx_qstats(adapter);
> + ibmveth_free_rx_qstats(adapter);
> + /* Probe failure never reaches ibmveth_remove(); clear before free so
> + * CMO get_desired_dma() cannot see a freed netdev on rebind.
> + */
> + dev_set_drvdata(&adapter->vdev->dev, NULL);
> + free_netdev(netdev);
> +}
> +
[ ... ]
> @@ -3332,12 +3651,20 @@ static void ibmveth_remove(struct vio_dev *dev)
> struct ibmveth_adapter *adapter = netdev_priv(netdev);
> int i;
>
> - cancel_work_sync(&adapter->work);
> -
> for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
> kobject_put(&adapter->rx_buff_pool[0][i].kobj);
>
> + /*
> + * Unregister first so NAPI/xmit cannot re-arm reset work after we
> + * cancel it. cancel_work_sync() before unregister left a window
> + * where poll could schedule_work() and the worker ran after
> + * free_netdev().
> + */
> unregister_netdev(netdev);
> + cancel_work_sync(&adapter->work);
> +
> + ibmveth_free_tx_qstats(adapter);
> + ibmveth_free_rx_qstats(adapter);
>
> free_netdev(netdev);
> dev_set_drvdata(&dev->dev, NULL);
[Severity: High]
With cancel_work_sync() now running after unregister_netdev(), can the
reset worker re-open an already unregistered netdev?
adapter->work is queued from several error paths, for example
ibmveth_replenish_task() on an invalid free_map entry or on MQ H_FUNCTION
after LPM, and from ibmveth_poll_skip_bad_correlator():
schedule_work(&adapter->work);
The handler has no registration or running check:
drivers/net/ethernet/ibm/ibmveth.c:ibmveth_reset() {
rtnl_lock();
dev_close(adapter->netdev);
dev_open(adapter->netdev, NULL);
rtnl_unlock();
}
A worker already queued (or already blocked in rtnl_lock() while
unregister_netdev() holds rtnl) proceeds once unregistration finishes.
dev_close() is then a no-op, and the re-open is not rejected:
net/core/dev.c:netif_open() {
if (dev->flags & IFF_UP)
return 0;
ret = __dev_open(dev, extack);
}
net/core/dev.c:__dev_open() {
ASSERT_RTNL();
dev_addr_check(dev);
if (!netif_device_present(dev)) {
...
if (!ret && ops->ndo_open)
ret = ops->ndo_open(dev);
}
IFF_UP was cleared by unregister and __LINK_STATE_PRESENT is never
cleared (the driver does not call netif_device_detach()), so
ibmveth_open() runs again on the unregistered device: it re-requests the
RX IRQs with dev_id = &adapter->napi[i], re-registers the logical LAN
with PHYP, and re-arms DMA into freshly allocated buffer lists.
cancel_work_sync() then returns and remove() continues into
ibmveth_free_tx_qstats() / ibmveth_free_rx_qstats() and free_netdev(),
so the installed IRQ handler and the hypervisor are left pointing at
freed memory. The next RX interrupt would execute
adapter->rx_qstats[qindex].interrupts++;
in ibmveth_interrupt() against a freed adapter and a freed qstat array.
Would a netif_running() or reg_state check inside ibmveth_reset(), or a
"removing" flag set before unregister_netdev(), close this?
