On 5/7/2024 4:27 PM, Morten Brørup wrote: >> From: Ferruh Yigit [mailto:ferruh.yi...@amd.com] >> Sent: Friday, 3 May 2024 17.46 >> >> 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/ >> >> v2: >> * Remove wrapping check for stats >> >> v3: >> * counter and offset put into same struct per stats >> * Use atomic load / store for stats values >> --- > > Note: My comments below relate to software PMDs only. > > Design for the following invariants: > 1. "counter" may increase at any time. (So stopping forwarding is not > required.) >
Mattias mentioned a case [1] that may end up 'offset > count', for being safe side we may start with restriction. [1] https://inbox.dpdk.org/dev/20240425174617.2126159-1-ferruh.yi...@amd.com/T/#m29cd179228c164181d2bb7dea716dee6e91ab169 > 2. "counter" may not decrease. > 3. "offset" is always <= "counter". > > So: > > Stats_get() must read "offset" before "counter"; if "counter" races to > increase in the mean time, it doesn't hurt. Stats_get() is a relatively > "cold" function, so barriers etc. are acceptable. > > Assuming that stats_add() lazy-writes "counter"; if stats_get() reads an old > value, its result will be slightly off, but not negative. > > Similarly for stats_reset(), which obviously reads "counter" before writing > "offset"; if "counter" races to increase in the mean time, the too low > "offset" will not cause negative stats from stats_get(). > ack on above items. > > And a requested change for performance: > >> +struct stats { >> + uint64_t counter; >> + uint64_t offset; >> +}; > > The "offset" is cold. > Stats_add(), which is the only hot function, only touches "counter". > > Instead of having a struct with {counter, offset}, I strongly prefer having > them separate. > E.g. as a struct defining the set of statistics (e.g. pkts, bytes, errors), > instantiated once for the counters (in a hot zone of the device data > structure) and once for the offsets (in a cold zone of the device data > structure). > There could be variants of this "set of statistics" struct, e.g. one for RX > and a different one for TX. (Each variant would be instantiated twice, once > for counters, and once for offsets.) > Although having them together was logical, good point from performance perspective, let me work on it.
