Re: [PATCH v2 0/4] NUMA CPUs 'auto-fill' for incomplete topologies

On 6/17/20 10:08 PM, Daniel Henrique Barboza wrote: > > > On 6/17/20 4:19 PM, Michal Privoznik wrote: >> On 6/10/20 8:35 PM, Daniel Henrique Barboza wrote: >>> changes in v2: >>> - removed patch 5/5 >>> >>> Gitlab link: https://gitlab.com/danielhb/libvirt/-/tree/vcpus_numa_v2 >>> >>> v1 link: https://www.redhat.com/archives/libvir-list/2020-June/msg00016.html >>> >>> >>> Daniel Henrique Barboza (4): >>>    numa_conf.c: add helper functions for cpumap operations >>>    qemu_domain.c: NUMA CPUs auto-fill for incomplete topologies >>>    qemuxml2xmltest.c: add NUMA vcpus auto fill tests >>>    formatdomain.html.in: document the NUMA cpus auto fill feature >>> >>>   docs/formatdomain.html.in                     | 11 ++++- >>>   src/conf/numa_conf.c                          | 46 ++++++++++++++++++ >>>   src/conf/numa_conf.h                          |  3 ++ >>>   src/libvirt_private.syms                      |  1 + >>>   src/qemu/qemu_domain.c                        | 47 +++++++++++++++++++ >>>   src/qemu/qemu_domain.h                        |  4 ++ >>>   src/qemu/qemu_driver.c                        |  9 ++++ >>>   .../numavcpus-topology-mismatch.xml           | 37 +++++++++++++++ >>>   ...avcpus-topology-mismatch.x86_64-latest.xml | 38 +++++++++++++++ >>>   tests/qemuxml2xmltest.c                       |  1 + >>>   10 files changed, 196 insertions(+), 1 deletion(-) >>>   create mode 100644 tests/qemuxml2argvdata/numavcpus-topology-mismatch.xml >>>   create mode 100644 tests/qemuxml2xmloutdata/numavcpus-topology-mismatch.x86_64-latest.xml >>> >> >> Patches look good to me. > > Thanks for the review! > >> >> My only concern is that I plan to introduce vCPU-less NUMA nodes [1] (because of HMAT [2]). But I guess if user assigns vCPUs to NUMA nodes fully, then we still can have vCPU-less nodes because your code would be NOP, right? > > It'll be a NOP because the sum of CPUs in the NUMA topology would be equal to the > maxcpus declared in <vcpus> > > Now, for the new use case you're going to introduce, you'll need to either > (1) forbid incomplete NUMA nodes entirely for this case or (2) check how QEMU > fills in the vcpus in this scenario. > > For (2) my brutal uneducated guess is that the behavior would be similar, but populating > the first non-cpuless NUMA node (which wouldn't be necessarily node0). This can be > arranged by creating a function that returns whether a node is cpu-less and using the > first non-cpuless cpu in the qemuDomainDefNumaCPUsRectify() function (patch 2) instead > of node0. You'll want to check it with QEMU first (Igor Mammedov perhaps?) to ensure > that this is what QEMU would do in these cases. > > TBH I believe that cpu-less NUMA nodes is quite an advanced feature and (1) is > a good approach for that, specially because there is no existing guests in the > wild that would be impacted by this restriction since Libvirt does not support > it yet. Yes, for qemu 2.7+ in qemuValidateDomainDef() if topology is specified then we require full vCPU to NUMA assignment. Well, we warn users if it is not the case (see QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS check and code around). Anyways, as I said your code is okay (I'm fixing couple of small nits) and pushing.

Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt; Do you think it's worth documenting in the release notes too?

Michal