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 11B4242805; Wed, 22 Mar 2023 18:07:55 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EF5F740E09; Wed, 22 Mar 2023 18:07:54 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id AF4AE40A84 for ; Wed, 22 Mar 2023 18:07:53 +0100 (CET) Content-class: urn:content-classes:message Subject: RE: [PATCH 0/7] replace rte atomics with GCC builtin atomics MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Date: Wed, 22 Mar 2023 18:07:50 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D877E9@smartserver.smartshare.dk> X-MimeOLE: Produced By Microsoft Exchange V6.5 In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH 0/7] replace rte atomics with GCC builtin atomics Thread-Index: Adlc0x2jp4hZ/JHqTt+4XE4Z0dKiCQAAqo6gAAGcVLAAAOhc4A== 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> <20230322152932.GB29057@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D877E8@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Honnappa Nagarahalli" , "Tyler Retzlaff" Cc: "Stephen Hemminger" , , "Ruifeng Wang" , , "nd" , "nd" 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 > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] > Sent: Wednesday, 22 March 2023 17.40 >=20 > > From: Morten Br=F8rup > > Sent: Wednesday, March 22, 2023 11:14 AM > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > Sent: Wednesday, 22 March 2023 16.30 > > > > > > On Wed, Mar 22, 2023 at 03:58:07PM +0100, Morten Br=F8rup 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=F8rup = 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. > > > > No worries... you are rushing through quite an effort for this, so a > little > > confusion is perfectly understandable. Especially when I'm replying = to > an ageing > > email. :-) > > > > > > > > let me ask for clarification in relation to this series. > > > > > > isn't that every single usage of the rte_atomic APIs? > > > > Probably, yes. > > > > > 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); > > > > Exactly. And something similar for the rte_atomicXX_t variables > changed to > > intXX_t, such as the packet counters. > > > > Realistically, I don't expect the maintainers to clean them up = anytime > soon. The > > purpose is to make the FIXMEs stick until someone eventually cleans > them up, so > > they are not forgotten as time passes. > Cleaning up the rte_atomic APIs is a different effort. There is = already > lot of effort that has gone into this and there is more effort = happening > (rte_ring being a painful one) >=20 > Instead of having FIXME, why not just send a separate patch with = SEQ_CST > (still a search and replace)? We can leave the tougher ones like > rte_ring as they are being worked on. The FIXME makes it possible in the future to differentiate between the = instances that still need review and the instances that have been = reviewed where SEQ_CST was the correct choice. (Similarly for the choice = of type for variables previously rte_atomicNN_t.) >=20 > > > > > > > > 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. > > > > Great. > > > > > > > > > > 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. > > > > Yes. An early merge would also provide more time for reviewing and > optimizing > > the memory order of the most important atomic operations. >=20