diff mbox series

[2/2] mempool: use rte constant macro instead of GCC builtin

Message ID 1710970416-27841-3-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series provide toolchain abstracted __builtin_constant_p |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-intel-Performance pending Performance Testing pending RETEST #3
ci/iol-mellanox-Performance success Performance Testing PASS RETEST #3
ci/iol-abi-testing success Testing PASS RETEST #3
ci/iol-marvell-Functional success Functional Testing PASS RETEST #3
ci/iol-compile-amd64-testing success Testing PASS RETEST #3
ci/iol-unit-amd64-testing fail Testing issues RETEST #3
ci/iol-unit-arm64-testing success Testing PASS RETEST #3
ci/iol-compile-arm64-testing success Testing PASS RETEST #3
ci/iol-sample-apps-testing success Testing PASS RETEST #3
ci/iol-broadcom-Performance success Performance Testing PASS RETEST #3
ci/iol-intel-Functional success Functional Testing PASS RETEST #3
ci/iol-broadcom-Functional success Functional Testing PASS RETEST #3

Commit Message

Tyler Retzlaff March 20, 2024, 9:33 p.m. UTC 
Use newly introduced __rte_constant(e) macro instead of directly using
__builtin_constant_p() allowing mempool to be built by MSVC.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/mempool/rte_mempool.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Morten Brørup March 26, 2024, 9:57 a.m. UTC | #1 
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 20 March 2024 22.34
> 
> Use newly introduced __rte_constant(e) macro instead of directly using
> __builtin_constant_p() allowing mempool to be built by MSVC.
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
Andrew Rybchenko May 29, 2024, 11:42 a.m. UTC | #2 
On 3/26/24 12:57, Morten Brørup wrote:
>> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
>> Sent: Wednesday, 20 March 2024 22.34
>>
>> Use newly introduced __rte_constant(e) macro instead of directly using
>> __builtin_constant_p() allowing mempool to be built by MSVC.
>>
>> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>> ---
> 
> Reviewed-by: Morten Brørup <mb@smartsharesystems.com>
> 

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Thomas Monjalon May 29, 2024, 2:51 p.m. UTC | #3 
20/03/2024 22:33, Tyler Retzlaff:
> Use newly introduced __rte_constant(e) macro instead of directly using
> __builtin_constant_p() allowing mempool to be built by MSVC.

Does it mean we should enable mempool build?
If yes, please send a v2.
David Marchand June 14, 2024, 2:32 p.m. UTC | #4 
On Wed, May 29, 2024 at 4:51 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 20/03/2024 22:33, Tyler Retzlaff:
> > Use newly introduced __rte_constant(e) macro instead of directly using
> > __builtin_constant_p() allowing mempool to be built by MSVC.
>
> Does it mean we should enable mempool build?
> If yes, please send a v2.

I guess now it is possible, as I merged some other patches on mempool
from Stephen that were for MSVC.
Tyler, can you send a v2 so it passes through the CI?

Thanks.
Thomas Monjalon July 3, 2024, 1:16 p.m. UTC | #5 
14/06/2024 16:32, David Marchand:
> On Wed, May 29, 2024 at 4:51 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 20/03/2024 22:33, Tyler Retzlaff:
> > > Use newly introduced __rte_constant(e) macro instead of directly using
> > > __builtin_constant_p() allowing mempool to be built by MSVC.
> >
> > Does it mean we should enable mempool build?
> > If yes, please send a v2.
> 
> I guess now it is possible, as I merged some other patches on mempool
> from Stephen that were for MSVC.
> Tyler, can you send a v2 so it passes through the CI?

I tried a retest last week and there is this failure on Ubuntu 24.04
that I don't manage to reproduce locally:

/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2 null where non-null expected [-Werror=nonnull]
29 |   return __builtin___memcpy_chk (__dest, __src, __len,
|          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
30 |                                  __glibc_objsize0 (__dest));
|                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call to built-in function '__builtin___memcpy_chk'
In function 'memcpy',
inlined from 'pcapng_add_option' at ../lib/pcapng/rte_pcapng.c:131:2,
inlined from 'rte_pcapng_write_stats' at ../lib/pcapng/rte_pcapng.c:371:9:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2 null where non-null expected [-Werror=nonnull]
29 |   return __builtin___memcpy_chk (__dest, __src, __len,
|          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
30 |                                  __glibc_objsize0 (__dest));
|                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call to built-in function '__builtin___memcpy_chk'
Morten Brørup July 3, 2024, 1:49 p.m. UTC | #6 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, 3 July 2024 15.17
> 
> 14/06/2024 16:32, David Marchand:
> > On Wed, May 29, 2024 at 4:51 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 20/03/2024 22:33, Tyler Retzlaff:
> > > > Use newly introduced __rte_constant(e) macro instead of directly using
> > > > __builtin_constant_p() allowing mempool to be built by MSVC.
> > >
> > > Does it mean we should enable mempool build?
> > > If yes, please send a v2.
> >
> > I guess now it is possible, as I merged some other patches on mempool
> > from Stephen that were for MSVC.
> > Tyler, can you send a v2 so it passes through the CI?
> 
> I tried a retest last week and there is this failure on Ubuntu 24.04
> that I don't manage to reproduce locally:
> 
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2
> null where non-null expected [-Werror=nonnull]
> 29 |   return __builtin___memcpy_chk (__dest, __src, __len,
> |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 30 |                                  __glibc_objsize0 (__dest));
> |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call
> to built-in function '__builtin___memcpy_chk'
> In function 'memcpy',
> inlined from 'pcapng_add_option' at ../lib/pcapng/rte_pcapng.c:131:2,

pcapng_add_option() in rte_pcapng.c has memcpy() on line 132 [1] (and has a fix for this error, by comparing len > 0 before calling memcpy()); older versions had memcpy() on line 131, so the CI must be building with an older version of rte_pcapng.c.

[1]: https://elixir.bootlin.com/dpdk/v24.07-rc1/source/lib/pcapng/rte_pcapng.c#L132
[2]: https://elixir.bootlin.com/dpdk/v24.03/source/lib/pcapng/rte_pcapng.c#L131

> inlined from 'rte_pcapng_write_stats' at ../lib/pcapng/rte_pcapng.c:371:9:
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2
> null where non-null expected [-Werror=nonnull]
> 29 |   return __builtin___memcpy_chk (__dest, __src, __len,
> |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 30 |                                  __glibc_objsize0 (__dest));
> |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call
> to built-in function '__builtin___memcpy_chk'
> 
>
David Marchand July 3, 2024, 2:22 p.m. UTC | #7 
On Wed, Jul 3, 2024 at 3:49 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Wednesday, 3 July 2024 15.17
> >
> > 14/06/2024 16:32, David Marchand:
> > > On Wed, May 29, 2024 at 4:51 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > >
> > > > 20/03/2024 22:33, Tyler Retzlaff:
> > > > > Use newly introduced __rte_constant(e) macro instead of directly using
> > > > > __builtin_constant_p() allowing mempool to be built by MSVC.
> > > >
> > > > Does it mean we should enable mempool build?
> > > > If yes, please send a v2.
> > >
> > > I guess now it is possible, as I merged some other patches on mempool
> > > from Stephen that were for MSVC.
> > > Tyler, can you send a v2 so it passes through the CI?
> >
> > I tried a retest last week and there is this failure on Ubuntu 24.04
> > that I don't manage to reproduce locally:
> >
> > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error: argument 2
> > null where non-null expected [-Werror=nonnull]
> > 29 |   return __builtin___memcpy_chk (__dest, __src, __len,
> > |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 30 |                                  __glibc_objsize0 (__dest));
> > |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: note: in a call
> > to built-in function '__builtin___memcpy_chk'
> > In function 'memcpy',
> > inlined from 'pcapng_add_option' at ../lib/pcapng/rte_pcapng.c:131:2,
>
> pcapng_add_option() in rte_pcapng.c has memcpy() on line 132 [1] (and has a fix for this error, by comparing len > 0 before calling memcpy()); older versions had memcpy() on line 131, so the CI must be building with an older version of rte_pcapng.c.
>
> [1]: https://elixir.bootlin.com/dpdk/v24.07-rc1/source/lib/pcapng/rte_pcapng.c#L132
> [2]: https://elixir.bootlin.com/dpdk/v24.03/source/lib/pcapng/rte_pcapng.c#L131

This is likely the reason.
Looking at the report:

http://mails.dpdk.org/archives/test-report/2024-June/717951.html

_Testing issues RETEST #1_

Submitter: Tyler Retzlaff <roretzla at linux.microsoft.com>
Date: Wednesday, March 20 2024 21:33:36
DPDK git baseline: Repo:dpdk
  Branch: master
  CommitID:80ecef6d1f71fcebc0a51d7cabc51f73ee142ff2


$ git describe --contains 80ecef6d1f71fcebc0a51d7cabc51f73ee142ff2
v24.03-rc3^0


From the discussions on the retest mechanism, I understand we need to
ask for a rebase.
I sent a new retest. Let's see...
Patrick Robb July 3, 2024, 3:28 p.m. UTC | #8 
On Wed, Jul 3, 2024 at 10:22 AM David Marchand
<david.marchand@redhat.com> wrote:
>
>
>
> From the discussions on the retest mechanism, I understand we need to
> ask for a rebase.
> I sent a new retest. Let's see...
>

Hi,

That makes sense that we need to re-apply on the latest mainline and
retest. But, the "rebase" feature for rechecks which you are referring
to is still in development. So, unfortunately your request won't work
with the automated system - it's just going to recheck from the DPDK
artifact created in March when Tyler originally submitted the series.

For now, I can manually trigger a job in our CI platform which will
re-apply Tyler's patch to the up to date tree, and rerun all testing.

Thanks!
diff mbox series

Patch

diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
index 23fd5c8..a3564fb 100644
--- a/lib/mempool/rte_mempool.h
+++ b/lib/mempool/rte_mempool.h
@@ -1521,7 +1521,7 @@  struct rte_mempool_cache *
 	/* The cache is a stack, so copy will be in reverse order. */
 	cache_objs = &cache->objs[cache->len];
 
-	if (__extension__(__builtin_constant_p(n)) && n <= cache->len) {
+	if (__rte_constant(n) && n <= cache->len) {
 		/*
 		 * The request size is known at build time, and
 		 * the entire request can be satisfied from the cache,
@@ -1542,8 +1542,7 @@  struct rte_mempool_cache *
 	 * If the request size 'n' is known at build time, the above comparison
 	 * ensures that n > cache->len here, so omit RTE_MIN().
 	 */
-	len = __extension__(__builtin_constant_p(n)) ? cache->len :
-			RTE_MIN(n, cache->len);
+	len = __rte_constant(n) ? cache->len : RTE_MIN(n, cache->len);
 	cache->len -= len;
 	remaining = n - len;
 	for (index = 0; index < len; index++)
@@ -1554,7 +1553,7 @@  struct rte_mempool_cache *
 	 * where the entire request can be satisfied from the cache
 	 * has already been handled above, so omit handling it here.
 	 */
-	if (!__extension__(__builtin_constant_p(n)) && remaining == 0) {
+	if (!__rte_constant(n) && remaining == 0) {
 		/* The entire request is satisfied from the cache. */
 
 		RTE_MEMPOOL_CACHE_STAT_ADD(cache, get_success_bulk, 1);