| Message ID | 20200507173958.25894-1-philmd@redhat.com |
|---|---|
| Headers | show |
| Series | exec: Shear 'exec/ram_addr.h' and make NVMe device target-agnostic | expand |
On 07/05/20 19:39, Philippe Mathieu-Daudé wrote: > Stefan suggested to make qemu_ram_writeback() target agnostic, > Paolo to add memory_region_msync(), and Peter to remove > "exec/ram_addr.h" [*]. > > I let a single function in this file, > cpu_physical_memory_sync_dirty_bitmap(), to let the maintainer > have the pleasure to remove this header definitively himself :) I don't think this is a good idea. :) "exec/ram_addr.h" is a good place for functions that work on ram-addr_t and/or RAMBlock data. There should very few of these, since these are mostly an internal concept that should only be used for live migration. You could: - figure out which files actually need to include exec/ram_addr.h. There's already very few of them. - move the large functions to a new .c file, ramblock.c. Figure out which can be static, move the declarations for the others to ramblock.h - kill ram_addr.h and include ramblock.h instead. Not coincidentially, qemu_ram_writeback() takes a RAMBlock*, and it ends up in ramblock.h. Also, this is orthogonal to adding the wrapper memory_region_msync. Thanks, Paolo
Patchew URL: https://patchew.org/QEMU/20200507173958.25894-1-philmd@redhat.com/
Hi,
This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
TEST check-qtest-aarch64: tests/qtest/arm-cpu-features
Broken pipe
/tmp/qemu-test/src/tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
ERROR - too few tests run (expected 5, got 0)
make: *** [check-qtest-aarch64] Error 1
make: *** Waiting for unfinished jobs....
TEST check-unit: tests/check-block-qdict
TEST check-unit: tests/test-char
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=aa05160ac8ef4a8aabb028db62e978dc', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-sj3y_tsg/src/docker-src.2020-05-08-02.05.43.23802:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=aa05160ac8ef4a8aabb028db62e978dc
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-sj3y_tsg/src'
make: *** [docker-run-test-quick@centos7] Error 2
real 14m12.181s
user 0m9.451s
The full log is available at
http://patchew.org/logs/20200507173958.25894-1-philmd@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Paolo Bonzini <pbonzini@redhat.com> wrote: > On 07/05/20 19:39, Philippe Mathieu-Daudé wrote: >> Stefan suggested to make qemu_ram_writeback() target agnostic, >> Paolo to add memory_region_msync(), and Peter to remove >> "exec/ram_addr.h" [*]. >> >> I let a single function in this file, >> cpu_physical_memory_sync_dirty_bitmap(), to let the maintainer >> have the pleasure to remove this header definitively himself :) > > I don't think this is a good idea. :) > > "exec/ram_addr.h" is a good place for functions that work on ram-addr_t > and/or RAMBlock data. There should very few of these, since these are > mostly an internal concept that should only be used for live migration. > You could: > > - figure out which files actually need to include exec/ram_addr.h. > There's already very few of them. ram_addr.h looks really "not dangerous", I think that I preffer the memory-internal.h or whatever name that implies that you should think twice before using that file. My main problem with that include are: - cpu_physical_memory_set_dirty_lebitmap() - cpu_physical_memory_sync_dirty_bitmap() Both are long, both are complex, and if one changes them, it is very probably that you end breaking some random architecture in TCG (being there, done that). As you said, that functions are used only in a couple of places. I haven't meassured the impact of moving it to a .c file, but I would preffer it if performance don't suffer. > > - move the large functions to a new .c file, ramblock.c. Figure out > which can be static, move the declarations for the others to ramblock.h Ok, it appears that we kind of agree. > - kill ram_addr.h and include ramblock.h instead. I created ramblock.h initially (/me checks) because if you just need to walk a ramblock (that is clearly target independent code), you needed to become target dependent, due to the definitions that are there inside. I don't care one way or another, just that we don't create the old dependency. > Not coincidentially, qemu_ram_writeback() takes a RAMBlock*, and it ends > up in ramblock.h. Thanks, Juan.