> From: Stephen Hemminger [mailto:step...@networkplumber.org] > Sent: Wednesday, 22 May 2024 21.54 > > On Wed, 22 May 2024 20:09:23 +0200 > Morten Brørup <m...@smartsharesystems.com> wrote: > > > > There are no size_bins in the current version of the patch. > > > And the number of counters in ethdev part are small so it is less of > a > > > concern. > > > The code is easier to maintain if the counter object is self > contained. > > > > I agree that there are advantages to keeping the counter object self > contained. > > > > However, these counters are generic, so we cannot assume that there > are only very few, based on how the current software device drivers use > them. > > > > Someone might want to add size_bins to the software device drivers. > > And someone else might want to collect many counters in some > application or library structure. > > > No. > The implementation should be as simple and as small as possible for the > use case > that is presented in the patch series.
I checked a random one of the use cases presented, rte_eth_af_packet: The pkt_rx_queue struct grows to two cache lines, with the packets counter in the first cache line and the other counters in the second cache line. By moving pkt_rx_queue's "sockfd" field below the pointers, the structure is better packed. If following my proposal, i.e. keeping the counters grouped together (and the offsets grouped together), all three counters stay within the first cache line (and the offsets go into the second). The pkt_tx_queue struct also grows to two cache lines, with the first two counters in the first cache line and the third counter in the second cache line. With my proposal, the counters fit within the first cache line. > Doing something more complex > leads to the > classic YAGNI situation, where when the new case really happens the > implemenation > just doesn't quite fit. I disagree about YAGNI for this patch. We *do* need the counter API to have the offset separate from the counter to avoid a performance degradation. It is only slightly more complex, so I'm not convinced it's going to be a problem. Passing a pointer to the offset as an additional parameter to fetch() and reset() is straightforward. And using two instances of struct rte_eth_counters, one for counters and one for offsets, isn't complex either. The rte_eth_af_packet use case shows that the risk of touching an increased number of cache lines (by moving the offsets into the hot part of the structures) is real.
