From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A472742C0C; Fri, 2 Jun 2023 06:18:17 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 38A20406B8; Fri, 2 Jun 2023 06:18:17 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 9031040693 for ; Fri, 2 Jun 2023 06:18:15 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id A8A1220FCD39; Thu, 1 Jun 2023 21:18:14 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com A8A1220FCD39 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1685679494; bh=glOjo6q+Uy5uMLwNuzEpdg5+B+mGZdU5kN3lkH5779M=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=I19hJHEk7b6ZOLskiKpcAs68uMw5J3INEJJO+h3sZfo8GS+WYnoajKnUv3ljBtsKT XhoYM3jbyuvgGvHlPERYn4ijwS0zu9IkTqhhIRAyWh57LoCPAvDCN5A2fO4fJxPIbX mBGAJBYEax1PUhVn6iKYODyuMNMsFgm3lDY7TP/g= Date: Thu, 1 Jun 2023 21:18:14 -0700 From: Tyler Retzlaff To: David Marchand Cc: dev@dpdk.org, Honnappa.Nagarahalli@arm.com, Ruifeng.Wang@arm.com, thomas@monjalon.net, Maxime Coquelin Subject: Re: [PATCH 0/3] use C11 memory model GCC builtin atomics Message-ID: <20230602041814.GA27303@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1679927420-26737-1-git-send-email-roretzla@linux.microsoft.com> <20230524160508.GA9733@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230524160508.GA9733@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > > wrote: > > > > > > Replace the use of __sync__and_fetch and __sync_fetch_and_ 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_ with __atomic_ using > SEQ_CST ordering. > > When the __atomic_ builtins were initially introduced they generated > sub-optimal (you can interpret as slower) codegen relative to the > existing __sync_ 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