On Tue, 7 May 2024 14:49:19 +0100 Ferruh Yigit <ferruh.yi...@amd.com> wrote:
> On 5/7/2024 8:23 AM, Mattias Rönnblom wrote: > > On 2024-04-28 17:11, Mattias Rönnblom wrote: > >> On 2024-04-26 16:38, Ferruh Yigit wrote: > >>> For stats reset, use an offset instead of zeroing out actual stats > >>> values, > >>> get_stats() displays diff between stats and offset. > >>> This way stats only updated in datapath and offset only updated in stats > >>> reset function. This makes stats reset function more reliable. > >>> > >>> As stats only written by single thread, we can remove 'volatile' > >>> qualifier > >>> which should improve the performance in datapath. > >>> > >> > >> volatile wouldn't help you if you had multiple writers, so that can't > >> be the reason for its removal. It would be more accurate to say it > >> should be replaced with atomic updates. If you don't use volatile and > >> don't use atomics, you have to consider if the compiler can reach the > >> conclusion that it does not need to store the counter value for future > >> use *for that thread*. Since otherwise, I don't think the store > >> actually needs to occur. Since DPDK statistics tend to work, it's > >> pretty obvious that current compilers tend not to reach this conclusion. > >> > >> If this should be done 100% properly, the update operation should be a > >> non-atomic load, non-atomic add, and an atomic store. Similarly, for > >> the reset, the offset store should be atomic. > >> > >> Considered the state of the rest of the DPDK code base, I think a > >> non-atomic, non-volatile solution is also fine. > >> > >> (That said, I think we're better off just deprecating stats reset > >> altogether, and returning -ENOTSUP here meanwhile.) > >> > >>> Signed-off-by: Ferruh Yigit <ferruh.yi...@amd.com> > >>> --- > >>> Cc: Mattias Rönnblom <mattias.ronnb...@ericsson.com> > >>> Cc: Stephen Hemminger <step...@networkplumber.org> > >>> Cc: Morten Brørup <m...@smartsharesystems.com> > >>> > >>> This update triggered by mail list discussion [1]. > >>> > >>> [1] > >>> https://inbox.dpdk.org/dev/3b2cf48e-2293-4226-b6cd-5f4dd3969...@lysator.liu.se/ I would prefer that the SW statistics be handled generically by ethdev layers and used by all such drivers. The most complete version of SW stats now is in the virtio driver. If reset needs to be reliable (debatable), then it needs to be done without atomics.
