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
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
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
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
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
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
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>
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>
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>
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
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.
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. >
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
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