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

[Ian Romanick]

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

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