On 5/3/2024 11:00 PM, Stephen Hemminger wrote: > On Fri, 3 May 2024 16:45:47 +0100 > Ferruh Yigit <ferruh.yi...@amd.com> 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. >> >> While updating around, 'igb_stats' parameter renamed as 'stats'. >> >> 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/ > > > NAK > > I did not hear a good argument why atomic or volatile was necessary in the > first place. > Why? >
Sure, the patch is done as RFC intentionally to discuss the approach. Agree that volatile and atomics (fetch + add + store) is not required for thread synchronization, as only one CPU updates stats. Even this understanding is important because there are PMDs using full atomics for stats update, like null PMD [1], this will help up clear them. And there is a case, stats reset and stats updated in different threads simultaneously, for this 'volatile' is not sufficient anyway and full atomics is required. As this will cause performance impact we are already saying stats update and reset can't happen at the same time [2]. With this update volatile and atomics are not required for this case too. (Also using offset to increase stats reset reliability.) In this patch volatile replaced with atomic load and atomic store (not atomic fetch and add), to ensure that stats stored to memory and not kept in device registers only. With volatile, it is guaranteed that updated stats stored back to memory, but without volatile and atomics I am not sure if this is guaranteed. Practically I can see this working, but theoretically not sure. This is similar concern with change in your patch that is casting to volatile to ensure value read from memory [3]. Expectation is, only atomics load and store will have smaller performance impact than volatile, ensuring memory load and store when needed. [1] https://git.dpdk.org/dpdk/tree/drivers/net/null/rte_eth_null.c?h=v24.03#n105 [2] https://patches.dpdk.org/project/dpdk/patch/20240425165308.1078454-1-ferruh.yi...@amd.com/ [3] https://inbox.dpdk.org/dev/20240430154129.7347-1-step...@networkplumber.org/ `#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))` > Why is this driver special (a snowflake) compared to all the other drivers > doing software > statistics (tap, virtio, xdp, ring, memif, netvsc, vmware)? > Nothing special at all, only discussion started based on af_packet implementation. If we give a decision based on this RFC, same logic can be followed with existing or new software PMDs.
