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

[mingming cao <mmc@linux.ibm.com>]

On 11/12/26 3:57 PM, netdev-bot+sashiko@kernel.org wrote:
> 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.
>
> [ ... ]
The lock is only there to get a consistent snapshot of the live and
retired no-buffer counts. The retire path moves the live count into the
retired one and clears it, so without the lock a reader could see a
mixed pair and report a transient double count or a lower
rx_missed_errors.

It's a cost on the stats readers, not a correctness problem in the RX
path. The reader holds each per-queue lock just for two loads, with no
allocation, DMA mapping or hcalls under it. It can still contend with a
refill that holds replenish_lock though, so I agree a lockless counter
is the better shape.

I don't think that should block the series. If there's a respin I'll
fold in a single combined monotonic value that the writers update and
readers load with READ_ONCE(); otherwise I'll send it as a follow-up
cleanup.

Thanks,
Mingming