Message ID | 1679927420-26737-1-git-send-email-roretzla@linux.microsoft.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5D4484284B; Mon, 27 Mar 2023 16:30:24 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E893640EE1; Mon, 27 Mar 2023 16:30:23 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id E463F40ED8 for <dev@dpdk.org>; Mon, 27 Mar 2023 16:30:22 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 3286520FD8A2; Mon, 27 Mar 2023 07:30:22 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3286520FD8A2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1679927422; bh=RMS3xFY7cVIvupl/Ml/h8g/MMLLkibW31+DgY1YTsYU=; h=From:To:Cc:Subject:Date:From; b=pNs7w029xaoRfkDJVzZEKLJuJ0jUqG/Ah6/uQ1kfJuBgOeLrhTxgCz+j5VYhJrfVQ jEUKPgsMPL55S99vPGzKGbS/tmOqzLqrJe+1nM8aHgacYwmpCnAdig9AgrmDN+zgsS hsi+3sn3pJPZYRk3Z9FrGn3/uDN6BtqdmSxuxEOc= From: Tyler Retzlaff <roretzla@linux.microsoft.com> To: dev@dpdk.org Cc: Honnappa.Nagarahalli@arm.com, Ruifeng.Wang@arm.com, thomas@monjalon.net, Tyler Retzlaff <roretzla@linux.microsoft.com> Subject: [PATCH 0/3] use C11 memory model GCC builtin atomics Date: Mon, 27 Mar 2023 07:30:17 -0700 Message-Id: <1679927420-26737-1-git-send-email-roretzla@linux.microsoft.com> X-Mailer: git-send-email 1.8.3.1 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org |
Series |
use C11 memory model GCC builtin atomics
|
|
Message
Tyler Retzlaff
March 27, 2023, 2:30 p.m. UTC
Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics with GCC C11 memory model __atomic builtins. This series contributes to converging on standard atomics in 23.11 but is kept separate as there may be sensitivity to converting from __sync to the C11 memory model builtins. Tyler Retzlaff (3): bus/vmbus: use C11 memory model GCC builtin atomics crypto/ccp: use C11 memory model GCC builtin atomics eal: use C11 memory model GCC builtin atomics drivers/bus/vmbus/vmbus_channel.c | 2 +- drivers/crypto/ccp/ccp_dev.c | 6 ++++-- lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++---------------- 3 files changed, 21 insertions(+), 19 deletions(-)
Comments
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > Sent: Monday, 27 March 2023 16.30 > > Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics > with GCC C11 memory model __atomic builtins. > > This series contributes to converging on standard atomics in 23.11 but is > kept separate as there may be sensitivity to converting from __sync to the > C11 memory model builtins. > > Tyler Retzlaff (3): > bus/vmbus: use C11 memory model GCC builtin atomics > crypto/ccp: use C11 memory model GCC builtin atomics > eal: use C11 memory model GCC builtin atomics > > drivers/bus/vmbus/vmbus_channel.c | 2 +- > drivers/crypto/ccp/ccp_dev.c | 6 ++++-- > lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++---------------- > 3 files changed, 21 insertions(+), 19 deletions(-) > > -- > 1.8.3.1 > Series-reviewed-by: Morten Brørup <mb@smartsharesystems.com>
On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff <roretzla@linux.microsoft.com> wrote: > > Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics > with GCC C11 memory model __atomic builtins. > > This series contributes to converging on standard atomics in 23.11 but is > kept separate as there may be sensitivity to converting from __sync to the > C11 memory model builtins. - Looking at the patches, I thought the conversion was rather straightforward. But this mention about "sensitivity" stopped me from merging. Did I miss some risk with the changes of this series? > > Tyler Retzlaff (3): > bus/vmbus: use C11 memory model GCC builtin atomics > crypto/ccp: use C11 memory model GCC builtin atomics > eal: use C11 memory model GCC builtin atomics > > drivers/bus/vmbus/vmbus_channel.c | 2 +- > drivers/crypto/ccp/ccp_dev.c | 6 ++++-- > lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++---------------- > 3 files changed, 21 insertions(+), 19 deletions(-) - I noticed that the vhost library has been providing an internal wrapper for some __sync atomic with older GCC. Some details are in the commitlog c16915b87109 ("vhost: improve dirty pages logging performance"). Could it affect the existing legacy API performance?
On Wed, May 24, 2023 at 02:51:50PM +0200, David Marchand wrote: > On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff > <roretzla@linux.microsoft.com> wrote: > > > > Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics > > with GCC C11 memory model __atomic builtins. > > > > This series contributes to converging on standard atomics in 23.11 but is > > kept separate as there may be sensitivity to converting from __sync to the > > C11 memory model builtins. > > - Looking at the patches, I thought the conversion was rather straightforward. > But this mention about "sensitivity" stopped me from merging. > Did I miss some risk with the changes of this series? > > > > > > Tyler Retzlaff (3): > > bus/vmbus: use C11 memory model GCC builtin atomics > > crypto/ccp: use C11 memory model GCC builtin atomics > > eal: use C11 memory model GCC builtin atomics > > > > drivers/bus/vmbus/vmbus_channel.c | 2 +- > > drivers/crypto/ccp/ccp_dev.c | 6 ++++-- > > lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++---------------- > > 3 files changed, 21 insertions(+), 19 deletions(-) > > > - I noticed that the vhost library has been providing an internal > wrapper for some __sync atomic with older GCC. > Some details are in the commitlog c16915b87109 ("vhost: improve dirty > pages logging performance"). > > Could it affect the existing legacy API performance? Yes. gcc documents that you can replace __sync_<op> with __atomic_<op> using SEQ_CST ordering. When the __atomic_<op> builtins were initially introduced they generated sub-optimal (you can interpret as slower) codegen relative to the existing __sync_<op> builtins which was fixed in later gcc releases. I do not know the actual version of gcc, but the commit you reference indicates GCC_VERSION < 70100 is that boundary. I (perhaps incorrectly) assumed that if the CI performance tests didn't indicate a regression that the replacement of the remaining and minimal use of the legacy API would have negligable impact. If this is a bad assumption or there are concerns, I could update the series to do the conditional __sync vs __atomic throughout. Let me know how you'd like to proceed. Thanks! > > -- > David Marchand
On Wed, May 24, 2023 at 09:05:08AM -0700, Tyler Retzlaff wrote: > On Wed, May 24, 2023 at 02:51:50PM +0200, David Marchand wrote: > > On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff > > <roretzla@linux.microsoft.com> wrote: > > > > > > Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics > > > with GCC C11 memory model __atomic builtins. > > > > > > This series contributes to converging on standard atomics in 23.11 but is > > > kept separate as there may be sensitivity to converting from __sync to the > > > C11 memory model builtins. > > > > - Looking at the patches, I thought the conversion was rather straightforward. > > But this mention about "sensitivity" stopped me from merging. > > Did I miss some risk with the changes of this series? > > > > > > > > > > Tyler Retzlaff (3): > > > bus/vmbus: use C11 memory model GCC builtin atomics > > > crypto/ccp: use C11 memory model GCC builtin atomics > > > eal: use C11 memory model GCC builtin atomics > > > > > > drivers/bus/vmbus/vmbus_channel.c | 2 +- > > > drivers/crypto/ccp/ccp_dev.c | 6 ++++-- > > > lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++---------------- > > > 3 files changed, 21 insertions(+), 19 deletions(-) > > > > > > - I noticed that the vhost library has been providing an internal > > wrapper for some __sync atomic with older GCC. > > Some details are in the commitlog c16915b87109 ("vhost: improve dirty > > pages logging performance"). > > > > Could it affect the existing legacy API performance? > > Yes. > > gcc documents that you can replace __sync_<op> with __atomic_<op> using > SEQ_CST ordering. > > When the __atomic_<op> builtins were initially introduced they generated > sub-optimal (you can interpret as slower) codegen relative to the > existing __sync_<op> builtins which was fixed in later gcc releases. > > I do not know the actual version of gcc, but the commit you reference > indicates GCC_VERSION < 70100 is that boundary. > > I (perhaps incorrectly) assumed that if the CI performance tests didn't > indicate a regression that the replacement of the remaining and minimal > use of the legacy API would have negligable impact. > > If this is a bad assumption or there are concerns, I could update the series > to do the conditional __sync vs __atomic throughout. > > Let me know how you'd like to proceed. Anything further here or want to keep it as is? > > Thanks! > > > > > -- > > David Marchand
On Fri, Jun 2, 2023 at 6:18 AM Tyler Retzlaff <roretzla@linux.microsoft.com> wrote: > > On Wed, May 24, 2023 at 09:05:08AM -0700, Tyler Retzlaff wrote: > > On Wed, May 24, 2023 at 02:51:50PM +0200, David Marchand wrote: > > > On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff > > > <roretzla@linux.microsoft.com> wrote: > > > > > > > > Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics > > > > with GCC C11 memory model __atomic builtins. > > > > > > > > This series contributes to converging on standard atomics in 23.11 but is > > > > kept separate as there may be sensitivity to converting from __sync to the > > > > C11 memory model builtins. > > > > > > - Looking at the patches, I thought the conversion was rather straightforward. > > > But this mention about "sensitivity" stopped me from merging. > > > Did I miss some risk with the changes of this series? > > > > > > > > > > > > > > Tyler Retzlaff (3): > > > > bus/vmbus: use C11 memory model GCC builtin atomics > > > > crypto/ccp: use C11 memory model GCC builtin atomics > > > > eal: use C11 memory model GCC builtin atomics > > > > > > > > drivers/bus/vmbus/vmbus_channel.c | 2 +- > > > > drivers/crypto/ccp/ccp_dev.c | 6 ++++-- > > > > lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++---------------- > > > > 3 files changed, 21 insertions(+), 19 deletions(-) > > > > > > > > > - I noticed that the vhost library has been providing an internal > > > wrapper for some __sync atomic with older GCC. > > > Some details are in the commitlog c16915b87109 ("vhost: improve dirty > > > pages logging performance"). > > > > > > Could it affect the existing legacy API performance? > > > > Yes. > > > > gcc documents that you can replace __sync_<op> with __atomic_<op> using > > SEQ_CST ordering. > > > > When the __atomic_<op> builtins were initially introduced they generated > > sub-optimal (you can interpret as slower) codegen relative to the > > existing __sync_<op> builtins which was fixed in later gcc releases. > > > > I do not know the actual version of gcc, but the commit you reference > > indicates GCC_VERSION < 70100 is that boundary. > > > > I (perhaps incorrectly) assumed that if the CI performance tests didn't > > indicate a regression that the replacement of the remaining and minimal > > use of the legacy API would have negligable impact. I don't think all the targets/tests are run in the CI (when compared to what is done during a -rcX validation). But I can understand this assumption. > > > > If this is a bad assumption or there are concerns, I could update the series > > to do the conditional __sync vs __atomic throughout. Nobody else complained/reacted on the topic. The conditional would have to be duplicated in various places. So ok, let's go with this approach and see if the rc1 validation (which may run more tests) catches something. > > > > Let me know how you'd like to proceed. > > Anything further here or want to keep it as is? Nop, thanks.
On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff <roretzla@linux.microsoft.com> wrote: > > Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics > with GCC C11 memory model __atomic builtins. > > This series contributes to converging on standard atomics in 23.11 but is > kept separate as there may be sensitivity to converting from __sync to the > C11 memory model builtins. > > Tyler Retzlaff (3): > bus/vmbus: use C11 memory model GCC builtin atomics > crypto/ccp: use C11 memory model GCC builtin atomics > eal: use C11 memory model GCC builtin atomics > > drivers/bus/vmbus/vmbus_channel.c | 2 +- > drivers/crypto/ccp/ccp_dev.c | 6 ++++-- > lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++---------------- > 3 files changed, 21 insertions(+), 19 deletions(-) Please use get-maintainers.sh when submitting patches. The change looks ok to me and rc1 validation will confirm if there is an impact on performance. Series applied, thanks.