Raise VIF limit from 7 to 16 by calculating max_grant_frames on domain creation (#6577)

xenopsd currently hardcodes 64 as the value for `max_grant_frames` for
all domains. This limits how many grants a domain can allocate, thus
limiting the number of VIFs and VBDs a domain can have.
`max_grant_frames` can be changed for individual VMs through specifying
a particular value in `platform`, but it's much better to be able to
estimate how many grant frames a VM would need based on the number of
VBDs and VIFs.

Implement this and add some notes on the difficulties and imprecisions
of such an estimation.

This allows raising the number of supported VIFs from the current limit
of 7 - we've picked 16 as a sweet spot, though this could be discussed
further. (note that it's the `allowed_vifs` limit that's changed, VIFs
can be created in arbitrary numbers on the CLI and by clients not
honouring the `allowed_vifs` advice).

Given the current behaviour of the XenServer/XCP-ng system
(hypervisor+drivers), where more VIFs allow for higher overall
networking throughput, this is highly beneficial - in testing overall
throughput with 16 VIFs was 18-27% higher than with 8 VIFs (tested with
multiple iperf3 instances running on all interfaces simultaneously). The
following table shows the performance per-VIF and per-VM on a host with
16 pcpus with 8 vCPUs for domU and dom0 each:

![image](https://github.com/user-attachments/assets/f61488bf-90f0-4cbd-8896-176e053f708f)

Moreover, some users coming from VMWare are used to networking setups
with dozens of VIFs, and this is a step towards allowing that without
encountering any other bottlenecks in the system.

Most of this work (except the last commit) was tested very thoroughly at
XenServer, since it was initially intended to raise the supported limit
of VBDs (it passed Ring3 BST+BVT multiple times, and ran through the
config limits test several times). Raising the number of supported VBDs
has other issues so was abandoned, but this PR solves all the issues for
VIFs.
The new limit was tested with XCP-ng, with a new test being added to our
CI (xcp-ng/xcp-ng-tests#338), which will verify
the ability to hit the new limit and perform well at it.

Author: last-genius

ocaml/xapi/xapi_vm_helpers.ml

@@ -1304,9 +1304,9 @@ let allowed_VBD_devices_HVM_floppy =
     (fun x -> Device_number.(make Floppy ~disk:x ~partition:0))
     (inclusive_range 0 1)
 
-let allowed_VIF_devices_HVM = vif_inclusive_range 0 6
+let allowed_VIF_devices_HVM = vif_inclusive_range 0 15
 
-let allowed_VIF_devices_PV = vif_inclusive_range 0 6
+let allowed_VIF_devices_PV = vif_inclusive_range 0 15
 
 (** [possible_VBD_devices_of_string s] returns a list of Device_number.t which
     	represent possible interpretations of [s]. *)

ocaml/xenopsd/lib/xenops_server.ml

@@ -848,10 +848,11 @@ module Queues = struct
 
   let get tag qs =
     with_lock qs.m (fun () ->
-        if StringMap.mem tag qs.qs then
-          StringMap.find tag qs.qs
-        else
-          Queue.create ()
+        match StringMap.find_opt tag qs.qs with
+        | Some x ->
+            x
+        | None ->
+            Queue.create ()
     )
 
   let tags qs =
@@ -862,10 +863,11 @@ module Queues = struct
   let push_with_coalesce should_keep tag item qs =
     with_lock qs.m (fun () ->
         let q =
-          if StringMap.mem tag qs.qs then
-            StringMap.find tag qs.qs
-          else
-            Queue.create ()
+          match StringMap.find_opt tag qs.qs with
+          | Some x ->
+              x
+          | None ->
+              Queue.create ()
         in
         push_with_coalesce should_keep item q ;
         qs.qs <- StringMap.add tag q qs.qs ;
@@ -2297,11 +2299,18 @@ let rec perform_atomic ~progress_callback ?result (op : atomic)
       debug "VM.destroy %s" id ;
       B.VM.destroy t (VM_DB.read_exn id)
   | VM_create (id, memory_upper_bound, final_id, no_sharept) ->
-      debug "VM.create %s memory_upper_bound = %s" id
+      let num_of_vbds = List.length (VBD_DB.vbds id) in
+      let num_of_vifs = List.length (VIF_DB.vifs id) in
+      debug
+        "VM.create %s memory_upper_bound = %s, num_of_vbds = %d, num_of_vifs = \
+         %d"
+        id
         (Option.value ~default:"None"
            (Option.map Int64.to_string memory_upper_bound)
-        ) ;
+        )
+        num_of_vbds num_of_vifs ;
       B.VM.create t memory_upper_bound (VM_DB.read_exn id) final_id no_sharept
+        num_of_vbds num_of_vifs
   | VM_build (id, force) ->
       debug "VM.build %s" id ;
       let vbds : Vbd.t list = VBD_DB.vbds id |> vbd_plug_order in

ocaml/xenopsd/lib/xenops_server_plugin.ml

@@ -84,6 +84,8 @@ module type S = sig
       -> Vm.t
       -> Vm.id option
       -> bool (* no_sharept*)
+      -> int (* num_of_vbds *)
+      -> int (* num_of_vifs *)
       -> unit
 
     val build :

ocaml/xenopsd/lib/xenops_server_simulator.ml

@@ -547,7 +547,8 @@ module VM = struct
 
   let remove _vm = ()
 
-  let create _ memory_limit vm _ _ = with_lock m (create_nolock memory_limit vm)
+  let create _ memory_limit vm _ _ _ _ =
+    with_lock m (create_nolock memory_limit vm)
 
   let destroy _ vm = with_lock m (destroy_nolock vm)
 

ocaml/xenopsd/lib/xenops_utils.ml

@@ -227,11 +227,13 @@ module MemFS = struct
       match (path, fs) with
       | [], Dir d ->
           d
-      | p :: ps, Dir d ->
-          if StringMap.mem p !d then
-            aux ps (StringMap.find p !d)
-          else
+      | p :: ps, Dir d -> (
+        match StringMap.find_opt p !d with
+        | Some x ->
+            aux ps x
+        | None ->
             raise Not_dir
+      )
       | _, Leaf _ ->
           raise Not_dir
     in
@@ -285,14 +287,13 @@ module MemFS = struct
           (fun p ->
             let dir = dir_locked (dirname p) in
             let deletable =
-              if StringMap.mem (filename p) !dir then
-                match StringMap.find (filename p) !dir with
-                | Dir child ->
-                    StringMap.is_empty !child
-                | Leaf _ ->
-                    true
-              else
-                false
+              match StringMap.find_opt (filename p) !dir with
+              | Some (Dir child) ->
+                  StringMap.is_empty !child
+              | Some (Leaf _) ->
+                  true
+              | None ->
+                  false
             in
             if deletable then dir := StringMap.remove (filename p) !dir
           )

ocaml/xenopsd/xc/domain.ml

@@ -269,7 +269,8 @@ let wait_xen_free_mem ~xc ?(maximum_wait_time_seconds = 64) required_memory_kib
   in
   wait 0
 
-let make ~xc ~xs vm_info vcpus domain_config uuid final_uuid no_sharept =
+let make ~xc ~xs vm_info vcpus domain_config uuid final_uuid no_sharept
+    num_of_vbds num_of_vifs =
   let open Xenctrl in
   let host_info = Xenctrl.physinfo xc in
 
@@ -385,12 +386,80 @@ let make ~xc ~xs vm_info vcpus domain_config uuid final_uuid no_sharept =
     ; max_evtchn_port= -1
     ; max_grant_frames=
         ( try int_of_string (List.assoc "max_grant_frames" vm_info.platformdata)
-          with _ -> 64
+          with _ ->
+            let max_per_vif = 8 in
+            (* 1 VIF takes up (256 rx entries + 256 tx entries) * 8 queues max
+               * 8 bytes per grant table entry / 4096 bytes size of frame *)
+            let reasonable_per_vbd = 1 in
+            (* (1 ring (itself taking up one granted page) + 1 ring *
+               32 requests * 11 grant refs contained in each * 8 bytes ) /
+               4096 bytes size of frame = 0.6875, rounded up *)
+            let frames_number =
+              max 64
+                ((max_per_vif * (num_of_vifs + 1))
+                + (reasonable_per_vbd * (num_of_vbds + 1))
+                )
+            in
+            debug "estimated max_grant_frames = %d" frames_number ;
+            frames_number
+            (* max_per_vif * (num_of_vifs + 1 hotplugged future one) +
+               max_per_vbd * (num_of_vbds + 1 hotplugged future one) *)
+
+            (* NOTE: While the VIF calculation is precise, the VBD one is a
+               very rough approximation of a reasonable value of
+               RING_SIZE * MAX_SEGMENTS_PER_REQUEST + PAGES_FOR_RING_ITSELF
+               The following points should allow for a rough understanding
+               of the scale of the problem of better estimation:
+
+               1) The blkfront driver can consume different numbers of grant
+               pages depending on the features advertised by the back driver
+               (and negotiated with it). These features can differ per VBD, and
+               right now aren't even known at the time of domain creation.
+               These include:
+                 1.1) indirect segments - these contain
+                 BLKIF_MAX_INDIRECT_PAGES_PER_REQUEST grants at most, and each
+                 of these frames contains GRANTS_PER_INDIRECT_FRAME grants in
+                 turn (stored in blkif_request_segment).
+                 In practice, this means a catastrophic explosion - we should
+                 not really aim to detect if indirect requests feature is on,
+                 but turn it off to get reasonable estimates.
+                 1.2) persistent grants - these are an optimization, so
+                 shouldn't really change the calculations, worst case is none
+                 of the grants are persistent.
+                 1.3) multi-page rings - these change the RING_SIZE, but not in
+                 a trivial manner (see ring-page-order)
+                 1.4) multi-queue - these change the number of rings, adding
+                 another multiplier.
+               2) The "8 bytes" multiplier for a grant table entry only applies
+               to grants_v1. v2 grants take up 16 bytes per entry. And it's
+               impossible to detect this feature at the moment.
+               3) A dynamically-sized grant table itself could be a solution?
+               Used to exist before, caused a lot of XSAs, hard to get right.
+               4) Drivers might need to be more explicitly limited in how many
+               pages they can consume
+               5) VBD backdriver's features should be managed by XAPI on the
+               object itself and (their max bound) known at the time of domain
+               creation.
+
+               So for this estimate, there is only 1 ring which is 1 page, with
+               32 entries, each entry (request) can have up to 11 pages
+               (excluding indirect pages and other complications).
+
+               SEE: xen-blkfront.c, blkif.h, and the backdriver to understand
+               the process of negotiation (visible in xenstore, in kernel
+               module parameters in the sys filesystem afterwards)
+            *)
         )
     ; max_maptrack_frames=
         ( try
             int_of_string (List.assoc "max_maptrack_frames" vm_info.platformdata)
-          with _ -> 1024
+          with _ ->
+            0
+            (* This should be >0 only for driver domains (Dom0 startup is not
+               handled by the toolstack), which currently do not exist.
+               To support these in the future, xenopsd would need to check what
+               type of domain is being started.
+            *)
         )
     ; max_grant_version=
         (if List.mem CAP_Gnttab_v2 host_info.capabilities then 2 else 1)

ocaml/xenopsd/xc/domain.mli

@@ -149,6 +149,8 @@ val make :
   -> [`VM] Uuidx.t
   -> string option
   -> bool (* no_sharept *)
+  -> int (* num_of_vbds *)
+  -> int (* num_of_vifs *)
   -> domid
 (** Create a fresh (empty) domain with a specific UUID, returning the domain ID *)
 

ocaml/xenopsd/xc/xenops_server_xen.ml

@@ -1389,7 +1389,8 @@ module VM = struct
         in
         (device_id, revision)
 
-  let create_exn task memory_upper_bound vm final_id no_sharept =
+  let create_exn task memory_upper_bound vm final_id no_sharept num_of_vbds
+      num_of_vifs =
     let k = vm.Vm.id in
     with_xc_and_xs (fun xc xs ->
         (* Ensure the DB contains something for this VM - this is to avoid a
@@ -1518,7 +1519,8 @@ module VM = struct
                   let create_info = generate_create_info ~xs vm persistent in
                   let domid =
                     Domain.make ~xc ~xs create_info vm.vcpu_max domain_config
-                      (uuid_of_vm vm) final_id no_sharept
+                      (uuid_of_vm vm) final_id no_sharept num_of_vbds
+                      num_of_vifs
                   in
                   Mem.transfer_reservation_to_domain dbg domid reservation_id ;
                   let initial_target =