On Fri, Mar 20, 2020 at 10:31 AM Honnappa Nagarahalli <honnappa.nagaraha...@arm.com> wrote: > > <snip> > > > Subject: Re: [PATCH v3 00/12] generic rte atomic APIs deprecate proposal > > > > 18/03/2020 15:01, Van Haaren, Harry: > > > Hi Phil & Honnappa, > > > > > > From: Phil Yang <phil.y...@arm.com> > > > > > > > > DPDK provides generic rte_atomic APIs to do several atomic operations. > > > > These APIs are using the deprecated __sync built-ins and enforce > > > > full memory barriers on aarch64. However, full barriers are not > > > > necessary in many use cases. In order to address such use cases, C > > > > language offers > > > > C11 atomic APIs. The C11 atomic APIs provide finer memory barrier > > > > control by making use of the memory ordering parameter provided by the > > user. > > > > Various patches submitted in the past [2] and the patches in this > > > > series indicate significant performance gains on multiple aarch64 > > > > CPUs and no performance loss on x86. > > > > > > > > But the existing rte_atomic API implementations cannot be changed as > > > > the APIs do not take the memory ordering parameter. The only choice > > > > available is replacing the usage of the rte_atomic APIs with C11 > > > > atomic APIs. In order to make this change, the following steps are > > proposed: > > > > > > > > [1] deprecate rte_atomic APIs so that future patches do not use > > > > rte_atomic APIs (a script is added to flag the usages). > > > > [2] refactor the code that uses rte_atomic APIs to use c11 atomic APIs. > > > > > > On [1] above, I feel deprecating DPDKs atomic functions and failing > > > checkpatch is a bit sudden. Perhaps noting that in a future release > > > (20.11?) DPDK will move to a > > > C11 based atomics model is a more gradual step to achieving the goal, > > > and at that point add a checkpatch warning for additions of rte_atomic*? > > > > > > More on [2] in context below. > > > > > > The above is my point-of-view, of course I'd like more people from the > > > DPDK community to provide their input too. > > > > > > > > > > This patchset contains: > > > > 1) the checkpatch script changes to flag rte_atomic API usage in > > > > patches. > > > > 2) changes to programmer guide describing writing efficient code for > > aarch64. > > > > 3) changes to various libraries to make use of c11 atomic APIs. > > > > > > > > We are planning to replicate this idea across all the other > > > > libraries, drivers, examples, test applications. In the next phase, > > > > we will add changes to the mbuf, the EAL interrupts and the event timer > > adapter libraries. > > > > > > About ~6/12 patches of this C11 set are targeting the Service Cores > > > area of DPDK. I have some concerns over increased complexity of C11 > > implementation vs the (already complex) rte_atomic implementation today. > > > I see other patchsets enabling C11 across other DPDK components, so > > > maybe we should also discuss C11 enabling in a wider context that just > > service cores? > > > > > > I don't think it fair to expect all developers to be well versed in > > > C11 atomic semantics like understanding the complex interactions between > > the various C11 RELEASE, AQUIRE barriers requires. > > > > > > As maintainer of Service Cores I'm hesitant to accept the large-scale > > > refactor of atomic-implementation, as it could lead to racey bugs that > > > are likely extremely difficult to track down. (The recent race-on-exit has > > proven the difficulty in reproducing, and that's with an atomics model I'm > > quite familiar with). > > > > > > Let me be very clear: I don't wish to block a C11 atomic > > > implementation, but I'd like to discuss how we (DPDK community) can > > > best port-to and maintain a complex multi-threaded service library with > > best-in-class performance for the workload. > > > > > > To put some discussions/solutions on the table: > > > - Shared Maintainership of a component? > > > Split in functionality and C11 atomics implementation > > > Obviously there would be collaboration required in such a case. > > > - Maybe shared maintainership is too much? > > > A gentlemans/womans agreement of "helping out" with C11 atomics > > debug is enough? > > > > > > > > > Hope my concerns are understandable, and of course input/feedback > > > welcomed! -Harry > > > > Thanks for raising the issue Harry. > > > > I think we should have at least two official maintainers for C11 atomics in > > general. > Sure, I can volunteer. > > > C11 conversion is a progressive effort being done, and should be merged step > > by step. > Agree, the changes need to be understood, it is not a search and replace > effort. The changes will come-in in stages unless others join the effort. > The concern I have is about the new patches that get added. I think we need > to stop the new patches from using rte_atomic APIs, otherwise we might be > making these changes forever.
+1. We must define a time frame otherwise we might be making these changes forever. > > > If C11 maintainers fail to fix some issues on time, then we can hold the > > effort. > > Does it make sense? > I am fine with this approach. But, I think we need to have a deadline in mind > to complete the work. > > > >
