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.

Sign up for GitHub

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

Jump to bottom

memory.cc: fix cross-shard shrinking realloc #2217

Closed
wants to merge 1 commit into from
Closed

memory.cc: fix cross-shard shrinking realloc #2217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/core/memory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2286,8 +2286,6 @@ void* realloc(void* ptr, size_t size) {
abort();
}
// if we're here, it's a non-null seastar memory ptr
// or original functions aren't available.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part of the comment was obsolete after some changes to the logic above which means we don't reach here in the "original functions not available" case.

// at any rate, using the seastar allocator is OK now.
auto old_size = ptr ? object_size(ptr) : 0;
if (size == old_size) {
return ptr;
Expand All @@ -2296,10 +2294,16 @@ void* realloc(void* ptr, size_t size) {
::free(ptr);
return nullptr;
}
if (size < old_size) {
if (size < old_size && cpu_pages::is_local_pointer(ptr)) {
// local pointers can sometimes be shrunk by returning freed
// pages to the buddy allocator
seastar::memory::shrink(ptr, size);
return ptr;
}

// either a request to realloc larger than the existing allocation size
// or a cross-shard pointer: in either case we allocate a new local
// pointer and copy the contents
auto nptr = malloc(size);
if (!nptr) {
return nptr;
Expand Down
55 changes: 55 additions & 0 deletions tests/unit/alloc_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
*/

#include <seastar/core/memory.hh>
#include <seastar/core/shard_id.hh>
#include <seastar/core/smp.hh>
#include <seastar/core/temporary_buffer.hh>
#include <seastar/testing/perf_tests.hh>
#include <seastar/testing/test_case.hh>
#include <seastar/testing/thread_test_case.hh>
#include <seastar/util/log.hh>
#include <seastar/util/memory_diagnostics.hh>

Expand Down Expand Up @@ -155,6 +157,59 @@ SEASTAR_TEST_CASE(test_memory_diagnostics) {
return make_ready_future<>();
}

SEASTAR_THREAD_TEST_CASE(test_cross_thread_realloc) {
Copy link
Contributor

@tchaikov tchaikov May 17, 2024 •
edited

Choose a reason for hiding this comment

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

the purpose of this test is to verify the behavior of seastar allocator, not the one provided by libc, so probably we should guard it like

#ifndef SEASTAR_DEFAULT_ALLOCATOR

SEASTAR_THREAD_TEST_CASE(test_cross_thread_realloc) {
   // ...
}
#endif

i.e., to move the #ifndef SEASTAR_DEFAULT_ALLOCATOR at the end of this test up before it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deliberately left in both modes, because it works in both modes. What's the harm? Leaving it in both modes has some advantages two major ones which are:

  • the test will run in ASAN etc which is only available in the non-ss-allocator modes, to catch bugs in it
  • be able to catch invalid assumptions even in debug mode (which is nice for dev for several reasons)

I have been mostly under the assumption that we should gate tests with SEASTAR_DEFAULT_ALLOCATOR only if they would fail or fail to compile in the default allocator mode.

Copy link
Contributor

@tchaikov tchaikov May 31, 2024 •
edited

Choose a reason for hiding this comment

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

I see. Makes sense. As we also have tests testing std::string when testing seastar::sstring for the similar reasons. So I don't insist.

// Tests that realloc seems to do the right thing with various sizes of
// buffer, including cases where the initial allocation is on another
// shard.
// Needs at least 2 shards to usefully test the cross-shard aspect but
// still passes when only 1 shard is used.
auto do_xshard_realloc = [](bool cross_shard, size_t initial_size, size_t realloc_size) {
BOOST_TEST_CONTEXT("cross_shard=" << cross_shard << ", initial="
<< initial_size << ", realloc_size=" << realloc_size) {

auto other_shard = (this_shard_id() + cross_shard) % smp::count;

char *p = static_cast<char *>(malloc(initial_size));

// write some sentinels and check them after realloc
// x start of region
// y end of realloc'd region (if it falls within the initial size)
// z end of initial region
if (initial_size > 0) {
p[0] = 'x';
p[initial_size - 1] = 'z';
if (realloc_size > 0 && realloc_size <= initial_size) {
p[realloc_size - 1] = 'y';
}
}
smp::submit_to(other_shard, [=] {
char* p2 = static_cast<char *>(realloc(p, realloc_size));
if (initial_size > 0 && realloc_size > 0) {
BOOST_REQUIRE_EQUAL(p2[0], 'x');
if (realloc_size <= initial_size) {
BOOST_REQUIRE_EQUAL(p2[realloc_size - 1], 'y');
}
if (realloc_size > initial_size) {
BOOST_REQUIRE_EQUAL(p2[initial_size - 1], 'z');
}
}
free(p2);
}).get();
}
};

for (auto& cross_shard : {false, true}) {
do_xshard_realloc(cross_shard, 0, 0);
do_xshard_realloc(cross_shard, 0, 1);
do_xshard_realloc(cross_shard, 1, 0);
do_xshard_realloc(cross_shard, 50, 100);
do_xshard_realloc(cross_shard, 100, 50);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case (among others) failed with an assertion failure before the fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: In such cases I like to add a code comment (not a github comment) saying something like:

// reproduces issue #2202:

It can be useful to know why a test exists, and also if it fails on some old branch or something, you will know which fix you forgot to include.

do_xshard_realloc(cross_shard, 100000, 500000);
do_xshard_realloc(cross_shard, 500000, 100000);
}
}


#ifndef SEASTAR_DEFAULT_ALLOCATOR

struct thread_alloc_info {
Expand Down