From: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
To: Thomas Monjalon <thomas@monjalon.net>,
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
Amit Prakash Shukla <amitprakashs@marvell.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
"david.marchand@redhat.com" <david.marchand@redhat.com>,
"bruce.richardson@intel.com" <bruce.richardson@intel.com>,
"ferruh.yigit@amd.com" <ferruh.yigit@amd.com>
Subject: Re: [EXT] Re: [PATCH] ring: compilation fix with GCC-12
Date: Mon, 13 Feb 2023 01:48:15 +0000 [thread overview]
Message-ID: <0c9daa31-5f4c-5252-c4f0-0e45ac5d2953@yandex.ru> (raw)
In-Reply-To: <2906200.X9hSmTKtgW@thomas>
13/01/2023 13:11, Thomas Monjalon пишет:
> 13/01/2023 13:39, Amit Prakash Shukla:
>> From: Thomas Monjalon <thomas@monjalon.net>
>>> 23/08/2022 11:38, Amit Prakash Shukla:
>>>> From: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
>>>>> 06/08/2022 19:35, Honnappa Nagarahalli пишет:
>>>>>>> Replacing memcpy with rte_memcpy fixes the GCC-12 compilation
>>> issue.
>>>>>>
>>>>>> Any reason why this replacement fixes the problem?
>>>>>> Do you have any performance numbers with this change?
>>>>>>
>>>>>>> Also it would be better to change to rte_memcpy as the function
>>>>>>> is called in fastpath.
>>>>>>
>>>>>> On Arm platforms, memcpy in the later versions has the best
>>> performance.
>>>>>
>>>>> I agree with Honnappa, it is better to keep memcpy() here.
>>>>> Actually what is strange - why it ends up in
>>>>> __rte_ring_dequeue_elems_128() at all?
>>>>> Inside rxa_intr_ring_dequeue() we clearly doing: rte_ring_dequeue(),
>>>>> which should boil down to ___rte_ring_dequeue_elems_64().
>>>>> it should go to __rte_ring_dequeue_elems_128() at all.
>>>>
>>>> I agree. After having close look and doing few experiments, ideally it
>>>> should not be going to __rte_ring_dequeue_elems_128().
>>>> Sizeof(in call of rte_ring_enqueue_elem) gets evaluated at compile
>>>> time which in this case it is evaluated to 8 bytes so
>>>> __rte_ring_dequeue_elems_128() shall not be in the path. Looks like more
>>> of a gcc-12 bug.?
>>>>
>>>>> Another q - is this warning happens only on arm platforms?
>>>>
>>>> Warning is observed on x86 with build type as debug.
>>>> "meson --werror --buildtype=debug build"
>>>
>>> I confirm the compilation issue on x86 with GCC 12 in a debug build.
>>>
>>> We need to find a workaround.
>>> Is it reported to GCC already?
>>>
>> I found an old gcc bug reporting similar issue. This bug seems to be re-opened recently in Dec-2022. Not sure if it is reopened specifically for gcc-12.
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89689
>
> Please would you like to open a bug specific to GCC 12?
>
>> Kevin has push a work around for DPDK-21.11.3.
>> https://git.dpdk.org/dpdk-stable/commit/?h=21.11&id=e1d728588dc73af9ed60cc0074d51a7f24b2ba60
>
> In the meantime we could use Kevin's workaround:
>
> #if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 120000)
> #pragma GCC diagnostic push
> #pragma GCC diagnostic ignored "-Wstringop-overflow"
> #pragma GCC diagnostic ignored "-Wstringop-overread"
> #endif
>
> Opinions?
>
>
Yep, disable warnings should work.
Anoter way to consider - change enqueue/dequeue_elems_128()
functions to not use memcpy() at all.
Instead of that they can copy 2*num 64-bit entities directly,
same as _64_ versions do.
Something like the patch below.
That's pretty similar to what Amit initially proposed,
but without rte_memcpy() involvement.
Performance-wise I don't expect noticeable difference with
what we have right now.
But sure, we'll need to do extra checks here.
diff --git a/lib/ring/rte_ring_elem_pvt.h b/lib/ring/rte_ring_elem_pvt.h
index 83788c56e6..de79040618 100644
--- a/lib/ring/rte_ring_elem_pvt.h
+++ b/lib/ring/rte_ring_elem_pvt.h
@@ -93,25 +93,32 @@ __rte_ring_enqueue_elems_128(struct rte_ring *r,
uint32_t prod_head,
unsigned int i;
const uint32_t size = r->size;
uint32_t idx = prod_head & r->mask;
- rte_int128_t *ring = (rte_int128_t *)&r[1];
- const rte_int128_t *obj = (const rte_int128_t *)obj_table;
+ uint64_t *ring = (uint64_t *)&r[1];
+ const unaligned_uint64_t *obj = (const unaligned_uint64_t *)obj_table;
if (likely(idx + n <= size)) {
- for (i = 0; i < (n & ~0x1); i += 2, idx += 2)
- memcpy((void *)(ring + idx),
- (const void *)(obj + i), 32);
+ idx *= 2;
+ for (i = 0; i < 2 * (n & ~0x1); i += 4, idx += 4) {
+ ring[idx] = obj[i];
+ ring[idx + 1] = obj[i + 1];
+ ring[idx + 2] = obj[i + 2];
+ ring[idx + 3] = obj[i + 3];
+ }
switch (n & 0x1) {
case 1:
- memcpy((void *)(ring + idx),
- (const void *)(obj + i), 16);
+ ring[idx] = obj[i];
+ ring[idx + 1] = obj[i + 1];
}
} else {
- for (i = 0; idx < size; i++, idx++)
- memcpy((void *)(ring + idx),
- (const void *)(obj + i), 16);
+ idx *= 2;
+ for (i = 0; idx < 2 * size; i += 2, idx += 2) {
+ ring[idx] = obj[i];
+ ring[idx + 1] = obj[i + 1];
+ }
/* Start at the beginning */
- for (idx = 0; i < n; i++, idx++)
- memcpy((void *)(ring + idx),
- (const void *)(obj + i), 16);
+ for (idx = 0; i < 2 * n; i += 2, idx += 2) {
+ ring[idx] = obj[i];
+ ring[idx + 1] = obj[i + 1];
+ }
}
}
@@ -227,21 +234,32 @@ __rte_ring_dequeue_elems_128(struct rte_ring *r,
uint32_t prod_head,
unsigned int i;
const uint32_t size = r->size;
uint32_t idx = prod_head & r->mask;
- rte_int128_t *ring = (rte_int128_t *)&r[1];
- rte_int128_t *obj = (rte_int128_t *)obj_table;
+ uint64_t *ring = (uint64_t *)&r[1];
+ unaligned_uint64_t *obj = (unaligned_uint64_t *)obj_table;
if (likely(idx + n <= size)) {
- for (i = 0; i < (n & ~0x1); i += 2, idx += 2)
- memcpy((void *)(obj + i), (void *)(ring + idx), 32);
+ idx *= 2;
+ for (i = 0; i < 2 * (n & ~0x1); i += 4, idx += 4) {
+ obj[i] = ring[idx];
+ obj[i + 1] = ring[idx + 1];
+ obj[i + 2] = ring[idx + 2];
+ obj[i + 3] = ring[idx + 3];
+ }
switch (n & 0x1) {
case 1:
- memcpy((void *)(obj + i), (void *)(ring + idx), 16);
+ obj[i] = ring[idx];
+ obj[i + 1] = ring[idx + 1];
}
} else {
- for (i = 0; idx < size; i++, idx++)
- memcpy((void *)(obj + i), (void *)(ring + idx), 16);
+ idx *= 2;
+ for (i = 0; idx < 2 * size; i += 2, idx += 2) {
+ obj[i] = ring[idx];
+ obj[i + 1] = ring[idx + 1];
+ }
/* Start at the beginning */
- for (idx = 0; i < n; i++, idx++)
- memcpy((void *)(obj + i), (void *)(ring + idx), 16);
+ for (idx = 0; i < 2 * n; i += 2, idx += 2) {
+ obj[i] = ring[idx];
+ obj[i + 1] = ring[idx + 1];
+ }
}
}
prev parent reply other threads:[~2023-02-13 1:48 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-05 9:03 Amit Prakash Shukla
2022-08-05 15:37 ` Stephen Hemminger
2022-08-06 18:35 ` Honnappa Nagarahalli
2022-08-07 12:26 ` Konstantin Ananyev
2022-08-23 9:38 ` [EXT] " Amit Prakash Shukla
2022-08-23 9:41 ` Amit Prakash Shukla
2023-01-12 21:41 ` Thomas Monjalon
2023-01-13 12:39 ` Amit Prakash Shukla
2023-01-13 13:11 ` Thomas Monjalon
2023-02-13 1:48 ` Konstantin Ananyev [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0c9daa31-5f4c-5252-c4f0-0e45ac5d2953@yandex.ru \
--to=konstantin.v.ananyev@yandex.ru \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=amitprakashs@marvell.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=jerinj@marvell.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).