[dpdk-dev] [PATCH 0/3] add lock-free stack support discovery

[dpdk-dev] [PATCH 0/3] add lock-free stack support discovery

[Stanislaw Kardach]

The lock-free stack implementation (RTE_STACK_F_LF) is supported only on a
subset of platforms, namely x86_64 and arm64. Platforms supporting 128b atomics
have to opt-in to a generic or C11 implementations. All other platforms use a
stubbed implementation for push/pop operations which are basically NOPs.
However rte_stack_create() will not fail and application can proceed assuming
it has a working lock-free stack.

This means that among other things the stack_lf fast and perf tests will fail
as if implementation is wrong (which one can argue is). Therefore this patchset
tries to give user a way to check whether a lock_free is supported or not both
at compile time (build flag) and at runtime (ENOTSUP errno in rte_stack_create).

I have added cc to stable@dpdk.org because check-git-log.sh suggested it. I'm
not sure if adding a binary compatible change to API is worth stable@dpdk.org.

Cc: stable@dpdk.org

Stanislaw Kardach (3):
  stack: update lock-free supported archs
  stack: add compile flag for lock-free support
  test: run lock-free stack tests when supported

 app/test/test_stack.c                  | 4 ++++
 app/test/test_stack_perf.c             | 4 ++++
 doc/guides/rel_notes/release_21_05.rst | 4 ++++
 lib/librte_stack/rte_stack.c           | 4 +++-
 lib/librte_stack/rte_stack.h           | 3 ++-
 lib/librte_stack/rte_stack_lf.h        | 5 +++++
 6 files changed, 22 insertions(+), 2 deletions(-)

-- 
2.27.0

[dpdk-dev] [PATCH 1/3] stack: update lock-free supported archs

[Stanislaw Kardach]

Since 7911ba047 lock-free stack is supported on arm64 but this
description was missing from the doxygen for the flag.

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Fixes: 7911ba0473e0 ("stack: enable lock-free implementation for aarch64")
Cc: phil.yang@arm.com
Cc: stable@dpdk.org
---
 lib/librte_stack/rte_stack.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_stack/rte_stack.h b/lib/librte_stack/rte_stack.h
index 395b9ef83..b82c74e72 100644
--- a/lib/librte_stack/rte_stack.h
+++ b/lib/librte_stack/rte_stack.h
@@ -89,7 +89,7 @@ struct rte_stack {
 
 /**
  * The stack uses lock-free push and pop functions. This flag is only
- * supported on x86_64 platforms, currently.
+ * supported on x86_64 or arm64 platforms, currently.
  */
 #define RTE_STACK_F_LF 0x0001
 
-- 
2.27.0

[dpdk-dev] [PATCH 2/3] stack: add lock-free support indication

[Stanislaw Kardach]

Currently it is impossible to detect programatically whether lock-free
implementation of rte_stack is supported. One could check whether the
header guard for lock-free stubs is defined (_RTE_STACK_LF_STUBS_H_) but
that's an unstable implementation detail. Because of that currently all
lock-free ring creations silently succeed (as long as the stack header
is 16B long) which later leads to push and pop operations being NOPs.
The observable effect is that stack_lf_autotest fails on platforms not
supporting the lock-free. Instead it should just skip the lock-free test
altogether.

This commit adds a new errno value (ENOTSUP) that may be returned by
rte_stack_create() to indicate that a given combination of flags is not
supported on a current platform.
This is detected by checking a compile-time flag in the include logic in
rte_stack_lf.h which may be used by applications to check the lock-free
support at compile time.

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Fixes: 7911ba0473e0 ("stack: enable lock-free implementation for aarch64")
Cc: phil.yang@arm.com
Cc: stable@dpdk.org
---
 doc/guides/rel_notes/release_21_05.rst | 4 ++++
 lib/librte_stack/rte_stack.c           | 4 +++-
 lib/librte_stack/rte_stack.h           | 1 +
 lib/librte_stack/rte_stack_lf.h        | 5 +++++
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/doc/guides/rel_notes/release_21_05.rst b/doc/guides/rel_notes/release_21_05.rst
index 6f5858c8f..42ed60da8 100644
--- a/doc/guides/rel_notes/release_21_05.rst
+++ b/doc/guides/rel_notes/release_21_05.rst
@@ -166,6 +166,10 @@ API Changes
 * pci: The value ``PCI_ANY_ID`` is marked as deprecated
   and can be replaced with ``RTE_PCI_ANY_ID``.
 
+* Lock-free ``rte_stack`` no longer silently ignores push and pop when it's not
+  supported on the current platform. Instead ``rte_stack_create()`` fails and
+  ``rte_errno`` is set to ``ENOTSUP``.
+
 
 ABI Changes
 -----------
diff --git a/lib/librte_stack/rte_stack.c b/lib/librte_stack/rte_stack.c
index 8a51fba17..10d3b2eeb 100644
--- a/lib/librte_stack/rte_stack.c
+++ b/lib/librte_stack/rte_stack.c
@@ -64,9 +64,11 @@ rte_stack_create(const char *name, unsigned int count, int socket_id,
 
 #ifdef RTE_ARCH_64
 	RTE_BUILD_BUG_ON(sizeof(struct rte_stack_lf_head) != 16);
-#else
+#endif
+#if !defined(RTE_STACK_LF_SUPPORTED)
 	if (flags & RTE_STACK_F_LF) {
 		STACK_LOG_ERR("Lock-free stack is not supported on your platform\n");
+		rte_errno = ENOTSUP;
 		return NULL;
 	}
 #endif
diff --git a/lib/librte_stack/rte_stack.h b/lib/librte_stack/rte_stack.h
index b82c74e72..27640f87b 100644
--- a/lib/librte_stack/rte_stack.h
+++ b/lib/librte_stack/rte_stack.h
@@ -205,6 +205,7 @@ rte_stack_free_count(struct rte_stack *s)
  *    - EEXIST - a stack with the same name already exists
  *    - ENOMEM - insufficient memory to create the stack
  *    - ENAMETOOLONG - name size exceeds RTE_STACK_NAMESIZE
+ *    - ENOTSUP - platform does not support given flags combination.
  */
 struct rte_stack *
 rte_stack_create(const char *name, unsigned int count, int socket_id,
diff --git a/lib/librte_stack/rte_stack_lf.h b/lib/librte_stack/rte_stack_lf.h
index eb106e64e..f2b012cd0 100644
--- a/lib/librte_stack/rte_stack_lf.h
+++ b/lib/librte_stack/rte_stack_lf.h
@@ -13,6 +13,11 @@
 #else
 #include "rte_stack_lf_generic.h"
 #endif
+
+/**
+ * Indicates that RTE_STACK_F_LF is supported.
+ */
+#define RTE_STACK_LF_SUPPORTED
 #endif
 
 /**
-- 
2.27.0

[dpdk-dev] [PATCH 3/3] test: run lock-free stack tests when supported

[Stanislaw Kardach]

Use the recently added RTE_STACK_LF_SUPPORTED flag to disable the
lock-free stack tests at the compile time.
Perf test doesn't fail because rte_ring_create() succeeds, however
marking this test as skipped gives a better indication of what actually
was tested.

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Cc: stable@dpdk.org
---
 app/test/test_stack.c      | 4 ++++
 app/test/test_stack_perf.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/app/test/test_stack.c b/app/test/test_stack.c
index 02422a32d..00efb38e2 100644
--- a/app/test/test_stack.c
+++ b/app/test/test_stack.c
@@ -373,7 +373,11 @@ test_stack(void)
 static int
 test_lf_stack(void)
 {
+#if defined(RTE_STACK_LF_SUPPORTED)
 	return __test_stack(RTE_STACK_F_LF);
+#else
+	return TEST_SKIPPED;
+#endif
 }
 
 REGISTER_TEST_COMMAND(stack_autotest, test_stack);
diff --git a/app/test/test_stack_perf.c b/app/test/test_stack_perf.c
index 3590625c4..4ee40d5d1 100644
--- a/app/test/test_stack_perf.c
+++ b/app/test/test_stack_perf.c
@@ -349,7 +349,11 @@ test_stack_perf(void)
 static int
 test_lf_stack_perf(void)
 {
+#if defined(RTE_STACK_LF_SUPPORTED)
 	return __test_stack_perf(RTE_STACK_F_LF);
+#else
+	return TEST_SKIPPED;
+#endif
 }
 
 REGISTER_TEST_COMMAND(stack_perf_autotest, test_stack_perf);
-- 
2.27.0

Re: [dpdk-dev] [PATCH 0/3] add lock-free stack support discovery

[David Marchand]

On Mon, Apr 12, 2021 at 10:29 AM Stanislaw Kardach <kda@semihalf.com> wrote:
>
> The lock-free stack implementation (RTE_STACK_F_LF) is supported only on a
> subset of platforms, namely x86_64 and arm64. Platforms supporting 128b atomics
> have to opt-in to a generic or C11 implementations. All other platforms use a
> stubbed implementation for push/pop operations which are basically NOPs.
> However rte_stack_create() will not fail and application can proceed assuming
> it has a working lock-free stack.

Did you actually hit this issue or is this only theoretical?
I can only think of ppc64 displaying such behavior.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 0/3] add lock-free stack support discovery

[Stanisław Kardach]

On Fri, Apr 16, 2021 at 08:34:29AM +0200, David Marchand wrote:
> On Mon, Apr 12, 2021 at 10:29 AM Stanislaw Kardach <kda@semihalf.com> wrote:
> >
> > The lock-free stack implementation (RTE_STACK_F_LF) is supported only on a
> > subset of platforms, namely x86_64 and arm64. Platforms supporting 128b atomics
> > have to opt-in to a generic or C11 implementations. All other platforms use a
> > stubbed implementation for push/pop operations which are basically NOPs.
> > However rte_stack_create() will not fail and application can proceed assuming
> > it has a working lock-free stack.
>
> Did you actually hit this issue or is this only theoretical?
> I can only think of ppc64 displaying such behavior.
>
I actually hit this issue while working on a RISC-V port.
My reasoning here is that sooner or later someone else will stumble upon
this, either on ppc64 or while trying to port to some new platform.
It is also a really nasty limitation do debug given the silent nature of
the failure.

>
> --
> David Marchand
>

--
Best Regards,
Stanislaw Kardach

Re: [dpdk-dev] [PATCH 1/3] stack: update lock-free supported archs

[Olivier Matz]

On Mon, Apr 12, 2021 at 10:28:59AM +0200, Stanislaw Kardach wrote:
> Since 7911ba047 lock-free stack is supported on arm64 but this
> description was missing from the doxygen for the flag.
> 
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> Fixes: 7911ba0473e0 ("stack: enable lock-free implementation for aarch64")
> Cc: phil.yang@arm.com
> Cc: stable@dpdk.org

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH 2/3] stack: add lock-free support indication

[Olivier Matz]

On Mon, Apr 12, 2021 at 10:29:00AM +0200, Stanislaw Kardach wrote:
> Currently it is impossible to detect programatically whether lock-free
> implementation of rte_stack is supported. One could check whether the
> header guard for lock-free stubs is defined (_RTE_STACK_LF_STUBS_H_) but
> that's an unstable implementation detail. Because of that currently all
> lock-free ring creations silently succeed (as long as the stack header
> is 16B long) which later leads to push and pop operations being NOPs.
> The observable effect is that stack_lf_autotest fails on platforms not
> supporting the lock-free. Instead it should just skip the lock-free test
> altogether.
> 
> This commit adds a new errno value (ENOTSUP) that may be returned by
> rte_stack_create() to indicate that a given combination of flags is not
> supported on a current platform.
> This is detected by checking a compile-time flag in the include logic in
> rte_stack_lf.h which may be used by applications to check the lock-free
> support at compile time.
> 
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> Fixes: 7911ba0473e0 ("stack: enable lock-free implementation for aarch64")
> Cc: phil.yang@arm.com
> Cc: stable@dpdk.org

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH 3/3] test: run lock-free stack tests when supported

[Olivier Matz]

On Mon, Apr 12, 2021 at 10:29:01AM +0200, Stanislaw Kardach wrote:
> Use the recently added RTE_STACK_LF_SUPPORTED flag to disable the
> lock-free stack tests at the compile time.
> Perf test doesn't fail because rte_ring_create() succeeds, however
> marking this test as skipped gives a better indication of what actually
> was tested.
> 
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> Cc: stable@dpdk.org

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH 0/3] add lock-free stack support discovery

[David Marchand]

On Mon, Apr 12, 2021 at 10:29 AM Stanislaw Kardach <kda@semihalf.com> wrote:
>
> The lock-free stack implementation (RTE_STACK_F_LF) is supported only on a
> subset of platforms, namely x86_64 and arm64. Platforms supporting 128b atomics
> have to opt-in to a generic or C11 implementations. All other platforms use a
> stubbed implementation for push/pop operations which are basically NOPs.
> However rte_stack_create() will not fail and application can proceed assuming
> it has a working lock-free stack.
>
> This means that among other things the stack_lf fast and perf tests will fail
> as if implementation is wrong (which one can argue is). Therefore this patchset
> tries to give user a way to check whether a lock_free is supported or not both
> at compile time (build flag) and at runtime (ENOTSUP errno in rte_stack_create).
>
> I have added cc to stable@dpdk.org because check-git-log.sh suggested it. I'm
> not sure if adding a binary compatible change to API is worth stable@dpdk.org.
>
> Cc: stable@dpdk.org

The issue was hit while porting to a new architecture.
The feature is broken in existing stable releases and it won't get
fixed by this change.

I'd rather not backport it.

Opinions?


-- 
David Marchand

Re: [dpdk-dev] [PATCH 0/3] add lock-free stack support discovery

[Olivier Matz]

On Mon, May 03, 2021 at 04:21:25PM +0200, David Marchand wrote:
> On Mon, Apr 12, 2021 at 10:29 AM Stanislaw Kardach <kda@semihalf.com> wrote:
> >
> > The lock-free stack implementation (RTE_STACK_F_LF) is supported only on a
> > subset of platforms, namely x86_64 and arm64. Platforms supporting 128b atomics
> > have to opt-in to a generic or C11 implementations. All other platforms use a
> > stubbed implementation for push/pop operations which are basically NOPs.
> > However rte_stack_create() will not fail and application can proceed assuming
> > it has a working lock-free stack.
> >
> > This means that among other things the stack_lf fast and perf tests will fail
> > as if implementation is wrong (which one can argue is). Therefore this patchset
> > tries to give user a way to check whether a lock_free is supported or not both
> > at compile time (build flag) and at runtime (ENOTSUP errno in rte_stack_create).
> >
> > I have added cc to stable@dpdk.org because check-git-log.sh suggested it. I'm
> > not sure if adding a binary compatible change to API is worth stable@dpdk.org.
> >
> > Cc: stable@dpdk.org
> 
> The issue was hit while porting to a new architecture.
> The feature is broken in existing stable releases and it won't get
> fixed by this change.
> 
> I'd rather not backport it.
> 
> Opinions?

Agreed.

Re: [dpdk-dev] [PATCH 0/3] add lock-free stack support discovery

[Stanisław Kardach]

On Mon, 3 May 2021, 16:28 Olivier Matz, <olivier.matz@6wind.com> wrote:

> On Mon, May 03, 2021 at 04:21:25PM +0200, David Marchand wrote:
> > On Mon, Apr 12, 2021 at 10:29 AM Stanislaw Kardach <kda@semihalf.com>
> wrote:
> > >
> > > The lock-free stack implementation (RTE_STACK_F_LF) is supported only
> on a
> > > subset of platforms, namely x86_64 and arm64. Platforms supporting
> 128b atomics
> > > have to opt-in to a generic or C11 implementations. All other
> platforms use a
> > > stubbed implementation for push/pop operations which are basically
> NOPs.
> > > However rte_stack_create() will not fail and application can proceed
> assuming
> > > it has a working lock-free stack.
> > >
> > > This means that among other things the stack_lf fast and perf tests
> will fail
> > > as if implementation is wrong (which one can argue is). Therefore this
> patchset
> > > tries to give user a way to check whether a lock_free is supported or
> not both
> > > at compile time (build flag) and at runtime (ENOTSUP errno in
> rte_stack_create).
> > >
> > > I have added cc to stable@dpdk.org because check-git-log.sh suggested
> it. I'm
> > > not sure if adding a binary compatible change to API is worth
> stable@dpdk.org.
> > >
> > > Cc: stable@dpdk.org
> >
> > The issue was hit while porting to a new architecture.
> > The feature is broken in existing stable releases and it won't get
> > fixed by this change.
> >
> > I'd rather not backport it.
> >
> > Opinions?
>
> Agreed.
>
Agreed.

>

Re: [dpdk-dev] [PATCH 0/3] add lock-free stack support discovery

[David Marchand]

On Mon, May 3, 2021 at 8:35 PM Stanisław Kardach <kda@semihalf.com> wrote:
> On Mon, 3 May 2021, 16:28 Olivier Matz, <olivier.matz@6wind.com> wrote:
>> On Mon, May 03, 2021 at 04:21:25PM +0200, David Marchand wrote:
>> > On Mon, Apr 12, 2021 at 10:29 AM Stanislaw Kardach <kda@semihalf.com> wrote:
>> > >
>> > > I have added cc to stable@dpdk.org because check-git-log.sh suggested it. I'm
>> > > not sure if adding a binary compatible change to API is worth stable@dpdk.org.
>> > >
>> > > Cc: stable@dpdk.org
>> >
>> > The issue was hit while porting to a new architecture.
>> > The feature is broken in existing stable releases and it won't get
>> > fixed by this change.
>> >
>> > I'd rather not backport it.
>> >
>> > Opinions?
>>
>> Agreed.
>
> Agreed.


Ok, thanks.
I'll take this series dropping Cc: stable.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 0/3] add lock-free stack support discovery

[David Marchand]

On Mon, Apr 12, 2021 at 10:29 AM Stanislaw Kardach <kda@semihalf.com> wrote:
>
> The lock-free stack implementation (RTE_STACK_F_LF) is supported only on a
> subset of platforms, namely x86_64 and arm64. Platforms supporting 128b atomics
> have to opt-in to a generic or C11 implementations. All other platforms use a
> stubbed implementation for push/pop operations which are basically NOPs.
> However rte_stack_create() will not fail and application can proceed assuming
> it has a working lock-free stack.
>
> This means that among other things the stack_lf fast and perf tests will fail
> as if implementation is wrong (which one can argue is). Therefore this patchset
> tries to give user a way to check whether a lock_free is supported or not both
> at compile time (build flag) and at runtime (ENOTSUP errno in rte_stack_create).

Series applied.
Thanks Stanislaw!


-- 
David Marchand