Re: [PATCH 18/15] kvm: Replace vcpu->swait with rcuwait

Paolo Bonzini
2020-03-20T11:21:02Z

On 20/03/20 09:55, Davidlohr Bueso wrote:
> Only compiled and tested on x86.

It shows :) as the __KVM_HAVE_ARCH_WQP case is broken. But no problem,
Paul and I can pick this up and fix it.

This is missing:

diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index 506e4df2d730..6e5d85ba588d 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -78,7 +78,7 @@ struct kvmppc_vcore {
struct kvm_vcpu *runnable_threads[MAX_SMT_THREADS];
struct list_head preempt_list;
spinlock_t lock;
- struct swait_queue_head wq;
+ struct rcuwait wait;
spinlock_t stoltb_lock; /* protects stolen_tb and preempt_tb */
u64 stolen_tb;
u64 preempt_tb;
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 1af96fb5dc6f..8c8122c30b89 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -754,7 +754,7 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
if (err)
goto out_vcpu_uninit;

- vcpu->arch.wqp = &vcpu->wq;
+ vcpu->arch.waitp = &vcpu->wait;
kvmppc_create_vcpu_debugfs(vcpu, vcpu->vcpu_id);
return 0;

and...

> -static inline struct swait_queue_head *kvm_arch_vcpu_wq(struct kvm_vcpu *vcpu)
> +static inline struct rcuwait *kvm_arch_vcpu_get_wait(struct kvm_vcpu *vcpu)
> {
> #ifdef __KVM_HAVE_ARCH_WQP
> - return vcpu->arch.wqp;
> + return vcpu->arch.wait;

... this needs to be vcpu->arch.waitp. That should be it.

Thanks!

Paolo

> #else
> - return &vcpu->wq;
> + return &vcpu->wait;
> #endif
> }
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 0d9438e9de2a..4be71cb58691 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)
> kvm_timer_blocking(vcpu);
>
> /*
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index eda7b624eab8..4a704866e9b6 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);
>
> if (vcpu->arch.power_off || vcpu->arch.pause) {
> /* Awaken to handle a signal, request we sleep again later. */
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index 15e5b037f92d..10b533f641a6 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -80,8 +80,7 @@ static void async_pf_execute(struct work_struct *work)
>
> trace_kvm_async_pf_completed(addr, cr2_or_gpa);
>
> - if (swq_has_sleeper(&vcpu->wq))
> - swake_up_one(&vcpu->wq);
> + rcuwait_wake_up(&vcpu->wait);
>
> mmput(mm);
> kvm_put_kvm(vcpu->kvm);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 70f03ce0e5c1..6b49dcb321e2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -343,7 +343,7 @@ static void kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
> vcpu->kvm = kvm;
> vcpu->vcpu_id = id;
> vcpu->pid = NULL;
> - init_swait_queue_head(&vcpu->wq);
> + rcuwait_init(&vcpu->wait);
> kvm_async_pf_vcpu_init(vcpu);
>
> vcpu->pre_pcpu = -1;
> @@ -2465,9 +2465,8 @@ static int kvm_vcpu_check_block(struct kvm_vcpu *vcpu)
> void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> {
> ktime_t start, cur;
> - DECLARE_SWAITQUEUE(wait);
> - bool waited = false;
> u64 block_ns;
> + int block_check = -EINTR;
>
> kvm_arch_vcpu_blocking(vcpu);
>
> @@ -2487,21 +2486,14 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> ++vcpu->stat.halt_poll_invalid;
> goto out;
> }
> +
> cur = ktime_get();
> } while (single_task_running() && ktime_before(cur, stop));
> }
>
> - for (;;) {
> - prepare_to_swait_exclusive(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
> -
> - if (kvm_vcpu_check_block(vcpu) < 0)
> - break;
> -
> - waited = true;
> - schedule();
> - }
> -
> - finish_swait(&vcpu->wq, &wait);
> + rcuwait_wait_event(&vcpu->wait,
> + (block_check = kvm_vcpu_check_block(vcpu)) < 0,
> + TASK_INTERRUPTIBLE);
> cur = ktime_get();
> out:
> kvm_arch_vcpu_unblocking(vcpu);
> @@ -2525,18 +2517,18 @@ void kvm_vcpu_block(struct kvm_vcpu *vcpu)
> }
> }
>
> - trace_kvm_vcpu_wakeup(block_ns, waited, vcpu_valid_wakeup(vcpu));
> + trace_kvm_vcpu_wakeup(block_ns, block_check < 0 ? false : true,
> + vcpu_valid_wakeup(vcpu));
> kvm_arch_vcpu_block_finish(vcpu);
> }
> EXPORT_SYMBOL_GPL(kvm_vcpu_block);
>
> bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu)
> {
> - struct swait_queue_head *wqp;
> + struct rcuwait *wait;
>
> - wqp = kvm_arch_vcpu_wq(vcpu);
> - if (swq_has_sleeper(wqp)) {
> - swake_up_one(wqp);
> + wait = kvm_arch_vcpu_get_wait(vcpu);
> + if (rcuwait_wake_up(wait)) {
> WRITE_ONCE(vcpu->ready, true);
> ++vcpu->stat.halt_wakeup;
> return true;
> @@ -2678,7 +2670,8 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
> continue;
> if (vcpu == me)
> continue;
> - if (swait_active(&vcpu->wq) && !vcpu_dy_runnable(vcpu))
> + if (rcu_dereference(vcpu->wait.task) &&
> + !vcpu_dy_runnable(vcpu))
> continue;
> if (READ_ONCE(vcpu->preempted) && yield_to_kernel_mode &&
> !kvm_arch_vcpu_in_kernel(vcpu))
>