lkml.org 
[lkml]   [2020]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 4/5] kvm: Replace vcpu->swait with rcuwait
On Tue, 21 Apr 2020 21:07:38 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> The use of any sort of waitqueue (simple or regular) for
> wait/waking vcpus has always been an overkill and semantically
> wrong. Because this is per-vcpu (which is blocked) there is
> only ever a single waiting vcpu, thus no need for any sort of
> queue.
>
> As such, make use of the rcuwait primitive, with the following
> considerations:
>
> - rcuwait already provides the proper barriers that serialize
> concurrent waiter and waker.
>
> - Task wakeup is done in rcu read critical region, with a
> stable task pointer.
>
> - Because there is no concurrency among waiters, we need
> not worry about rcuwait_wait_event() calls corrupting
> the wait->task. As a consequence, this saves the locking
> done in swait when modifying the queue. This also applies
> to per-vcore wait for powerpc kvm-hv.
>
> The x86 tscdeadline_latency test mentioned in 8577370fb0cb
> ("KVM: Use simple waitqueue for vcpu->wq") shows that, on avg,
> latency is reduced by around 15-20% with this change.
>
> Cc: Paul Mackerras <paulus@ozlabs.org>
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: linux-mips@vger.kernel.org
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> arch/mips/kvm/mips.c | 6 ++----
> arch/powerpc/include/asm/kvm_book3s.h | 2 +-
> arch/powerpc/include/asm/kvm_host.h | 2 +-
> arch/powerpc/kvm/book3s_hv.c | 22 ++++++++--------------
> arch/powerpc/kvm/powerpc.c | 2 +-
> arch/x86/kvm/lapic.c | 2 +-
> include/linux/kvm_host.h | 10 +++++-----
> virt/kvm/arm/arch_timer.c | 2 +-
> virt/kvm/arm/arm.c | 9 +++++----
> virt/kvm/async_pf.c | 3 +--
> virt/kvm/kvm_main.c | 19 +++++++++----------
> 11 files changed, 35 insertions(+), 44 deletions(-)

[...]

I should have tested it *before* acking it, really.

> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 93bd59b46848..b2805105bbe5 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -593,7 +593,7 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
> if (map.emul_ptimer)
> soft_timer_cancel(&map.emul_ptimer->hrtimer);
>
> - if (swait_active(kvm_arch_vcpu_wq(vcpu)))
> + if (rcu_dereference(kvm_arch_vpu_get_wait(vcpu)) != NULL)

This doesn't compile (wrong function name, and rcu_dereference takes a
variable). But whatever it would do if we fixed it looks dodgy. it isn't
the rcuwait structure that you want to dereference, but rcuwait->task
(we are checking whether we are called because we are blocking or being
preempted).

> kvm_timer_blocking(vcpu);
>
> /*
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 48d0ec44ad77..f94a10bb1251 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -579,16 +579,17 @@ void kvm_arm_resume_guest(struct kvm *kvm)
>
> kvm_for_each_vcpu(i, vcpu, kvm) {
> vcpu->arch.pause = false;
> - swake_up_one(kvm_arch_vcpu_wq(vcpu));
> + rcuwait_wake_up(kvm_arch_vcpu_get_wait(vcpu));
> }
> }
>
> static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
> {
> - struct swait_queue_head *wq = kvm_arch_vcpu_wq(vcpu);
> + struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);
>
> - swait_event_interruptible_exclusive(*wq, ((!vcpu->arch.power_off) &&
> - (!vcpu->arch.pause)));
> + rcuwait_wait_event(*wait,
> + (!vcpu->arch.power_off) &&(!vcpu->arch.pause),
> + TASK_INTERRUPTIBLE);

As noticed by the kbuild robot, this doesn't compile either.

I fixed it as follow, and it survived a very basic test run in a model
(more testing later).

Thanks,

M.

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index b2805105bbe56..2dbd14dcae9fb 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -569,6 +569,7 @@ bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)

void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
{
+ struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);
struct arch_timer_cpu *timer = vcpu_timer(vcpu);
struct timer_map map;

@@ -593,7 +594,7 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
if (map.emul_ptimer)
soft_timer_cancel(&map.emul_ptimer->hrtimer);

- if (rcu_dereference(kvm_arch_vpu_get_wait(vcpu)) != NULL)
+ if (rcu_dereference(wait->task))
kvm_timer_blocking(vcpu);

/*
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index f94a10bb1251b..479f36d02418d 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -587,7 +587,7 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu)
{
struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);

- rcuwait_wait_event(*wait,
+ rcuwait_wait_event(wait,
(!vcpu->arch.power_off) &&(!vcpu->arch.pause),
TASK_INTERRUPTIBLE);

--
Jazz is not dead. It just smells funny...

\
 
 \ /
  Last update: 2020-04-23 10:42    [W:0.602 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site