| Message ID | 1452166994-11344-2-git-send-email-yoshikawa_takuya_b1@lab.ntt.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(-)