Re: [libvirt] [PATCH 0/7] iothread api followups
>> w/r/t patch 7 - while it's a "nice to have" I think its far more >> relevant to list the Resource(s) associated with the thread than the CPU >> time used. Listing the Resource was rejected in a much earlier patch > > Actually, the "resources" is a configruation option and I don't see a > way to change it unless you unplug the disk while CPU time is a > statistics function that changes a lot. > Correct, but I also think it's more useful to display resources in IOThreads output. If I'm a user I would rather know which resources are assigned to which IOThreads rather than how much CPU Time they're using. Since CPU Time is a statistic, perhaps it belongs in the stats output rather than the IOThreads information output. As it stands, I have to make my own "map" by dumping the xml, looking for the iothreads attribute in a disk element and then do the correlation. Again - don't forget to update the libvirt-python and libvirt-perl code. >> review, so I don't see why listing the CPU time is important and failing >> in qemuDomainGetIOThreadsLive because we cannot get the that time, but >> yet deciding later on to not print it if it doesn't exist doesn't make >> total sense. > > If the VM is offline, then the CPU time is obviously not filled, while > when it's live we should return it. You missed my point - if for some reason we fail to get the CPU Time from the QEMU call - we fail the entire call now. If the time wasn't available we don't print it. So why fail the call just because the time isn't available (for whatever reason). It's not all that important, but once done I would hope the bz associated will also get updated to list the new data column so that whomever tests and documents this can do so properly. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/7] iothread api followups
On Wed, Mar 25, 2015 at 15:00:58 -0400, John Ferlan wrote: > > > On 03/25/2015 02:39 PM, Ján Tomko wrote: > > Looking at the proposed SetIOThread API, I noticed > > that the virsh command for printing the info is named 'iothreadsinfo'. > > This seemed counter-intuitive for me. > > > > Since the API has not been released yet, I propose a rename of the command > > to: > > iothreadinfo (patch 5) > > and the API for freeing one struct to: > > virDomainIOThreadInfoFree (patch 1) > > > > I don't feel as strongly about renaming the virDomainGetIOThreadsInfo API > > (patches 3, 4) or the internal APIs (patch 2). > > > > Looking at virVcpuInfoPtr (which might not be the best inspiration), > > I realized including the cpu time might be a good idea. > > > > Ján Tomko (7): > > Rename virDomainIOThreadsInfoFree to virDomainIOThreadInfoFree > > Rename qemuMonitorIOThreadsInfo* to qemuMonitorIOThreadInfo* > > Rename DomainGetIOThreadsInfo to DomainGetIOThreadInfo > > gendispatch: remove IOThreads from name fixups > > virsh: rename iothreadsinfo to iothreadinfo > > Do not use vshPrintPinInfo in iothreadinfo > > add cpu time to iothreadinfo > > > > daemon/remote.c | 15 +++-- > > include/libvirt/libvirt-domain.h | 5 +++-- > > src/driver-hypervisor.h | 4 ++-- > > src/libvirt-domain.c | 20 - > > src/libvirt_public.syms | 4 ++-- > > src/qemu/qemu_driver.c | 24 ++-- > > src/qemu/qemu_monitor.c | 4 ++-- > > src/qemu/qemu_monitor.h | 10 - > > src/qemu/qemu_monitor_json.c | 8 +++ > > src/qemu/qemu_monitor_json.h | 2 +- > > src/qemu/qemu_process.c | 4 ++-- > > src/remote/remote_driver.c | 23 ++-- > > src/remote/remote_protocol.x | 11 +- > > src/remote_protocol-structs | 7 +++--- > > src/rpc/gendispatch.pl | 1 - > > tests/qemumonitorjsontest.c | 4 ++-- > > tools/virsh-domain.c | 47 > > > > 17 files changed, 113 insertions(+), 80 deletions(-) > > > > > Didn't dig into all the details, but use of IOThreads v IOThread was > mostly an artifact of the technology name. I'm ambivalent to the naming > issue. You'll have to have follow-ups in libvirt-python and > libvirt-perl though. Sometimes the *s* is relevant though - as in we're > returning all IOThreads found vs returning just one IOThread > > w/r/t virsh iothreadsinfo vs. iothreadinfo - I see the latter as > displaying just one rather than multiple. > > w/r/t patch 6 - does the output change? IOW: The current way displays > "1", "3", "0-3", etc. Does the method you're proposing change to the > "y---", "--y-", "" etc. method? I prefer the former The commit > message would need to list the change if there is one. The output does not change. In fact I think the virsh reimpl of virBitmapFormat should be killed and replaced with the function we already have. > > w/r/t patch 7 - while it's a "nice to have" I think its far more > relevant to list the Resource(s) associated with the thread than the CPU > time used. Listing the Resource was rejected in a much earlier patch Actually, the "resources" is a configruation option and I don't see a way to change it unless you unplug the disk while CPU time is a statistics function that changes a lot. > review, so I don't see why listing the CPU time is important and failing > in qemuDomainGetIOThreadsLive because we cannot get the that time, but > yet deciding later on to not print it if it doesn't exist doesn't make > total sense. If the VM is offline, then the CPU time is obviously not filled, while when it's live we should return it. Peter signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/7] iothread api followups
On 03/25/2015 02:39 PM, Ján Tomko wrote: > Looking at the proposed SetIOThread API, I noticed > that the virsh command for printing the info is named 'iothreadsinfo'. > This seemed counter-intuitive for me. > > Since the API has not been released yet, I propose a rename of the command to: > iothreadinfo (patch 5) > and the API for freeing one struct to: > virDomainIOThreadInfoFree (patch 1) > > I don't feel as strongly about renaming the virDomainGetIOThreadsInfo API > (patches 3, 4) or the internal APIs (patch 2). > > Looking at virVcpuInfoPtr (which might not be the best inspiration), > I realized including the cpu time might be a good idea. > > Ján Tomko (7): > Rename virDomainIOThreadsInfoFree to virDomainIOThreadInfoFree > Rename qemuMonitorIOThreadsInfo* to qemuMonitorIOThreadInfo* > Rename DomainGetIOThreadsInfo to DomainGetIOThreadInfo > gendispatch: remove IOThreads from name fixups > virsh: rename iothreadsinfo to iothreadinfo > Do not use vshPrintPinInfo in iothreadinfo > add cpu time to iothreadinfo > > daemon/remote.c | 15 +++-- > include/libvirt/libvirt-domain.h | 5 +++-- > src/driver-hypervisor.h | 4 ++-- > src/libvirt-domain.c | 20 - > src/libvirt_public.syms | 4 ++-- > src/qemu/qemu_driver.c | 24 ++-- > src/qemu/qemu_monitor.c | 4 ++-- > src/qemu/qemu_monitor.h | 10 - > src/qemu/qemu_monitor_json.c | 8 +++ > src/qemu/qemu_monitor_json.h | 2 +- > src/qemu/qemu_process.c | 4 ++-- > src/remote/remote_driver.c | 23 ++-- > src/remote/remote_protocol.x | 11 +- > src/remote_protocol-structs | 7 +++--- > src/rpc/gendispatch.pl | 1 - > tests/qemumonitorjsontest.c | 4 ++-- > tools/virsh-domain.c | 47 > > 17 files changed, 113 insertions(+), 80 deletions(-) > Didn't dig into all the details, but use of IOThreads v IOThread was mostly an artifact of the technology name. I'm ambivalent to the naming issue. You'll have to have follow-ups in libvirt-python and libvirt-perl though. Sometimes the *s* is relevant though - as in we're returning all IOThreads found vs returning just one IOThread w/r/t virsh iothreadsinfo vs. iothreadinfo - I see the latter as displaying just one rather than multiple. w/r/t patch 6 - does the output change? IOW: The current way displays "1", "3", "0-3", etc. Does the method you're proposing change to the "y---", "--y-", "" etc. method? I prefer the former The commit message would need to list the change if there is one. w/r/t patch 7 - while it's a "nice to have" I think its far more relevant to list the Resource(s) associated with the thread than the CPU time used. Listing the Resource was rejected in a much earlier patch review, so I don't see why listing the CPU time is important and failing in qemuDomainGetIOThreadsLive because we cannot get the that time, but yet deciding later on to not print it if it doesn't exist doesn't make total sense. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/7] iothread api followups
Looking at the proposed SetIOThread API, I noticed that the virsh command for printing the info is named 'iothreadsinfo'. This seemed counter-intuitive for me. Since the API has not been released yet, I propose a rename of the command to: iothreadinfo (patch 5) and the API for freeing one struct to: virDomainIOThreadInfoFree (patch 1) I don't feel as strongly about renaming the virDomainGetIOThreadsInfo API (patches 3, 4) or the internal APIs (patch 2). Looking at virVcpuInfoPtr (which might not be the best inspiration), I realized including the cpu time might be a good idea. Ján Tomko (7): Rename virDomainIOThreadsInfoFree to virDomainIOThreadInfoFree Rename qemuMonitorIOThreadsInfo* to qemuMonitorIOThreadInfo* Rename DomainGetIOThreadsInfo to DomainGetIOThreadInfo gendispatch: remove IOThreads from name fixups virsh: rename iothreadsinfo to iothreadinfo Do not use vshPrintPinInfo in iothreadinfo add cpu time to iothreadinfo daemon/remote.c | 15 +++-- include/libvirt/libvirt-domain.h | 5 +++-- src/driver-hypervisor.h | 4 ++-- src/libvirt-domain.c | 20 - src/libvirt_public.syms | 4 ++-- src/qemu/qemu_driver.c | 24 ++-- src/qemu/qemu_monitor.c | 4 ++-- src/qemu/qemu_monitor.h | 10 - src/qemu/qemu_monitor_json.c | 8 +++ src/qemu/qemu_monitor_json.h | 2 +- src/qemu/qemu_process.c | 4 ++-- src/remote/remote_driver.c | 23 ++-- src/remote/remote_protocol.x | 11 +- src/remote_protocol-structs | 7 +++--- src/rpc/gendispatch.pl | 1 - tests/qemumonitorjsontest.c | 4 ++-- tools/virsh-domain.c | 47 17 files changed, 113 insertions(+), 80 deletions(-) -- 2.0.5 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
