On 21.11.18 22:23, Ian Romanick wrote:
> On 11/21/2018 12:23 PM, Haehnle, Nicolai wrote:
>> On 21.11.18 19:19, Ian Romanick wrote:
>>> On 11/21/2018 03:08 AM, Haehnle, Nicolai wrote:
>>>> On 21.11.18 01:39, Ian Romanick wrote:
>>>>> From: Ian Romanick <ian.d.roman...@intel.com>
>>>>>
>>>>> Ralloc has a feature that all allocations from a temporary memory
>>>>> context can be whisked away in a single call without fear of leaks. As
>>>>> the slab allocator is designed for use in multhreaded scenarios with a
>>>>> child pool per CPU, it lacks this feature. However, many users will be
>>>>> single threaded with a single child pool. For these users, we can have
>>>>> our cake and eat it too.
>>>>>
>>>>> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
>>>>> Cc: Nicolai Hähnle <nicolai.haeh...@amd.com>
>>>>> ---
>>>>> src/util/slab.c | 21 +++++++++++++++++++++
>>>>> src/util/slab.h | 1 +
>>>>> 2 files changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/src/util/slab.c b/src/util/slab.c
>>>>> index 5f048666b56..1bcc1db6e09 100644
>>>>> --- a/src/util/slab.c
>>>>> +++ b/src/util/slab.c
>>>>> @@ -172,6 +172,27 @@ void slab_destroy_child(struct slab_child_pool *pool)
>>>>> pool->parent = NULL;
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * Flush all allocations from a pool. Single-threaded (no mutex).
>>>>> + */
>>>>> +void
>>>>> +slab_flush_st(struct slab_mempool *parent)
>>>>
>>>> The name of the function argument should be "pool" for consistency.
>>>
>>> This is one thing that annoyed me while writing this function.
>>> Sometimes "pool" is a slab_mempool. Sometimes "pool" is a
>>> slab_child_pool. What do you do when you have both? For the most part,
>>> the thing that gets the most use in a function is the thing called pool.
>>> In this case, that's the slab_child_pool, but that's not the parameter
>>> passed in.
>>>
>>> slab_alloc_st, slab_free_st, slab_create, and slab_destroy are the only
>>> other functions that take a slab_mempool. These functions work around
>>> this issue by immediately calling some other public function with
>>> &pool->child. I didn't want to make a public function to flush an
>>> arbitrary slab_child_pool because that's very dangerous in the general
>>> case. Having a one-line public function that calls a private function
>>> will just beg some newbie to come along and fix it... and end up with
>>> the same problem. :)
>>>
>>>>> +{
>>>>> + struct slab_child_pool *const pool = &parent->child;
>>>>> +
>>>>> + assert(pool->migrated == NULL);
>>>>> + assert(pool->parent == parent);
>>>>
>>>> I'm surprised this works. Why isn't pool->parent == &parent->parent?
>>>
>>> This is just a sanity check on an invariant of the data structure. If
>>> the slab_mempool wasn't initialized or if it has been destroyed,
>>> parent->child.parent will be garbage or NULL.
>>
>> Right, I get that, but pool->parent is of type slab_parent_pool while
>> parent is of type slab_mempool. I don't see how that adds up.
>
> Ah, now I see what you're saying. Ignoring the other naming discussion,
> this should be
>
> assert(pool->parent == &parent->parent);
>
> On that topic... how would you feel about changing the "pool" parameter
> to be "mempool" in the 5 places (counting the one added by this patch)
> where that parameter has type 'struct slab_mempool *'? Then we'll
> universally have 'struct slab_child_pool *pool' and 'struct slab_mempool
> *mempool'.
+1, that's a good idea!
Cheers,
Nicolai
>
>> Cheers,
>> Nicolai
>>
>>>> Or, with a consistently named function argument, why isn't
>>>> pool->child.parent == &pool->parent?
>>>>
>>>> The intention of the patch looks fine though.
>>>>
>>>> Cheers,
>>>> Nicolai
>>>>
>>>>
>>>>> +
>>>>> + while (pool->pages) {
>>>>> + struct slab_page_header *page = pool->pages;
>>>>> + pool->pages = page->u.next;
>>>>> +
>>>>> + free(page);
>>>>> + }
>>>>> +
>>>>> + pool->free = NULL;
>>>>> +}
>>>>> +
>>>>> static bool
>>>>> slab_add_new_page(struct slab_child_pool *pool)
>>>>> {
>>>>> diff --git a/src/util/slab.h b/src/util/slab.h
>>>>> index e83f8ec1a0e..a4279d8e65b 100644
>>>>> --- a/src/util/slab.h
>>>>> +++ b/src/util/slab.h
>>>>> @@ -90,5 +90,6 @@ void slab_create(struct slab_mempool *pool,
>>>>> void slab_destroy(struct slab_mempool *pool);
>>>>> void *slab_alloc_st(struct slab_mempool *pool);
>>>>> void slab_free_st(struct slab_mempool *pool, void *ptr);
>>>>> +void slab_flush_st(struct slab_mempool *parent);
>>>>>
>>>>> #endif
>>>>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
