| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-11 | |||
| 13:43:50 | stephenfin | if we want to loosen that constraint in the future, grand, but it should be done across the board (so hugepages too) | |
| 13:44:21 | luyao | sean-k-mooney: http://paste.openstack.org/show/775121/ | |
| 13:44:33 | efried | I don't understand the issue, so I'm just operating at a very abstract level. My understanding: | |
| 13:44:33 | efried | - The way other resources "do it" is bad | |
| 13:44:33 | efried | - The way pmem is doing it in the current PS is bad, but different bad from ^ | |
| 13:44:33 | efried | - Doing it "good" is hard | |
| 13:44:33 | efried | - So do it the same bad as the "other resources" for now | |
| 13:44:34 | efried | - And some time in the future, make all the things do it good. | |
| 13:44:34 | efried | - ...unless we can do it good now, easily. | |
| 13:44:58 | sean-k-mooney | luyao: remove the numa element on line 7-9 | |
| 13:45:22 | sean-k-mooney | if you have numa element in the cpu element i think you need node to be se | |
| 13:45:26 | sean-k-mooney | *set | |
| 13:45:32 | sean-k-mooney | if you dont you should not | |
| 13:46:13 | sean-k-mooney | efried: by they i dont think doing it "good" was hard but let not dwell on that | |
| 13:46:16 | stephenfin | luyao: Yeah, you're hitting this https://github.com/libvirt/libvirt/blob/master/src/qemu/qemu_domain.c#L11525-L11532 | |
| 13:46:41 | sean-k-mooney | efried: also the way the other numa toplogy stuff works is not bad | |
| 13:46:54 | stephenfin | - The way other resources "do it" is bad | |
| 13:46:56 | stephenfin | it's not bad | |
| 13:47:02 | sean-k-mooney | i just wish it was less copled | |
| 13:47:02 | stephenfin | there's a very good reason we do that | |
| 13:47:25 | efried | okay, this reinforces that I should not be involved with this part of the series. | |
| 13:47:36 | stephenfin | not at all | |
| 13:47:48 | stephenfin | just that we have strong opinions about this stuff ;) | |
| 13:47:52 | sean-k-mooney | efried: this is why we do it https://software.intel.com/en-us/articles/openstack-epa-feature-breakdown-and-analysis#inpage-nav-6-3 | |
| 13:48:31 | efried | sean-k-mooney: ^ meaning that even if there's only one NUMA node, turning NUMA "on" is better perf than leaving it "off"? | |
| 13:48:43 | sean-k-mooney | the numa affintiy on old systems gave you like 7-10% performacne improvement on its own | |
| 13:49:27 | sean-k-mooney | efried: yes because if the host has multiple numa nodes the vm wont use remote memory form the other numa node or float across them | |
| 13:49:43 | sean-k-mooney | if the host has one numa nod its has no effect | |
| 13:50:03 | efried | yahbut... | |
| 13:50:19 | efried | what if my VM needs more CPUs than are available in one NUMA node? | |
| 13:50:41 | sean-k-mooney | create a vm with 2 numa nodes | |
| 13:50:49 | efried | no, I mean... | |
| 13:50:52 | sean-k-mooney | it will perfome better | |
| 13:51:15 | efried | IIUC we're doing this thing to create an implicit single-NUMA guest if no NUMA topo was specified? To realize this perf gain. | |
| 13:51:16 | sean-k-mooney | i get the argument that people dont care | |
| 13:51:20 | sean-k-mooney | and i want ot support that | |
| 13:51:41 | sean-k-mooney | but i think we can support that by just no setteing lines 7-9 in http://paste.openstack.org/show/775121/ | |
| 13:51:56 | sean-k-mooney | efried: no we are not | |
| 13:52:02 | luyao | sean-k-mooney: got a error if i remove numa , Total size of memory devices exceeds the total memory size | |
| 13:52:07 | sean-k-mooney | the current code does not have the performace gain | |
| 13:52:16 | stephenfin | luyao: Can you paste? | |
| 13:52:46 | sean-k-mooney | i thikn we still need to reserv dim slots | |
| 13:52:50 | luyao | efried: Rui told me dean trove also has the permission to merge | |
| 13:53:05 | sean-k-mooney | <maxMemory slots='16' unit='KiB'>1524288</maxMemory> | |
| 13:53:29 | sean-k-mooney | maybe not | |
| 13:56:10 | luyao | sean-k-mooney, stephenfin: http://paste.openstack.org/show/775126/ | |
| 13:57:36 | mriedem | artom: https://review.opendev.org/#/c/640021/50 | |
| 13:57:50 | mriedem | dansmith: i think a couple of questions for you to opine on in there as well ^ | |
| 13:58:49 | efried | o/ dtroyer | |
| 13:58:49 | efried | sean-k-mooney, would you please explain to dtroyer what needs to happen in that CI repo to make it right? | |
| 13:58:49 | dtroyer | o/ | |
| 13:59:02 | sean-k-mooney | can you try one last thing can you sett <maxMemory slots="16" unit='GiB'>8</maxMemory> to inclnde the pmem but set <memory unit='KiB'>524288</memory> | |
| 13:59:04 | sean-k-mooney | <currentMemory unit='KiB'>524288</currentMemory> | |
| 13:59:11 | sean-k-mooney | to a lower value | |
| 14:00:02 | sean-k-mooney | if that does not work then in the sake of time i think we have to live with this for Train | |
| 14:00:09 | artom | mriedem, ack, looking | |
| 14:00:23 | sean-k-mooney | and in Ussuri we need to find a way to converge this somehow | |
| 14:02:06 | stephenfin | luyao: I'm assuming that's because you're setting <maxMemory> and not <memory> | |
| 14:02:19 | sean-k-mooney | yep | |
| 14:02:21 | stephenfin | You were able to omit that before because "In case NUMA is configured for the guest the memory element can be omitted" | |
| 14:02:25 | stephenfin | from https://libvirt.org/formatdomain.html#elementsMemoryAllocation | |
| 14:02:35 | stephenfin | sean-k-mooney: Cool. Thought that's what you were saying but I wasn't sure :) | |
| 14:02:38 | efried | dtroyer: the simple solution is removing this thing https://github.com/intel/Intel-OpenStack-CI-jobs/pull/1/commits/5d4a998a890fef3a2b0a46f73d26aa1deeb3cd8b | |
| 14:02:51 | artom | mriedem, about the qemu/kvm stuff - it's not "lower" that's the problem, is that the code used to check for "kvm", but then we realized that it should be checking for "qemu", and not all of the tests were changed | |
| 14:03:13 | sean-k-mooney | the max memory is the maium memory the geust can boot with or have hotplugged as addtional memeory | |
| 14:03:25 | artom | mriedem, the first one is harmless, but second one actually needs to change, because it was triggering a false positive by passing the hypervisor check before the thing we were actually testing | |
| 14:03:39 | sean-k-mooney | so we shoudl be setting that to guest ram + nvdim size | |
| 14:03:52 | sean-k-mooney | and the guest memory shoudl be set to the ram size in the flavor | |
| 14:04:08 | dtroyer | efried: would getting rid of the hard-coded ps13 work? | |
| 14:04:10 | efried | dtroyer: I think that's what we want long term. For now it would be nice if it only ran the pmem tests against patches that have at least https://review.opendev.org/#/c/679640/ involved -- otherwise it's running on everything and will therefore fail handily. | |
| 14:04:31 | efried | dtroyer: idk, does refs/changes/$gerritnum work without /$psnum ? | |
| 14:04:32 | sean-k-mooney | it would fail until its merged but it would work after that | |
| 14:04:36 | mriedem | artom: ok i guess i can buy that second one | |
| 14:04:41 | mriedem | artom: so might as well leave them | |
| 14:04:41 | dtroyer | yeah, the filter needs cleaning up, its pretty open now | |
| 14:05:10 | sean-k-mooney | dtroyer: i dont know if the ci is using the git driver or github dirver | |
| 14:05:13 | efried | dtroyer: I always use git review -d, never git fetch | |
| 14:05:28 | sean-k-mooney | dtroyer: if its using the github dirver then https://review.opendev.org/#/c/681474/2 should work | |
| 14:05:43 | sean-k-mooney | efried: same | |
| 14:06:03 | efried | sean-k-mooney: yeah, unless the CI env doesn't have git-review installed :P | |
| 14:06:16 | dtroyer | mee too… I approved that PR, we can iterate from there | |
| 14:06:18 | sean-k-mooney | efried: no it will work | |
| 14:06:41 | sean-k-mooney | this shoudl all be done via the zuul cloner | |
| 14:06:59 | sean-k-mooney | e.g. zuul should be preparing all the repos before the job starts | |
| 14:07:12 | sean-k-mooney | and then the jobs should use those prepreapred repos | |
| 14:07:27 | sean-k-mooney | so we shoudl not use git fetach or git review in the scripts | |
| 14:08:14 | sean-k-mooney | dtroyer: oh i actully didnt mean for it to be aproved since i think there was an internal one coming but i guess thats fine too | |
| 14:08:19 | luyao | sean-k-mooney: http://paste.openstack.org/show/775129/ | |
| 14:09:01 | dansmith | mriedem: just got off a call so I'll look at that deprecation patch in a minute | |
| 14:09:26 | dtroyer | sean-k-mooney: I think Rui is looking at this in the internal env, yes, I'm not sure where he is on that... | |
| 14:09:33 | dansmith | mriedem: considering how long things are taking to get through the gate, are you comfortable going ahead and letting the bottom three patches there go in or do you want to wait? | |
| 14:10:36 | alex_xu | dtroyer: the mroning, Rui and Dolpher talk with me we can remove the hardcode patch checkout and add a filter. Dolpher will do that when he wakeup I think. Dolpher in the US now | |
| 14:11:03 | stephenfin | luyao, sean-k-mooney: so that's triggering this https://github.com/libvirt/libvirt/blob/master/src/qemu/qemu_domain.c#L11604-L11615 | |
| 14:11:08 | dtroyer | alex_xu: ok, thanks, I'll check with him | |
| 14:11:15 | alex_xu | dtroyer: thanks | |
| 14:12:43 | luyao | sean-k-mooney: that's it! | |
| 14:13:17 | alex_xu | yea...that's it | |
| 14:13:19 | sean-k-mooney | this really isnt a hotplug but i guess they build pmem on that backend | |
| 14:13:43 | luyao | stephenfin: thanks | |
| 14:13:49 | stephenfin | so it looks like we need a guest NUMA topology, meaning we're back to "do we have to treat VPMEM different from hugepages/other things that imply a NUMA topology in nova" | |