On Tue, 7 May 2024 14:48:51 +0100 Ferruh Yigit <ferruh.yi...@amd.com> wrote:
> 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. The device register worry, can just be handled with a compiler barrier. Does not need the stronger guarantee of atomic or volatile.
