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

Hi, Answers below.... On 8/7/24 08:23, Martin Kletzander wrote: > 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. > Oh ok, now I understand the question. I don't believe I've tested that either. I'll see if libvirt lets the user do that or not and verify. Good catch. >>>> { >>>> +    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.