[Mesa-dev] [PATCH v2] util/slab: re-design to allow migration between pools (v2)

[Nicolai Hähnle]

On 30.09.2016 15:47, Marek Olšák wrote:
> On Fri, Sep 30, 2016 at 3:08 PM, Bas Nieuwenhuizen
> <bas at basnieuwenhuizen.nl> wrote:
>> On Fri, Sep 30, 2016 at 2:13 PM, Marek Olšák <maraeo at 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?

Thanks,
Nicolai

>
> Really? Thanks, I didn't know that.
>
> Marek
>