Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
On Thu, 2020-12-03 at 17:04 -0300, Daniel Henrique Barboza wrote: > On 12/3/20 11:37 AM, Andrea Bolognani wrote: > > This is where I'm a bit confused. IIUC the new value for , > > 1572992 KiB, is exactly 1 GiB (initial NUMA memory) + 512 MiB (NVDIMM > > guest area size) + 128 KiB (NVDIMM label size). Is that the value we > > expect users to see in the XML? If the label size were not there I > > would certainly say yes, but those extra 128 KiB make me pause. Then > > again, the for the element also > > includes the label size, so perhaps it's all good? I just want to > > make sure it is intentional :) > > This is intentional. The target_size of the NVDIMM must contain the > size of the guest visible area (256MB aligned) plus the label_size. > > > The last bit of confusion is given by the fact that the > > element is not updated along with the > > element. How will that work? Do I understand correctly that the guest > > will actually get the full size, but if a memory balloon is > > also present then the difference between and > > will be (temporarily) returned to the host using that mechanism? > > Yes. is the max amount of memory the guest can have at boot > time. For our case (pSeries) it consists of the base RAM + space for > the DMA window for VFIO devices and PHBs and hotplug. This is what is > being directly impacted by patch 06 and this series as a whole. > > is represented by our internal value of def->mem.cur_balloon. > If there is a balloon device then follows the lead > of the device. If there is no RAM ballooning, > def->mem.cur_balloon = = . Thank you for your explanation! It all sounds good :) -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
On 12/3/20 11:37 AM, Andrea Bolognani wrote: On Wed, 2020-12-02 at 15:08 +0100, Andrea Bolognani wrote: On Wed, 2020-12-02 at 09:14 -0300, Daniel Henrique Barboza wrote: Andrea/Peter, do you want to take a look again to see if there's something that I missed? Yeah, sorry for not being very responsive, and thanks a lot Michal for picking up the slack! I'll try to give it at least a quick look by the end of the day. Sorry I didn't managed to get back to you yesterday but, given that we managed to get this at least partially wrong in every previous iteration, you'll hopefully forgive me for being perhaps a bit overcautious O:-) Always good to try to minimize error :) As mentioned elsewhere, in the process of trying to convince myself that these changes are in fact correct I found it useful be able to make a direct comparison between the ABI_UPDATE case and the vanilla one, and to facilitate that I've produced a couple of simple patches[1] that I'm hoping you'll agree to rebase your work on top of. I have actually already done that locally, so feel free to simply pick up the corresponding branch[2]. That's what I'll end up doing. I'll probably push patches 1, 2 and 4 first, since they got the R-bs and aren't affected by this rebase, then I'll make more adjustments based on your review and post a v3. Anyway, assuming you're working from that branch, here are the differences that are introduced by using ABI_UPDATE: $ diff -Naru tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml --- tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml 2020-12-03 14:19:21.486145577 +0100 +++ tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml 2020-12-03 14:57:09.857621727 +0100 @@ -24,13 +24,13 @@ -523264 +524288 -524287 +524288 $ diff -Naru tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args --- tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args 2020-12-03 14:19:21.486145577 +0100 +++ tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args 2020-12-03 14:57:09.857621727 +0100 @@ -11,7 +11,7 @@ -name QEMUGuest1 \ -S \ -machine pseries,accel=kvm,usb=off,dump-guest-core=off \ --m size=1310720k,slots=16,maxmem=4194304k \ +-m size=1048576k,slots=16,maxmem=4194304k \ -realtime mlock=off \ -smp 1,sockets=1,cores=1,threads=1 \ -object memory-backend-ram,id=memdimm0,size=536870912 \ $ You explain very well the command line change in the commit message for patch 6/6, and the output XML now reflects the aligned size for DIMMs that was used on the command line even before your changes, so this all looks good. $ diff -Naru tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml --- tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml 2020-12-03 14:57:09.857621727 +0100 +++ tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml 2020-12-03 14:57:09.857621727 +0100 @@ -2,7 +2,7 @@ QEMUGuest1 c7a5fdbd-edaf-9455-926a-d65c16db1809 1099511627776 - 1267710 + 1572992 1267710 2 @@ -34,7 +34,7 @@ /tmp/nvdimm -55 +524416 0 128 $ diff -Naru tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.args $ This is where I'm a bit confused. IIUC the new value for , 1572992 KiB, is exactly 1 GiB (initial NUMA memory) + 512 MiB (NVDIMM guest area size) + 128 KiB (NVDIMM label size). Is that the value we expect users to see in the XML? If the label size were not there I would certainly say yes, but those extra 128 KiB make me pause. Then again, the for the element also includes the label size, so perhaps it's all good? I just want to make sure it is intentional :) This is intentional. The target_size of the NVDIMM must contain the size of the guest visible area (256MB aligned) plus the label_size. The last bit of confusion is given by the fact that the element is not updated along with the element. How will that work? Do I understand correctly that the guest will actually get the full size, but if a memory balloon is also present then the difference between and will be (temporarily) returned to the host using that mechanism? Yes. is the max amount of memory the guest can have at boot time. For our case (pSeries) it consists of the base RAM + space for the DMA window for VFIO devices and PHBs and hotplug. This is what is being directly impacted by patch 06 and this series as a whole. is represented
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
On Wed, 2020-12-02 at 15:08 +0100, Andrea Bolognani wrote: > On Wed, 2020-12-02 at 09:14 -0300, Daniel Henrique Barboza wrote: > > Andrea/Peter, do you want to take a look again to see if there's > > something that I missed? > > Yeah, sorry for not being very responsive, and thanks a lot Michal > for picking up the slack! I'll try to give it at least a quick look > by the end of the day. Sorry I didn't managed to get back to you yesterday but, given that we managed to get this at least partially wrong in every previous iteration, you'll hopefully forgive me for being perhaps a bit overcautious O:-) As mentioned elsewhere, in the process of trying to convince myself that these changes are in fact correct I found it useful be able to make a direct comparison between the ABI_UPDATE case and the vanilla one, and to facilitate that I've produced a couple of simple patches[1] that I'm hoping you'll agree to rebase your work on top of. I have actually already done that locally, so feel free to simply pick up the corresponding branch[2]. Anyway, assuming you're working from that branch, here are the differences that are introduced by using ABI_UPDATE: $ diff -Naru tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml --- tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml 2020-12-03 14:19:21.486145577 +0100 +++ tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml 2020-12-03 14:57:09.857621727 +0100 @@ -24,13 +24,13 @@ -523264 +524288 -524287 +524288 $ diff -Naru tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args --- tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args 2020-12-03 14:19:21.486145577 +0100 +++ tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args 2020-12-03 14:57:09.857621727 +0100 @@ -11,7 +11,7 @@ -name QEMUGuest1 \ -S \ -machine pseries,accel=kvm,usb=off,dump-guest-core=off \ --m size=1310720k,slots=16,maxmem=4194304k \ +-m size=1048576k,slots=16,maxmem=4194304k \ -realtime mlock=off \ -smp 1,sockets=1,cores=1,threads=1 \ -object memory-backend-ram,id=memdimm0,size=536870912 \ $ You explain very well the command line change in the commit message for patch 6/6, and the output XML now reflects the aligned size for DIMMs that was used on the command line even before your changes, so this all looks good. $ diff -Naru tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml --- tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml 2020-12-03 14:57:09.857621727 +0100 +++ tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml 2020-12-03 14:57:09.857621727 +0100 @@ -2,7 +2,7 @@ QEMUGuest1 c7a5fdbd-edaf-9455-926a-d65c16db1809 1099511627776 - 1267710 + 1572992 1267710 2 @@ -34,7 +34,7 @@ /tmp/nvdimm -55 +524416 0 128 $ diff -Naru tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.args $ This is where I'm a bit confused. IIUC the new value for , 1572992 KiB, is exactly 1 GiB (initial NUMA memory) + 512 MiB (NVDIMM guest area size) + 128 KiB (NVDIMM label size). Is that the value we expect users to see in the XML? If the label size were not there I would certainly say yes, but those extra 128 KiB make me pause. Then again, the for the element also includes the label size, so perhaps it's all good? I just want to make sure it is intentional :) The last bit of confusion is given by the fact that the element is not updated along with the element. How will that work? Do I understand correctly that the guest will actually get the full size, but if a memory balloon is also present then the difference between and will be (temporarily) returned to the host using that mechanism? Sorry again for all the questions and for delaying your work. I'm just trying to make sure we don't have to come back to it again in a couple of months O:-) [1] https://www.redhat.com/archives/libvir-list/2020-December/msg00244.html [2] https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
On Wed, 2020-12-02 at 09:14 -0300, Daniel Henrique Barboza wrote: > On 12/2/20 4:17 AM, Michal Privoznik wrote: > > On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote: > > > Daniel Henrique Barboza (6): > > >qemu_process.c: check migrateURI when setting > > > VIR_QEMU_PROCESS_START_NEW > > >qemu: move memory size align to qemuProcessPrepareDomain() > > >Revert "domain_conf.c: auto-align pSeries NVDIMM in > > > virDomainMemoryDefPostParse()" > > >qemuxml2xmltest.c: honor ARG_PARSEFLAGS > > >qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE > > >qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE > > > > > Reviewed-by: Michal Privoznik > > Thanks for the review! > > Andrea/Peter, do you want to take a look again to see if there's > something that I missed? Yeah, sorry for not being very responsive, and thanks a lot Michal for picking up the slack! I'll try to give it at least a quick look by the end of the day. -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
On 12/2/20 4:17 AM, Michal Privoznik wrote: On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote: Hi, This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work. Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed. Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done. Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed. Test cases were added to help me diagnose and assert what I was changing and what would remain untouched. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE src/conf/domain_conf.c | 23 + src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c | 39 ++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 + ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 + tests/qemuxml2xmltest.c | 20 +++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml Reviewed-by: Michal Privoznik Thanks for the review! Andrea/Peter, do you want to take a look again to see if there's something that I missed? DHB Michal
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote: Hi, This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work. Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed. Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done. Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed. Test cases were added to help me diagnose and assert what I was changing and what would remain untouched. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE src/conf/domain_conf.c| 23 + src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c| 39 ++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 + ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 + tests/qemuxml2xmltest.c | 20 +++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml Reviewed-by: Michal Privoznik Michal
Re: [PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
Ping On 11/18/20 4:58 PM, Daniel Henrique Barboza wrote: Hi, This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work. Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed. Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done. Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed. Test cases were added to help me diagnose and assert what I was changing and what would remain untouched. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE src/conf/domain_conf.c| 23 + src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c| 39 ++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 + ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 + tests/qemuxml2xmltest.c | 20 +++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml
