On Tue, Oct 4, 2016 at 4:30 PM, Nicolai Hähnle <nhaeh...@gmail.com> wrote: > On 30.09.2016 15:47, Marek Olšák wrote: >> >> On Fri, Sep 30, 2016 at 3:08 PM, Bas Nieuwenhuizen >> <b...@basnieuwenhuizen.nl> wrote: >>> >>> On Fri, Sep 30, 2016 at 2:13 PM, Marek Olšák <mar...@gmail.com> wrote: >>>> >>>> intptr_t reads and writes aren't atomic. p_atomic_set and >>>> p_atomic_read functions don't do anything for atomicity. See: >>>> >>>> #define p_atomic_set(_v, _i) (*(_v) = (_i)) >>>> #define p_atomic_read(_v) (*(_v)) >>> >>> >>> That implementation seems bogus to me, as the compiler sees none of >>> them as atomic and therefore the compiler can do strange stuff. >>> >>> why are intptr_t reads/writes less atomic than int32_t? IIRC on x86_64 >>> aligned 64-bit accesses are atomic, and on x86 intptr_t is just 32 >>> bits. > > > I looked into a number of options for p_atomic_set/read and just sent around > a patch which (to my understanding) definitely guarantees sufficient > atomicity and memory ordering on GCC >= 4.7. > > Without that patch (and so I suspect also on GCC < 4.7), the memory accesses > are still de facto atomic as Bas wrote. Furthermore, most of the necessary > ordering guarantees are established by the calls to mtx_lock/unlock > functions. > > Without that patch (and also with that patch but with GCC < 4.7), there is > actually still the possibility that the write of page->u.num_remaining in > slab_destroy_child is moved to after the loop over the page's elements. GCC > doesn't actually do it in practice, but it is a gap which we probably have > to live with unless we introduce some ugly workarounds. > > Note that this is only ever a problem in the situation where an allocation > is freed with a different child pool than the one it was allocated from. In > other words, the new code is (as far as I can see) only buggy in the case > where the old code was even buggier. > > For what it's worth, I'm going to use p_atomic_set also for > page->u.num_remaining. This is not strictly needed, since the > acquire/release on elt->owner already establishes the necessary ordering > already, but it should help clarity. > > Do you agree with this plan?
Yes, it sounds good to me. Marek _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
