| 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 |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | warning | coding style issues |
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.
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).
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.
<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
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 --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.**
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(-)