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 28B104280B; Wed, 22 Mar 2023 16:29:36 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BA12D40E09; Wed, 22 Mar 2023 16:29:35 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id E2E1240A84 for ; Wed, 22 Mar 2023 16:29:33 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id F3CB720FBA83; Wed, 22 Mar 2023 08:29:32 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com F3CB720FBA83 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1679498973; bh=gNT1E+Jm0kpCO9jD+FHTkiCWNUlUvyurQ4Gy34JnQ3c=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WmfYLGfx3K9C15nh4on4SXW80BxWdpn+c2OKQdmUsvo6wOjkDYk2ex1Eyu1i+Vih7 5U2rbo5uR319sL3JXWmOFSSY458xLtoMThhivGAiPIybVjk6J+Gd3xXs1dNwxCmGvo GUIkkPCed4ivypjUiIxOB71+VppPlAP6QGCHp5Lg= Date: Wed, 22 Mar 2023 08:29:32 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Stephen Hemminger , dev@dpdk.org, Honnappa.Nagarahalli@arm.com, Ruifeng.Wang@arm.com, thomas@monjalon.net Subject: Re: [PATCH 0/7] replace rte atomics with GCC builtin atomics Message-ID: <20230322152932.GB29057@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1679084388-19267-1-git-send-email-roretzla@linux.microsoft.com> <20230317144226.2f26bad1@hermes.local> <20230317214910.GA31884@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D877E1@smartserver.smartshare.dk> <20230322142134.GA29057@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D877E6@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D877E6@smartserver.smartshare.dk> 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, Mar 22, 2023 at 03:58:07PM +0100, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Wednesday, 22 March 2023 15.22 > > > > On Wed, Mar 22, 2023 at 12:28:44PM +0100, Morten Brørup wrote: > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > > Sent: Friday, 17 March 2023 22.49 > > > > > > > > On Fri, Mar 17, 2023 at 02:42:26PM -0700, Stephen Hemminger wrote: > > > > > On Fri, 17 Mar 2023 13:19:41 -0700 > > > > > Tyler Retzlaff wrote: > > > > > > > > > > > Replace the use of rte_atomic.h types and functions, instead use GCC > > > > > > supplied C++11 memory model builtins. > > > > > > > > > > > > This series covers the libraries and drivers that are built on > > Windows. > > > > > > > > > > > > The code has be converted to use the __atomic builtins but there are > > > > > > additional during conversion i notice that there may be some issues > > > > > > that need to be addressed. > > > > > > > > > > I don't think all these cmpset need to use SEQ_CST. > > > > > Especially for the places where it is used a loop, might > > > > > be more efficient with some of the other memory models. > > > > > > > > i agree. > > > > > > > > however, i'm not trying to improve the code with this change, just > > > > decouple it from rte_atomics.h so trying my best to avoid any > > > > unnecessary semantic change. > > > > > > > > certainly if the maintainers of this code wish to weaken the ordering > > > > where appropriate after the change is merged they can do so and handily > > > > this change has enabled them to do so easily allowing them to test just > > > > their change in isolation. > > > > > > I agree with the two-step approach, where this first step is a simple > > search-and-replacement; but I insist that you add a FIXME or similar note > > where you have blindly used SEQ_CST, indicating that the memory order needs to > > be reviewed and potentially corrected. > > > > i think the maintainers need to take some responsibility, if they see > > optimizations they missed when previously writing the code they need to > > follow up with a patch themselves. i can't do everything for them and > > marking things i'm not sure about will only lead to me having to churn > > patch series to remove the unwanted comments later. > > The previous atomic functions didn't have the "memory order" parameter, so the maintainers didn't have to think about it - and thus they didn't miss any optimizations when accepting the code. > > I also agree 100 % that it is not your responsibility to consider or determine which memory order is appropriate! > > But I think you should mark the locations where you are changing from the old rte_atomic functions (where no memory order optimization was available) to the new functions - to highlight where the option of memory ordering has been introduced and knowingly ignored (by you). > first, i have to apologize i confused myself about which of the many patch series i have up right now that you were commenting on. let me ask for clarification in relation to this series. isn't that every single usage of the rte_atomic APIs? i mean are you literally asking for the entire patch series to look like the following patch snippet with the expectation that maintainers will come along and clean up/review after this series is merged? -rte_atomic_add32(&o, v); +//FIXME: opportunity for relaxing ordering constraint, please review +__atomic_fetch_add(&o, v, order); this would just be a mechanical addition to this series so i can certainly accomodate that, i thought something more complicated was being asked for. if this is all, then sure no problem. > > keep in mind i have to touch each of these again when converting to > > standard so that's a better time to review ~everything in more detail > > because when converting to standard that's when suddenly you get a bunch > > of code generation that is "fallback" to seq_cst that isn't happening now. > > I think you should to do it when replacing the rte_atomic functions with the __atomic functions. It will make it easier to see where the memory order was knowingly ignored, and should be reviewed for optimization. > > > > > the series that converts to standard needs to be up for review as soon > > as possible to maximize available time for feedback before 23.11 so it > > would be better to get the simpler cut & paste normalizing the code out > > of the way to unblock that series submission. > > > > > > > > Also, in a couple of the drivers, you are using int64_t for packet counters. > > These cannot be negative and should be uint64_t. And AFAIK, such counters can > > use RELAXED memory order. > > > > i know you don't mean to say i selected the types and rather that the > > types that were selected are not quite correct for their usage. > > Yes; the previous types were also signed, and you didn't change that. > > > again > > on the review that actually adopts std atomics is a better place to make > > any potential type changes since we are "breaking" the API for 23.11 > > anyway. further, the std atomics series technically changes all the > > types so it's probably better to make one type change then rather than > > one now and one later. > > > > i think it would be best to get these validated and merged asap so we > > can get to the std atomics review. when that series is up let's discuss > > further how i can mark areas of concern, with that series i expect there > > will have to be some changes in order to avoid minor regressions. > > > > thanks! > > I thought it would be better to catch these details (i.e. memory ordering and signedness) early on, but I now understand that you planned to do it in a later step. So I'll let you proceed as you have planned. > > Thanks for all your work on this, Tyler. It is much appreciated! again, sorry for the confusion the sooner i can get some of these merged the easier it will be for me to manage the final series. i hope david/thomas can merge the simple normalization patches as soon as 23.03 cycle is complete. > > -Morten