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

[Paolo Abeni <pabeni@redhat.com>]

On 10/20/26 23:21, Sebastian Andrzej Siewior wrote:
> 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.
Sebastian, my understanding is that your doubts are mostly because you
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