| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-11 | |||
| 13:35:23 | mriedem | is dan doing reviews on the other patches later up the stack? | |
| 13:35:38 | mriedem | just wait for me to hit https://review.opendev.org/#/c/640021/50 again quick | |
| 13:36:55 | artom | Ack | |
| 13:37:40 | openstackgerrit | sean mooney proposed openstack/nova master: [DNM] pmem pull request test. https://review.opendev.org/681474 | |
| 13:37:40 | efried | stephenfin, sean-k-mooney, luyao: here's spec discussion about numa stuff https://review.opendev.org/#/c/601596/16/specs/train/approved/virtual-persistent-memory.rst@270 | |
| 13:38:50 | efried | and the one below it | |
| 13:39:10 | sean-k-mooney | efried: yes they have implemented what the spec said but its still problemenatic | |
| 13:39:34 | efried | but you agreed it was okay to be consistent with the other problematic things that do the same? | |
| 13:39:44 | sean-k-mooney | it is not consitent4 | |
| 13:39:50 | stephenfin | yeah, it's not consistent | |
| 13:39:54 | stephenfin | that's the bit I don't like | |
| 13:39:59 | stephenfin | at all at all | |
| 13:40:00 | sean-k-mooney | the other thing actully pin the cores and memoyr to a host numa node | |
| 13:40:23 | efried | oh, so it's bad in a different way than the other things are bad | |
| 13:40:31 | efried | and we'd rather have it be consistently bad. | |
| 13:40:35 | sean-k-mooney | the other things imporve performance | |
| 13:40:51 | efried | but we're agreed it's too hard to have it be good (and inconsistent) until "later". | |
| 13:40:52 | sean-k-mooney | this does not and it make the other code more complex | |
| 13:41:26 | sean-k-mooney | efried: it hink we can get the same beahvior if we jsut dont set the node elemnt in the xml | |
| 13:41:34 | stephenfin | efried: I'm not sure what you mean | |
| 13:41:36 | sean-k-mooney | and dont create a numa toplogy at all | |
| 13:41:54 | stephenfin | we're saying it should either not have a NUMA topology, if that's possible (which sean-k-mooney thinks it is) | |
| 13:42:09 | stephenfin | and if it has a NUMA topology, it should behave like everything else with NUMA topologies | |
| 13:43:13 | stephenfin | namely, each guest NUMA node is mapped to a unique host NUMA node | |
| 13:43:49 | sean-k-mooney | im going to quickly update my multi numa test and start stacking the latest version of atroms code then manully try to test if a numa toplogy is need for pmem | |
| 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 | - So do it the same bad as the "other resources" for now | |
| 13:44:33 | efried | - Doing it "good" is hard | |
| 13:44:33 | efried | - The way pmem is doing it in the current PS is bad, but different bad from ^ | |
| 13:44:33 | efried | - The way other resources "do it" is bad | |
| 13:44:33 | efried | I don't understand the issue, so I'm just operating at a very abstract level. My understanding: | |
| 13:44:34 | efried | - ...unless we can do it good now, easily. | |
| 13:44:34 | efried | - And some time in the future, make all the things do it good. | |
| 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 | stephenfin | there's a very good reason we do that | |
| 13:47:02 | sean-k-mooney | i just wish it was less copled | |
| 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 | dtroyer | o/ | |
| 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 | efried | o/ dtroyer | |
| 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 | dtroyer | yeah, the filter needs cleaning up, its pretty open now | |
| 14:04:41 | mriedem | artom: so might as well leave them | |