From 95dbc42a73f94750a92484259f7bbef7b43ee04e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= Date: Sun, 21 Apr 2024 14:29:03 +0100 Subject: [PATCH 1/2] CP-51690: [prep] Xapi_periodic_scheduler: Factor out Delay.wait call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No functional change. Signed-off-by: Edwin Török --- ocaml/xapi/xapi_periodic_scheduler.ml | 31 +++++++++++++++------------ 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/ocaml/xapi/xapi_periodic_scheduler.ml b/ocaml/xapi/xapi_periodic_scheduler.ml index 1edcb938857..25bb7a49f73 100644 --- a/ocaml/xapi/xapi_periodic_scheduler.ml +++ b/ocaml/xapi/xapi_periodic_scheduler.ml @@ -58,6 +58,22 @@ let remove_from_queue name = if index > -1 then Ipq.remove queue index +let wait_next sleep = + try ignore (Delay.wait delay sleep) + with e -> + let detailed_msg = + match e with + | Unix.Unix_error (code, _, _) -> + Unix.error_message code + | _ -> + "unknown error" + in + error + "Could not schedule interruptable delay (%s). Falling back to normal \ + delay. New events may be missed." + detailed_msg ; + Thread.delay sleep + let loop () = debug "Periodic scheduler started" ; try @@ -85,20 +101,7 @@ let loop () = |> Mtime.Span.add (Clock.span 0.001) |> Scheduler.span_to_s in - try ignore (Delay.wait delay sleep) - with e -> - let detailed_msg = - match e with - | Unix.Unix_error (code, _, _) -> - Unix.error_message code - | _ -> - "unknown error" - in - error - "Could not schedule interruptable delay (%s). Falling back to \ - normal delay. New events may be missed." - detailed_msg ; - Thread.delay sleep + wait_next sleep done with _ -> error From 68af6ce342206115eb8b2739d355c7d2248ec845 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= Date: Sun, 21 Apr 2024 14:29:38 +0100 Subject: [PATCH 2/2] CP-51690: [bugfix] Xapi_periodic_scheduler: avoid 10s sleep on empty queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wake up the scheduler immediately when there are more tasks. Otherwise timeouts <10s may not work correctly, and it is difficult to test the periodic scheduler if you need to wait 10s for it to start working. If there are no tasks, then it will still sleep efficiently, but as soon as more tasks are added (with [~signal:true], which is the default) it will immediately wake up and calculate the next sleep time. In practice it is probably quite rare for XAPI's queue to be empty (there are usually periodic tasks), but we cannot rely on this. Signed-off-by: Edwin Török --- ocaml/tests/test_event.ml | 32 +++++++++++++++++++++++++++ ocaml/xapi/xapi_periodic_scheduler.ml | 2 +- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/ocaml/tests/test_event.ml b/ocaml/tests/test_event.ml index 9078244462b..d36dba90eff 100644 --- a/ocaml/tests/test_event.ml +++ b/ocaml/tests/test_event.ml @@ -277,6 +277,37 @@ let object_level_event_test _session_id = if !failure then Alcotest.fail "failed to see object-level event change" +let test_short_oneshot () = + (* don't call event_setup_common here, it'll register a dummy event and hide the bug *) + let started = ref false in + let m = Mutex.create () in + let cond = Condition.create () in + let scheduler () = + Mutex.lock m ; + started := true ; + Condition.broadcast cond ; + Mutex.unlock m ; + Xapi_periodic_scheduler.loop () + in + ignore (Thread.create scheduler ()) ; + (* ensure scheduler sees an empty queue , by waiting for it to start *) + Mutex.lock m ; + while not !started do + Condition.wait cond m + done ; + Mutex.unlock m ; + (* run the scheduler, let it realize its queue is empty, + a Thread.yield is not enough due to the use of debug which would yield back almost immediately. + *) + Thread.delay 1. ; + let fired = Atomic.make false in + let fire () = Atomic.set fired true in + let task = "test_oneshot" in + Xapi_periodic_scheduler.add_to_queue task Xapi_periodic_scheduler.OneShot 1. + fire ; + Thread.delay 2. ; + assert (Atomic.get fired) + let test = [ ("test_event_from_timeout", `Slow, test_event_from_timeout) @@ -287,4 +318,5 @@ let test = ; ("test_event_from", `Quick, event_from_test) ; ("test_event_from_parallel", `Slow, event_from_parallel_test) ; ("test_event_object_level_event", `Slow, object_level_event_test) + ; ("test_short_oneshot", `Slow, test_short_oneshot) ] diff --git a/ocaml/xapi/xapi_periodic_scheduler.ml b/ocaml/xapi/xapi_periodic_scheduler.ml index 25bb7a49f73..7463c55c12b 100644 --- a/ocaml/xapi/xapi_periodic_scheduler.ml +++ b/ocaml/xapi/xapi_periodic_scheduler.ml @@ -80,7 +80,7 @@ let loop () = while true do let empty = with_lock lock (fun () -> Ipq.is_empty queue) in if empty then - Thread.delay 10.0 + wait_next 10.0 (* Doesn't happen often - the queue isn't usually empty *) else let next = with_lock lock (fun () -> Ipq.maximum queue) in