On Thu, Oct 01, 2015 at 07:01:17PM -0400, John Ferlan wrote:
On 10/01/2015 08:10 AM, Martin Kletzander wrote:Of course this will be used only in case we don't need the memory-backend-file backend, so it should not fire until later.So if I'm reading things right... when building the numa string, if it's determine that "-object" and ",memdev=ram-node%zu" will be required, then it's also required to have "-mem-alloc -mem-path ..." (furrowing through the qemu docs was more difficult than I thought)
This was harder for me to grasp as well, I'll try to explain. If you use -object memory-backend-file, that object itself has a path where to use hugepages from. If you instead use '-numa ...,mem=SIZE', then there is no way to say where from can this particular node take the hugepages, but you can use '-mem-prealloc -mem-path /path/to/hugetlbfs/mount' to specify it for the whole domain. So you need '-mem-path' if hugepages backing is requested and there is no need for -object memory-backend-file'. I hope that's understandable, let me know if I should try explaining it more clearly.
Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> --- src/qemu/qemu_command.c | 4 ++++ 1 file changed, 4 insertions(+)Perhaps "outside" the domain of these changes; however, I note that the call to qemuBuildNumaArgStr is from qemuBuildCommandLine as follows: if (virDomainNumaGetNodeCount(def->numa)) { if (qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0) one of the first things done in qemuBuildNumaArgStr: size_t ncells = virDomainNumaGetNodeCount(def->numa); oh and of course the pesky const long system_page_size = virGetSystemPageSizeKB(); I mentioned previously BTW: Not noted by Coverity, but interestingly later in the code there's: if (ncells) { /* Fortunately, we allow only guest NUMA nodes to be continuous * starting from zero. */ pos = ncells - 1; } Which if I'm reading things correct - cannot happen.
I haven't noticed this, but the way I'm reading it is that qemuBuildNumaArgStr() is called only if there are some numa nodes, so this will happen *every* time qemuBuildNumaArgStr().
Just a note - you don't have to change/fix this, but well if you want to... ACK to what's here as it seems reasonable - perhaps a better reason in the commit message would help
I'll update the commit message, reading it after myself now I don't understand it either :)
Johndiff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 17e5cfd71702..321a5e931350 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8132,6 +8132,10 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, } } + if (!needBackend && + qemuBuildMemPathStr(cfg, def, qemuCaps, cmd) < 0) + goto cleanup; + for (i = 0; i < ncells; i++) { VIR_FREE(cpumask); if (!(cpumask = virBitmapFormat(virDomainNumaGetNodeCpumask(def->numa, i))))
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list