On 18.07.2019 15:32, Van Haaren, Harry wrote: >> -----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?
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 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
