Skip to content

Disable posix_memalign by default#15544

Merged
alalek merged 2 commits intoopencv:3.4from
mshabunin:disable_posix_memalign
Oct 9, 2019
Merged

Disable posix_memalign by default#15544
alalek merged 2 commits intoopencv:3.4from
mshabunin:disable_posix_memalign

Conversation

@mshabunin
Copy link
Copy Markdown
Contributor

@mshabunin mshabunin commented Sep 19, 2019

resolves #15526

This pullrequest changes

Do not use posix_memalign due to possible problems with memory fragmentation in glibc.

Build CMake's option:

  • OPENCV_ENABLE_MEMALIGN - enabled by default on *NIX

Runtime environment parameter:

  • OPENCV_ENABLE_MEMALIGN - disabled by default (Linux + glibc)

valgrind, ASAN and other sanitizers runs must have OPENCV_ENABLE_MEMALIGN=1 for reliable results.

@mshabunin mshabunin requested a review from alalek September 19, 2019 12:26
@alalek
Copy link
Copy Markdown
Member

alalek commented Sep 19, 2019

It should be enabled by default.
Custom alignment hacks block detection of out of buffer access via valgrind or sanitizers.

@mshabunin
Copy link
Copy Markdown
Contributor Author

There is severe issue in glibc which affects image processing use cases: repeated allocation/deallocation of several large aligned buffers interleaved with smaller allocations cause memory exhaust.

See other examples in the related issue. I think we should disable it by default and enable only for dynamic analysis.

Reproducer from the issue attachments:

#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#define ALIGNMENT        64               // 16 works, 32 fails, 64 fails, 128 works, 256 fails
#define LARGE_ALLOC_SIZE 16 * 1024 * 1024 // 1, 2, 4, 8, 16 fail, 32, 64, 128 work (all in MB)
#define SMALL_ALLOC_SIZE 25               // <= 24 works, > 24 breaks

void* work() {
    // Temporary buffers for the computation
    void* tmp1 = aligned_alloc(ALIGNMENT, LARGE_ALLOC_SIZE);
    void* tmp2 = aligned_alloc(ALIGNMENT, LARGE_ALLOC_SIZE);

    // Ensure memory is actually mapped
    memset(tmp1, 0, LARGE_ALLOC_SIZE);
    memset(tmp2, 0, LARGE_ALLOC_SIZE);

    // Buffer for the result
    void* result = malloc(SMALL_ALLOC_SIZE);

    free(tmp2);
    free(tmp1);

    return result;
}

int main() {
    for (;;) {
        usleep(1000);          // To make it easier to observe
        void* result = work(); // Normally the result is stored
    }
}

Graph of RSS/VSZ (~12 sec):
image

@mshabunin
Copy link
Copy Markdown
Contributor Author

For correct dynamic analysis results with custom alignment we can leverage manual poisoning in ASAN and special macros for valgrind.

@mshabunin mshabunin force-pushed the disable_posix_memalign branch from 55bb3bf to aecfe1e Compare September 25, 2019 11:43
@mshabunin mshabunin force-pushed the disable_posix_memalign branch from aecfe1e to 59efaf2 Compare September 25, 2019 11:47
CMakeLists.txt Outdated
CHECK_INCLUDE_FILE(malloc.h HAVE_MALLOC_H)
if(HAVE_MALLOC_H)
CHECK_SYMBOL_EXISTS(memalign malloc.h HAVE_MEMALIGN)
if(ENABLE_ALIGNED_MALLOC)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter name mismatch with OCV_OPTION().

}

#if defined HAVE_POSIX_MEMALIGN || defined HAVE_MEMALIGN
static const bool useMemalign = cv::utils::getConfigurationParameterBool("OPENCV_ENABLE_MEMALIGN", false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid "initialization order fiasco".

Variable can be used below without proper initialization and then crash in mismatched fastFree() calls.

@mshabunin mshabunin force-pushed the disable_posix_memalign branch from 1770c7a to c405e77 Compare October 9, 2019 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants