include/sysemu/sysemu.h | 1 + target/i386/cpu.h | 9 +++++++++ target/i386/cpu.c | 19 ++++++++++++++----- target/i386/kvm.c | 30 ++++++++++++++++++++++++++++++ vl.c | 6 ++++++ qemu-options.hx | 9 +++++++-- 6 files changed, 67 insertions(+), 7 deletions(-)
This adds ability to expose host CPU power management capabilities to guests. For intel guests, this is sufficient for guest to enable low power CPU power management. For AMD guests it isn't sufficient, deeper C-states are entered using System-IO. mwait based power management is tied closely to specifics of CPUID, making migration challenging. At this point only the non-migrateable -cpu host is supported. With this patch applied, VM latency is within the noise of baremetal for some benchmarks. perf bench sched pipe results: Before: 6.452 sec After: 4.382 sec Baremetal: 4.136 sec Michael S. Tsirkin (2): kvm: support -realtime cpu-pm=on|off i386/cpu: make -cpu host support monitor/mwait include/sysemu/sysemu.h | 1 + target/i386/cpu.h | 9 +++++++++ target/i386/cpu.c | 19 ++++++++++++++----- target/i386/kvm.c | 30 ++++++++++++++++++++++++++++++ vl.c | 6 ++++++ qemu-options.hx | 9 +++++++-- 6 files changed, 67 insertions(+), 7 deletions(-) -- MST
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180612184616.90838-1-mst@redhat.com Subject: [Qemu-devel] [PATCH v2 0/2] kvm: x86 CPU power management === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20180612184616.90838-1-mst@redhat.com -> patchew/20180612184616.90838-1-mst@redhat.com Switched to a new branch 'test' b92893898a i386/cpu: make -cpu host support monitor/mwait 26a0f862a2 kvm: support -realtime cpu-pm=on|off === OUTPUT BEGIN === Checking PATCH 1/2: kvm: support -realtime cpu-pm=on|off... WARNING: line over 80 characters #85: FILE: target/i386/kvm.c:1401: + error_report("kvm: guest stopping CPU not supported: %s", strerror(-ret)); ERROR: do not initialise globals to 0 or NULL #100: FILE: vl.c:145: +bool enable_cpu_pm = false; total: 1 errors, 1 warnings, 82 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 2/2: i386/cpu: make -cpu host support monitor/mwait... WARNING: line over 80 characters #82: FILE: target/i386/kvm.c:371: + int disable_exits = kvm_check_extension(s, KVM_CAP_X86_DISABLE_EXITS); total: 0 errors, 1 warnings, 60 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Tue, Jun 12, 2018 at 12:18:03PM -0700, no-reply@patchew.org wrote: > Checking PATCH 1/2: kvm: support -realtime cpu-pm=on|off... > WARNING: line over 80 characters > #85: FILE: target/i386/kvm.c:1401: > + error_report("kvm: guest stopping CPU not supported: %s", strerror(-ret)); > > ERROR: do not initialise globals to 0 or NULL > #100: FILE: vl.c:145: > +bool enable_cpu_pm = false; > > total: 1 errors, 1 warnings, 82 lines checked > > Your patch has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. We seem to violate this left and right. Let's drop this? -- MST
On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote: > This adds ability to expose host CPU power management capabilities to > guests. For intel guests, this is sufficient for guest to enable > low power CPU power management. For AMD guests it isn't sufficient, > deeper C-states are entered using System-IO. > > mwait based power management is tied closely to specifics of CPUID, > making migration challenging. At this point only the non-migrateable > -cpu host is supported. > > With this patch applied, VM latency is within the noise of > baremetal for some benchmarks. > > perf bench sched pipe results: > Before: > 6.452 sec > After: > 4.382 sec > Baremetal: > 4.136 sec > > Michael S. Tsirkin (2): > kvm: support -realtime cpu-pm=on|off > i386/cpu: make -cpu host support monitor/mwait > > include/sysemu/sysemu.h | 1 + > target/i386/cpu.h | 9 +++++++++ > target/i386/cpu.c | 19 ++++++++++++++----- > target/i386/kvm.c | 30 ++++++++++++++++++++++++++++++ > vl.c | 6 ++++++ > qemu-options.hx | 9 +++++++-- > 6 files changed, 67 insertions(+), 7 deletions(-) > > -- > MST Hi Michael, 1) Command line option interface Why is this not an optional cpu feature such as the other features? -cpu CPU,+mwait rather than a separate, architecture independent "-realtime cpu-pm=on|off" command line option? 2) Migration Isnt it sufficient to check that both CPUID leafs are the same, to allow migration ? 1. Check that the processor supports MONITOR and MWAIT. If CPUID.01H:ECX.MONITOR[bit 3] = 1, MONITOR and MWAIT are available at ring 0. 2. Query the smallest and largest line size that MONITOR uses. Use CPUID.05H:EAX.smallest[bits 15:0];EBX.largest[bits15:0].
On Wed, Jun 13, 2018 at 07:34:53PM -0300, Marcelo Tosatti wrote: > On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote: > > This adds ability to expose host CPU power management capabilities to > > guests. For intel guests, this is sufficient for guest to enable > > low power CPU power management. For AMD guests it isn't sufficient, > > deeper C-states are entered using System-IO. > > > > mwait based power management is tied closely to specifics of CPUID, > > making migration challenging. At this point only the non-migrateable > > -cpu host is supported. > > > > With this patch applied, VM latency is within the noise of > > baremetal for some benchmarks. > > > > perf bench sched pipe results: > > Before: > > 6.452 sec > > After: > > 4.382 sec > > Baremetal: > > 4.136 sec > > > > Michael S. Tsirkin (2): > > kvm: support -realtime cpu-pm=on|off > > i386/cpu: make -cpu host support monitor/mwait > > > > include/sysemu/sysemu.h | 1 + > > target/i386/cpu.h | 9 +++++++++ > > target/i386/cpu.c | 19 ++++++++++++++----- > > target/i386/kvm.c | 30 ++++++++++++++++++++++++++++++ > > vl.c | 6 ++++++ > > qemu-options.hx | 9 +++++++-- > > 6 files changed, 67 insertions(+), 7 deletions(-) > > > > -- > > MST > > Hi Michael, > > 1) Command line option interface > > Why is this not an optional cpu feature such as the other features? > > > -cpu CPU,+mwait > > rather than a separate, architecture independent "-realtime cpu-pm=on|off" > command line option? Because it's not just a guest flag. With guest pm on, one guest can severely affect the latency of others on the same host CPU. > 2) Migration > > Isnt it sufficient to check that both CPUID leafs are the same, > to allow migration ? Not at the moment since linux guests use mwait hints and latency values from a table in intel_idle. If the host and guest models do not match, mwait will get a wrong hint. It will not do the right thing then! You want exactly the same host CPU for it to work. This isn't different from how -host cpu works generally. > 1. Check that the processor supports MONITOR and MWAIT. If > CPUID.01H:ECX.MONITOR[bit 3] = 1, MONITOR and MWAIT are available at > ring 0. > > 2. Query the smallest and largest line size that MONITOR uses. > Use CPUID.05H:EAX.smallest[bits 15:0];EBX.largest[bits15:0]. >
On Thu, Jun 14, 2018 at 02:37:28AM +0300, Michael S. Tsirkin wrote: > On Wed, Jun 13, 2018 at 07:34:53PM -0300, Marcelo Tosatti wrote: > > On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote: > > > This adds ability to expose host CPU power management capabilities to > > > guests. For intel guests, this is sufficient for guest to enable > > > low power CPU power management. For AMD guests it isn't sufficient, > > > deeper C-states are entered using System-IO. > > > > > > mwait based power management is tied closely to specifics of CPUID, > > > making migration challenging. At this point only the non-migrateable > > > -cpu host is supported. > > > > > > With this patch applied, VM latency is within the noise of > > > baremetal for some benchmarks. > > > > > > perf bench sched pipe results: > > > Before: > > > 6.452 sec > > > After: > > > 4.382 sec > > > Baremetal: > > > 4.136 sec > > > > > > Michael S. Tsirkin (2): > > > kvm: support -realtime cpu-pm=on|off > > > i386/cpu: make -cpu host support monitor/mwait > > > > > > include/sysemu/sysemu.h | 1 + > > > target/i386/cpu.h | 9 +++++++++ > > > target/i386/cpu.c | 19 ++++++++++++++----- > > > target/i386/kvm.c | 30 ++++++++++++++++++++++++++++++ > > > vl.c | 6 ++++++ > > > qemu-options.hx | 9 +++++++-- > > > 6 files changed, 67 insertions(+), 7 deletions(-) > > > > > > -- > > > MST > > > > Hi Michael, > > > > 1) Command line option interface > > > > Why is this not an optional cpu feature such as the other features? > > > > > > -cpu CPU,+mwait > > > > rather than a separate, architecture independent "-realtime cpu-pm=on|off" > > command line option? > > Because it's not just a guest flag. With guest pm on, one guest > can severely affect the latency of others on the same host CPU. How so ? > > 2) Migration > > > > Isnt it sufficient to check that both CPUID leafs are the same, > > to allow migration ? > > Not at the moment since linux guests use mwait hints and latency values > from a table in intel_idle. If the host and guest models do not match, > mwait will get a wrong hint. > > It will not do the right thing then! > > You want exactly the same host CPU for it to work. > > This isn't different from how -host cpu works generally. Ok, makes sense. > > 1. Check that the processor supports MONITOR and MWAIT. If > > CPUID.01H:ECX.MONITOR[bit 3] = 1, MONITOR and MWAIT are available at > > ring 0. > > > > 2. Query the smallest and largest line size that MONITOR uses. > > Use CPUID.05H:EAX.smallest[bits 15:0];EBX.largest[bits15:0]. > >
On Fri, Jun 15, 2018 at 03:32:27PM -0300, Marcelo Tosatti wrote: > On Thu, Jun 14, 2018 at 02:37:28AM +0300, Michael S. Tsirkin wrote: > > On Wed, Jun 13, 2018 at 07:34:53PM -0300, Marcelo Tosatti wrote: > > > On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote: > > > > This adds ability to expose host CPU power management capabilities to > > > > guests. For intel guests, this is sufficient for guest to enable > > > > low power CPU power management. For AMD guests it isn't sufficient, > > > > deeper C-states are entered using System-IO. > > > > > > > > mwait based power management is tied closely to specifics of CPUID, > > > > making migration challenging. At this point only the non-migrateable > > > > -cpu host is supported. > > > > > > > > With this patch applied, VM latency is within the noise of > > > > baremetal for some benchmarks. > > > > > > > > perf bench sched pipe results: > > > > Before: > > > > 6.452 sec > > > > After: > > > > 4.382 sec > > > > Baremetal: > > > > 4.136 sec > > > > > > > > Michael S. Tsirkin (2): > > > > kvm: support -realtime cpu-pm=on|off > > > > i386/cpu: make -cpu host support monitor/mwait > > > > > > > > include/sysemu/sysemu.h | 1 + > > > > target/i386/cpu.h | 9 +++++++++ > > > > target/i386/cpu.c | 19 ++++++++++++++----- > > > > target/i386/kvm.c | 30 ++++++++++++++++++++++++++++++ > > > > vl.c | 6 ++++++ > > > > qemu-options.hx | 9 +++++++-- > > > > 6 files changed, 67 insertions(+), 7 deletions(-) > > > > > > > > -- > > > > MST > > > > > > Hi Michael, > > > > > > 1) Command line option interface > > > > > > Why is this not an optional cpu feature such as the other features? > > > > > > > > > -cpu CPU,+mwait > > > > > > rather than a separate, architecture independent "-realtime cpu-pm=on|off" > > > command line option? > > > > Because it's not just a guest flag. With guest pm on, one guest > > can severely affect the latency of others on the same host CPU. > > How so ? Look at drivers/idle/intel_idle.c There are states with exit latencies of 10000 > > > 2) Migration > > > > > > Isnt it sufficient to check that both CPUID leafs are the same, > > > to allow migration ? > > > > Not at the moment since linux guests use mwait hints and latency values > > from a table in intel_idle. If the host and guest models do not match, > > mwait will get a wrong hint. > > > > It will not do the right thing then! > > > > You want exactly the same host CPU for it to work. > > > > This isn't different from how -host cpu works generally. > > Ok, makes sense. > > > > 1. Check that the processor supports MONITOR and MWAIT. If > > > CPUID.01H:ECX.MONITOR[bit 3] = 1, MONITOR and MWAIT are available at > > > ring 0. > > > > > > 2. Query the smallest and largest line size that MONITOR uses. > > > Use CPUID.05H:EAX.smallest[bits 15:0];EBX.largest[bits15:0]. > > >
On Tue, Jun 12, 2018 at 09:47:11PM +0300, Michael S. Tsirkin wrote: > This adds ability to expose host CPU power management capabilities to > guests. For intel guests, this is sufficient for guest to enable > low power CPU power management. For AMD guests it isn't sufficient, > deeper C-states are entered using System-IO. > > mwait based power management is tied closely to specifics of CPUID, > making migration challenging. At this point only the non-migrateable > -cpu host is supported. > > With this patch applied, VM latency is within the noise of > baremetal for some benchmarks. > > perf bench sched pipe results: > Before: > 6.452 sec > After: > 4.382 sec > Baremetal: > 4.136 sec > > Michael S. Tsirkin (2): > kvm: support -realtime cpu-pm=on|off IMHO this really shouldn't be under the -realtime flag. I don't think the -realtime flag should ever have been introduced, and we certainly shouldn't add more stuff under it. "-realtime" is referring to a very specific use case, while the properties listed under it are all general purpose features. Real time guests just happen to be one possible use case, but it is valid to use them for non-real time guests. IOW, I think we should just have this as an option under -cpu or some other *functionally* named option, not a option named after a specific usage scenario. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 14/06/2018 10:18, Daniel P. Berrangé wrote: > I don't think > the -realtime flag should ever have been introduced, and we certainly > shouldn't add more stuff under it. > > "-realtime" is referring to a very specific use case, while the > properties listed under it are all general purpose features. Real > time guests just happen to be one possible use case, but it is > valid to use them for non-real time guests. > > IOW, I think we should just have this as an option under -cpu or > some other *functionally* named option, not a option named after > a specific usage scenario. "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a device option, while this is about host behavior. "-realtime"'s name is awful, but I still think it's the best place for this option. Maybe we could call it "-realtime power-mgmt={host|guest}". A separate issue is whether the same flag should control both KVM_CAP_X86_DISABLE_EXITS and the monitor/mwait CPUID leaf. Eduardo, what do you think? Paolo
On Thu, Jun 14, 2018 at 05:40:41PM +0200, Paolo Bonzini wrote: > On 14/06/2018 10:18, Daniel P. Berrangé wrote: > > I don't think > > the -realtime flag should ever have been introduced, and we certainly > > shouldn't add more stuff under it. > > > > "-realtime" is referring to a very specific use case, while the > > properties listed under it are all general purpose features. Real > > time guests just happen to be one possible use case, but it is > > valid to use them for non-real time guests. > > > > IOW, I think we should just have this as an option under -cpu or > > some other *functionally* named option, not a option named after > > a specific usage scenario. > > "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a > device option, while this is about host behavior. "-realtime"'s name is > awful, but I still think it's the best place for this option. Maybe we > could call it "-realtime power-mgmt={host|guest}". If none of the existing ones are a suitable fit, then we should just introduce a new CLI arg instead stuffing it into somewhere odd. IOW, why not just "--power-mgmt host|guest" Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 14/06/2018 17:44, Daniel P. Berrangé wrote: >> "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a >> device option, while this is about host behavior. "-realtime"'s name is >> awful, but I still think it's the best place for this option. Maybe we >> could call it "-realtime power-mgmt={host|guest}". > If none of the existing ones are a suitable fit, then we should just > introduce a new CLI arg instead stuffing it into somewhere odd. It is related to mlock in my opinion. Both options are about applying settings that reduce latency but may not be applicable in general. Maybe "-realtime" could be renamed to "-perftune" or something like that, but I'd like to keep the grouping. Paolo > IOW, why not just "--power-mgmt host|guest"
On Thu, Jun 14, 2018 at 10:32:00PM +0200, Paolo Bonzini wrote: > On 14/06/2018 17:44, Daniel P. Berrangé wrote: > >> "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a > >> device option, while this is about host behavior. "-realtime"'s name is > >> awful, but I still think it's the best place for this option. Maybe we > >> could call it "-realtime power-mgmt={host|guest}". > > If none of the existing ones are a suitable fit, then we should just > > introduce a new CLI arg instead stuffing it into somewhere odd. > > It is related to mlock in my opinion. Both options are about applying > settings that reduce latency but may not be applicable in general. I would add, it's also about host resource handling. With mlock guest memory is locked so there's no memory overcommit. With cpu-pm guest does not give up host cpu which mostly disables host cpu overcommit. > Maybe "-realtime" could be renamed to "-perftune" or something like > that, but I'd like to keep the grouping. > > Paolo -overcommit ? > > IOW, why not just "--power-mgmt host|guest"
On Thu, Jun 14, 2018 at 05:40:41PM +0200, Paolo Bonzini wrote: > On 14/06/2018 10:18, Daniel P. Berrangé wrote: > > I don't think > > the -realtime flag should ever have been introduced, and we certainly > > shouldn't add more stuff under it. > > > > "-realtime" is referring to a very specific use case, while the > > properties listed under it are all general purpose features. Real > > time guests just happen to be one possible use case, but it is > > valid to use them for non-real time guests. > > > > IOW, I think we should just have this as an option under -cpu or > > some other *functionally* named option, not a option named after > > a specific usage scenario. > > "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a > device option, while this is about host behavior. "-realtime"'s name is > awful, but I still think it's the best place for this option. Maybe we > could call it "-realtime power-mgmt={host|guest}". > > A separate issue is whether the same flag should control both > KVM_CAP_X86_DISABLE_EXITS and the monitor/mwait CPUID leaf. Eduardo, > what do you think? Making "-cpu host" be affected by a host-side option is acceptable to me. A "-cpu" option would be more appropriate if we decide to allow monitor/mwait be enabled for other CPU models too. -- Eduardo
On 14/06/2018 18:53, Eduardo Habkost wrote: >> >> A separate issue is whether the same flag should control both >> KVM_CAP_X86_DISABLE_EXITS and the monitor/mwait CPUID leaf. Eduardo, >> what do you think? > Making "-cpu host" be affected by a host-side option is > acceptable to me. A "-cpu" option would be more appropriate if > we decide to allow monitor/mwait be enabled for other CPU models > too. Sounds fair enough, we should probably add a note in the documentation of "-cpu host" though. Paolo
On Thu, Jun 14, 2018 at 05:40:41PM +0200, Paolo Bonzini wrote: > On 14/06/2018 10:18, Daniel P. Berrangé wrote: > > I don't think > > the -realtime flag should ever have been introduced, and we certainly > > shouldn't add more stuff under it. > > > > "-realtime" is referring to a very specific use case, while the > > properties listed under it are all general purpose features. Real > > time guests just happen to be one possible use case, but it is > > valid to use them for non-real time guests. > > > > IOW, I think we should just have this as an option under -cpu or > > some other *functionally* named option, not a option named after > > a specific usage scenario. > > "-cpu" is certainly wrong for KVM_CAP_X86_DISABLE_EXITS. "-cpu" is a > device option, while this is about host behavior. "-realtime"'s name is > awful, but I still think it's the best place for this option. Maybe we > could call it "-realtime power-mgmt={host|guest}". I don't feel it's a good name. The new flag allows guest to put CPU into a low power state without host knowing about it. This will affect other guests on the same host CPU. It doesn't however limit power management to guest only, things like speedstep remain under host control. > A separate issue is whether the same flag should control both > KVM_CAP_X86_DISABLE_EXITS and the monitor/mwait CPUID leaf. Eduardo, > what do you think? > > Paolo It's just for -cpu host.
(Cc: Thomas Huth) On Thu, Jun 14, 2018 at 09:18:00AM +0100, Daniel P. Berrangé wrote: [...] > IMHO this really shouldn't be under the -realtime flag. I don't think > the -realtime flag should ever have been introduced, and we certainly > shouldn't add more stuff under it. > > "-realtime" is referring to a very specific use case, while the > properties listed under it are all general purpose features. Real > time guests just happen to be one possible use case, but it is > valid to use them for non-real time guests. Given what you say above, then it sounds like the "-realtime" QEMU option should be deprecated, and removed in a future release. [...] -- /kashyap
On Fri, Jun 22, 2018 at 03:06:59PM +0200, Kashyap Chamarthy wrote: > (Cc: Thomas Huth) > > On Thu, Jun 14, 2018 at 09:18:00AM +0100, Daniel P. Berrangé wrote: > > [...] > > > IMHO this really shouldn't be under the -realtime flag. I don't think > > the -realtime flag should ever have been introduced, and we certainly > > shouldn't add more stuff under it. > > > > "-realtime" is referring to a very specific use case, while the > > properties listed under it are all general purpose features. Real > > time guests just happen to be one possible use case, but it is > > valid to use them for non-real time guests. > > Given what you say above, then it sounds like the "-realtime" QEMU > option should be deprecated, and removed in a future release. > > [...] > As a patch on top, sure. > -- > /kashyap
© 2016 - 2024 Red Hat, Inc.