> -----Original Message----- > From: Ilya Maximets [mailto:i.maxim...@samsung.com] > Sent: Thursday, July 18, 2019 12:34 PM > To: Van Haaren, Harry <harry.van.haa...@intel.com>; d...@openvswitch.org > Cc: echau...@redhat.com; malvika.gu...@arm.com; Stokes, Ian > <ian.sto...@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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev