[PATCH] net/ixgbe: fix inverted test and set conditional

[PATCH] net/ixgbe: fix inverted test and set conditional

[Tyler Retzlaff]

net/ixgbe: fix inverted test and set conditional

I either dropped `!` in the offending series or just failed to notice that
the return value for __atomic_test_and_set is inverted relative to
rte_atomic32_test_and_set.

Tyler Retzlaff (1):
  net/ixgbe: fix inverted test and set conditional

 drivers/net/ixgbe/ixgbe_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.8.3.1

[PATCH] net/ixgbe: fix inverted test and set conditional

[Tyler Retzlaff]

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")
Cc: roretzla@linux.microsoft.com
Cc: david.marchand@redhat.com
Cc: qiming.yang@intel.com

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 43aea2e..5f73ae8 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4322,7 +4322,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
 		if (ixgbe_get_media_type(hw) == ixgbe_media_type_fiber) {
 			ixgbe_dev_wait_setup_link_complete(dev, 0);
 			/* NOTE: review for potential ordering optimization */
-			if (__atomic_test_and_set(&ad->link_thread_running, __ATOMIC_SEQ_CST)) {
+			if (!__atomic_test_and_set(&ad->link_thread_running, __ATOMIC_SEQ_CST)) {
 				/* To avoid race condition between threads, set
 				 * the IXGBE_FLAG_NEED_LINK_CONFIG flag only
 				 * when there is no link thread running.
-- 
1.8.3.1

RE: [PATCH] net/ixgbe: fix inverted test and set conditional

[Chen, LingliX]

> -----Original Message-----
> From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Sent: Friday, June 16, 2023 3:30 AM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Wu, Wenjun1
> <wenjun1.wu@intel.com>; mb@smartsharesystems.com; Tyler Retzlaff
> <roretzla@linux.microsoft.com>; david.marchand@redhat.com
> Subject: [PATCH] net/ixgbe: fix inverted test and set conditional
> 
> 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")
> Cc: roretzla@linux.microsoft.com
> Cc: david.marchand@redhat.com
> Cc: qiming.yang@intel.com
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---

Tested-by: Lingli Chen <linglix.chen@intel.com>

Re: [PATCH] net/ixgbe: fix inverted test and set conditional

[David Marchand]

On Thu, Jun 15, 2023 at 9:30 PM Tyler Retzlaff
<roretzla@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 <roretzla@linux.microsoft.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>

Thanks for the fix Tyler.


-- 
David Marchand

RE: [PATCH] net/ixgbe: fix inverted test and set conditional

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Monday, 19 June 2023 09.54
> 
> On Thu, Jun 15, 2023 at 9:30 PM Tyler Retzlaff
> <roretzla@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 <roretzla@linux.microsoft.com>
> Reviewed-by: David Marchand <david.marchand@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()?

PS:
Tyler, the Solarflare driver also uses rte_atomic32_test_and_set(); maybe you missed in the original patch series.
https://elixir.bootlin.com/dpdk/v23.07-rc1/A/ident/rte_atomic32_test_and_set

Re: [PATCH] net/ixgbe: fix inverted test and set conditional

[Tyler Retzlaff]

On Mon, Jun 19, 2023 at 10:19:00AM +0200, Morten Brørup wrote:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Monday, 19 June 2023 09.54
> > 
> > On Thu, Jun 15, 2023 at 9:30 PM Tyler Retzlaff
> > <roretzla@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 <roretzla@linux.microsoft.com>
> > Reviewed-by: David Marchand <david.marchand@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
>