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

travisdowns · 2024-05-03T04:12:17Z

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.

travisdowns · 2024-05-03T04:12:57Z

src/core/memory.cc

@@ -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.


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.

travisdowns · 2024-05-03T04:13:48Z

tests/unit/alloc_test.cc

+        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);


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

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.

xemul

LGTM, but I'd appreciate one (or more) more pair of eyes on it

tchaikov

left some comments.

tchaikov · 2024-05-17T08:51:29Z

tests/unit/alloc_test.cc

@@ -19,11 +19,13 @@
 * Copyright (C) 2015 Cloudius Systems, Ltd.
 */

+#include "seastar/core/shard_id.hh"


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>

Time for clang-format in seastar, WDYT?

Done in b7e6a4d. In my defense this was auto-added by my IDE which I guess doesn't understand such concerns :).

Time for clang-format in seastar, WDYT?

yeah, actually, i am all for it. (just not sure about the rigid yet merciful maintainers). if every goes well, i would like to use clang-tidy to order the #includes , and clang-include-cleaner to keep the #include in a good shape.

tchaikov · 2024-05-17T08:55:20Z

tests/unit/alloc_test.cc

+    // 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


could you help me understand what "usefull test" means? is it a typo? or it's just me who have trouble understanding this. if "usefull" is removed, i would be able to comprehend this.

it should be usefully, LMK if it makes sense and I will fix it

Fixed to usefully in b7e6a4d, let me know if it's clear.

tchaikov · 2024-05-17T08:59:45Z

tests/unit/alloc_test.cc

+    // 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) { 


no need to capture with =. actually, nothing gets captured.

Thanks, removed in push b7e6a4d

tchaikov · 2024-05-17T09:44:14Z

tests/unit/alloc_test.cc

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

+SEASTAR_THREAD_TEST_CASE(test_cross_thread_realloc) {


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.

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.

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.

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.

tchaikov

lgtm

tchaikov · 2024-05-31T22:06:54Z

@scylladb/seastar-maint hello maintainers, could you help merge this change?

nyh · 2024-06-02T14:09:07Z

tests/unit/alloc_test.cc

+        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);


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.

travisdowns · 2024-06-03T02:30:01Z

@nyh wrote:

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

Good point, thanks for the feedback and I'll try to incorporate this in the future!

travisdowns commented May 3, 2024

View reviewed changes

tchaikov self-requested a review May 5, 2024 12:50

xemul approved these changes May 16, 2024

View reviewed changes

tchaikov reviewed May 17, 2024

View reviewed changes

travisdowns force-pushed the td-scylla-xshard-realloc branch from 4de7499 to b7e6a4d Compare May 31, 2024 19:28

tchaikov approved these changes May 31, 2024

View reviewed changes

nyh approved these changes Jun 2, 2024

View reviewed changes

nyh closed this in 2bc4f22 Jun 2, 2024

travisdowns deleted the td-scylla-xshard-realloc branch June 3, 2024 02:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

travisdowns commented May 3, 2024

travisdowns May 3, 2024

travisdowns May 3, 2024

nyh Jun 2, 2024

xemul left a comment

tchaikov left a comment

tchaikov May 17, 2024

travisdowns May 31, 2024

travisdowns May 31, 2024 •

edited

Loading

tchaikov May 31, 2024 •

edited

Loading

tchaikov May 17, 2024 •

edited

Loading

travisdowns May 31, 2024

travisdowns May 31, 2024

tchaikov May 31, 2024

tchaikov May 17, 2024

travisdowns May 31, 2024

tchaikov May 17, 2024 •

edited

Loading

travisdowns May 31, 2024 •

edited

Loading

tchaikov May 31, 2024 •

edited

Loading

tchaikov left a comment

tchaikov commented May 31, 2024

nyh Jun 2, 2024

travisdowns commented Jun 3, 2024

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

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

Conversation

travisdowns commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xemul left a comment

Choose a reason for hiding this comment

tchaikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisdowns May 31, 2024 • edited Loading

Choose a reason for hiding this comment

tchaikov May 31, 2024 • edited Loading

Choose a reason for hiding this comment

tchaikov May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov May 17, 2024 • edited Loading

Choose a reason for hiding this comment

travisdowns May 31, 2024 • edited Loading

Choose a reason for hiding this comment

tchaikov May 31, 2024 • edited Loading

Choose a reason for hiding this comment

tchaikov left a comment

Choose a reason for hiding this comment

tchaikov commented May 31, 2024

Choose a reason for hiding this comment

travisdowns commented Jun 3, 2024

travisdowns May 31, 2024 •

edited

Loading

tchaikov May 31, 2024 •

edited

Loading

tchaikov May 17, 2024 •

edited

Loading

tchaikov May 17, 2024 •

edited

Loading

travisdowns May 31, 2024 •

edited

Loading

tchaikov May 31, 2024 •

edited

Loading