Re: [PATCH] net/af_packet: fix statistics

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

On 5/1/2024 5:43 PM, Stephen Hemminger wrote:
> On Wed, 1 May 2024 17:25:59 +0100
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>>>  - Do not mark statistics as volatile.
>>>    Instead, READ_ONCE() where necessary.
>>>   
>>
>> I did similar [1], and Mattias has some comments on it.
>> Issue is not in the reader (stats_get) side. Without volatile writer
>> (datapath thread) may end up *not* storing updated stats to memory.
>>
>> For reader side, I expect value not been in the register when function
>> called, so it ends up reading from memory, which doesn't require
>> volatile casting.
> 
> These are per-queue stats, and anybody foolish enough to have multiple
> threads sharing same queue without locking is playing with fire.
> It is documented that DPDK PMD's don't support that.
>

I am not referring multiple core sharing a queue, this is wrong for DPDK.

For single core case, if a variable doesn't have 'volatile' qualifier
and load/stores are not atomic, as compiler is not aware that there may
be other threads will access this value, what prevents compiler to hold
stats value in a register and store it memory at later stage, let say at
the end of the application forwarding loop?

For this case keeping 'volatile' qualifier works. But Mattias has a
suggestion to use "normal load + normal add + atomic store" in datapath.


For getting stats, which will be in different thread, you are already
casting it to 'volatile' to enforce compiler read from memory each time.


Additionally when we take stats reset into account, which can happen in
different thread, 'volatile' is not enough.
For reliable stats reset, either we need to use full atomics, or offset
logic which I am trying to in other patch.