Skip to content

Commit

Permalink
utils/logalloc: do not allocate memory in reclaim_timer::report()
Browse files Browse the repository at this point in the history
before this change, `reclaim_timer::report()` calls

```c++
fmt::format(", at {}", current_backtrace())
```

which allocates a `std::string` on heap, so it can fail and throw. in
that case, `std::terminate()` is called. but at that moment, the reason
why `reclaim_timer::report()` gets called is that we fail to reclaim
memory for the caller. so we are more likely to run into this issue. anyway,
we should not allocate memory in this path.

in this change, a dedicated printer is created so that we don't format
to a temporary `std::string`, and instead write directly to the buffer
of logger. this avoids the memory allocation.

Fixes #18099
Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes #18100

(cherry picked from commit fcf7ca5)
  • Loading branch information
tchaikov authored and denesb committed Apr 2, 2024
1 parent 7e05a54 commit d6c7a26
Showing 1 changed file with 33 additions and 1 deletion.
34 changes: 33 additions & 1 deletion utils/logalloc.cc
Expand Up @@ -204,6 +204,37 @@ migrate_fn_type::unregister_migrator(uint32_t index) {
static_migrators().remove(index);
}


namespace {

// for printing extra message in reclaim_timer::report() when stall is detected.
//
// this helper struct is deliberately introduced to ensure no dynamic
// allocations in reclaim_timer::report(), which is involved in handling OOMs.
struct extra_msg_when_stall_detected {
bool stall_detected;
saved_backtrace backtrace;
extra_msg_when_stall_detected(bool detected, saved_backtrace&& backtrace)
: stall_detected{detected}
, backtrace{std::move(backtrace)}
{}
};

}

template <>
struct fmt::formatter<extra_msg_when_stall_detected> {
constexpr auto parse(format_parse_context& ctx) { return ctx.begin(); }
auto format(const extra_msg_when_stall_detected& msg, fmt::format_context& ctx) const {
if (msg.stall_detected) {
return fmt::format_to(ctx.out(), ", at {}", msg.backtrace);
} else {
return ctx.out();
}
}
};


namespace logalloc {

// LSA-specific bad_alloc variant which allows adding additional information on
Expand Down Expand Up @@ -1511,7 +1542,8 @@ void reclaim_timer::report() const noexcept {
auto time_level = _stall_detected ? log_level::warn : log_level::debug;
auto info_level = _stall_detected ? log_level::info : log_level::debug;
auto MiB = 1024*1024;
auto msg_extra = _stall_detected ? fmt::format(", at {}", current_backtrace()) : "";
auto msg_extra = extra_msg_when_stall_detected(_stall_detected,
_stall_detected ? current_backtrace() : saved_backtrace{});

timing_logger.log(time_level, "{} took {} us, trying to release {:.3f} MiB {}preemptibly, reserve: {{goal: {}, max: {}}}{}",
_name, (_duration + 500ns) / 1us, (float)_memory_to_release / MiB, _preemptible ? "" : "non-",
Expand Down

0 comments on commit d6c7a26

Please sign in to comment.