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

[Van Haaren, Harry]

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


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


> +        VLOG_DBG("Block scratch array resized to %"PRIu32, count);

And a PRIu32 fixup, thanks.


I'll push v14 shortly, appreciate the input. -Harry