On Mon, Jun 19, 2023 at 10:19:00AM +0200, Morten Brørup wrote: > > From: David Marchand [mailto:david.march...@redhat.com] > > Sent: Monday, 19 June 2023 09.54 > > > > On Thu, Jun 15, 2023 at 9:30 PM Tyler Retzlaff > > <roret...@linux.microsoft.com> wrote: > > > > > > Correct a mistake when converting ixgbe to use __atomic_test_and_set > > > instead of rte_atomic32_test_and_set. The return value from > > > __atomic_test_and_set is inverted relative to rte_atomic32_test_and_set. > > > > > > Fixes: e90baf6b82f6 ("net/ixgbe: replace legacy atomics with GCC builtin > > atomics") > > > > > > Signed-off-by: Tyler Retzlaff <roret...@linux.microsoft.com> > > Reviewed-by: David Marchand <david.march...@redhat.com> > > > > Thanks for the fix Tyler. > > rte_atomic32_test_and_set() returns the inverse of __atomic_test_and_set()! > > We need to provide a prominent warning about this when deprecating > rte_atomic32_test_and_set(), or everyone else will make the same mistake as > Tyler, i.e. perform a simple search/replace with the new function name, not > noticing that the function's return value is inverted. > Perhaps we can also add some build time warning when using > rte_atomic32_test_and_set()?
i'll probably take care of this work myself after stdatomics (assuming it is something people want) and hopefully i won't make that mistake again. for now i don't want to confuse the dev list with too many patches about 'atomics' to avoid overload/confusion for david and thomas. > > PS: > Tyler, the Solarflare driver also uses rte_atomic32_test_and_set(); maybe you > missed in the original patch series. it was intentionally skipped (for now). i only did the conversion for drivers that are built on windows. the assumption being the rte_atomicXX api deprecation is independent of switching to C11 stdatomic. i don't intend to ever make available rte_atomicXX on msvc/windows combination since it is being deprecated. as mentioned above as i expand the code that is built for msvc/windows it will then force me to adapt more of the rte_atomicsXX code eventually leading to most of it being gone (at least for libraries). > https://elixir.bootlin.com/dpdk/v23.07-rc1/A/ident/rte_atomic32_test_and_set >
