<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. > 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. >
