Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] Statistics readers now take every RX queue's replenish_lock
with spin_lock_bh().
--
ibmveth: Add per-queue RX and TX statistics collection
This patch moves the adapter-wide RX and TX counters into per-queue
ibmveth_rx_queue_stats and ibmveth_tx_queue_stats arrays allocated at
probe. It exports packets, bytes and hardware drops through
netdev_stat_ops, adds ndo_get_stats64(), and keeps the existing ethtool -S
keys by summing the per-queue slots. It also adds a per-queue
no_buffer_retired carry so that rx_no_buffer never goes backwards across a
reopen.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index a0c3c06d2b9d9..45adf6e9be09b 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -1063,7 +1197,45 @@ 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;
> + qstats = &adapter->rx_qstats[queue_index];
> +
> + if (drops < qstats->no_buffer_drops)
> + IBMVETH_STAT_ADD(qstats->no_buffer_retired,
> + qstats->no_buffer_drops);
> + WRITE_ONCE(qstats->no_buffer_drops, drops);
> +}
> +
> +/* Snapshot live+retired under replenish_lock; see retire_rx_no_buffer(). */
> +static u64 ibmveth_rx_no_buffer_pair(struct ibmveth_adapter *adapter, int i)
> +{
> + struct ibmveth_rx_q *rxq = &adapter->rx_queue[i];
> + struct ibmveth_rx_queue_stats *qstats = &adapter->rx_qstats[i];
> + u64 drops, retired;
> +
> + spin_lock_bh(&rxq->replenish_lock);
> + drops = qstats->no_buffer_drops;
> + retired = qstats->no_buffer_retired;
> + spin_unlock_bh(&rxq->replenish_lock);
> + return drops + retired;
> +}
[Severity: Medium]
Should the stats readers be taking replenish_lock here?
This lock sits on the RX datapath. ibmveth_remove_buffer_from_pool() takes
it from NAPI for every received frame. ibmveth_replenish_task() holds it
for a whole multi-pool refill, which includes netdev_alloc_skb(),
dma_map_single_attrs() and the h_add_logical_lan_buffers*() hcalls.
The new ndo_get_stats64 calls this helper for every slot, whether the
queue is live or not:
ibmveth_get_stats64() {
for (i = 0; i < IBMVETH_MAX_RX_QUEUES; i++) {
...
stats->rx_missed_errors +=
ibmveth_rx_no_buffer_pair(adapter, i);
}
}
The rx_no_buffer ethtool -S key, ibmveth_get_queue_stats_rx() and
ibmveth_get_base_stats() also call it.
As a result, every /proc/net/dev read, RTM_GETLINK dump or bonding/team
stats poll takes up to 16 BH-disabling spinlocks. Unprivileged users can
trigger these reads.
Each acquisition pulls the lock cacheline away from the NAPI CPUs. A
reader can also spin with BH disabled for as long as a refill takes, for
example during the initial replenish in ibmveth_open().
Before this patch the driver had no ndo_get_stats64, and netdev->stats was
read without taking any lock.
Writers already hold replenish_lock. Could they keep a single combined
monotonic value (retired plus live drops) updated with WRITE_ONCE()? Then
readers could use READ_ONCE() and skip the lock.
This also looks unchanged at the end of the series, in "ibmveth: Complete
set_channels down-path". There, ibmveth_rx_no_buffer_pair() still uses
spin_lock_bh() and ibmveth_get_stats64() still calls it for every slot.
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109235556.60604-1-mmc%40linux.ibm.com
