Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new parameter to support changing backend mem path #3666

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

fbq815
Copy link
Contributor

@fbq815 fbq815 commented Apr 18, 2023

Memory: Add a new parameter "vm_mem_backend_path", this will controls
the "mem-path" in memory object

ID: 2187577

Signed-off-by: Boqiao Fu [email protected]

@fbq815 fbq815 changed the title Decoupling memory-backend and numa/hugepage for virtiofs Decoupling memory-backend and numa/hugepage Apr 18, 2023
@fbq815 fbq815 changed the title Decoupling memory-backend and numa/hugepage Decoupling memory-backend and numa/hugepage for the default memory backend Apr 18, 2023
@fbq815
Copy link
Contributor Author

fbq815 commented Apr 18, 2023

@PaulYuuu @YongxueHong @xiagao @zhenyzha could you please help me review this patch

virttest/qemu_vm.py Outdated Show resolved Hide resolved
@PaulYuuu
Copy link
Contributor

In brief, this PR is not related to numa/hugepage, it fixes the bug when vm_mem_backend = memory-backend-file but cannot set the mem-path without hugepage.

@fbq815 fbq815 changed the title Decoupling memory-backend and numa/hugepage for the default memory backend Decoupling memory-backend and hugepage Apr 19, 2023
@fbq815
Copy link
Contributor Author

fbq815 commented Apr 19, 2023

In brief, this PR is not related to numa/hugepage, it fixes the bug when vm_mem_backend = memory-backend-file but cannot set the mem-path without hugepage.

Updated

@PaulYuuu
Copy link
Contributor

Hello @mcasquer, would you also help to review? This is a fix that vm_mem_backend is memory-backend-file but cannot set mem-path if we don't want to use hugepage_path.

@mcasquer
Copy link
Contributor

Hello @mcasquer, would you also help to review? This is a fix that vm_mem_backend is memory-backend-file but cannot set mem-path if we don't want to use hugepage_path.

Hello @PaulYuuu I understand this patch is meant to enable the possibility of setting a mem-path that could be no hugepage path, right? In that case seems correct, but just one thing, is LOG.error("expect mem-path for %s", mem_backend) enough to finish the related cases as ERROR?

@PaulYuuu
Copy link
Contributor

Hello @PaulYuuu I understand this patch is meant to enable the possibility of setting a mem-path that could be no hugepage path, right? In that case seems correct, but just one thing, is LOG.error("expect mem-path for %s", mem_backend) enough to finish the related cases as ERROR?

@fbq815 can you explain your code design?

@fbq815
Copy link
Contributor Author

fbq815 commented Apr 20, 2023

Hello @mcasquer, would you also help to review? This is a fix that vm_mem_backend is memory-backend-file but cannot set mem-path if we don't want to use hugepage_path.

Hello @PaulYuuu I understand this patch is meant to enable the possibility of setting a mem-path that could be no hugepage path, right? In that case seems correct, but just one thing, is LOG.error("expect mem-path for %s", mem_backend) enough to finish the related cases as ERROR?

seems we don't use test.fail in avocado-vt before, thus I keep the same behaviour in this patch and we could stop the test case in tp-qemu. Also, if there's negative test in the future, it won't be influenced by the error but fail would.

@mcasquer
Copy link
Contributor

seems we don't use test.fail in avocado-vt before, thus I keep the same behaviour in this patch and we could stop the test case in tp-qemu. Also, if there's negative test in the future, it won't be influenced by the error but fail would.

@fbq815 Got your point, I will run some hugepage cases later using this patch and update here the results

@fbq815
Copy link
Contributor Author

fbq815 commented Apr 20, 2023

Test result on s390x (after removing extra memory-backend, could run this case with expected backend):
From 1 tests executed, 1 passed and 0 warned - success rate of 100.00%

@mcasquer
Copy link
Contributor

After applying the current patch hugepage auto cases pass as expected

 (1/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.q35: STARTED
 (1/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.q35: PASS (109.19 s)
 (2/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.non_existent_mem_path.q35: STARTED
 (2/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.non_existent_mem_path.q35: PASS (109.89 s)
 (3/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_reset.q35: STARTED
 (3/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_reset.q35: SKIP: The current hugepage size on host does not match the expected hugepage size.
 (4/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_reset.on_numa_node.q35: STARTED
 (4/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_reset.on_numa_node.q35: SKIP: The current hugepage size on host does not match the expected hugepage size.
RESULTS    : PASS 2 | ERROR 0 | FAIL 0 | SKIP 2 | WARN 0 | INTERRUPT 0 | CANCEL 0

Doing some manual testing, I take hugepage_mem_stress.non_existent_mem_path case and if setup_hugepages is kept it always sets the mem-path to /mnt/kvm_hugepage even with vm_mem_mem-path and vm_mem_backend included.
If setup_hugepages is removed, then case fails as there is no mem-path defined.

[stdlog]     -object '{"size": 16106127360, "id": "mem-machine_mem", "qom-type": "memory-backend-file"}'  \
(status: 1,    output: 'qemu-kvm: -object {"size": 16106127360, "id": "mem-machine_mem", "qom-type": "memory-backend-file"}: Parameter \'mem-path\' is missing')

@fbq815 is this latter case supposed to work with this modifications? IIUC this patch is meant to use a non-hugepage mem-path, thanks!

@fbq815
Copy link
Contributor Author

fbq815 commented Apr 21, 2023

After applying the current patch hugepage auto cases pass as expected

 (1/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.q35: STARTED
 (1/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.q35: PASS (109.19 s)
 (2/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.non_existent_mem_path.q35: STARTED
 (2/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.non_existent_mem_path.q35: PASS (109.89 s)
 (3/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_reset.q35: STARTED
 (3/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_reset.q35: SKIP: The current hugepage size on host does not match the expected hugepage size.
 (4/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_reset.on_numa_node.q35: STARTED
 (4/4) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_reset.on_numa_node.q35: SKIP: The current hugepage size on host does not match the expected hugepage size.
RESULTS    : PASS 2 | ERROR 0 | FAIL 0 | SKIP 2 | WARN 0 | INTERRUPT 0 | CANCEL 0

Doing some manual testing, I take hugepage_mem_stress.non_existent_mem_path case and if setup_hugepages is kept it always sets the mem-path to /mnt/kvm_hugepage even with vm_mem_mem-path and vm_mem_backend included. If setup_hugepages is removed, then case fails as there is no mem-path defined.

[stdlog]     -object '{"size": 16106127360, "id": "mem-machine_mem", "qom-type": "memory-backend-file"}'  \
(status: 1,    output: 'qemu-kvm: -object {"size": 16106127360, "id": "mem-machine_mem", "qom-type": "memory-backend-file"}: Parameter \'mem-path\' is missing')

@fbq815 is this latter case supposed to work with this modifications? IIUC this patch is meant to use a non-hugepage mem-path, thanks!

hi @mcasquer you could use "vm_mem_mem-path" to set the mem_path, then you won't need the hugepage anymore

@mcasquer
Copy link
Contributor

hi @mcasquer you could use "vm_mem_mem-path" to set the mem_path, then you won't need the hugepage anymore

@fbq815 I am already using it

    variants:
        - @default:
        - non_existent_mem_path:
            non_existent_point = yes
            vm_mem_mem-path = "/tmp/mario"
            pre_command = "mkdir -p ${vm_mem_mem-path}"
            post_command = "rm -rf ${vm_mem_mem-path}"

@fbq815
Copy link
Contributor Author

fbq815 commented May 9, 2023

hi @mcasquer you could use "vm_mem_mem-path" to set the mem_path, then you won't need the hugepage anymore

@fbq815 I am already using it

    variants:
        - @default:
        - non_existent_mem_path:
            non_existent_point = yes
            vm_mem_mem-path = "/tmp/mario"
            pre_command = "mkdir -p ${vm_mem_mem-path}"
            post_command = "rm -rf ${vm_mem_mem-path}"

Hi, sorry for the late reply, you need to remove setup_hugepages =yes and add vm_mem_backend = /tmp/mario, vm_mem_mem-path is used for select the mem-path when there are multiple choices

@fbq815 fbq815 force-pushed the 2187577 branch 2 times, most recently from 8a4ca4a to db6adee Compare May 10, 2023 07:29
@fbq815
Copy link
Contributor Author

fbq815 commented May 10, 2023

@mcasquer I could get a PASS without setup_hugepages =yes
JOB ID : c95d46c720ea4822afb7a2e517bbaee34cb12f69
JOB LOG : /root/avocado/job-results/job-2023-05-10T03.27-c95d46c/job.log
(1/1) Host_RHEL.m8.u9.nographic.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.8.9.0.s390x.io-github-autotest-qemu.hugepage_mem_stress.non_existent_mem_path.s390-virtio: STARTED
(1/1) Host_RHEL.m8.u9.nographic.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.8.9.0.s390x.io-github-autotest-qemu.hugepage_mem_stress.non_existent_mem_path.s390-virtio: PASS (77.91 s)
RESULTS : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB HTML : /root/avocado/job-results/job-2023-05-10T03.27-c95d46c/results.html
JOB TIME : 79.18 s

@mcasquer
Copy link
Contributor

@fbq815 case pass successfully using the new vm_mem_mem-path parameter

 (1/2) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.q35: STARTED
 (1/2) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.q35: PASS (132.58 s)
 (2/2) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.non_existent_mem_path.q35: STARTED
 (2/2) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.3.0.x86_64.io-github-autotest-qemu.hugepage_mem_stress.non_existent_mem_path.q35: PASS (155.43 s)
RESULTS    : PASS 2 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

The mem-path is setted properly in automation

[stdlog]     -object '{"size": 65498251264, "mem-path": "/tmp/mario", "id": "mem-machine_mem", "qom-type": "memory-backend-file"}'  \

Copy link
Contributor

@mcasquer mcasquer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@PaulYuuu PaulYuuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

virttest/qemu_vm.py Outdated Show resolved Hide resolved
@fbq815
Copy link
Contributor Author

fbq815 commented May 11, 2023

@YongxueHong could you please help me merge this patch?

@YongxueHong
Copy link
Contributor

Hi @yanan-fu
Would you like to help review it?
Thanks.
CC @luckyh

@fbq815 fbq815 force-pushed the 2187577 branch 2 times, most recently from 183b6a2 to 91506a7 Compare June 30, 2023 10:06
virttest/qemu_vm.py Outdated Show resolved Hide resolved
virttest/qemu_vm.py Outdated Show resolved Hide resolved
virttest/qemu_vm.py Outdated Show resolved Hide resolved
virttest/shared/cfg/base.cfg Outdated Show resolved Hide resolved
virttest/shared/cfg/base.cfg Outdated Show resolved Hide resolved
@fbq815 fbq815 force-pushed the 2187577 branch 3 times, most recently from 8253461 to 2b118f0 Compare July 13, 2023 09:32
@fbq815 fbq815 changed the title Decoupling memory-backend and hugepage Add new parameter to support changing backend mem path Jul 13, 2023
@fbq815
Copy link
Contributor Author

fbq815 commented Jul 18, 2023

@PaulYuuu @yanan-fu @YongxueHong please help me review this patch again

Copy link
Contributor

@YongxueHong YongxueHong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor

@yanan-fu yanan-fu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acked-by: Yanan Fu [email protected]

Memory: Add a new parameter "vm_mem_backend_path", this will controls
the "mem-path" in memory object

Signed-off-by: Boqiao Fu <[email protected]>
@YongxueHong
Copy link
Contributor

CC @xiagao
FYI

@YongxueHong
Copy link
Contributor

Hi @PaulYuuu @mcasquer
Do you have any comments on the latest changes?
Thanks.

@mcasquer
Copy link
Contributor

Hi @PaulYuuu @mcasquer Do you have any comments on the latest changes? Thanks.

After applying this patch, I am trying to run the hugepage_mem_stress test case including in the cfg:

vm_mem_backend = "memory-backend-file"
vm_mem_backend_path = /tmp/mario
pre_command = "mkdir -p ${vm_mem_backend_path}"
post_command = "rm -rf ${vm_mem_backend_path}"

And removing:

setup_hugepages = yes
hugepage_path = /mnt/tmp

The test suite cannot be created
FYI @fbq815

@fbq815
Copy link
Contributor Author

fbq815 commented Jul 27, 2023

hugepage_mem_stress

The test suite cannot be created even I didn't apply this patch on s390x, I don't think the root cause is this patch

@zhenyzha
Copy link
Contributor

zhenyzha commented Aug 9, 2023

@mcasquer @fbq815
Hi guys, about hugepage_mem_stress , is there a conclusion now?

@mcasquer
Copy link
Contributor

mcasquer commented Aug 9, 2023

@mcasquer @fbq815 Hi guys, about hugepage_mem_stress , is there a conclusion now?

@zhenyzha I will give a try with virtio_fs_readonly and provide the results

If at this point the patch is ok, we can compare what's missing for hugepage_mem_stress

@mcasquer
Copy link
Contributor

@fbq815 virtio_fs_readonly test case fails after applying this patch and updating the cfg file you provided.

 (1/1) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.8.9.0.x86_64.io-github-autotest-qemu.virtio_fs_readonly.q35: STARTED
 (1/1) Host_RHEL.m9.u3.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.8.9.0.x86_64.io-github-autotest-qemu.virtio_fs_readonly.q35: ERROR: VM creation command failed:    'MALLOC_PERTURB_=1  /usr/libexec/qemu-kvm -S  -name \'avocado-vt-vm1\'  -sandbox on  -blockdev \'{"node-name": "file_ovmf_code", "driver": "file", "filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd", "auto-read-only": true, "... (10.09 s)

The message from debug.log

(status: 1,    output: 'qemu-kvm: -numa node,memdev=mem-mem1,nodeid=0: memdev=mem-mem1 is ambiguous')

The root cause is that there is no memory object so the numa node reference is ambiguous, i.e. -numa node,memdev=mem-mem1,nodeid=0 \

@mcasquer
Copy link
Contributor

After trying again following @fbq815 suggestion, the qemu-kvm cmd line seems to be correct:

/usr/libexec/qemu-kvm \
...
-object memory-backend-file,size=4G,mem-path=/dev/shm,share=yes,id=mem-mem1 \
...

I am facing the following error in the test case execution, but it seems it is not related with the patch

2023-08-11 07:02:51,601 avocado.virttest.virt_vm ERROR| Console is not responsive.

Copy link
Contributor

@mcasquer mcasquer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@YongxueHong
Copy link
Contributor

Thanks all, let's merge it.

@YongxueHong YongxueHong merged commit fe0abdc into avocado-framework:master Aug 15, 2023
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants