Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory leak in volk_load_preferences #440

Open
dernasherbrezon opened this issue Jan 17, 2021 · 10 comments · May be fixed by #453
Open

Memory leak in volk_load_preferences #440

dernasherbrezon opened this issue Jan 17, 2021 · 10 comments · May be fixed by #453

Comments

@dernasherbrezon
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

jdemel commented Jan 19, 2021

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:

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
Copy link
Contributor Author

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
Copy link
Contributor

jdemel commented Jan 25, 2021

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
Copy link
Contributor

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
Copy link
Member

bastibl commented Jan 26, 2021

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
Copy link
Contributor

jdemel commented Jan 27, 2021

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
Copy link
Contributor

jdemel commented Jan 27, 2021

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.

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
Copy link
Contributor

jdemel commented Jan 29, 2021

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.

jdemel added a commit to jdemel/volk that referenced this issue Feb 21, 2021
Instead of static variables in a function, we store preferences in a
struct and use an `atomic_int` to prevent any more than one thread from
loading preferences.

Fixes gnuradio#440

Signed-off-by: Johannes Demel <[email protected]>
@jdemel jdemel linked a pull request Feb 21, 2021 that will close this issue
jdemel added a commit to jdemel/volk that referenced this issue Feb 21, 2021
Instead of static variables in a function, we store preferences in a
struct and use an `atomic_int` to prevent any more than one thread from
loading preferences.

Fixes gnuradio#440

Signed-off-by: Johannes Demel <[email protected]>
jdemel added a commit to jdemel/volk that referenced this issue Mar 4, 2021
Instead of static variables in a function, we store preferences in a
struct and use an `atomic_int` to prevent any more than one thread from
loading preferences.

Fixes gnuradio#440

Signed-off-by: Johannes Demel <[email protected]>
jdemel added a commit to jdemel/volk that referenced this issue Jan 14, 2023
Instead of static variables in a function, we store preferences in a
struct and use an `atomic_int` to prevent any more than one thread from
loading preferences.

Fixes gnuradio#440

Signed-off-by: Johannes Demel <[email protected]>
jdemel added a commit to jdemel/volk that referenced this issue Nov 5, 2023
Instead of static variables in a function, we store preferences in a
struct and use an `atomic_int` to prevent any more than one thread from
loading preferences.

Fixes gnuradio#440

Signed-off-by: Johannes Demel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants