On Tue, Jun 6, 2023 at 11:45 PM Tyler Retzlaff <roret...@linux.microsoft.com> wrote: > > Replace the use of rte_atomic.h types and functions, instead use GCC > supplied C++11 memory model builtins. > > This series covers the libraries and drivers that are built on Windows. > > The code has be converted to use the __atomic builtins but there are > additional during conversion I notice that there may be some issues > that need to be addressed. > > I'll comment in the patches where my concerns are so the maintainers > may comment. > > v5: > * use relaxed ordering for counter increments in net/ring patch > * remove note comments from net/ring patch > > v4: > > * drop patch for lib/ring it will be provided by ARM / Honnappa > * rebase for changes in dma/idxd merge > * adapt __atomic_fetch_sub(...) - 1 == 0 to be (__atomic_fetch_sub(...) == > 1) > as per feedback. > * drop one /* NOTE: review for potential ordering optimization */ since > the note reference non-critical to perf control path. > > note: > > Remainder of the NOTE comments have been retained since there > seems to be no consensus but stronger opinion/argument to keep > expressed. while I generally agree that changes should not > include ``TODO'' style comments I also agree that without these > comments in your face people are very unlikely to feel compelled > to make the review they are trying to solicit without them. if > it is absolute that the series won't be merged with them then I > will remove them, but please be explicit soon. > > v3: > * style, don't use c99 comments > > v2: > * comment code where optimizations may be possible now that memory > order can be specified. > * comment code where operations should potentially be atomic so that > maintainers can review. > * change a couple of variables labeled as counters to be unsigned. > > Tyler Retzlaff (6): > stack: replace rte atomics with GCC builtin atomics > dma/idxd: replace rte atomics with GCC builtin atomics > net/ice: replace rte atomics with GCC builtin atomics > net/ixgbe: replace rte atomics with GCC builtin atomics > net/null: replace rte atomics with GCC builtin atomics > net/ring: replace rte atomics with GCC builtin atomics > > drivers/dma/idxd/idxd_internal.h | 3 +-- > drivers/dma/idxd/idxd_pci.c | 11 ++++++----- > drivers/net/ice/ice_dcf.c | 1 - > drivers/net/ice/ice_dcf_ethdev.c | 1 - > drivers/net/ice/ice_ethdev.c | 12 ++++++++---- > drivers/net/ixgbe/ixgbe_bypass.c | 1 - > drivers/net/ixgbe/ixgbe_ethdev.c | 18 ++++++++++++------ > drivers/net/ixgbe/ixgbe_ethdev.h | 3 ++- > drivers/net/ixgbe/ixgbe_flow.c | 1 - > drivers/net/ixgbe/ixgbe_rxtx.c | 1 - > drivers/net/null/rte_eth_null.c | 28 ++++++++++++++++++---------- > drivers/net/ring/rte_eth_ring.c | 20 ++++++++++---------- > lib/stack/rte_stack_lf_generic.h | 16 +++++++++------- > 13 files changed, 66 insertions(+), 50 deletions(-)
I am not really enthousiastic about those NOTE:. I would prefer we get an explicit go/nogo from each maintainers, but this did not happen. I think that this indicates that those NOTE: will rot in the code now. Thomas proposed to track those NOTE: in the release announce mail and that we ping maintainers regularly. Let's see how it goes. I am merging this series so we can progress on the $SUBJECT. Series applied, thanks. Tyler, about the patch on the ring library that was dropped by got no viable alternative, I'll wait for a decision from ARM and you. -- David Marchand
