Revert "pw_multibuf: Replace Mutex with ISL"

This reverts commit 18442a7. 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<Mutex> 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 <[email protected]> > Presubmit-Verified: CQ Bot Account <[email protected]> > Commit-Queue: Auto-Submit <[email protected]> > Reviewed-by: Alexei Frolov <[email protected]> Bug: 385791125 Change-Id: I757347806e716d33aa3900251664651909c1a8a5 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/256433 Lint: Lint 🤖 <[email protected]> Presubmit-Verified: CQ Bot Account <[email protected]> Docs-Not-Needed: David Rees <[email protected]> Commit-Queue: David Rees <[email protected]> Reviewed-by: David Rees <[email protected]> Reviewed-by: Ted Pudlik <[email protected]> Reviewed-by: Jason Graffius <[email protected]>
google · Dec 27, 2024 · a567ea4 · a567ea4
1 parent 7cb8a82
commit a567ea4
Show file tree

Hide file tree

Showing 12 changed files with 15 additions and 17 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -120,7 +120,7 @@ class CentralTest : public ::testing::Test {
   bt::gap::testing::FakeAdapter adapter_{async_dispatcher_};
 
   pw::multibuf::test::SimpleAllocatorForTest</*kDataSizeBytes=*/2024,
-                                             /*kMetaSizeBytes=*/2024>
+                                             /*kMetaSizeBytes=*/3000>
       multibuf_allocator_;
   std::optional<Central> central_;
 };

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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<MemoryAvailableDelegate> mem_delegates_
       PW_GUARDED_BY(lock_);
 };

diff --git 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
@@ -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<internal::LinkedRegionTracker> regions_ PW_GUARDED_BY(lock_);
   pw::allocator::Allocator& metadata_alloc_;
   const ByteSpan data_area_;