RE: [PATCH 0/7] replace rte atomics with GCC builtin atomics

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

> -----Original Message-----
> From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Sent: Monday, May 1, 2023 10:38 PM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Cc: Morten Brørup <mb@smartsharesystems.com>; Stephen Hemminger
> <stephen@networkplumber.org>; dev@dpdk.org; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; thomas@monjalon.net; nd <nd@arm.com>
> Subject: Re: [PATCH 0/7] replace rte atomics with GCC builtin atomics
> 
> 
> On Wed, Mar 22, 2023 at 11:06:08AM -0700, Tyler Retzlaff wrote:
> > On Wed, Mar 22, 2023 at 05:38:12PM +0000, Honnappa Nagarahalli wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Morten Br�rup <mb@smartsharesystems.com>
> > > > Sent: Wednesday, March 22, 2023 12:08 PM
> > > > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Tyler
> > > > Retzlaff <roretzla@linux.microsoft.com>
> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>;
> dev@dpdk.org;
> > > > Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net; nd
> > > > <nd@arm.com>; nd <nd@arm.com>
> > > > Subject: RE: [PATCH 0/7] replace rte atomics with GCC builtin
> > > > atomics
> > > >
> > > > > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
> > > > > Sent: Wednesday, 22 March 2023 17.40
> > > > >
> > > > > > From: Morten Br�rup <mb@smartsharesystems.com>
> > > > > > 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�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.
> > > > > >
> > > > > > 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)
> > > > >
> > > > > 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.)
> > > Apologies, relooked at the heading of this patch, got confused with other
> patches.
> >
> > yeah, i did the same thing this morning :)
> >
> > >
> > > The changes Arm had done for rte_atomic_ to __atomic_xxx were not direct
> replacements. The algorithms were studied, relaxed where required, race
> conditions fixed, performance benchmarked. IMO, we need to go through the
> same steps here.
> > >
> > > I looked at the series, we should just review the patch and make suggested
> changes. Are we constrained by any deadlines for this work?
> >
> > i'm going to say yes but i'll qualify. the use of the rte_atomic_xxx
> > APIs drags in extra work when creating a series that performs the
> > actual conversions to the standard atomics.
> >
> > if i don't decouple ring from rte_atomic_xxx that means i have to go
> > convert all the rte_atomic.h to standard atomics and working around
> > some of the implementation detail to do it is very time consuming.
> > which then has further flow on effects because then i have to go fix
> > every single driver that is still using rte_atomic.h.
> >
> > incidentally i have a work in progress to decouple everything from
> > rte_atomic.h (including all drivers) but it would really negatively
> > impact getting standard atomics introduced if we had to serialize the
> > introduction behind a total removal of rte_atomic or had to make
> > changes to every consumer of the old rte_atomic APIs.
> >
> > if we can get by with a comment on the rte_atomic_xxx lines in this
> > series it would be helpful. when we bring the next series for standard
> > atomics i'm not adverse to introducing changes to the ordering in that
> > series if requested so long as i can get the series up 'soon' so there
> > is lots of review time runway for 23.11.
> >
> > >
> > > I would suggest to drop 1/7. Arm is working on removing the non-C11
> algorithm for rte_ring (not sure if we will be successful). I think it is better to
> explore this approach rather than the changes in patch 1/7.
> >
> > i think my answer here is timing. i'd rather take the work from arm
> > but if it isn't coming for a while then it becomes a blocker.
> >
> > we're waiting for the 23.07 start before this series can be merged.
> > how about we re-evaluate where arm is at when the merge window opens.
> > we can then decide to drop 1/7 or not at that time?
> 
> ping?
> 
> any update if there is going to be a series from arm as an acceptable
> replacement for patch 1/7? otherwise i think we should take the patch as is. it
> isn't altering the semantics of the code and is fairly low line count change so
> shouldn't distrupt any out of tree work as a result of the churn.
Yes, we are working on a patch. There is a RFC [1], but we are still working on proving if the algorithm is correct. But, the plan is to find a solution (or present alternatives if there are no solutions) in 23.07 release.

[1] https://patchwork.dpdk.org/project/dpdk/patch/20230421191642.217011-1-wathsala.vithanage@arm.com/

> 
> please update asap, this is one of the two series that is preventing submission of
> the first series converting to standard atomics for review.
> 
> thanks!
> 
> >
> > ty