Re: [PATCH] net/af_packet: cache align Rx/Tx structs

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 4/25/2024 10:26 AM, Mattias Rönnblom wrote:
> On 2024-04-25 01:55, Stephen Hemminger wrote:
>> On Thu, 25 Apr 2024 00:27:36 +0200
>> Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>>
>>> On 2024-04-24 21:13, Stephen Hemminger wrote:
>>>> On Wed, 24 Apr 2024 18:50:50 +0100
>>>> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>>>   
>>>>>> I don't know how slow af_packet is, but if you care about
>>>>>> performance,
>>>>>> you don't want to use atomic add for statistics.
>>>>>>       
>>>>>
>>>>> There are a few soft drivers already using atomics adds for
>>>>> updating stats.
>>>>> If we document expectations from 'rte_eth_stats_reset()', we can
>>>>> update
>>>>> those usages.
>>>>
>>>> Using atomic add is lots of extra overhead. The statistics are not
>>>> guaranteed
>>>> to be perfect.  If nothing else, the bytes and packets can be skewed.
>>>>    
>>>
>>> The sad thing here is that in case the counters are reset within the
>>> load-modify-store cycle of the lcore counter update, the reset may end
>>> up being a nop. So, it's not like you missed a packet or two, or suffer
>>> some transient inconsistency, but you completed and permanently ignored
>>> the reset request.
>>
>> That is one of the many reasons the Linux kernel intentionally did not
>> implement a reset statistics operation.
> 
> DPDK should deprecate statistics reset, it seems to me.
> 
> The current API is broken anyway, if you care about correctness. A reset
> function must return the current state of the counters, at the point
> them being reset. Otherwise, a higher layer may miss counter updates.
> 
> The af_packet PMD should return -ENOTSUP on reset (which is allowed by
> the API).
> 
> Maintaining an offset-since-last-reset for counters is a control plane
> thing, and shouldn't be in PMDs. (If MT-safe reset for SW-managed
> counters are to be expected from the PMDs, we should have some helper
> API to facilitate its efficient & correct-enough implementation.)
>

statistics reset works for HW devices, instead of removing statics reset
I am for documenting API that it may be not reliable, I can send a patch
for it.

With above change, we can be more relax on stats update specially for
soft drivers, and can convert atomic_add stats updates to "atomic load +
add + atomic store".

Does this plan make sense?