diff mbox series

[09/11] test/ring: disable problematic tests for RISC-V

Message ID 20220505173003.3242618-10-kda@semihalf.com (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series Introduce support for RISC-V architecture | expand

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Stanisław Kardach May 5, 2022, 5:30 p.m. UTC
When compiling for RISC-V in debug mode the large amount of inlining in
test_ring_basic_ex() and test_ring_with_exact_size() (in test_ring.c)
leads to large loop bodies. This causes 'goto' and 'for' loop
PC-relative jumps generated by the compiler to go beyond the architecture
limitation of +/-1MB offset (the 'j <offset>' instruction). This
instruction should not be generated by the compiler since C language does
not limit the maximum distance for 'goto' or 'for' loop jumps.

This only happens in the unit test for ring which tries to perform long
loops with ring enqueue/dequeue and it seems to be caused by excessive
__rte_always_inline usage. ring perf test compiles just fine under
debug.

To work around this, disable the offending tests in debug mode.

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
Sponsored-by: Sam Grove <sam.grove@sifive.com>
---
 app/test/test_ring.c                   | 8 ++++++++
 config/riscv/meson.build               | 5 +++++
 doc/guides/rel_notes/release_22_07.rst | 3 ++-
 3 files changed, 15 insertions(+), 1 deletion(-)

Comments

Stephen Hemminger May 5, 2022, 5:35 p.m. UTC | #1
On Thu,  5 May 2022 19:30:01 +0200
Stanislaw Kardach <kda@semihalf.com> wrote:

> When compiling for RISC-V in debug mode the large amount of inlining in
> test_ring_basic_ex() and test_ring_with_exact_size() (in test_ring.c)
> leads to large loop bodies. This causes 'goto' and 'for' loop
> PC-relative jumps generated by the compiler to go beyond the architecture
> limitation of +/-1MB offset (the 'j <offset>' instruction). This
> instruction should not be generated by the compiler since C language does
> not limit the maximum distance for 'goto' or 'for' loop jumps.
> 
> This only happens in the unit test for ring which tries to perform long
> loops with ring enqueue/dequeue and it seems to be caused by excessive
> __rte_always_inline usage. ring perf test compiles just fine under
> debug.
> 
> To work around this, disable the offending tests in debug mode.
> 
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
> Sponsored-by: Sam Grove <sam.grove@sifive.com>
> ---

It seems to me that fixing the excessive inlining in the ring code
could benefit all architectures, rather than neutering the tests
on RISCV.
Stanisław Kardach May 5, 2022, 5:43 p.m. UTC | #2
On Thu, May 5, 2022 at 7:35 PM Stephen Hemminger <stephen@networkplumber.org>
wrote:

> On Thu,  5 May 2022 19:30:01 +0200
> Stanislaw Kardach <kda@semihalf.com> wrote:
>
> > When compiling for RISC-V in debug mode the large amount of inlining in
> > test_ring_basic_ex() and test_ring_with_exact_size() (in test_ring.c)
> > leads to large loop bodies. This causes 'goto' and 'for' loop
> > PC-relative jumps generated by the compiler to go beyond the architecture
> > limitation of +/-1MB offset (the 'j <offset>' instruction). This
> > instruction should not be generated by the compiler since C language does
> > not limit the maximum distance for 'goto' or 'for' loop jumps.
> >
> > This only happens in the unit test for ring which tries to perform long
> > loops with ring enqueue/dequeue and it seems to be caused by excessive
> > __rte_always_inline usage. ring perf test compiles just fine under
> > debug.
> >
> > To work around this, disable the offending tests in debug mode.
> >
> > Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> > Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
> > Sponsored-by: Sam Grove <sam.grove@sifive.com>
> > ---
>
> It seems to me that fixing the excessive inlining in the ring code
> could benefit all architectures, rather than neutering the tests
> on RISCV.
>
 True. Since this only happened in the tests that I've mentioned, my other
approach was to introduce a "slow" wrapper over test_ring_dequeue|enqueue()
which did not force inlining via __rte_always_inline and use it in
functional test functions. However after talking with Thomas Monjalon we've
decided to guard the debug build of RISC-V only.
Another thing is that this is a clear bug in the compiler, the relaxation
of the jump should not be done since RISC-V has long jump construct for
arbitrary jumps (auipc+jalr).
Stephen Hemminger May 5, 2022, 6:06 p.m. UTC | #3
On Thu, 5 May 2022 19:43:43 +0200
Stanisław Kardach <kda@semihalf.com> wrote:

> On Thu, May 5, 2022 at 7:35 PM Stephen Hemminger <stephen@networkplumber.org>
> wrote:
> 
> > On Thu,  5 May 2022 19:30:01 +0200
> > Stanislaw Kardach <kda@semihalf.com> wrote:
> >  
> > > When compiling for RISC-V in debug mode the large amount of inlining in
> > > test_ring_basic_ex() and test_ring_with_exact_size() (in test_ring.c)
> > > leads to large loop bodies. This causes 'goto' and 'for' loop
> > > PC-relative jumps generated by the compiler to go beyond the architecture
> > > limitation of +/-1MB offset (the 'j <offset>' instruction). This
> > > instruction should not be generated by the compiler since C language does
> > > not limit the maximum distance for 'goto' or 'for' loop jumps.
> > >
> > > This only happens in the unit test for ring which tries to perform long
> > > loops with ring enqueue/dequeue and it seems to be caused by excessive
> > > __rte_always_inline usage. ring perf test compiles just fine under
> > > debug.
> > >
> > > To work around this, disable the offending tests in debug mode.
> > >
> > > Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> > > Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
> > > Sponsored-by: Sam Grove <sam.grove@sifive.com>
> > > ---  
> >
> > It seems to me that fixing the excessive inlining in the ring code
> > could benefit all architectures, rather than neutering the tests
> > on RISCV.
> >  
>  True. Since this only happened in the tests that I've mentioned, my other
> approach was to introduce a "slow" wrapper over test_ring_dequeue|enqueue()
> which did not force inlining via __rte_always_inline and use it in
> functional test functions. However after talking with Thomas Monjalon we've
> decided to guard the debug build of RISC-V only.
> Another thing is that this is a clear bug in the compiler, the relaxation
> of the jump should not be done since RISC-V has long jump construct for
> arbitrary jumps (auipc+jalr).

There is no good reason the __rte_always_inline should be in the ring code.
The purpose of always inline should be only for code that would break if
not inlined.
Honnappa Nagarahalli May 10, 2022, 11:28 p.m. UTC | #4
<snip>

> 
> When compiling for RISC-V in debug mode the large amount of inlining in
> test_ring_basic_ex() and test_ring_with_exact_size() (in test_ring.c) leads to
> large loop bodies. This causes 'goto' and 'for' loop PC-relative jumps generated
> by the compiler to go beyond the architecture limitation of +/-1MB offset (the
> 'j <offset>' instruction). This instruction should not be generated by the
> compiler since C language does not limit the maximum distance for 'goto' or
> 'for' loop jumps.
> 
> This only happens in the unit test for ring which tries to perform long loops
> with ring enqueue/dequeue and it seems to be caused by excessive
> __rte_always_inline usage. ring perf test compiles just fine under debug.
> 
> To work around this, disable the offending tests in debug mode.
Is this still required given you have submitted [1]

[1] http://patches.dpdk.org/project/dpdk/patch/20220510115758.457794-1-kda@semihalf.com/

> 
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
> Sponsored-by: Sam Grove <sam.grove@sifive.com>
> ---
>  app/test/test_ring.c                   | 8 ++++++++
>  config/riscv/meson.build               | 5 +++++
>  doc/guides/rel_notes/release_22_07.rst | 3 ++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test/test_ring.c b/app/test/test_ring.c index
> bde33ab4a1..7d809c147b 100644
> --- a/app/test/test_ring.c
> +++ b/app/test/test_ring.c
> @@ -955,6 +955,7 @@ test_ring_burst_bulk_tests4(unsigned int test_idx)
>  	return -1;
>  }
> 
> +#if !defined(RTE_RISCV_WO_DISABLE_RING_TESTS)
>  /*
>   * Test default, single element, bulk and burst APIs
>   */
> @@ -1189,6 +1190,7 @@ test_ring_with_exact_size(void)
>  	rte_ring_free(exact_sz_r);
>  	return -1;
>  }
> +#endif
> 
>  static int
>  test_ring(void)
> @@ -1200,12 +1202,18 @@ test_ring(void)
>  	if (test_ring_negative_tests() < 0)
>  		goto test_fail;
> 
> +/* Disable the following tests on RISC-V in debug mode. This is a
> +work-around
> + * GCC bug for RISC-V which fails to generate proper jumps for loops
> +with large
> + * bodies.
> + */
> +#if !defined(RTE_RISCV_WO_DISABLE_RING_TESTS)
>  	/* Some basic operations */
>  	if (test_ring_basic_ex() < 0)
>  		goto test_fail;
> 
>  	if (test_ring_with_exact_size() < 0)
>  		goto test_fail;
> +#endif
> 
>  	/* Burst and bulk operations with sp/sc, mp/mc and default.
>  	 * The test cases are split into smaller test cases to diff --git
> a/config/riscv/meson.build b/config/riscv/meson.build index
> 0c16c31fc2..50d0b513bf 100644
> --- a/config/riscv/meson.build
> +++ b/config/riscv/meson.build
> @@ -141,3 +141,8 @@ foreach flag: dpdk_flags  endforeach  message('Using
> machine args: @0@'.format(machine_args))
> 
> +# Enable work-around for ring unit tests in debug mode which fail to
> +link # properly due to bad code generation by GCC.
> +if get_option('optimization') == '0' or get_option('optimization') == 'g'
> +    add_project_arguments('-DRTE_RISCV_WO_DISABLE_RING_TESTS',
> +language: 'c') endif
> diff --git a/doc/guides/rel_notes/release_22_07.rst
> b/doc/guides/rel_notes/release_22_07.rst
> index 453591e568..4d64b68dfd 100644
> --- a/doc/guides/rel_notes/release_22_07.rst
> +++ b/doc/guides/rel_notes/release_22_07.rst
> @@ -76,7 +76,8 @@ New Features
>    * Debug build of ``app/test/dpdk-test`` fails currently on RISC-V due to
>      seemingly invalid loop and goto jump code generation by GCC in
>      ``test_ring.c`` where extensive inlining increases the code size beyond the
> -    capability of the generated instruction (JAL: +/-1MB PC-relative).
> +    capability of the generated instruction (JAL: +/-1MB PC-relative). The
> +    workaround is to disable ``test_ring_basic_ex()`` and
> ``test_ring_with_exact_size()`` on RISC-V on ``-O0`` or ``-Og``.
> 
>  * **Updated Intel iavf driver.**
> 
> --
> 2.30.2
Stanisław Kardach May 11, 2022, 10:07 a.m. UTC | #5
On Wed, May 11, 2022 at 1:28 AM Honnappa Nagarahalli
<Honnappa.Nagarahalli@arm.com> wrote:
>
> <snip>
>
> >
> > When compiling for RISC-V in debug mode the large amount of inlining in
> > test_ring_basic_ex() and test_ring_with_exact_size() (in test_ring.c) leads to
> > large loop bodies. This causes 'goto' and 'for' loop PC-relative jumps generated
> > by the compiler to go beyond the architecture limitation of +/-1MB offset (the
> > 'j <offset>' instruction). This instruction should not be generated by the
> > compiler since C language does not limit the maximum distance for 'goto' or
> > 'for' loop jumps.
> >
> > This only happens in the unit test for ring which tries to perform long loops
> > with ring enqueue/dequeue and it seems to be caused by excessive
> > __rte_always_inline usage. ring perf test compiles just fine under debug.
> >
> > To work around this, disable the offending tests in debug mode.
> Is this still required given you have submitted [1]
>
> [1] http://patches.dpdk.org/project/dpdk/patch/20220510115758.457794-1-kda@semihalf.com/
Correct, this patch is no longer necessary and marked as superseded in
patchwork. I did send v2 and v3 series without it. Thank you for
taking a look.

>
> >
> > Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> > Sponsored-by: Frank Zhao <Frank.Zhao@starfivetech.com>
> > Sponsored-by: Sam Grove <sam.grove@sifive.com>
> > ---
> >  app/test/test_ring.c                   | 8 ++++++++
> >  config/riscv/meson.build               | 5 +++++
> >  doc/guides/rel_notes/release_22_07.rst | 3 ++-
> >  3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test/test_ring.c b/app/test/test_ring.c index
> > bde33ab4a1..7d809c147b 100644
> > --- a/app/test/test_ring.c
> > +++ b/app/test/test_ring.c
> > @@ -955,6 +955,7 @@ test_ring_burst_bulk_tests4(unsigned int test_idx)
> >       return -1;
> >  }
> >
> > +#if !defined(RTE_RISCV_WO_DISABLE_RING_TESTS)
> >  /*
> >   * Test default, single element, bulk and burst APIs
> >   */
> > @@ -1189,6 +1190,7 @@ test_ring_with_exact_size(void)
> >       rte_ring_free(exact_sz_r);
> >       return -1;
> >  }
> > +#endif
> >
> >  static int
> >  test_ring(void)
> > @@ -1200,12 +1202,18 @@ test_ring(void)
> >       if (test_ring_negative_tests() < 0)
> >               goto test_fail;
> >
> > +/* Disable the following tests on RISC-V in debug mode. This is a
> > +work-around
> > + * GCC bug for RISC-V which fails to generate proper jumps for loops
> > +with large
> > + * bodies.
> > + */
> > +#if !defined(RTE_RISCV_WO_DISABLE_RING_TESTS)
> >       /* Some basic operations */
> >       if (test_ring_basic_ex() < 0)
> >               goto test_fail;
> >
> >       if (test_ring_with_exact_size() < 0)
> >               goto test_fail;
> > +#endif
> >
> >       /* Burst and bulk operations with sp/sc, mp/mc and default.
> >        * The test cases are split into smaller test cases to diff --git
> > a/config/riscv/meson.build b/config/riscv/meson.build index
> > 0c16c31fc2..50d0b513bf 100644
> > --- a/config/riscv/meson.build
> > +++ b/config/riscv/meson.build
> > @@ -141,3 +141,8 @@ foreach flag: dpdk_flags  endforeach  message('Using
> > machine args: @0@'.format(machine_args))
> >
> > +# Enable work-around for ring unit tests in debug mode which fail to
> > +link # properly due to bad code generation by GCC.
> > +if get_option('optimization') == '0' or get_option('optimization') == 'g'
> > +    add_project_arguments('-DRTE_RISCV_WO_DISABLE_RING_TESTS',
> > +language: 'c') endif
> > diff --git a/doc/guides/rel_notes/release_22_07.rst
> > b/doc/guides/rel_notes/release_22_07.rst
> > index 453591e568..4d64b68dfd 100644
> > --- a/doc/guides/rel_notes/release_22_07.rst
> > +++ b/doc/guides/rel_notes/release_22_07.rst
> > @@ -76,7 +76,8 @@ New Features
> >    * Debug build of ``app/test/dpdk-test`` fails currently on RISC-V due to
> >      seemingly invalid loop and goto jump code generation by GCC in
> >      ``test_ring.c`` where extensive inlining increases the code size beyond the
> > -    capability of the generated instruction (JAL: +/-1MB PC-relative).
> > +    capability of the generated instruction (JAL: +/-1MB PC-relative). The
> > +    workaround is to disable ``test_ring_basic_ex()`` and
> > ``test_ring_with_exact_size()`` on RISC-V on ``-O0`` or ``-Og``.
> >
> >  * **Updated Intel iavf driver.**
> >
> > --
> > 2.30.2
>
diff mbox series

Patch

diff --git a/app/test/test_ring.c b/app/test/test_ring.c
index bde33ab4a1..7d809c147b 100644
--- a/app/test/test_ring.c
+++ b/app/test/test_ring.c
@@ -955,6 +955,7 @@  test_ring_burst_bulk_tests4(unsigned int test_idx)
 	return -1;
 }
 
+#if !defined(RTE_RISCV_WO_DISABLE_RING_TESTS)
 /*
  * Test default, single element, bulk and burst APIs
  */
@@ -1189,6 +1190,7 @@  test_ring_with_exact_size(void)
 	rte_ring_free(exact_sz_r);
 	return -1;
 }
+#endif
 
 static int
 test_ring(void)
@@ -1200,12 +1202,18 @@  test_ring(void)
 	if (test_ring_negative_tests() < 0)
 		goto test_fail;
 
+/* Disable the following tests on RISC-V in debug mode. This is a work-around
+ * GCC bug for RISC-V which fails to generate proper jumps for loops with large
+ * bodies.
+ */
+#if !defined(RTE_RISCV_WO_DISABLE_RING_TESTS)
 	/* Some basic operations */
 	if (test_ring_basic_ex() < 0)
 		goto test_fail;
 
 	if (test_ring_with_exact_size() < 0)
 		goto test_fail;
+#endif
 
 	/* Burst and bulk operations with sp/sc, mp/mc and default.
 	 * The test cases are split into smaller test cases to
diff --git a/config/riscv/meson.build b/config/riscv/meson.build
index 0c16c31fc2..50d0b513bf 100644
--- a/config/riscv/meson.build
+++ b/config/riscv/meson.build
@@ -141,3 +141,8 @@  foreach flag: dpdk_flags
 endforeach
 message('Using machine args: @0@'.format(machine_args))
 
+# Enable work-around for ring unit tests in debug mode which fail to link
+# properly due to bad code generation by GCC.
+if get_option('optimization') == '0' or get_option('optimization') == 'g'
+    add_project_arguments('-DRTE_RISCV_WO_DISABLE_RING_TESTS', language: 'c')
+endif
diff --git a/doc/guides/rel_notes/release_22_07.rst b/doc/guides/rel_notes/release_22_07.rst
index 453591e568..4d64b68dfd 100644
--- a/doc/guides/rel_notes/release_22_07.rst
+++ b/doc/guides/rel_notes/release_22_07.rst
@@ -76,7 +76,8 @@  New Features
   * Debug build of ``app/test/dpdk-test`` fails currently on RISC-V due to
     seemingly invalid loop and goto jump code generation by GCC in
     ``test_ring.c`` where extensive inlining increases the code size beyond the
-    capability of the generated instruction (JAL: +/-1MB PC-relative).
+    capability of the generated instruction (JAL: +/-1MB PC-relative). The
+    workaround is to disable ``test_ring_basic_ex()`` and ``test_ring_with_exact_size()`` on RISC-V on ``-O0`` or ``-Og``.
 
 * **Updated Intel iavf driver.**