[PATCH 0/2] KVM: Save two bits in vcpu->requests

[PATCH 0/2] KVM: Save two bits in vcpu->requests

[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

[PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests

[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

[PATCH 2/2] KVM: Remove KVM_REQ_MCLOCK_INPROGRESS to save a bit in vcpu->requests

[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

Re: [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests

[Paolo Bonzini]

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
> 

Re: [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests

[Takuya Yoshikawa]

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

Re: [PATCH 1/2] KVM: Remove unused KVM_REQ_KICK to save a bit in vcpu->requests

[Paolo Bonzini]

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