On Wed, May 24, 2023 at 10:06:24PM +0200, David Marchand wrote: > On Wed, May 24, 2023 at 5:47 PM Tyler Retzlaff > <roret...@linux.microsoft.com> wrote: > > On Wed, May 24, 2023 at 02:40:43PM +0200, David Marchand wrote: > > > Hello Tyler, > > > > > > On Thu, Mar 23, 2023 at 11:54 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. > > > > > > > > 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 (7): > > > > ring: replace rte atomics with GCC builtin atomics > > > > 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 | 8 +++++--- > > > > 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 | 26 ++++++++++++++++---------- > > > > lib/ring/rte_ring_core.h | 1 - > > > > lib/ring/rte_ring_generic_pvt.h | 12 ++++++++---- > > > > lib/stack/rte_stack_lf_generic.h | 16 +++++++++------- > > > > 15 files changed, 79 insertions(+), 53 deletions(-) > > > > > > > > > > There is still some code using the DPDK "legacy" atomic API, but I > > > guess this will be converted later. > > > > Yes, it will be converted later. > > > > If I did it correctly... the series was an attempt to move away > > from the legacy API where there was a dependency on EAL that would > > change when moving to stdatomic. I'm hoping that the remaining use of > > the legacy API are not sensitive to the theoretical ABI surface > > changing when that move is complete. > > Ok. > > > > > As you proposed, I dropped patch 1 on the ring library (waiting for > > > ARM to provide an alternative) and applied this series, thanks. > > > > > > Note: Thomas, Ferruh, we will have to be careful when merging subtrees > > > to make sure we are not reintroducing those again (like for example > > > net/ice). > > Well, I have some second thought about this series so I did not push > it to dpdk.org yet.
Understood. It's very important to have these reviewed well so no objection just hope we can get them reviewed properly soon. > Drivers maintainers were not copied so I would like another pair of > eyes on the series: ideally no /* Note: */ should be left when merging > those patches. The /* Note: */ was explicitly requested by other reviewers as they were concerned we would lose track of opportunities to weaken ordering after switching from __sync to __atomic. Is your request that the comments now be removed? Thanks! > I'll reply individually on the patches. > > > -- > David Marchand
