On 5/7/2024 3:52 PM, Stephen Hemminger wrote: > 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. >
Based on Morten's email, counter being stored late to memory may not be an issue, so may not need even compiler barrier, let me check again.
