[Mesa-dev] [PATCH 3/7] util/slab: Add function to flush allocations from a child pool

[Haehnle, Nicolai]

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.romanick at 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.romanick at intel.com>
>>>>> Cc: Nicolai Hähnle <nicolai.haehnle at 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
>>>>>