XSI-2155/CA-411684: When a host has most of its memory used, restarted VMs are not placed in a single NUMA node (#6929)

Xenopsd takes the minimum between the actual free memory on a NUMA node,
and an estimate.
The estimate never got implemented, so it just took the last free memory
value, which resulted in the free memory only ever decreasing on a NUMA
node, and once the host memory is fully used NUMA optimization would
stop working for newly (re)booted VMs, even if meanwhile other VMs were
stopped.

Master has a different fix (it uses claims, and tracks memory usage
accurately), but that requires a newer version of Xen.
Initially it was decided that this bug wouldn't get fixed on the LCM
branch, but that decision has changed now.

Instead it should track how much memory the pending VM starts are using.
This estimate can become inaccurate fairly quickly on old versions of
Xen without per-node claims. To reduce that window we only use the
estimates when we have other pending domain builds.
When we have 0 pending domain builds, we reset the estimate to the
actual free memory on the node (note: reset, *not* take the minimum).

Then finally we take the minimum between the actual free memory on a
node and the above estimate.

In practice this is not as simple as that, because Xen doesn't always
listen to us when we give it a CPU affinity hint, and sometimes it
places the memory on other nodes. This happens particularly when the
last 4GiB (the 32-bit DMA zone) is used up on node0: Xen would only use
this as a last resort (some devices can only work if you have memory
available here).
To fix this xenopsd considers node0 to have less memory available than
it actually does (so it'll prefer other nodes instead of using up the
last 4GiB). And finally it'll spread the last VMs across all nodes, when
we can't accurately estimate how much memory ends up where (we don't
know how much of the DMA32 heap is already used or not by devices).

This bug also affects master in a different way: there due to the claims
Xen does *exactly* what we ask it to do, and will completely deplete the
memory on node0. But that could cause failures with some devices, so we
might need to include the 2nd commit in some form on master too (maybe
don't reserve the full 4GiB, but only a few hundred MiB?).

Some manual testing shows promising results: previously VMs would get
all/all affinity, and after this fix they get the proper affinity again.
Getting the automated tests to pass is still an open problem due to the
4GiB handling, which will require some test changes too to not be so
strict, so opening this as a draft.

Author: edwintorok

ocaml/xenopsd/lib/topology.ml

@@ -46,6 +46,9 @@ module NUMAResource = struct
 
   let min_memory r1 r2 = {r1 with memfree= min r1.memfree r2.memfree}
 
+  let shrink_memory r mem =
+    {r with memfree= Int64.max 0L (Int64.sub r.memfree mem)}
+
   let pp_dump =
     Fmt.(
       Dump.record

ocaml/xenopsd/lib/topology.mli

@@ -56,6 +56,9 @@ module NUMAResource : sig
   val min_memory : t -> t -> t
   (** [min_memory a b] is [a] with the memory set to the minimum of [a] and [b] *)
 
+  val shrink_memory : t -> int64 -> t
+  (** [shrink_memory a mem] is [a] with the memory set to [max 0 (a-mem)] *)
+
   val pp_dump : t Fmt.t
   (** [pp_dump ppf v] pretty-prints [v] on [ppf] *)
 end
@@ -89,6 +92,8 @@ module NUMA : sig
   (** A NUMA node index. Distinct from an int to avoid mixing with CPU numbers *)
   type node = private Node of int
 
+  module NodeMap : Map.S with type key = node
+
   val make :
        distances:int array array
     -> cpu_to_node:int array

ocaml/xenopsd/lib/xenopsd.ml

@@ -72,6 +72,8 @@ let numa_placement_compat = ref true
 
 let numa_best_effort_prio_mem_only = ref false
 
+let numa_reserve_node0_dmaheap_bits = ref 31
+
 (* O(N^2) operations, until we get a xenstore cache, so use a small number here *)
 let vm_guest_agent_xenstore_quota = ref 128
 
@@ -282,6 +284,12 @@ let options =
        filtered NUMA nodes based on available memory. Only use if there are \
        issues with the new best effort policy"
     )
+  ; ( "numa-reserve-node0-dmaheap-bits"
+    , Arg.Int (fun x -> numa_reserve_node0_dmaheap_bits := x)
+    , (fun () -> string_of_int !numa_reserve_node0_dmaheap_bits)
+    , "Reserve 2^N bytes on node0 in the NUMA planner, similar to how Xen would\n\
+      \  protect the low 4GiB of the memory for the DMA heap"
+    )
   ; ( "pci-quarantine"
     , Arg.Bool (fun b -> pci_quarantine := b)
     , (fun () -> string_of_bool !pci_quarantine)

ocaml/xenopsd/xc/domain.ml

@@ -984,9 +984,134 @@ let numa_hierarchy =
      NUMA.make ~distances ~cpu_to_node ~node_cores
     )
 
+module NUMAResources = struct
+  module DomidMap = Map.Make (Int)
+
+  type node_memusage = int64 Topology.NUMA.NodeMap.t
+
+  type t = {
+      host: Topology.NUMAResource.t Topology.NUMA.NodeMap.t
+    ; estimated: node_memusage DomidMap.t
+    ; outstanding_builds: node_memusage DomidMap.t
+  }
+
+  let node_memusage_of_domidmap node t =
+    DomidMap.fold
+      (fun _ nodemap acc ->
+        Topology.NUMA.NodeMap.find_opt node nodemap
+        |> Option.value ~default:0L
+        |> Int64.add acc
+      )
+      t 0L
+
+  let to_nodes t =
+    let open Topology in
+    t.host
+    |> NUMA.NodeMap.mapi @@ fun node r ->
+       NUMAResource.shrink_memory r
+         (Int64.add
+            (node_memusage_of_domidmap node t.estimated)
+            (node_memusage_of_domidmap node t.outstanding_builds)
+         )
+
+  let of_host nodes =
+    {
+      host= nodes |> Topology.NUMA.NodeMap.of_seq
+    ; estimated= DomidMap.empty
+    ; outstanding_builds= DomidMap.empty
+    }
+
+  let empty =
+    {
+      host= Topology.NUMA.NodeMap.empty
+    ; estimated= DomidMap.empty
+    ; outstanding_builds= DomidMap.empty
+    }
+
+  let pp_dump_nodemap pp_elt =
+    let open Topology.NUMA in
+    Fmt.(Dump.iter_bindings NodeMap.iter (any "nodemap") pp_dump_node pp_elt)
+
+  let pp_dump_domidmap pp_elt =
+    Fmt.(Dump.iter_bindings DomidMap.iter (any "domidmap") int pp_elt)
+
+  let pp_dump_numa_resources =
+    Fmt.Dump.(
+      record
+        [
+          field "host"
+            (fun t -> t.host)
+            (pp_dump_nodemap Topology.NUMAResource.pp_dump)
+        ; field "estimated"
+            (fun t -> t.estimated)
+            (pp_dump_domidmap @@ pp_dump_nodemap Fmt.int64)
+        ; field "outstanding_builds"
+            (fun t -> t.outstanding_builds)
+            (pp_dump_domidmap @@ pp_dump_nodemap Fmt.int64)
+        ]
+    )
+
+  let add_outstanding domid plan t =
+    D.debug "domid %u: adding outstanding NUMA memory usage estimate: %s" domid
+      ((Fmt.to_to_string @@ pp_dump_nodemap Fmt.int64) plan) ;
+    {t with outstanding_builds= DomidMap.add domid plan t.outstanding_builds}
+
+  let remove_outstanding domid t =
+    D.debug "domid %u: removing outstanding NUMA memory usage estimate" domid ;
+    {t with outstanding_builds= DomidMap.remove domid t.outstanding_builds}
+
+  let transfer_outstanding domid t =
+    D.debug "domid %u: transfering outstanding NUMA memory usage estimate" domid ;
+    DomidMap.find_opt domid t.outstanding_builds
+    |> Option.fold ~none:t ~some:(fun plan ->
+        {
+          t with
+          estimated= DomidMap.add domid plan t.estimated
+        ; outstanding_builds= DomidMap.remove domid t.outstanding_builds
+        }
+    )
+
+  let estimate_plan nodes mem plan =
+    let open Topology in
+    let _, _, nodemap =
+      List.fold_left
+        (fun (mem, remaining_nodes, acc) node ->
+          let per_node_usage =
+            Int64.div mem (remaining_nodes |> Int64.of_int)
+          in
+          let r = NUMA.NodeMap.find node nodes in
+          let allocated_from_node =
+            Int64.min per_node_usage r.NUMAResource.memfree
+          in
+          ( Int64.sub mem allocated_from_node
+          , remaining_nodes - 1
+          , NUMA.NodeMap.add node allocated_from_node acc
+          )
+        )
+        (mem, List.length plan, NUMA.NodeMap.empty)
+        plan
+    in
+    nodemap
+end
+
 let numa_mutex = Mutex.create ()
 
-let numa_resources = ref None
+(* protected by numa_mutex *)
+let numa_resources = ref NUMAResources.empty
+
+let numa_reserve_node0 mem =
+  let reserved = Int64.shift_left 1L !Xenopsd.numa_reserve_node0_dmaheap_bits in
+  let mem' = Int64.(max 0L (sub mem reserved)) in
+  (* Prefer nodes other than node0 when node0 is low on memory.
+     This is reserved for the DMA heap in Xen, and even if we try to make a VM
+     use it up, Xen will move some of its memory to other nodes to avoid
+     running out.
+     Without the claim API we have no way of forcing or querying this
+     (and even with the claim API it'd be desirable to avoid running out on
+     node0).
+   *)
+  D.debug "numa_reserve_node0, reducing free memory: %Lu -> %Lu" mem mem' ;
+  mem'
 
 let numa_init () =
   let xcext = Xenctrlext.get_handle () in
@@ -1007,28 +1132,48 @@ let set_affinity = function
   | Xenops_server.Soft ->
       Xenctrlext.vcpu_setaffinity_soft
 
-let numa_placement domid ~vcpus ~cores ~memory affinity =
+let numa_placement domid ~vcpus ~cores ~memory ~required_free affinity =
   let open Xenctrlext in
   let open Topology in
+  let open NUMAResources in
   with_lock numa_mutex (fun () ->
       let ( let* ) = Option.bind in
       let xcext = get_handle () in
       let* host = Lazy.force numa_hierarchy in
       let numa_meminfo = (numainfo xcext).memory |> Array.to_seq in
       let nodes =
         Seq.map2
-          (fun node m -> NUMA.resource host node ~memory:m.memfree)
+          (fun node m ->
+            ( node
+            , let (NUMA.Node nodeid) = node in
+              let memory = m.memfree in
+              let memory =
+                if nodeid = 0 then
+                  numa_reserve_node0 memory
+                else
+                  memory
+              in
+              NUMA.resource host node ~memory
+            )
+          )
           (NUMA.nodes host) numa_meminfo
       in
       let vm = NUMARequest.make ~memory ~vcpus ~cores in
+
+      let previous = !numa_resources in
+      if DomidMap.is_empty previous.outstanding_builds then begin
+        (* no outstanding domain builds: synchronize actual free node memory
+           with host *)
+          numa_resources := of_host nodes ;
+          D.debug "no outstanding domain builds, dropping NUMA usage estimates"
+      end ;
+      D.debug "numa_resources(in): %s"
+      @@ Fmt.to_to_string pp_dump_numa_resources !numa_resources ;
+
+      let estimated_nodes = to_nodes !numa_resources in
       let nodea =
-        match !numa_resources with
-        | None ->
-            Array.of_seq nodes
-        | Some a ->
-            Array.map2 NUMAResource.min_memory (Array.of_seq nodes) a
+        estimated_nodes |> NUMA.NodeMap.to_seq |> Seq.map snd |> Array.of_seq
       in
-      numa_resources := Some nodea ;
       let cpu_affinity, memory_plan =
         match Softaffinity.plan ~vm host nodea with
         | None ->
@@ -1037,6 +1182,12 @@ let numa_placement domid ~vcpus ~cores ~memory affinity =
         | Some (cpu_affinity, mem_plan) ->
             (Some cpu_affinity, mem_plan)
       in
+      numa_resources :=
+        add_outstanding domid
+          (estimate_plan estimated_nodes required_free memory_plan)
+          !numa_resources ;
+      D.debug "numa_resources(out): %s"
+      @@ Fmt.to_to_string pp_dump_numa_resources !numa_resources ;
       let set_vcpu_affinity = function
         | None ->
             D.debug "%s: not setting vcpu affinity for domain %d" __FUNCTION__
@@ -1177,6 +1328,7 @@ let build_pre ~xc ~xs ~vcpus ~memory ~hard_affinity domid =
               in
               numa_placement domid ~vcpus ~cores
                 ~memory:(Int64.mul memory.xen_max_mib 1048576L)
+                ~required_free:(Int64.mul memory.required_host_free_mib 1048576L)
                 affinity
               |> Option.map fst
         )
@@ -1358,6 +1510,11 @@ let build (task : Xenops_task.task_handle) ~xc ~xs ~store_domid ~console_domid
   let target_mib = Memory.mib_of_kib_used target_kib in
   (* Sanity check. *)
   assert (target_mib <= static_max_mib) ;
+  let finally () =
+    with_lock numa_mutex @@ fun () ->
+    numa_resources := NUMAResources.remove_outstanding domid !numa_resources
+  in
+  Fun.protect ~finally @@ fun () ->
   let store_mfn, store_port, console_mfn, console_port, vm_stuff, domain_type =
     match info.priv with
     | BuildHVM hvminfo ->
@@ -1444,6 +1601,10 @@ let build (task : Xenops_task.task_handle) ~xc ~xs ~store_domid ~console_domid
         )
   in
   let local_stuff = console_keys console_port console_mfn in
+  let () =
+    with_lock numa_mutex @@ fun () ->
+    numa_resources := NUMAResources.transfer_outstanding domid !numa_resources
+  in
   build_post ~xc ~xs ~target_mib ~static_max_mib domid domain_type store_mfn
     store_port local_stuff vm_stuff