[ovs-dev] [PATCH v13 4/5] dpif-netdev: Refactor generic implementation

Ilya Maximets i.maximets at samsung.com
Thu Jul 18 12:42:26 UTC 2019


On 18.07.2019 15:32, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets at samsung.com]
>> Sent: Thursday, July 18, 2019 12:34 PM
>> To: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at openvswitch.org
>> Cc: echaudro at redhat.com; malvika.gupta at arm.com; Stokes, Ian
>> <ian.stokes at intel.com>
>> Subject: Re: [PATCH v13 4/5] dpif-netdev: Refactor generic implementation
>>
>> On 17.07.2019 21:21, Harry van Haaren wrote:
> <snip>
>>> +/* Per thread data to store the blocks cache. The 'blocks_cache_count'
>> variable
>>> + * stores the size of the allocated space in uint64_t blocks (so * 8 to
>> get the
>>> + * size in bytes).
>>> + */
>>> +DEFINE_STATIC_PER_THREAD_DATA(uint64_t *, blocks_scratch_ptr, 0);
>>> +DEFINE_STATIC_PER_THREAD_DATA(uint32_t, blocks_scratch_count_ptr, 0);
>>
>>
>> Since you have a malloced data stored here it will be leaked on thread
>> destruction.
>> You need to use DEFINE_PER_THREAD_MALLOCED_DATA instead that will free the
>> memory
>> in this case.
> 
> 
> Yes correct - I had (wrongly) assumed this was the way to do this, nice to
> see that OVS has methods to handle cleanup of per-thread malloced data too. 
> 
> 
>> Please, consider following incremental:
> 
> Thanks - I've spotted a number of fixes/changes below, I'll list them.
> I'll rework the v14 based on your suggested code.
> 
> 
>> diff --git a/lib/dpif-netdev-lookup-generic.c b/lib/dpif-netdev-lookup-
>> generic.c
>> index 50edc483c..0addbad61 100644
>> --- a/lib/dpif-netdev-lookup-generic.c
>> +++ b/lib/dpif-netdev-lookup-generic.c
>> @@ -36,12 +36,34 @@ VLOG_DEFINE_THIS_MODULE(dpif_lookup_generic);
> <snip>
> 
>> +struct array {
>> +    uint32_t allocated; /* Number of elements allocated in 'elems'. */
>> +    uint64_t elems[];
>> +};
>> +
>> +/* Per thread data to store the blocks cache. */
>> +DEFINE_PER_THREAD_MALLOCED_DATA(struct array *, blocks_scratch_array);
> 
> This is the cleanup magic - on pthread_exit(),the handlers registered with
> pthread_cleanup_push() will get called? Or is there a different mechanism
> at play here?

It's the magic of pthread_key_create:
https://linux.die.net/man/3/pthread_key_create

> 
> 
>> +static inline uint64_t *
>> +get_blocks_scratch(uint32_t count)
>> +{
>> +    struct array *darray = blocks_scratch_array_get();
>> +
>> +    /* Check if this thread already has a large enough array allocated.
>> +     * This is a predictable UNLIKLEY branch as it will only occur once at
>> +     * startup, or if a subtable with higher blocks count is added.
>> +     */
>> +    if (OVS_UNLIKELY(!darray || darray->allocated < count)) {
>> +        /* Allocate new memory for blocks_scratch, and store new size. */
>> +        darray = xrealloc(darray,
> 
> Nice - xrealloc handles NULL correctly and saves use manually handling free().
> 
> 
>> +        darray->allocated = count;
>> +        blocks_scratch_array_set_unsafe(darray);
> 
> Clean way to set the data - nice, didn’t know of that function.

It exists only for MALLOCED_DATA. STATIC one doesn't have it, as it assumes to
be static.

> 
> 
>> +        VLOG_DBG("Block scratch array resized to %"PRIu32, count);
> 
> And a PRIu32 fixup, thanks.
> 
> 
> I'll push v14 shortly, appreciate the input. -Harry
> 


More information about the dev mailing list