2020-02-28 03:21:31

by Wanpeng Li

Subject: [PATCH v3] KVM: X86: Just one leader to trigger kvmclock sync request

From: Wanpeng Li <[email protected]>

In the progress of vCPUs creation, it queues a kvmclock sync worker to the global
workqueue before each vCPU creation completes. The workqueue subsystem guarantees
not to queue the already queued work, however, we can make the logic more clear by
make just one leader to trigger this kvmclock sync request and save on cacheline
boucing due to test_and_set_bit.

Signed-off-by: Wanpeng Li <[email protected]>
---
v2 -> v3:
* update patch description
v1 -> v2:
* check vcpu->vcpu_idx

arch/x86/kvm/x86.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fb5d64e..79bc995 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9390,8 +9390,9 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
if (!kvmclock_periodic_sync)
return;

- schedule_delayed_work(&kvm->arch.kvmclock_sync_work,
- KVMCLOCK_SYNC_PERIOD);
+ if (vcpu->vcpu_idx == 0)
+ schedule_delayed_work(&kvm->arch.kvmclock_sync_work,
+ KVMCLOCK_SYNC_PERIOD);
}

void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
--
2.7.4

2020-03-02 13:02:09

by Vitaly Kuznetsov

Subject: Re: [PATCH v3] KVM: X86: Just one leader to trigger kvmclock sync request

Wanpeng Li <[email protected]> writes:

> From: Wanpeng Li <[email protected]>
>
> In the progress of vCPUs creation, it queues a kvmclock sync worker to the global
> workqueue before each vCPU creation completes. The workqueue subsystem guarantees
> not to queue the already queued work, however, we can make the logic more clear by
> make just one leader to trigger this kvmclock sync request and save on cacheline
> boucing due to test_and_set_bit.
>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> v2 -> v3:
> * update patch description
> v1 -> v2:
> * check vcpu->vcpu_idx
>
> arch/x86/kvm/x86.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fb5d64e..79bc995 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9390,8 +9390,9 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
> if (!kvmclock_periodic_sync)
> return;
>
> - schedule_delayed_work(&kvm->arch.kvmclock_sync_work,
> - KVMCLOCK_SYNC_PERIOD);
> + if (vcpu->vcpu_idx == 0)
> + schedule_delayed_work(&kvm->arch.kvmclock_sync_work,
> + KVMCLOCK_SYNC_PERIOD);

I would've merged this new check with !kvmclock_periodic_sync above
making it more obvious when the work is scheduled

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5de200663f51..93550976f991 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9389,11 +9389,9 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)

mutex_unlock(&vcpu->mutex);

- if (!kvmclock_periodic_sync)
- return;
-
- schedule_delayed_work(&kvm->arch.kvmclock_sync_work,
- KVMCLOCK_SYNC_PERIOD);
+ if (vcpu->vcpu_idx == 0 && kvmclock_periodic_sync)
+ schedule_delayed_work(&kvm->arch.kvmclock_sync_work,
+ KVMCLOCK_SYNC_PERIOD);
}

> }
>
> void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)

With or without the change mentioned above,
Reviewed-by: Vitaly Kuznetsov <[email protected]>

--
Vitaly

2020-03-02 18:23:32

by Paolo Bonzini

Subject: Re: [PATCH v3] KVM: X86: Just one leader to trigger kvmclock sync request

On 02/03/20 14:01, Vitaly Kuznetsov wrote:
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9389,11 +9389,9 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
>
> mutex_unlock(&vcpu->mutex);
>
> - if (!kvmclock_periodic_sync)
> - return;
> -
> - schedule_delayed_work(&kvm->arch.kvmclock_sync_work,
> - KVMCLOCK_SYNC_PERIOD);
> + if (vcpu->vcpu_idx == 0 && kvmclock_periodic_sync)
> + schedule_delayed_work(&kvm->arch.kvmclock_sync_work,
> + KVMCLOCK_SYNC_PERIOD);
> }

> Reviewed-by: Vitaly Kuznetsov <[email protected]>

Good idea, I squashed the change.

Paolo