From: Tyler Retzlaff <roretzla@linux.microsoft.com>
To: "Morten Brørup" <mb@smartsharesystems.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>,
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
Date: Wed, 22 Mar 2023 08:29:32 -0700 [thread overview]
Message-ID: <20230322152932.GB29057@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D877E6@smartserver.smartshare.dk>
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 <roretzla@linux.microsoft.com> 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
next prev parent reply other threads:[~2023-03-22 15:29 UTC|newest]
Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-17 20:19 Tyler Retzlaff
2023-03-17 20:19 ` [PATCH 1/7] ring: " Tyler Retzlaff
2023-03-17 20:36 ` Tyler Retzlaff
2023-03-17 20:19 ` [PATCH 2/7] stack: " Tyler Retzlaff
2023-03-17 20:19 ` [PATCH 3/7] dma/idxd: " Tyler Retzlaff
2023-03-17 20:19 ` [PATCH 4/7] net/ice: " Tyler Retzlaff
2023-03-17 20:41 ` Tyler Retzlaff
2023-03-17 20:19 ` [PATCH 5/7] net/ixgbe: " Tyler Retzlaff
2023-03-17 20:19 ` [PATCH 6/7] net/null: " Tyler Retzlaff
2023-03-17 20:44 ` Tyler Retzlaff
2023-03-17 20:19 ` [PATCH 7/7] net/ring: " Tyler Retzlaff
2023-03-17 21:42 ` [PATCH 0/7] " Stephen Hemminger
2023-03-17 21:49 ` Tyler Retzlaff
2023-03-22 11:28 ` Morten Brørup
2023-03-22 14:21 ` Tyler Retzlaff
2023-03-22 14:58 ` Morten Brørup
2023-03-22 15:29 ` Tyler Retzlaff [this message]
2023-03-22 16:13 ` Morten Brørup
2023-03-22 16:40 ` Honnappa Nagarahalli
2023-03-22 17:07 ` Morten Brørup
2023-03-22 17:38 ` Honnappa Nagarahalli
2023-03-22 18:06 ` Tyler Retzlaff
2023-05-02 3:37 ` Tyler Retzlaff
2023-05-02 4:31 ` Honnappa Nagarahalli
2023-03-23 22:34 ` [PATCH v2 " Tyler Retzlaff
2023-03-23 22:34 ` [PATCH v2 1/7] ring: " Tyler Retzlaff
2023-03-23 22:34 ` [PATCH v2 2/7] stack: " Tyler Retzlaff
2023-03-23 22:34 ` [PATCH v2 3/7] dma/idxd: " Tyler Retzlaff
2023-03-23 22:34 ` [PATCH v2 4/7] net/ice: " Tyler Retzlaff
2023-03-23 22:34 ` [PATCH v2 5/7] net/ixgbe: " Tyler Retzlaff
2023-03-23 22:34 ` [PATCH v2 6/7] net/null: " Tyler Retzlaff
2023-03-23 22:34 ` [PATCH v2 7/7] net/ring: " Tyler Retzlaff
2023-03-24 7:07 ` [PATCH v2 0/7] " Morten Brørup
2023-03-23 22:53 ` [PATCH v3 " Tyler Retzlaff
2023-03-23 22:53 ` [PATCH v3 1/7] ring: " Tyler Retzlaff
2023-03-23 22:53 ` [PATCH v3 2/7] stack: " Tyler Retzlaff
2023-05-24 20:08 ` David Marchand
2023-03-23 22:53 ` [PATCH v3 3/7] dma/idxd: " Tyler Retzlaff
2023-05-24 20:09 ` David Marchand
2023-05-25 8:41 ` Bruce Richardson
2023-05-25 13:59 ` Morten Brørup
2023-05-25 12:57 ` Kevin Laatz
2023-03-23 22:53 ` [PATCH v3 4/7] net/ice: " Tyler Retzlaff
2023-05-24 20:10 ` David Marchand
2023-03-23 22:53 ` [PATCH v3 5/7] net/ixgbe: " Tyler Retzlaff
2023-05-24 20:11 ` David Marchand
2023-03-23 22:53 ` [PATCH v3 6/7] net/null: " Tyler Retzlaff
2023-05-24 20:13 ` David Marchand
2023-03-23 22:53 ` [PATCH v3 7/7] net/ring: " Tyler Retzlaff
2023-05-24 20:12 ` David Marchand
2023-05-25 8:44 ` Bruce Richardson
2023-03-24 7:09 ` [PATCH v3 0/7] " Morten Brørup
2023-03-24 19:22 ` Tyler Retzlaff
2023-05-24 12:40 ` David Marchand
2023-05-24 15:47 ` Tyler Retzlaff
2023-05-24 20:06 ` David Marchand
2023-05-24 22:50 ` Tyler Retzlaff
2023-05-24 22:56 ` Honnappa Nagarahalli
2023-05-25 0:02 ` Tyler Retzlaff
2023-05-25 7:50 ` Morten Brørup
2023-06-02 19:45 ` [PATCH v4 0/6] " Tyler Retzlaff
2023-06-02 19:45 ` [PATCH v4 1/6] stack: " Tyler Retzlaff
2023-06-02 19:45 ` [PATCH v4 2/6] dma/idxd: " Tyler Retzlaff
2023-06-02 19:45 ` [PATCH v4 3/6] net/ice: " Tyler Retzlaff
2023-06-02 19:45 ` [PATCH v4 4/6] net/ixgbe: " Tyler Retzlaff
2023-06-02 19:45 ` [PATCH v4 5/6] net/null: " Tyler Retzlaff
2023-06-02 19:45 ` [PATCH v4 6/6] net/ring: " Tyler Retzlaff
2023-06-05 8:27 ` Olivier Matz
2023-06-06 21:45 ` [PATCH v5 0/6] " Tyler Retzlaff
2023-06-06 21:45 ` [PATCH v5 1/6] stack: " Tyler Retzlaff
2023-06-06 21:45 ` [PATCH v5 2/6] dma/idxd: " Tyler Retzlaff
2023-06-06 21:45 ` [PATCH v5 3/6] net/ice: " Tyler Retzlaff
2023-06-06 21:45 ` [PATCH v5 4/6] net/ixgbe: " Tyler Retzlaff
2023-06-06 21:45 ` [PATCH v5 5/6] net/null: " Tyler Retzlaff
2023-06-06 21:45 ` [PATCH v5 6/6] net/ring: " Tyler Retzlaff
2023-06-09 15:01 ` [PATCH v5 0/6] " David Marchand
2023-06-09 15:13 ` Tyler Retzlaff
2023-06-22 19:59 ` Patrick Robb
2023-06-23 8:53 ` David Marchand
2023-06-23 21:37 ` Tyler Retzlaff
2023-06-28 14:01 ` Patrick Robb
2023-06-28 14:49 ` David Marchand
2023-06-23 21:35 ` Tyler Retzlaff
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=20230322152932.GB29057@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net \
--to=roretzla@linux.microsoft.com \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=Ruifeng.Wang@arm.com \
--cc=dev@dpdk.org \
--cc=mb@smartsharesystems.com \
--cc=stephen@networkplumber.org \
--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).