Re: [PATCH net-next v2 1/4] eth: mpnic: count packets, bytes, and drops per ring

[Sebastian Andrzej Siewior]

On 2026-10-19 19:05:43 [-0700], Jakub Kicinski wrote:
> 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.

> Anyway, I'll punt to Paolo for the final call but I think this is fine.
> 
> Acked-by: Jakub Kicinski <kuba@xxxxxxxxxx>

Sebastian