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

Defensive code for alloc/dealloc during TLS teardown #161

Merged
merged 35 commits into from
Apr 7, 2020
Merged

Conversation

mjp41
Copy link
Member

@mjp41 mjp41 commented Apr 1, 2020

If an allocation or deallocation occurs during TLS teardown, then it is
possible for a new allocator to be created and then this is leaked. On
the mimalloc-bench mstressN benchmark this was observed leading to a
large memory leak.

This fix, detects if we are in the TLS teardown phase, and if so,
the calls to alloc or dealloc must return the allocator once they have
perform the specific operation.

[Add:] Also fixes unimplemented ABA primitive for ARM. Currently uses locking.

[Add:] Some debug infrastructure to deal with Posix platforms.

@mjp41 mjp41 requested a review from davidchisnall April 1, 2020 09:36
@mjp41 mjp41 changed the title Defensive code for alloc/dealloc during TLS teardown Draft: Defensive code for alloc/dealloc during TLS teardown Apr 1, 2020
@mjp41
Copy link
Member Author

mjp41 commented Apr 1, 2020

Moved to draft due to failures on Arm Debug.

@mjp41 mjp41 mentioned this pull request Apr 2, 2020
@mjp41 mjp41 force-pushed the tls-teardown branch 2 times, most recently from 2e90811 to 5ac3501 Compare April 2, 2020 13:51
If an allocation or deallocation occurs during TLS teardown, then it is
possible for a new allocator to be created and then this is leaked. On
the mimalloc-bench mstressN benchmark this was observed leading to a
large memory leak.

This fix, detects if we are in the TLS teardown phase, and if so,
the calls to alloc or dealloc must return the allocator once they have
perform the specific operation.

Uses a separate variable to represent if a thread_local's destructor has
run already.  This is used to detect thread teardown to put the
allocator into a special slow path to avoid leaks.
Flush errors, print assert details, and present stack traces.
LL/SC implementation was broken, this replaces it with
a locking implementation. Changes the API to support LL/SC
for future implementation on ARM.
@mjp41
Copy link
Member Author

mjp41 commented Apr 3, 2020

I have addressed #50 with a locking based implementation. This should fix the ARM issues we were seeing.

@mjp41 mjp41 changed the title Draft: Defensive code for alloc/dealloc during TLS teardown Defensive code for alloc/dealloc during TLS teardown Apr 3, 2020
@mjp41 mjp41 requested a review from plietar April 3, 2020 11:00
src/ds/aba.h Show resolved Hide resolved
src/pal/pal_posix.h Show resolved Hide resolved
src/mem/alloc.h Show resolved Hide resolved
src/mem/threadalloc.h Show resolved Hide resolved
src/mem/threadalloc.h Outdated Show resolved Hide resolved
src/pal/pal_posix.h Show resolved Hide resolved
src/mem/threadalloc.h Outdated Show resolved Hide resolved
src/mem/threadalloc.h Show resolved Hide resolved
src/ds/aba.h Show resolved Hide resolved
src/mem/globalalloc.h Show resolved Hide resolved
src/mem/threadalloc.h Outdated Show resolved Hide resolved
Copy link
Contributor

@plietar plietar left a comment

Choose a reason for hiding this comment

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

LGTM, although I'm slightly concerned about the use of std::function. Creating them can result in an allocation, which could be bad if snmalloc is used as the default allocator.

We currently have few enough captures that it probably doesn't actually allocate (from what I can tell, GCC and clang can fit two words without allocation), but it feels pretty fragile.

Adding a template to InitThreadAllocator would help, but you can't have templated non type template arguments (ie. template<template<typename Fn> void* (*InitThreadAllocator)(const Fn&)>).

Instead we can wrap the two functions as static methods:

struct AllocInitializer {
  static bool needs_initialization() { [...] }
  template<typename Fn>
  static auto with_initialized_allocator(Fn& fn) {
    [...]
    auto value = fn(allocator);
    [...]
    return value;
  }
};

Then have Alloc parametrized over the concrete initializer instead and use it like:

template<typename Initializer, [...]>
struct Allocator {
  void* small_alloc_first_alloc(size_t size) {
    return Initializer::with_initialized_allocator([size](Allocator* allocator) {
      return allocator->alloc(size);
    });
  }
}

I think this is guaranteed to not have any allocation.

@mjp41
Copy link
Member Author

mjp41 commented Apr 7, 2020

LGTM, although I'm slightly concerned about the use of std::function. Creating them can result in an allocation, which could be bad if snmalloc is used as the default allocator.

We currently have few enough captures that it probably doesn't actually allocate (from what I can tell, GCC and clang can fit two words without allocation), but it feels pretty fragile.

Adding a template to InitThreadAllocator would help, but you can't have templated non type template arguments (ie. template<template<typename Fn> void* (*InitThreadAllocator)(const Fn&)>).

Instead we can wrap the two functions as static methods:

struct AllocInitializer {
  static bool needs_initialization() { [...] }
  template<typename Fn>
  static auto with_initialized_allocator(Fn& fn) {
    [...]
    auto value = fn(allocator);
    [...]
    return value;
  }
};

Then have Alloc parametrized over the concrete initializer instead and use it like:

template<typename Initializer, [...]>
struct Allocator {
  void* small_alloc_first_alloc(size_t size) {
    return Initializer::with_initialized_allocator([size](Allocator* allocator) {
      return allocator->alloc(size);
    });
  }
}

I think this is guaranteed to not have any allocation.

I really like this idea, I have not managed to get it to work with the current layering and header only nature AllocInitialiser needs to know about Alloc, and Alloc needs to know about AllocInitialiser. So that is a little problem.

I would like to not use std::function, and I think I can make this work. I propose I complete this PR, as it is already getting quite big, and submit this idea as a separate PR later.

Copy link
Contributor

@plietar plietar left a comment

Choose a reason for hiding this comment

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

I would like to not use std::function, and I think I can make this work. I propose I complete this PR, as it is already getting quite big, and submit this idea as a separate PR later.

Sure, sounds good.

@mjp41 mjp41 merged commit 74657d9 into master Apr 7, 2020
@mjp41 mjp41 deleted the tls-teardown branch April 7, 2020 14:41
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 this pull request may close these issues.

2 participants