Hi Igor, Igor Mammedov, Oct 16, 2024 at 13:52: > On Wed, 22 May 2024 17:34:49 +0200 > Anthony Harivel <ahari...@redhat.com> wrote: > >> Dear maintainers, >> >> First of all, thank you very much for your review of my patch >> [1]. > > I've tried to play with this feature and have a few questions about it >
Thanks for testing this new feature. > 1. trying to start with non accessible or not existent socket > -accel kvm,rapl=on,rapl-helper-socket=/tmp/socket > I get: > qemu-system-x86_64: -accel kvm,rapl=on,rapl-helper-socket=/tmp/socks: > vmsr socket opening failed > qemu-system-x86_64: -accel kvm,rapl=on,rapl-helper-socket=/tmp/socks: > kvm : error RAPL feature requirement not met > * is it possible to report actual OS error that happened during > open/connect, > instead of unhelpful 'socket opening failed'? > > What I see in vmsr_open_socket() error is ignored > and btw it's error leak as well > Shame you missed the 6 iterations of that patch that last for a year. I would have changed that directly ! Anyway I take note on that comment and will send a modification. > * 2nd line shouldn't be there if the 1st error already present. > > 2. getting periodic error on console where QEMU has been starter > # ./qemu-vmsr-helper -k /tmp/sock > ./qemu-system-x86_64 -snapshot -m 4G -accel > kvm,rapl=on,rapl-helper-socket=/tmp/sock rhel90.img -vnc :0 -cpu host > and let it run > > it appears rdmsr works (well, it returns some values at least) > however there are recurring errors in qemu's stderr(or out) > > qemu-system-x86_64: Error opening /proc/2496093/task/2496109/stat > qemu-system-x86_64: Error opening /proc/2496093/task/2496095/stat > > My guess it's some temporary threads, that come and go, but still > they shouldn't cause errors if it's normal operation. > There a patch in WIP that change this into a Tracepoint. Maybe you can SSH to the VM in meanwhile ? > Also on daemon side, I a few times got while guest was running: > qemu-vmsr-helper: Failed to open /proc at /proc/2496026/task/2496044 > qemu-vmsr-helper: Requested TID not in peer PID: 2496026 2496044 > though I can't reproduce it reliably This could happen only when a vCPU thread ID has changed between the call of a rdmsr throught the socket and the hepler that read the msr. No idea how a vCPU can change TID or shutdown that fast. > > 3. when starting daemon not as root, it starts 'fine' but later on complains > qemu-vmsr-helper: Failed to open MSR file at /dev/cpu/0/msr > perhaps it would be better to fail at start daemon if it doesn't have > access to necessary files. > Right taking a note on that as well. > 4. in case #3, guest also fails to start with errors: > qemu-system-x86_64: -accel kvm,rapl=on,rapl-helper-socket=/tmp/sock: > can't read any virtual msr > qemu-system-x86_64: -accel kvm,rapl=on,rapl-helper-socket=/tmp/sock: > kvm : error RAPL feature requirement not met > again line #2 is not useful and probably not needed (maybe make it > tracepoint) > and #1 is unhelpful - it would be better if it directed user to check > qemu-vmsr-helper > I will try to see how to improve that part. Thanks for your valuable feedback. > 5. does AMD have similar MSRs that we could use to make this feature > complete? > Yes but the address are completely different. However, this in my ToDo list. First I need way more feedback like yours to move on extending this feature. > 6. What happens to power accounting if host constantly migrates > vcpus between sockets, are values we are getting still correct/meaningful? > Or do we need to pin vcpus to get 'accurate' values? > It's taken into account during the ratio calculation which socket the vCPU has just been scheduled. But yes the value are more 'accurate' when the vCPU is pinned. > 7. do we have to have a dedicated thread for pooling data from daemon? > > Can we fetch data from vcpu thread that have accessed msr > (with some caching and rate limiting access to the daemon)? > This feature is revolving around a thread. Please look at the documentation is not already done: https://www.qemu.org/docs/master/specs/rapl-msr.html#high-level-implementation If we only fetch from vCPU thread, we won't have the consumption of the non-vcpu thread. They are taken into account in the total. Thanks again for your feedback. Anthony >> In this version (v6), I have attempted to address all the problems >> addressed by Daniel and Paolo during the last review. >> >> However, two open questions remains unanswered that would require the >> attention of a x86 maintainers: >> >> 1)Should I move from -kvm to -cpu the rapl feature ? [2] >> >> 2)Should I already rename to "rapl_vmsr_*" in order to anticipate the >> futur TMPI architecture ? [end of 3] >> >> Thank you again for your continued guidance. >> >> v5 -> v6 >> -------- >> - Better error consistency in qio_channel_get_peerpid() >> - Memory leak g_strdup_printf/g_build_filename corrected >> - Renaming several struct with "vmsr_*" for better namespace >> - Renamed several struct with "guest_*" for better comprehension >> - Optimization suggerate from Daniel >> - Crash problem solved [4] >> >> v4 -> v5 >> -------- >> >> - correct qio_channel_get_peerpid: return pid = -1 in case of error >> - Vmsr_helper: compile only for x86 >> - Vmsr_helper: use qio_channel_read/write_all >> - Vmsr_helper: abandon user/group >> - Vmsr_energy.c: correct all error_report >> - Vmsr thread: compute default socket path only once >> - Vmsr thread: open socket only once >> - Pass relevant QEMU CI >> >> v3 -> v4 >> -------- >> >> - Correct memory leaks with AddressSanitizer >> - Add sanity check for QEMU and qemu-vmsr-helper for checking if host is >> INTEL and if RAPL is activated. >> - Rename poor variables naming for easier comprehension >> - Move code that checks Host before creating the VMSR thread >> - Get rid of libnuma: create function that read sysfs for reading the >> Host topology instead >> >> v2 -> v3 >> -------- >> >> - Move all memory allocations from Clib to Glib >> - Compile on *BSD (working on Linux only) >> - No more limitation on the virtual package: each vCPU that belongs to >> the same virtual package is giving the same results like expected on >> a real CPU. >> This has been tested topology like: >> -smp 4,sockets=2 >> -smp 16,sockets=4,cores=2,threads=2 >> >> v1 -> v2 >> -------- >> >> - To overcome the CVE-2020-8694 a socket communication is created >> to a priviliged helper >> - Add the priviliged helper (qemu-vmsr-helper) >> - Add SO_PEERCRED in qio channel socket >> >> RFC -> v1 >> --------- >> >> - Add vmsr_* in front of all vmsr specific function >> - Change malloc()/calloc()... with all glib equivalent >> - Pre-allocate all dynamic memories when possible >> - Add a Documentation of implementation, limitation and usage >> >> Best regards, >> Anthony >> >> [1]: https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg01570.html >> [2]: https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg03947.html >> [3]: https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg02350.html >> [4]: https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg02481.html >> >> Anthony Harivel (3): >> qio: add support for SO_PEERCRED for socket channel >> tools: build qemu-vmsr-helper >> Add support for RAPL MSRs in KVM/Qemu >> >> accel/kvm/kvm-all.c | 27 ++ >> contrib/systemd/qemu-vmsr-helper.service | 15 + >> contrib/systemd/qemu-vmsr-helper.socket | 9 + >> docs/specs/index.rst | 1 + >> docs/specs/rapl-msr.rst | 155 +++++++ >> docs/tools/index.rst | 1 + >> docs/tools/qemu-vmsr-helper.rst | 89 ++++ >> include/io/channel.h | 21 + >> include/sysemu/kvm_int.h | 32 ++ >> io/channel-socket.c | 28 ++ >> io/channel.c | 13 + >> meson.build | 7 + >> target/i386/cpu.h | 8 + >> target/i386/kvm/kvm.c | 431 +++++++++++++++++- >> target/i386/kvm/meson.build | 1 + >> target/i386/kvm/vmsr_energy.c | 337 ++++++++++++++ >> target/i386/kvm/vmsr_energy.h | 99 +++++ >> tools/i386/qemu-vmsr-helper.c | 530 +++++++++++++++++++++++ >> tools/i386/rapl-msr-index.h | 28 ++ >> 19 files changed, 1831 insertions(+), 1 deletion(-) >> create mode 100644 contrib/systemd/qemu-vmsr-helper.service >> create mode 100644 contrib/systemd/qemu-vmsr-helper.socket >> create mode 100644 docs/specs/rapl-msr.rst >> create mode 100644 docs/tools/qemu-vmsr-helper.rst >> create mode 100644 target/i386/kvm/vmsr_energy.c >> create mode 100644 target/i386/kvm/vmsr_energy.h >> create mode 100644 tools/i386/qemu-vmsr-helper.c >> create mode 100644 tools/i386/rapl-msr-index.h >>
