* [PATCH 0/2] KVM: Save two bits in vcpu->requests @ 2016-01-07 11:43 Takuya Yoshikawa 2016-01-07 11:43 ` [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit " Takuya Yoshikawa 2016-01-07 11:43 ` [PATCH 2/2] KVM: Remove KVM_REQ_MCLOCK_INPROGRESS " Takuya Yoshikawa 0 siblings, 2 replies; 6+ messages in thread From: Takuya Yoshikawa @ 2016-01-07 11:43 UTC (permalink / raw) To: pbonzini; +Cc: kvm, Takuya Yoshikawa Note: I am submitting this now just to let you know this is possible. As you can see, this is not for v4.5, so I will rebase the patches later if necessary. Although I understand what the following patch tries to do, I do not think we should do the change right now: using bitmap APIs can make the code slower, so we should not make that change until it becomes really necessary. [PATCH v2] kvm: Make vcpu->requests as 64 bit bitmap http://www.spinics.net/lists/kvm/msg125726.html This patch set tries to give you some time to find a better solution: for arch-specific flags like KVM_REQ_HV_*, or for those rarely set, we may better use a separate variable than vcpu->requests. Takuya Yoshikawa (2): KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests KVM: Remove KVM_REQ_MCLOCK_INPROGRESS to save a bit in vcpu->requests arch/x86/kvm/x86.c | 2 -- include/linux/kvm_host.h | 46 ++++++++++++++++++++++------------------------ virt/kvm/kvm_main.c | 8 +++++--- 3 files changed, 27 insertions(+), 29 deletions(-) -- 2.1.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests 2016-01-07 11:43 [PATCH 0/2] KVM: Save two bits in vcpu->requests Takuya Yoshikawa @ 2016-01-07 11:43 ` Takuya Yoshikawa 2016-01-07 12:37 ` Paolo Bonzini 2016-01-07 11:43 ` [PATCH 2/2] KVM: Remove KVM_REQ_MCLOCK_INPROGRESS " Takuya Yoshikawa 1 sibling, 1 reply; 6+ messages in thread From: Takuya Yoshikawa @ 2016-01-07 11:43 UTC (permalink / raw) To: pbonzini; +Cc: kvm, Takuya Yoshikawa Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> --- include/linux/kvm_host.h | 45 ++++++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 61c3e6c..ca9b93e 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -122,29 +122,28 @@ static inline bool is_error_page(struct page *page) #define KVM_REQ_UNHALT 6 #define KVM_REQ_MMU_SYNC 7 #define KVM_REQ_CLOCK_UPDATE 8 -#define KVM_REQ_KICK 9 -#define KVM_REQ_DEACTIVATE_FPU 10 -#define KVM_REQ_EVENT 11 -#define KVM_REQ_APF_HALT 12 -#define KVM_REQ_STEAL_UPDATE 13 -#define KVM_REQ_NMI 14 -#define KVM_REQ_PMU 15 -#define KVM_REQ_PMI 16 -#define KVM_REQ_WATCHDOG 17 -#define KVM_REQ_MASTERCLOCK_UPDATE 18 -#define KVM_REQ_MCLOCK_INPROGRESS 19 -#define KVM_REQ_EPR_EXIT 20 -#define KVM_REQ_SCAN_IOAPIC 21 -#define KVM_REQ_GLOBAL_CLOCK_UPDATE 22 -#define KVM_REQ_ENABLE_IBS 23 -#define KVM_REQ_DISABLE_IBS 24 -#define KVM_REQ_APIC_PAGE_RELOAD 25 -#define KVM_REQ_SMI 26 -#define KVM_REQ_HV_CRASH 27 -#define KVM_REQ_IOAPIC_EOI_EXIT 28 -#define KVM_REQ_HV_RESET 29 -#define KVM_REQ_HV_EXIT 30 -#define KVM_REQ_HV_STIMER 31 +#define KVM_REQ_DEACTIVATE_FPU 9 +#define KVM_REQ_EVENT 10 +#define KVM_REQ_APF_HALT 11 +#define KVM_REQ_STEAL_UPDATE 12 +#define KVM_REQ_NMI 13 +#define KVM_REQ_PMU 14 +#define KVM_REQ_PMI 15 +#define KVM_REQ_WATCHDOG 16 +#define KVM_REQ_MASTERCLOCK_UPDATE 17 +#define KVM_REQ_MCLOCK_INPROGRESS 18 +#define KVM_REQ_EPR_EXIT 19 +#define KVM_REQ_SCAN_IOAPIC 20 +#define KVM_REQ_GLOBAL_CLOCK_UPDATE 21 +#define KVM_REQ_ENABLE_IBS 22 +#define KVM_REQ_DISABLE_IBS 23 +#define KVM_REQ_APIC_PAGE_RELOAD 24 +#define KVM_REQ_SMI 25 +#define KVM_REQ_HV_CRASH 26 +#define KVM_REQ_IOAPIC_EOI_EXIT 27 +#define KVM_REQ_HV_RESET 28 +#define KVM_REQ_HV_EXIT 29 +#define KVM_REQ_HV_STIMER 30 #define KVM_USERSPACE_IRQ_SOURCE_ID 0 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1 -- 2.1.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests 2016-01-07 11:43 ` [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit " Takuya Yoshikawa @ 2016-01-07 12:37 ` Paolo Bonzini 2016-01-08 1:47 ` Takuya Yoshikawa 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2016-01-07 12:37 UTC (permalink / raw) To: Takuya Yoshikawa; +Cc: kvm On 07/01/2016 12:43, Takuya Yoshikawa wrote: > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> > --- > include/linux/kvm_host.h | 45 ++++++++++++++++++++++----------------------- > 1 file changed, 22 insertions(+), 23 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 61c3e6c..ca9b93e 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -122,29 +122,28 @@ static inline bool is_error_page(struct page *page) > #define KVM_REQ_UNHALT 6 > #define KVM_REQ_MMU_SYNC 7 > #define KVM_REQ_CLOCK_UPDATE 8 > -#define KVM_REQ_KICK 9 I'd prefer to leave just an /* unused: 9 */ here. This patch can go in for 4.5. Regarding the other patch, KVM_REQ_MCLOCK_INPROGRESS is indeed not really necessary, see http://www.spinics.net/lists/kvm/msg95944.html and the follow-up. Were you thinking of the same? If so I would prefer to have some comments. Paolo > -#define KVM_REQ_DEACTIVATE_FPU 10 > -#define KVM_REQ_EVENT 11 > -#define KVM_REQ_APF_HALT 12 > -#define KVM_REQ_STEAL_UPDATE 13 > -#define KVM_REQ_NMI 14 > -#define KVM_REQ_PMU 15 > -#define KVM_REQ_PMI 16 > -#define KVM_REQ_WATCHDOG 17 > -#define KVM_REQ_MASTERCLOCK_UPDATE 18 > -#define KVM_REQ_MCLOCK_INPROGRESS 19 > -#define KVM_REQ_EPR_EXIT 20 > -#define KVM_REQ_SCAN_IOAPIC 21 > -#define KVM_REQ_GLOBAL_CLOCK_UPDATE 22 > -#define KVM_REQ_ENABLE_IBS 23 > -#define KVM_REQ_DISABLE_IBS 24 > -#define KVM_REQ_APIC_PAGE_RELOAD 25 > -#define KVM_REQ_SMI 26 > -#define KVM_REQ_HV_CRASH 27 > -#define KVM_REQ_IOAPIC_EOI_EXIT 28 > -#define KVM_REQ_HV_RESET 29 > -#define KVM_REQ_HV_EXIT 30 > -#define KVM_REQ_HV_STIMER 31 > +#define KVM_REQ_DEACTIVATE_FPU 9 > +#define KVM_REQ_EVENT 10 > +#define KVM_REQ_APF_HALT 11 > +#define KVM_REQ_STEAL_UPDATE 12 > +#define KVM_REQ_NMI 13 > +#define KVM_REQ_PMU 14 > +#define KVM_REQ_PMI 15 > +#define KVM_REQ_WATCHDOG 16 > +#define KVM_REQ_MASTERCLOCK_UPDATE 17 > +#define KVM_REQ_MCLOCK_INPROGRESS 18 > +#define KVM_REQ_EPR_EXIT 19 > +#define KVM_REQ_SCAN_IOAPIC 20 > +#define KVM_REQ_GLOBAL_CLOCK_UPDATE 21 > +#define KVM_REQ_ENABLE_IBS 22 > +#define KVM_REQ_DISABLE_IBS 23 > +#define KVM_REQ_APIC_PAGE_RELOAD 24 > +#define KVM_REQ_SMI 25 > +#define KVM_REQ_HV_CRASH 26 > +#define KVM_REQ_IOAPIC_EOI_EXIT 27 > +#define KVM_REQ_HV_RESET 28 > +#define KVM_REQ_HV_EXIT 29 > +#define KVM_REQ_HV_STIMER 30 > > #define KVM_USERSPACE_IRQ_SOURCE_ID 0 > #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests 2016-01-07 12:37 ` Paolo Bonzini @ 2016-01-08 1:47 ` Takuya Yoshikawa 2016-01-08 12:22 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Takuya Yoshikawa @ 2016-01-08 1:47 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm On 2016/01/07 21:37, Paolo Bonzini wrote: > On 07/01/2016 12:43, Takuya Yoshikawa wrote: >> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> >> --- >> include/linux/kvm_host.h | 45 ++++++++++++++++++++++----------------------- >> 1 file changed, 22 insertions(+), 23 deletions(-) >> >> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >> index 61c3e6c..ca9b93e 100644 >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -122,29 +122,28 @@ static inline bool is_error_page(struct page *page) >> #define KVM_REQ_UNHALT 6 >> #define KVM_REQ_MMU_SYNC 7 >> #define KVM_REQ_CLOCK_UPDATE 8 >> -#define KVM_REQ_KICK 9 > > I'd prefer to leave just an /* unused: 9 */ here. Since some request handlers can make other requests which need to be checked in the following path, this young number is valuable. In this sense, I agree. Your patch looks good to me. > This patch can go in for 4.5. Regarding the other patch, > KVM_REQ_MCLOCK_INPROGRESS is indeed not really necessary, see > http://www.spinics.net/lists/kvm/msg95944.html and the follow-up. Were > you thinking of the same? If so I would prefer to have some comments. I did not notice that discussion. What I have in mind is: http://www.spinics.net/lists/kvm/msg80477.html We have more requests than that time and the overhead of requests checking has become much worse: Hyper-V code may still add some more if-branches, which will always be false for many guests. To find a solution, I am doing some experiments and trying to re-implement Avi's for_each_set_bit based solution. The problem is that for_each_set_bit is too generic and involves too many function calls, which can actually make the code slower. But since vcpu->requests is unsigned long, we can directly use __ffs as kvm_mmu_write_protect_pt_masked() does. This is the reason why I object to expanding it to 64-bit size now. I still need to think carefully about the dependencies between a few request handlers and am not sure what I am thinking now will really work. Anyway, your patches make enough room for now, so I do not need to remove KVM_REQ_MCLOCK_INPROGRESS dummy request for that. It depends just on our preference. Takuya ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests 2016-01-08 1:47 ` Takuya Yoshikawa @ 2016-01-08 12:22 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2016-01-08 12:22 UTC (permalink / raw) To: Takuya Yoshikawa; +Cc: kvm On 08/01/2016 02:47, Takuya Yoshikawa wrote: > > Since some request handlers can make other requests which need to be > checked in the following path, this young number is valuable. In this > sense, I agree. Your patch looks good to me. > >> This patch can go in for 4.5. Regarding the other patch, >> KVM_REQ_MCLOCK_INPROGRESS is indeed not really necessary, see >> http://www.spinics.net/lists/kvm/msg95944.html and the follow-up. Were >> you thinking of the same? If so I would prefer to have some comments. > > I did not notice that discussion. > > What I have in mind is: > http://www.spinics.net/lists/kvm/msg80477.html > > We have more requests than that time and the overhead of > requests checking has become much worse: Hyper-V code may > still add some more if-branches, which will always be false > for many guests. True, on the other hand they will be well predicted. Any time I tried reordering kvm_check_request or other similar micro-optimizations, everything became slower. Same for replacing all the clear_bit with a single xchg and then testing the bits on the value that xchg returned. Even "obvious" replacements such as - if (vcpu->requests) + if (vcpu->requests & ~(1 << KVM_REQ_EVENT)) weren't clear winners! But anyway, this is where I would start from; not for_each_set_bit or __ffs. There are other optimizations that are easily done, unrelated to vcpu->requests: lapic_in_kernel is a dup of kvm_vcpu_has_lapic, but without the static key optimization. I prefer the lapic_in_kernel name, but it has the less efficient implementation. They ought to be merged. If you really want to free bits, a possible optimization would be to move KVM_REQ_REPORT_TPR_ACCESS, KVM_REQ_TRIPLE_FAULT, KVM_REQ_HV_CRASH, KVM_REQ_HV_RESET, and KVM_REQ_HV_EXIT to a separate vcpu->vmexit_requests field. You could either check it on every vmexit, or just consolidate them into a KVM_REQ_VMEXIT bit. > Anyway, your patches make enough room for now, so I do not need > to remove KVM_REQ_MCLOCK_INPROGRESS dummy request for that. It > depends just on our preference. I've never liked the dummy request really, but I've never felt like arguing for its removal either. :) Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: Remove KVM_REQ_MCLOCK_INPROGRESS to save a bit in vcpu->requests 2016-01-07 11:43 [PATCH 0/2] KVM: Save two bits in vcpu->requests Takuya Yoshikawa 2016-01-07 11:43 ` [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit " Takuya Yoshikawa @ 2016-01-07 11:43 ` Takuya Yoshikawa 1 sibling, 0 replies; 6+ messages in thread From: Takuya Yoshikawa @ 2016-01-07 11:43 UTC (permalink / raw) To: pbonzini; +Cc: kvm, Takuya Yoshikawa Now that we are running out of the bits in vcpu->requests, using one of them just to call kvm_make_all_cpus_request() with a valid request number should be avoided. This patch achieves this by making kvm_make_all_cpus_request() handle an empty request. Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> --- arch/x86/kvm/x86.c | 2 -- include/linux/kvm_host.h | 27 +++++++++++++-------------- virt/kvm/kvm_main.c | 8 +++++--- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index b6102c1..88260d0 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1701,8 +1701,6 @@ static void kvm_gen_update_masterclock(struct kvm *kvm) kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu); /* guest entries allowed */ - kvm_for_each_vcpu(i, vcpu, kvm) - clear_bit(KVM_REQ_MCLOCK_INPROGRESS, &vcpu->requests); spin_unlock(&ka->pvclock_gtod_sync_lock); #endif diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index ca9b93e..bb9ae4f 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -131,19 +131,18 @@ static inline bool is_error_page(struct page *page) #define KVM_REQ_PMI 15 #define KVM_REQ_WATCHDOG 16 #define KVM_REQ_MASTERCLOCK_UPDATE 17 -#define KVM_REQ_MCLOCK_INPROGRESS 18 -#define KVM_REQ_EPR_EXIT 19 -#define KVM_REQ_SCAN_IOAPIC 20 -#define KVM_REQ_GLOBAL_CLOCK_UPDATE 21 -#define KVM_REQ_ENABLE_IBS 22 -#define KVM_REQ_DISABLE_IBS 23 -#define KVM_REQ_APIC_PAGE_RELOAD 24 -#define KVM_REQ_SMI 25 -#define KVM_REQ_HV_CRASH 26 -#define KVM_REQ_IOAPIC_EOI_EXIT 27 -#define KVM_REQ_HV_RESET 28 -#define KVM_REQ_HV_EXIT 29 -#define KVM_REQ_HV_STIMER 30 +#define KVM_REQ_EPR_EXIT 18 +#define KVM_REQ_SCAN_IOAPIC 19 +#define KVM_REQ_GLOBAL_CLOCK_UPDATE 20 +#define KVM_REQ_ENABLE_IBS 21 +#define KVM_REQ_DISABLE_IBS 22 +#define KVM_REQ_APIC_PAGE_RELOAD 23 +#define KVM_REQ_SMI 24 +#define KVM_REQ_HV_CRASH 25 +#define KVM_REQ_IOAPIC_EOI_EXIT 26 +#define KVM_REQ_HV_RESET 27 +#define KVM_REQ_HV_EXIT 28 +#define KVM_REQ_HV_STIMER 29 #define KVM_USERSPACE_IRQ_SOURCE_ID 0 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1 @@ -685,7 +684,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm); void kvm_reload_remote_mmus(struct kvm *kvm); void kvm_make_mclock_inprogress_request(struct kvm *kvm); void kvm_make_scan_ioapic_request(struct kvm *kvm); -bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req); +bool kvm_make_all_cpus_request(struct kvm *kvm, int req); long kvm_arch_dev_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index be3cef1..0100a19 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -156,7 +156,7 @@ static void ack_flush(void *_completed) { } -bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) +bool kvm_make_all_cpus_request(struct kvm *kvm, int req) { int i, cpu, me; cpumask_var_t cpus; @@ -167,7 +167,9 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req) me = get_cpu(); kvm_for_each_vcpu(i, vcpu, kvm) { - kvm_make_request(req, vcpu); + if (req >= 0) + kvm_make_request(req, vcpu); + cpu = vcpu->cpu; /* Set ->requests bit before we read ->mode */ @@ -208,7 +210,7 @@ void kvm_reload_remote_mmus(struct kvm *kvm) void kvm_make_mclock_inprogress_request(struct kvm *kvm) { - kvm_make_all_cpus_request(kvm, KVM_REQ_MCLOCK_INPROGRESS); + kvm_make_all_cpus_request(kvm, -1); } void kvm_make_scan_ioapic_request(struct kvm *kvm) -- 2.1.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-08 12:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-07 11:43 [PATCH 0/2] KVM: Save two bits in vcpu->requests Takuya Yoshikawa 2016-01-07 11:43 ` [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit " Takuya Yoshikawa 2016-01-07 12:37 ` Paolo Bonzini 2016-01-08 1:47 ` Takuya Yoshikawa 2016-01-08 12:22 ` Paolo Bonzini 2016-01-07 11:43 ` [PATCH 2/2] KVM: Remove KVM_REQ_MCLOCK_INPROGRESS " Takuya Yoshikawa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).