Re: [PATCH v2 1/4] qemu.conf changes to support multiple memory backend

On Tue, Aug 06, 2024 at 03:45:54PM -0500, Michael Galaxy wrote: > Thank you so much for the detailed comments. I'll work on those. > > I believe you had a couple questions below on this one......inlined > ..... > > On 8/6/24 07:35, Martin Kletzander wrote: >> On Wed, Jun 05, 2024 at 04:37:35PM -0400, mgalaxy(a)akamai.com wrote: >>> From: Michael Galaxy <mgalaxy(a)akamai.com&gt; >>> >>> +                               _("Domain requesting configuration >>> for %1$lu NUMA nodes, but memory backing directory only has (%2$lu) >>> directory paths available. Either reduce this to one directory or >>> provide more paths to use."), >>> +                               numa_node_count, >>> + cfg->nb_memoryBackingDirs); >>> +                return -1; >>> +            } >>> + >>> +            path_index = virBitmapNextSetBit(numaBitmap, -1); >> >> What if the single pinning is to the third host node, but there are >> only >> two memoryBackingDirs supplied in the config? > > That case is totally supported (and expected). For example a *non* vNUMA > guests (a single virtual NUMA node) > running on a standard 2-socket (2-NUMA-node) host in a typical pizza > box. > > If I understood that correctly, that is supported without a problem. The > error above is describing the case that > a virtual machine has *more* virtual NUMA nodes configured than the host > has available to offer. In such a case, > either the user needs to provide more host nodes or turn this feature > off or use a single NUMA node (in which > case all the guest nodes would be placed in a single host node). > If the machine is only pinned to the third node and two memoryBackingDirs are supplied then path_index will be 2 and later accessing memoryBackingDirs[path_index] will crash.  Or maybe I have missed something.  That's why I asked since I'm unsure myself, I have not tested it.

>>> { >>> +    virQEMUDriver *driver = priv->driver; >>>     g_autofree char *path = NULL; >>> +    g_autoptr(virQEMUDriverConfig) cfg = >>> virQEMUDriverGetConfig(driver); >>> +    size_t i; >>> >>> -    if (qemuGetMemoryBackingPath(driver, vm->def, mem->info.alias, >>> &path) < 0) >>> -        return -1; >>> +    for (i = 0; i < cfg->nb_memoryBackingDirs; i++) { >>> +        virDomainXMLPrivateDataCallbacks *privateData = >>> (virDomainXMLPrivateDataCallbacks *) priv; >>> +        if (qemuGetMemoryBackingPath(def, privateData, i, >>> mem->info.alias, &path) < 0) >>> +            return -1; >>> >> >> I wonder why are the paths not saved in the status XML of the running >> domain.  Since they can be changed during the VM's lifetime (while >> restarting libvirt) it could cause issues during clean-up. Sure, >> unlink() will set errno to ENOENT, but the proper paths will not be >> cleaned up.  It'd also make this part of the code easier and safer to >> use from the callers.  But that's pre-existing. > > I'm not sure I'm following the question on this one. I do not think we > want the paths to be changing > during the VM's lifetime. Are you asking if we *want*  to support that? > The current > libvirt XML schema does not support this. The actual path is kept hidden > from the guest VM > and is only known internally to libvirt. > > Can you clarify? > Exactly, we do not want the paths to change.  But if you start the VM, then stop virtqemud/libvirtd (totally supported), change the config file to point to a different directory (or even different number of directories, I haven't even thought of that), then start the daemon back again, we'll have a problem when the VM is being cleaned up. For these kinds of situations we are keeping more pieces of information about running domains in a so called status XML.  This is saved on disk, but unavailable to the user so saving something there does not explicitly expose it to the user.  Having the paths saved there would make it nicer to clean up and it should've probably been done even with that one path that is supported nowadays.  I can have a look at fixing that at first and then your patches could be applied on top of that fix if that's fine with you or you can have a look at tackling that yourself, it should not be difficult, tests for that should be nice to write as well, it just needs to be done in a back-compat way, of course. See qemuDomainObjPrivateXMLFormat() and qemuDomainObjPrivateXMLParse() for some starting points.