From 88e69701bd8f2b48bfca17c0be290b79f55ca2fd Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Tue, 16 Jan 2018 15:54:03 +0200 Subject: [PATCH] Merge "Fix memory leak on zone reclaim" from Tomek "_free_segments_in_zones is not adjusted by segment_pool::reclaim_segments() for empty zones on reclaim under some conditions. For instance when some zone becomes empty due to regular free() and then reclaiming is called from the std allocator, and it is satisfied from a zone after the one which is empty. This would result in free memory in such zone to appear as being leaked due to corrupted free segment count, which may cause a later reclaim to fail. This could result in bad_allocs. The fix is to always collect such zones. Fixes #3129 Refs #3119 Refs #3120" * 'tgrabiec/fix-free_segments_in_zones-leak' of github.com:scylladb/seastar-dev: tests: lsa: Test _free_segments_in_zones is kept correct on reclaim lsa: Expose max_zone_segments for tests lsa: Expose tracker::non_lsa_used_space() lsa: Fix memory leak on zone reclaim (cherry picked from commit 4ad212dc01d40a3b76ec888ebd53db5a668fd298) --- tests/logalloc_test.cc | 36 ++++++++++++++++++++++++++++++++++++ utils/logalloc.cc | 25 +++++++++++++++---------- utils/logalloc.hh | 4 ++++ 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/tests/logalloc_test.cc b/tests/logalloc_test.cc index fe81414242..9db0943bfb 100644 --- a/tests/logalloc_test.cc +++ b/tests/logalloc_test.cc @@ -1171,3 +1171,39 @@ SEASTAR_TEST_CASE(test_reclaiming_runs_as_long_as_there_is_soft_pressure) { }); }); } + +SEASTAR_TEST_CASE(test_zone_reclaiming_preserves_free_size) { + return seastar::async([] { + region r; + with_allocator(r.allocator(), [&] { + chunked_fifo objs; + + auto zone_size = max_zone_segments * segment_size; + + // We need to generate 3 zones, so that at least one zone (not last) can be released fully. The first + // zone would not due to emergency reserve. + while (logalloc::shard_tracker().region_occupancy().used_space() < zone_size * 2 + zone_size / 4) { + objs.emplace_back(managed_bytes(managed_bytes::initialized_later(), 1024)); + } + + BOOST_TEST_MESSAGE(logalloc::shard_tracker().non_lsa_used_space()); + BOOST_TEST_MESSAGE(logalloc::shard_tracker().region_occupancy()); + + while (logalloc::shard_tracker().region_occupancy().used_space() >= logalloc::segment_size * 2) { + objs.pop_front(); + } + + BOOST_TEST_MESSAGE(logalloc::shard_tracker().non_lsa_used_space()); + BOOST_TEST_MESSAGE(logalloc::shard_tracker().region_occupancy()); + + auto before = logalloc::shard_tracker().non_lsa_used_space(); + logalloc::shard_tracker().reclaim(logalloc::segment_size); + auto after = logalloc::shard_tracker().non_lsa_used_space(); + + BOOST_TEST_MESSAGE(logalloc::shard_tracker().non_lsa_used_space()); + BOOST_TEST_MESSAGE(logalloc::shard_tracker().region_occupancy()); + + BOOST_REQUIRE(after <= before); + }); + }); +} diff --git a/utils/logalloc.cc b/utils/logalloc.cc index ff8b4a1079..1d67626d83 100644 --- a/utils/logalloc.cc +++ b/utils/logalloc.cc @@ -88,6 +88,7 @@ public: void reclaim_all_free_segments(); occupancy_stats region_occupancy(); occupancy_stats occupancy(); + size_t non_lsa_used_space(); void set_reclamation_step(size_t step_in_segments) { _reclamation_step = step_in_segments; } size_t reclamation_step() const { return _reclamation_step; } void enable_abort_on_bad_alloc() { _abort_on_bad_alloc = true; } @@ -124,6 +125,10 @@ occupancy_stats tracker::occupancy() { return _impl->occupancy(); } +size_t tracker::non_lsa_used_space() const { + return _impl->non_lsa_used_space(); +} + void tracker::full_compaction() { return _impl->full_compaction(); } @@ -335,7 +340,7 @@ static inline bool can_allocate_more_memory(size_t size) class segment_zone : public bi::set_base_hook<>, public bi::slist_base_hook<> { struct free_segment : public bi::slist_base_hook<> { }; - static constexpr size_t maximum_size = 256; + static constexpr size_t maximum_size = max_zone_segments; static constexpr size_t minimum_size = 16; static thread_local size_t next_attempt_size; @@ -613,10 +618,8 @@ size_t segment_pool::reclaim_segments(size_t target) { bi::slist zones_to_remove; for (auto& zone : _all_zones | boost::adaptors::reversed) { if (zone.empty()) { - if (reclaimed_segments < target || !zone.free_segment_count()) { - reclaimed_segments += zone.free_segment_count(); - zones_to_remove.push_front(zone); - } + reclaimed_segments += zone.free_segment_count(); + zones_to_remove.push_front(zone); } else if (zone.free_segment_count()) { _free_segments_in_zones += zone.free_segment_count(); zone.rebuild_free_segments_list(); @@ -1691,6 +1694,11 @@ occupancy_stats tracker::impl::occupancy() { return occ; } +size_t tracker::impl::non_lsa_used_space() { + auto free_space_in_zones = shard_segment_pool.free_segments_in_zones() * segment_size; + return memory::stats().allocated_memory() - region_occupancy().total_space() - free_space_in_zones; +} + void tracker::impl::reclaim_all_free_segments() { logger.debug("Reclaiming all free segments"); @@ -2021,11 +2029,8 @@ tracker::impl::impl() { sm::make_gauge("large_objects_total_space_bytes", [this] { return shard_segment_pool.non_lsa_memory_in_use(); }, sm::description("Holds a current size of allocated non-LSA memory.")), - sm::make_gauge("non_lsa_used_space_bytes", - [this] { - auto free_space_in_zones = shard_segment_pool.free_segments_in_zones() * segment_size; - return memory::stats().allocated_memory() - region_occupancy().total_space() - free_space_in_zones; - }, sm::description("Holds a current amount of used non-LSA memory.")), + sm::make_gauge("non_lsa_used_space_bytes", [this] { return non_lsa_used_space(); }, + sm::description("Holds a current amount of used non-LSA memory.")), sm::make_gauge("free_space_in_zones", [this] { return shard_segment_pool.free_segments_in_zones() * segment_size; }, sm::description("Holds a current amount of free memory in zones.")), diff --git a/utils/logalloc.hh b/utils/logalloc.hh index 5c2c4c6a52..45ce0634f2 100644 --- a/utils/logalloc.hh +++ b/utils/logalloc.hh @@ -42,6 +42,7 @@ class allocating_section; constexpr int segment_size_shift = 18; // 256K; see #151, #152 constexpr size_t segment_size = 1 << segment_size_shift; +constexpr size_t max_zone_segments = 256; // // Frees some amount of objects from the region to which it's attached. @@ -453,6 +454,9 @@ public: // Returns statistics for all segments allocated by LSA on this shard. occupancy_stats occupancy(); + // Returns amount of allocated memory not managed by LSA + size_t non_lsa_used_space() const; + impl& get_impl() { return *_impl; } // Set the minimum number of segments reclaimed during single reclamation cycle.