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