From a567ea447693e9ec278150d9929f16b008fb5f8b Mon Sep 17 00:00:00 2001 From: Alan Rosenthal Date: Fri, 27 Dec 2024 12:33:32 -0800 Subject: [PATCH] Revert "pw_multibuf: Replace Mutex with ISL" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 18442a7e69a5b1c5794f028b7d98082f58f0475a. This CL also bumps the kMetaSizeBytes of pw_bluetooth_sapphire/central_test.cc which is now required due to larger size of metadata objects. Reason for revert: Using ISL causes issues in downstream when used in conjunction with a SynchronizedAllocator since SimpleAllocator attempts to lock the latter's mutex within the ISL. Original change's description: > pw_multibuf: Replace Mutex with ISL > > Per-region mutexes have size overhead that is much larger than > affordable for individual chunk regions. Prior to this change, > SimpleAllocator's LinkedRegionTracker was 144 bytes (!) when compiled > for host. After this change, it is 48 bytes (which is still too large, > but now the size can be reasonably improved by reducing the number of > pointer and size pairs). > > Change-Id: Iae5b3e277d649b613f1b12be911464657e8890bf > Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/200996 > Pigweed-Auto-Submit: Taylor Cramer > Presubmit-Verified: CQ Bot Account > Commit-Queue: Auto-Submit > Reviewed-by: Alexei Frolov Bug: 385791125 Change-Id: I757347806e716d33aa3900251664651909c1a8a5 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/256433 Lint: Lint 🤖 Presubmit-Verified: CQ Bot Account Docs-Not-Needed: David Rees Commit-Queue: David Rees Reviewed-by: David Rees Reviewed-by: Ted Pudlik Reviewed-by: Jason Graffius --- pw_allocator/BUILD.bazel | 2 +- pw_allocator/BUILD.gn | 2 +- pw_allocator/CMakeLists.txt | 2 +- pw_allocator/public/pw_allocator/testing.h | 5 ++--- pw_bluetooth_sapphire/central_test.cc | 2 +- pw_bluetooth_sapphire/fuchsia/backends.bazelrc | 2 +- pw_multibuf/BUILD.bazel | 2 +- pw_multibuf/BUILD.gn | 2 +- pw_multibuf/CMakeLists.txt | 2 +- pw_multibuf/public/pw_multibuf/allocator.h | 4 ++-- pw_multibuf/public/pw_multibuf/chunk.h | 4 ++-- pw_multibuf/public/pw_multibuf/simple_allocator.h | 3 +-- 12 files changed, 15 insertions(+), 17 deletions(-) diff --git a/pw_allocator/BUILD.bazel b/pw_allocator/BUILD.bazel index 35471fe130..65cbeb5934 100644 --- a/pw_allocator/BUILD.bazel +++ b/pw_allocator/BUILD.bazel @@ -459,7 +459,7 @@ cc_library( "//pw_bytes", "//pw_result", "//pw_status", - "//pw_sync:interrupt_spin_lock", + "//pw_sync:mutex", "//pw_unit_test", ], ) diff --git a/pw_allocator/BUILD.gn b/pw_allocator/BUILD.gn index 765a2a48c8..830f21d27a 100644 --- a/pw_allocator/BUILD.gn +++ b/pw_allocator/BUILD.gn @@ -383,7 +383,7 @@ pw_source_set("testing") { ":buffer", ":first_fit", ":tracking_allocator", - "$dir_pw_sync:interrupt_spin_lock", + "$dir_pw_sync:mutex", dir_pw_bytes, dir_pw_result, dir_pw_status, diff --git a/pw_allocator/CMakeLists.txt b/pw_allocator/CMakeLists.txt index c247e4480d..b20f26a9d8 100644 --- a/pw_allocator/CMakeLists.txt +++ b/pw_allocator/CMakeLists.txt @@ -420,7 +420,7 @@ pw_add_library(pw_allocator.testing INTERFACE pw_bytes pw_result pw_status - pw_sync.interrupt_spin_lock + pw_sync.mutex pw_unit_test PRIVATE_DEPS pw_assert diff --git a/pw_allocator/public/pw_allocator/testing.h b/pw_allocator/public/pw_allocator/testing.h index d8bec5a8d9..dbacaccfdf 100644 --- a/pw_allocator/public/pw_allocator/testing.h +++ b/pw_allocator/public/pw_allocator/testing.h @@ -23,11 +23,10 @@ #include "pw_allocator/metrics.h" #include "pw_allocator/tracking_allocator.h" #include "pw_assert/assert.h" -#include "pw_assert/internal/check_impl.h" #include "pw_bytes/span.h" #include "pw_result/result.h" #include "pw_status/status.h" -#include "pw_sync/interrupt_spin_lock.h" +#include "pw_sync/mutex.h" #include "pw_tokenizer/tokenize.h" #include "pw_unit_test/framework.h" @@ -219,7 +218,7 @@ class SynchronizedAllocatorForTest : public Allocator { return GetInfo(base_, info_type, ptr); } - mutable pw::sync::InterruptSpinLock lock_; + mutable pw::sync::Mutex lock_; Base base_; }; diff --git a/pw_bluetooth_sapphire/central_test.cc b/pw_bluetooth_sapphire/central_test.cc index 8c41c3b55b..879a54c26e 100644 --- a/pw_bluetooth_sapphire/central_test.cc +++ b/pw_bluetooth_sapphire/central_test.cc @@ -120,7 +120,7 @@ class CentralTest : public ::testing::Test { bt::gap::testing::FakeAdapter adapter_{async_dispatcher_}; pw::multibuf::test::SimpleAllocatorForTest + /*kMetaSizeBytes=*/3000> multibuf_allocator_; std::optional central_; }; diff --git a/pw_bluetooth_sapphire/fuchsia/backends.bazelrc b/pw_bluetooth_sapphire/fuchsia/backends.bazelrc index 05365f1504..f283c9b778 100644 --- a/pw_bluetooth_sapphire/fuchsia/backends.bazelrc +++ b/pw_bluetooth_sapphire/fuchsia/backends.bazelrc @@ -30,7 +30,7 @@ common:fuchsia --@pigweed//pw_log:backend_impl=@pigweed//pw_build:empty_cc_libra common:fuchsia --@pigweed//pw_unit_test:backend=@pigweed//pw_unit_test:googletest common:fuchsia --@pigweed//pw_async:task_backend=@pigweed//pw_async_fuchsia:task common:fuchsia --@pigweed//pw_async:fake_dispatcher_backend=@pigweed//pw_async_fuchsia:fake_dispatcher -common:fuchsia --@pigweed//pw_sync:interrupt_spin_lock_backend=@pigweed//pw_sync_stl:interrupt_spin_lock +common:fuchsia --@pigweed//pw_sync:mutex_backend=@pigweed//pw_sync_stl:mutex # TODO: https://pwbug.dev/374340793 - This is a temporary workaround for now and # this command line option shouldn't be applied more widely. diff --git a/pw_multibuf/BUILD.bazel b/pw_multibuf/BUILD.bazel index de63900783..800c7adffd 100644 --- a/pw_multibuf/BUILD.bazel +++ b/pw_multibuf/BUILD.bazel @@ -30,7 +30,7 @@ cc_library( "//pw_bytes", "//pw_preprocessor", "//pw_span", - "//pw_sync:interrupt_spin_lock", + "//pw_sync:mutex", ], ) diff --git a/pw_multibuf/BUILD.gn b/pw_multibuf/BUILD.gn index 7d6f9f88b1..5f0bbfa0f0 100644 --- a/pw_multibuf/BUILD.gn +++ b/pw_multibuf/BUILD.gn @@ -29,7 +29,7 @@ pw_source_set("chunk") { public = [ "public/pw_multibuf/chunk.h" ] sources = [ "chunk.cc" ] public_deps = [ - "$dir_pw_sync:interrupt_spin_lock", + "$dir_pw_sync:mutex", dir_pw_assert, dir_pw_bytes, dir_pw_preprocessor, diff --git a/pw_multibuf/CMakeLists.txt b/pw_multibuf/CMakeLists.txt index a6a7010a55..f7d4c1b67a 100644 --- a/pw_multibuf/CMakeLists.txt +++ b/pw_multibuf/CMakeLists.txt @@ -24,7 +24,7 @@ pw_add_library(pw_multibuf.chunk STATIC pw_bytes pw_preprocessor pw_span - pw_sync.interrupt_spin_lock + pw_sync.mutex PRIVATE_DEPS pw_assert.check SOURCES diff --git a/pw_multibuf/public/pw_multibuf/allocator.h b/pw_multibuf/public/pw_multibuf/allocator.h index 995d2f35ee..2b6edbcf1e 100644 --- a/pw_multibuf/public/pw_multibuf/allocator.h +++ b/pw_multibuf/public/pw_multibuf/allocator.h @@ -18,7 +18,7 @@ #include "pw_containers/intrusive_forward_list.h" #include "pw_multibuf/multibuf.h" #include "pw_result/result.h" -#include "pw_sync/interrupt_spin_lock.h" +#include "pw_sync/mutex.h" namespace pw::multibuf { @@ -177,7 +177,7 @@ class MultiBufAllocator { size_t desired_size, ContiguityRequirement contiguity_requirement) = 0; - sync::InterruptSpinLock lock_; + sync::Mutex lock_; IntrusiveForwardList mem_delegates_ PW_GUARDED_BY(lock_); }; diff --git a/pw_multibuf/public/pw_multibuf/chunk.h b/pw_multibuf/public/pw_multibuf/chunk.h index 4f8f8c81a3..5952976fac 100644 --- a/pw_multibuf/public/pw_multibuf/chunk.h +++ b/pw_multibuf/public/pw_multibuf/chunk.h @@ -18,7 +18,7 @@ #include "pw_assert/assert.h" #include "pw_bytes/span.h" -#include "pw_sync/interrupt_spin_lock.h" +#include "pw_sync/mutex.h" namespace pw::multibuf { @@ -312,7 +312,7 @@ class ChunkRegionTracker { /// /// - know whether they can expand to fill neighboring regions of memory. /// - know when the last chunk has been destructed, triggering `Destroy`. - pw::sync::InterruptSpinLock lock_; + pw::sync::Mutex lock_; friend Chunk; }; diff --git a/pw_multibuf/public/pw_multibuf/simple_allocator.h b/pw_multibuf/public/pw_multibuf/simple_allocator.h index 5d155e39e8..f6793213e0 100644 --- a/pw_multibuf/public/pw_multibuf/simple_allocator.h +++ b/pw_multibuf/public/pw_multibuf/simple_allocator.h @@ -17,7 +17,6 @@ #include "pw_containers/intrusive_list.h" #include "pw_multibuf/allocator.h" #include "pw_multibuf/multibuf.h" -#include "pw_sync/interrupt_spin_lock.h" namespace pw::multibuf { @@ -151,7 +150,7 @@ class SimpleAllocator : public MultiBufAllocator { } } - pw::sync::InterruptSpinLock lock_; + pw::sync::Mutex lock_; IntrusiveList regions_ PW_GUARDED_BY(lock_); pw::allocator::Allocator& metadata_alloc_; const ByteSpan data_area_;