Memory leak in volk_load_preferences

[dernasherbrezon]

Looks like preferences are loaded on the first access, but never destroyed. Can't find any helped method to properly cleanup.

Got this while running valgrind:

==29410== 11,904 bytes in 1 blocks are still reachable in loss record 1 of 1
==29410==    at 0x4849F4C: realloc (vg_replace_malloc.c:785)
==29410==    by 0x497CD5F: volk_load_preferences (in /usr/local/lib/libvolk.so.2.0)

[rear1019]

This is not a memory leak to worry about and is only shown by Valgrind if explicitly instructed to do so. volk_rank_archs() which calls volk_load_preferences() makes sure that the memory is allocated only once:

    if (!prefs_loaded) {
        n_arch_prefs = volk_load_preferences(&volk_arch_prefs);
        prefs_loaded = 1;
    }

The memory will be freed by the operating system when the application using volk exists. It would be a “real” memory leak – “Definitely lost” in Valgrind – if the if guard wasn’t in place (because then a memory allocation would be performed every time a kernel is called).

This “leak” cannot be fixed unless a kind of deinit_volk() functions is added (and explicitly called).

See [1] for the different leak types in Valgrind (search for “Still reachable”).

[1] https://www.valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks

[dernasherbrezon]

I thought "memory leak" is a memory that leaked from the control and cannot be freed. Yes, something like deinit_volk() might be required for this. Or even better: explicit lifecycle management init_volk and deinit_volk.

That would bring several benefits:

• make application performance predictable. It won't load and parse file on a critical path.
• fail-fast if volk is unable to load preferences or if preferences corrupted &etc.

[jdemel]

Currently, you can just add you volk kernel calls to your code and the first time such a function is called, init_volk is run. With explicit calls to init_volk and in the end deinit_volk, we'd add some user complexity. e.g. when an where should GNU Radio call this functions? We can discuss how to add an additional interface to handle this explicitly on demand but I'd like the default to be as is.
Currently, if preferences are not available VOLK defaults to a "use the latest SIMD version available". i.e. prefer AVX over SSE and AVX512 over AVX2, etc.

If VOLK could spit out a warning if it couldn't find a preferences file that would be beneficial.

In terms of predictable performance:
The whole init code is called on the very first VOLK kernel call. At that stage, your application is unpredictable in terms of performance because we have no control over CPU internals. Only after a couple of iterations we can start to rely on performance metrics. So, we add a bit more unpredictability into a situation that is already unpredictable.

I know it's less than ideal and we might want to think of a more accessible way but:

volk/lib/volk_rank_archs.c

Lines 56 to 62 in cb87a41

	static volk_arch_pref_t* volk_arch_prefs;
	static size_t n_arch_prefs = 0;
	static int prefs_loaded = 0;
	if (!prefs_loaded) {
	n_arch_prefs = volk_load_preferences(&volk_arch_prefs);
	prefs_loaded = 1;
	}

I guess we can move this such that we have reliable access to this data structure. And then continue from there. Do you have any suggestions on what to change?

[dernasherbrezon]

we'd add some user complexity

Agree. This should be carefully introduced. Just wanted to highlight this weird behaviour.

when an where should GNU Radio call this functions?

I'm not writing GNU radio-related application :) But I would assume any application has some kind of lifecycle:

• initialise, allocate memory, start
• do some useful work
• de-initialize, free memory, stop

When the code provided as a library it is hard to control how it is going to be used. For example, volk can be used only once briefly and then never executed during application lifecycle.

[jdemel]

So yeah. It would be great to modify VOLK such that we can optionally call init and deinit. This should be optional though. Do you have a suggestion how we could achieve this?

Regarding the "volk can be used only once briefly". I agree that this might happen. But it is not the intended use case. VOLK is optimized towards speed for operations that happen frequently.

[rear1019]

I thought "memory leak" is a memory that leaked from the control and cannot be freed.

The pointer to the allocated memory is not lost at any time point. If we wanted to, we could release it. However, there is no point doing do so because the memory is used throughout the lifetime of an application. See the definition of “Still reachable” in Valgrind’s documentation [1].

• make application performance predictable. It won't load and parse file on a critical path.

The file is loaded only once. If this is really an issue – I doubt it is in any DSP heavy application – call any kernel once before starting main data processing. If the “unpredictability” of the dispatchers is an issue – once again, I doubt it is – call every used kernel once. Or don’t use unpredictable systems (GNU Radio 😉) in the first place.

It would be great to modify VOLK such that we can optionally call init and deinit. This should be optional though.

What’s the point if it’s optional and doesn’t provide any benefit? The reported “leak” is not a problem. Even though I suggested how it could be fixed, I don’t think (and never considered) we should. The increased complexity and burden for users of volk is not worth it.

[1] https://www.valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks

[bastibl]

The file is loaded only once. [...] Or don’t use unpredictable systems (GNU Radio wink) in the first place.

Are you sure? What about multi-threaded applications. Like GNU Radio. I only had a brief look, but I didn't find any locks.

[jdemel]

What’s the point if it’s optional and doesn’t provide any benefit? The reported “leak” is not a problem. Even though I suggested how it could be fixed, I don’t think (and never considered) we should. The increased complexity and burden for users of volk is not worth it.

I agree that we should not require any of this. I just imagined a mechanism where a user may call init and deinit. If a user does not, the automatic route is preferred and I'd argue the automatic mechanism is the preferred one. Mostly because:

If this is really an issue – I doubt it is in any DSP heavy application – call any kernel once before starting main data processing.

[jdemel]

The file is loaded only once. [...] Or don’t use unpredictable systems (GNU Radio wink) in the first place.

Are you sure? What about multi-threaded applications. Like GNU Radio. I only had a brief look, but I didn't find any locks.

There are no locks.

volk/lib/volk_rank_archs.c

Lines 55 to 62 in cb87a41

	size_t i;
	static volk_arch_pref_t* volk_arch_prefs;
	static size_t n_arch_prefs = 0;
	static int prefs_loaded = 0;
	if (!prefs_loaded) {
	n_arch_prefs = volk_load_preferences(&volk_arch_prefs);
	prefs_loaded = 1;
	}

And the called function doesn't have one either

volk/lib/volk_prefs.c

Lines 66 to 85 in cb87a41

	size_t volk_load_preferences(volk_arch_pref_t** prefs_res)
	{
	FILE* config_file;
	char path[512], line[512];
	size_t n_arch_prefs = 0;
	volk_arch_pref_t* prefs = NULL;
	// get the config path
	volk_get_config_path(path, true);
	if (!path[0])
	return n_arch_prefs; // no prefs found
	config_file = fopen(path, "r");
	if (!config_file)
	return n_arch_prefs; // no prefs found
	// reset the file pointer and write the prefs into volk_arch_prefs
	while (fgets(line, sizeof(line), config_file) != NULL) {
	void* new_prefs = realloc(prefs, (n_arch_prefs + 1) * sizeof(*prefs));
	if (!new_prefs) {
	printf("volk_load_preferences: bad malloc\n");

So far I haven't heard of any issues with this mechanism. And to conclude this:

volk/tmpl/volk.tmpl.c

Lines 144 to 160 in cb87a41

	static inline void __init_${kern.name}(void)
	{
	const char *name = get_machine()->${kern.name}_name;
	const char **impl_names = get_machine()->${kern.name}_impl_names;
	const int *impl_deps = get_machine()->${kern.name}_impl_deps;
	const bool *alignment = get_machine()->${kern.name}_impl_alignment;
	const size_t n_impls = get_machine()->${kern.name}_n_impls;
	const size_t index_a = volk_rank_archs(name, impl_names, impl_deps, alignment, n_impls, true/*aligned*/);
	const size_t index_u = volk_rank_archs(name, impl_names, impl_deps, alignment, n_impls, false/*unaligned*/);
	${kern.name}_a = get_machine()->${kern.name}_impls[index_a];
	${kern.name}_u = get_machine()->${kern.name}_impls[index_u];
	assert(${kern.name}_a);
	assert(${kern.name}_u);
	${kern.name} = &__${kern.name}_d;
	}

Here, we call volk_rank_archs.

[jdemel]

If we provide a deinit function, we should also spend some thought on how to make initialization thread safe. Unless, we decide that this is a user problem and VOLK does not make any guarantees to be thread safe. Though, I'd prefer to make sure initialization is thread-safe.

I assume a viable solution would rely on something with C atomic and C threads. This needs further investigation though.