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

[Marek Olšák]

On Tue, Oct 4, 2016 at 4:30 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> 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?

Yes, it sounds good to me.

Marek