* [PATCH] net/ixgbe: fix inverted test and set conditional
@ 2023-06-15 19:30 Tyler Retzlaff
2023-06-15 19:30 ` Tyler Retzlaff
0 siblings, 1 reply; 6+ messages in thread
From: Tyler Retzlaff @ 2023-06-15 19:30 UTC (permalink / raw)
To: dev; +Cc: Qiming Yang, Wenjun Wu, mb, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] net/ixgbe: fix inverted test and set conditional
2023-06-15 19:30 [PATCH] net/ixgbe: fix inverted test and set conditional Tyler Retzlaff
@ 2023-06-15 19:30 ` Tyler Retzlaff
2023-06-16 1:59 ` Chen, LingliX
2023-06-19 7:53 ` David Marchand
0 siblings, 2 replies; 6+ messages in thread
From: Tyler Retzlaff @ 2023-06-15 19:30 UTC (permalink / raw)
To: dev; +Cc: Qiming Yang, Wenjun Wu, mb, Tyler Retzlaff, david.marchand
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] net/ixgbe: fix inverted test and set conditional
2023-06-15 19:30 ` Tyler Retzlaff
@ 2023-06-16 1:59 ` Chen, LingliX
2023-06-19 7:53 ` David Marchand
1 sibling, 0 replies; 6+ messages in thread
From: Chen, LingliX @ 2023-06-16 1:59 UTC (permalink / raw)
To: dev; +Cc: Tyler Retzlaff
> -----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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/ixgbe: fix inverted test and set conditional
2023-06-15 19:30 ` Tyler Retzlaff
2023-06-16 1:59 ` Chen, LingliX
@ 2023-06-19 7:53 ` David Marchand
2023-06-19 8:19 ` Morten Brørup
1 sibling, 1 reply; 6+ messages in thread
From: David Marchand @ 2023-06-19 7:53 UTC (permalink / raw)
To: Tyler Retzlaff; +Cc: dev, Qiming Yang, Wenjun Wu, mb
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] net/ixgbe: fix inverted test and set conditional
2023-06-19 7:53 ` David Marchand
@ 2023-06-19 8:19 ` Morten Brørup
2023-06-19 16:27 ` Tyler Retzlaff
0 siblings, 1 reply; 6+ messages in thread
From: Morten Brørup @ 2023-06-19 8:19 UTC (permalink / raw)
To: David Marchand, Tyler Retzlaff, thomas; +Cc: dev, Qiming Yang, Wenjun Wu
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net/ixgbe: fix inverted test and set conditional
2023-06-19 8:19 ` Morten Brørup
@ 2023-06-19 16:27 ` Tyler Retzlaff
0 siblings, 0 replies; 6+ messages in thread
From: Tyler Retzlaff @ 2023-06-19 16:27 UTC (permalink / raw)
To: Morten Brørup; +Cc: David Marchand, thomas, dev, Qiming Yang, Wenjun Wu
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-19 16:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-15 19:30 [PATCH] net/ixgbe: fix inverted test and set conditional Tyler Retzlaff
2023-06-15 19:30 ` Tyler Retzlaff
2023-06-16 1:59 ` Chen, LingliX
2023-06-19 7:53 ` David Marchand
2023-06-19 8:19 ` Morten Brørup
2023-06-19 16:27 ` Tyler Retzlaff
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).