memory.cc: fix cross-shard shrinking realloc #2217
Conversation
Cross-shard shrinking realloc crashes because we assert that the incoming pointer is shard local inside the shrink method but there is no particular reason to assume this is the case with a realloc: the initial allocation may have been made on another shard. Fix this by falling through to the free/malloc/memcpy path. This also means that realloc using the same size is a way to "rehome" a possibly foreign pointer: this does nothing if the pointer is already local but it will allocate a local pointer and copy the allocation contents if not. Fixes scylladb#2202.
| @@ -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. | |||
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
This case (among others) failed with an assertion failure before the fix.
| @@ -19,11 +19,13 @@ | |||
| * Copyright (C) 2015 Cloudius Systems, Ltd. | |||
| */ | |||
|
|
|||
| #include "seastar/core/shard_id.hh" | |||
There was a problem hiding this comment.
to be more consistent, could you please use brackets here and probably order it alphabetically. so it like:
#include <seastar/core/memory.hh>
#include <seastar/core/shard_id.hh>
#include <seastar/core/smp.hh>
#include <seastar/core/temporary_buffer.hh>| // 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 usefull test the cross-shard aspect but |
There was a problem hiding this comment.
could you help me understand what "usefull test" means? is it a typo? or it's just me who have trouble understand this. if "usefull" is removed, i would be able to comprehend this.
| // shard. | ||
| // Needs at least 2 shards to usefull 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) { |
There was a problem hiding this comment.
no need to capture with =. actually, nothing gets captured.
| @@ -155,6 +157,59 @@ SEASTAR_TEST_CASE(test_memory_diagnostics) { | |||
| return make_ready_future<>(); | |||
| } | |||
|
|
|||
| SEASTAR_THREAD_TEST_CASE(test_cross_thread_realloc) { | |||
There was a problem hiding this comment.
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) {
// ...
}
#endifi.e., to move the #ifndef SEASTAR_DEFAULT_ALLOCATOR at the end of this test up before it.
Cross-shard shrinking realloc crashes because we assert that the incoming pointer is shard local inside the shrink method but there is no particular reason to assume this is the case with a realloc: the initial allocation may have been made on another shard.
Fix this by falling through to the free/malloc/memcpy path. This also means that realloc using the same size is a way to "rehome" a possibly foreign pointer: this does nothing if the pointer is already local but it will allocate a local pointer and copy the allocation contents if not.
Fixes #2202.