From 398719e5de239aa716e547291575dbdaaafe7c9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= Date: Wed, 5 Feb 2025 14:47:00 +0000 Subject: [PATCH 1/2] Add Pool_role.is_master benchmarks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Edwin Török --- ocaml/tests/bench/bench_cached_reads.ml | 16 ++++++++++++++++ ocaml/tests/bench/dune | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 ocaml/tests/bench/bench_cached_reads.ml diff --git a/ocaml/tests/bench/bench_cached_reads.ml b/ocaml/tests/bench/bench_cached_reads.ml new file mode 100644 index 00000000000..4bb780a5119 --- /dev/null +++ b/ocaml/tests/bench/bench_cached_reads.ml @@ -0,0 +1,16 @@ +open Bechamel + +let run () = + let _ : bool = Sys.opaque_identity (Pool_role.is_master ()) in + () + +let mutex_workload = + Bechamel_simple_cli.thread_workload ~before:ignore ~after:ignore ~run + +let free_threads (stop, t) = Atomic.set stop true ; Array.iter Thread.join t + +let benchmarks = + Test.make_grouped ~name:"Cached reads" + [Test.make ~name:"Pool_role.is_master" (Staged.stage Pool_role.is_master)] + +let () = Bechamel_simple_cli.cli ~workloads:[mutex_workload] benchmarks diff --git a/ocaml/tests/bench/dune b/ocaml/tests/bench/dune index 10cffadb857..0c088389dfe 100644 --- a/ocaml/tests/bench/dune +++ b/ocaml/tests/bench/dune @@ -1,4 +1,4 @@ (executables - (names bench_tracing bench_uuid bench_throttle2) + (names bench_tracing bench_uuid bench_throttle2 bench_cached_reads) (libraries tracing bechamel bechamel-notty notty.unix tracing_export threads.posix fmt notty uuid xapi_aux tests_common log xapi_internal) ) From b945e10b0484ba94ba4688f44caa73fde248a3e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= Date: Wed, 5 Feb 2025 14:50:05 +0000 Subject: [PATCH 2/2] Optimize fastpath in Pool_role MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use an Atomic that can be accessed without holding the mutex. On the fastpath, where the role is already set this returns much more quickly. On the slowpath we still acquire the mutex and update the value as we did before, although we read the atomic value again to check whether we've raced with another thread that has updated it meanwhile already. Before (measuring both unloaded, and loaded situation where other threads are accessing the mutex): ``` Running benchmarks (no workloads) ╭────────────────────────────────────┬───────────────────────────┬───────────────────────────┬───────────────────────────╮ │name │ major-allocated │ minor-allocated │ monotonic-clock │ ├────────────────────────────────────┼───────────────────────────┼───────────────────────────┼───────────────────────────┤ │ Cached reads/Pool_role.is_master │ 0.0000 mjw/run│ 4.0000 mnw/run│ 23.6039 ns/run│ ╰────────────────────────────────────┴───────────────────────────┴───────────────────────────┴───────────────────────────╯ Cached reads/Pool_role.is_master (ns): { monotonic-clock per run = 23.603889 (confidence: 23.644693 to 23.568067); r² = Some 0.999899 } Running benchmarks (workloads) ╭────────────────────────────────────┬───────────────────────────┬───────────────────────────┬───────────────────────────╮ │name │ major-allocated │ minor-allocated │ monotonic-clock │ ├────────────────────────────────────┼───────────────────────────┼───────────────────────────┼───────────────────────────┤ │ Cached reads/Pool_role.is_master │ 0.0000 mjw/run│ 7.4201 mnw/run│ 76.4067 ns/run│ ╰────────────────────────────────────┴───────────────────────────┴───────────────────────────┴───────────────────────────╯ Cached reads/Pool_role.is_master (ns): { monotonic-clock per run = 76.406674 (confidence: 86.718075 to 66.336404); r² = Some 0.608131 } ``` After: ``` Running benchmarks (no workloads) ╭────────────────────────────────────┬───────────────────────────┬───────────────────────────┬───────────────────────────╮ │name │ major-allocated │ minor-allocated │ monotonic-clock │ ├────────────────────────────────────┼───────────────────────────┼───────────────────────────┼───────────────────────────┤ │ Cached reads/Pool_role.is_master │ 0.0000 mjw/run│ 0.0000 mnw/run│ 3.1256 ns/run│ ╰────────────────────────────────────┴───────────────────────────┴───────────────────────────┴───────────────────────────╯ Cached reads/Pool_role.is_master (ns): { monotonic-clock per run = 3.125616 (confidence: 3.131412 to 3.120373); r² = Some 0.999885 } Running benchmarks (workloads) ╭────────────────────────────────────┬───────────────────────────┬───────────────────────────┬───────────────────────────╮ │name │ major-allocated │ minor-allocated │ monotonic-clock │ ├────────────────────────────────────┼───────────────────────────┼───────────────────────────┼───────────────────────────┤ │ Cached reads/Pool_role.is_master │ 0.0000 mjw/run│ 0.0000 mnw/run│ 6.2872 ns/run│ ╰────────────────────────────────────┴───────────────────────────┴───────────────────────────┴───────────────────────────╯ Cached reads/Pool_role.is_master (ns): { monotonic-clock per run = 6.287197 (confidence: 6.737363 to 5.830140); r² = Some 0.706843 } ``` Signed-off-by: Edwin Török --- ocaml/tests/bench/bench_cached_reads.ml | 2 -- ocaml/xapi-aux/pool_role.ml | 33 +++++++++++++------------ 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/ocaml/tests/bench/bench_cached_reads.ml b/ocaml/tests/bench/bench_cached_reads.ml index 4bb780a5119..e81a8991cb4 100644 --- a/ocaml/tests/bench/bench_cached_reads.ml +++ b/ocaml/tests/bench/bench_cached_reads.ml @@ -7,8 +7,6 @@ let run () = let mutex_workload = Bechamel_simple_cli.thread_workload ~before:ignore ~after:ignore ~run -let free_threads (stop, t) = Atomic.set stop true ; Array.iter Thread.join t - let benchmarks = Test.make_grouped ~name:"Cached reads" [Test.make ~name:"Pool_role.is_master" (Staged.stage Pool_role.is_master)] diff --git a/ocaml/xapi-aux/pool_role.ml b/ocaml/xapi-aux/pool_role.ml index 66411f14126..f7c9fb5ac0b 100644 --- a/ocaml/xapi-aux/pool_role.ml +++ b/ocaml/xapi-aux/pool_role.ml @@ -28,21 +28,19 @@ type t = (* IP address *) | Broken -let role = ref None +let role = Atomic.make None -let role_unit_tests = ref false +let role_unit_tests = Atomic.make false let role_m = Mutex.create () let with_pool_role_lock f = Xapi_stdext_threads.Threadext.Mutex.execute role_m f let set_pool_role_for_test () = - with_pool_role_lock (fun _ -> - role := Some Master ; - role_unit_tests := true - ) + Atomic.set role (Some Master) ; + Atomic.set role_unit_tests true -let is_unit_test () = with_pool_role_lock (fun _ -> !role_unit_tests) +let is_unit_test () = Atomic.get role_unit_tests let string_of = function | Master -> @@ -80,15 +78,18 @@ let read_pool_role () = ) let get_role () = - with_pool_role_lock (fun _ -> - match !role with - | Some x -> - x - | None -> - let r = read_pool_role () in - role := Some r ; - r - ) + match Atomic.get role with + | Some x -> + x + | None -> + with_pool_role_lock (fun _ -> + match Atomic.get role with + | Some x -> + x + | None -> + let r = read_pool_role () in + Atomic.set role (Some r) ; r + ) let is_master () = get_role () = Master