Re: [PATCH net-next v2 1/4] eth: mpnic: count packets, bytes, and drops per ring
From: Paolo Abeni
Date: Wed Oct 21 2026 - 04:26:49 EST
On 10/20/26 23:21, Sebastian Andrzej Siewior wrote:
On 2026-10-19 19:05:43 [-0700], Jakub Kicinski wrote:Sebastian, my understanding is that your doubts are mostly because you
On Mon, 19 Oct 2026 23:54:05 +0200 Sebastian Andrzej Siewior wrote:
On 2026-10-19 09:04:38 [-0400], Daniel Zahka wrote:
Why do you have the two mpnic_base_stats and fold the stats into into
it? My guess would be that on stop the counters are reset and you want
to preserve the stats.
Not sure I understand. I think you are asking about two things:
1. Why have two mpnic_base_stats?
To avoid a separate heap allocation that can fail. Handling an
allocation failure in the ndo_stop path when doing the aggregation into
the base stats is tricky.
Right. But looking at hinic3_get_stats64() for instance (I just grabbed
a random one) it simply does query the stats for each queue, add up,
return. It probably does not drop the queues on down, just reallocates
them while changing the number of queues (and maybe MTU).
I don't think pre-allocating stats for chips which supports thousands
of queues is reasonable.
I don't mean to pre-allocate stats. If device-down removes queues then
the stats gone, so what. The queues could remain.
2. Why split stats into base stats and per queue stats, and why reset
the per queue counts when the device transitions down?
This is the way the stats in netdev.yaml are exposed depending on the
'scope' request attribute. My reading of the relevant kernel docs is
that the per queue counters are counting events for the current live
queue, so it makes sense to reset them and fold those counts into the
base stats when the interface is transitioned down.
Is there any other driver doing this?
There is ::get_sset_count() and so on for the per-queue stats.
You may be mixing up queue stats (netlink) with the old ethtool -S
kitchen sink / garbage dump.
The statistics is folded on device-down into a global counter. So on
device-up the statistics is still available. At least that is my
understanding. This means the per-queue stats are lost and global stats
don't match the per queue stats anymore.
If the interface goes down and frees all its queues, then the stats are
gone, true. But preserving them at the price of two (down+up) forced RCU
grace period, I don't know. It looks like a bit too much.
This is a modern driver, the down / up path is not taken when interface
is reconfigured. It's only taken when the interface is actually taken
down or bought up. Which virtually never happens for a datacenter NIC.
So if this never happens, it might not be needed to preserve this. That
force-RCU might not be needed. This does happen on down and I would
assume that the device is no longer processing packets.
I don't mean to dismiss your concerns, we agonized over this off-list
for a week, and don't love the resulting flow, either. But I strongly
prefer the RCU grace period that never really runs, but is easy to
reason about, to more advanced fixes which use memory barriers etc.
Right. If you think is all makes sense and is needed then why not. It
just looks odd.
are unfamiliar with the current reference NIC stats scheme. That has
evolved significantly in recent time.
I think the approach here is actually quite nice, I don't see blockers.
/P