[-rt,1/2] KVM: use simple waitqueue for vcpu->wq

Message ID	20150114171459.593877145@redhat.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <kvm-owner@kernel.org> Message-Id: <20150114171459.593877145@redhat.com> User-Agent: quilt/0.60-1 Date: Wed, 14 Jan 2015 15:12:52 -0200 From: Marcelo Tosatti <mtosatti@redhat.com> To: linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org Cc: Luiz Capitulino <lcapitulino@redhat.com>, Rik van Riel <riel@redhat.com>, Steven Rostedt <srostedt@redhat.com>, Thomas Gleixner <tglx@linutronix.de>, kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com> Subject: [patch -rt 1/2] KVM: use simple waitqueue for vcpu->wq References: <20150114171251.882318257@redhat.com> Content-Disposition: inline; filename=kvm-wq-use-simple-waitqueue Sender: kvm-owner@vger.kernel.org Precedence: bulk

Marcelo Tosatti Jan. 14, 2015, 5:12 p.m. UTC

The problem:

On -RT, an emulated LAPIC timer instances has the following path:

1) hard interrupt
2) ksoftirqd is scheduled
3) ksoftirqd wakes up vcpu thread
4) vcpu thread is scheduled

This extra context switch introduces unnecessary latency in the 
LAPIC path for a KVM guest.

The solution:

Allow waking up vcpu thread from hardirq context,
thus avoiding the need for ksoftirqd to be scheduled.

Normal waitqueues make use of spinlocks, which on -RT 
are sleepable locks. Therefore, waking up a waitqueue 
waiter involves locking a sleeping lock, which 
is not allowed from hard interrupt context.

cyclictest command line:
# cyclictest -m -n -q -p99 -l 1000000 -h60  -D 1m

This patch reduces the average latency in my tests from 14us to 11us.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

---
 arch/arm/kvm/arm.c                  |    4 ++--
 arch/arm/kvm/psci.c                 |    4 ++--
 arch/mips/kvm/kvm_mips.c            |    8 ++++----
 arch/powerpc/include/asm/kvm_host.h |    4 ++--
 arch/powerpc/kvm/book3s_hv.c        |   20 ++++++++++----------
 arch/s390/include/asm/kvm_host.h    |    2 +-
 arch/s390/kvm/interrupt.c           |   22 ++++++++++------------
 arch/s390/kvm/sigp.c                |   16 ++++++++--------
 arch/x86/kvm/lapic.c                |    6 +++---
 include/linux/kvm_host.h            |    4 ++--
 virt/kvm/async_pf.c                 |    4 ++--
 virt/kvm/kvm_main.c                 |   16 ++++++++--------
 12 files changed, 54 insertions(+), 56 deletions(-)



--
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

Rik van Riel Jan. 14, 2015, 6:22 p.m. UTC | #1

On 01/14/2015 12:12 PM, Marcelo Tosatti wrote:
> The problem:
> 
> On -RT, an emulated LAPIC timer instances has the following path:
> 
> 1) hard interrupt
> 2) ksoftirqd is scheduled
> 3) ksoftirqd wakes up vcpu thread
> 4) vcpu thread is scheduled
> 
> This extra context switch introduces unnecessary latency in the 
> LAPIC path for a KVM guest.

> This patch reduces the average latency in my tests from 14us to 11us.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Acked-by: Rik van Riel <riel@redhat.com>

--
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

Steven Rostedt Jan. 16, 2015, 4:48 p.m. UTC | #2

On Fri, 16 Jan 2015 11:40:45 -0500
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> The problem:
> 
> On -RT, an emulated LAPIC timer instances has the following path:
> 
> 1) hard interrupt
> 2) ksoftirqd is scheduled
> 3) ksoftirqd wakes up vcpu thread
> 4) vcpu thread is scheduled
> 
> This extra context switch introduces unnecessary latency in the 
> LAPIC path for a KVM guest.
> 
> The solution:
> 
> Allow waking up vcpu thread from hardirq context,
> thus avoiding the need for ksoftirqd to be scheduled.
> 
> Normal waitqueues make use of spinlocks, which on -RT 
> are sleepable locks. Therefore, waking up a waitqueue 
> waiter involves locking a sleeping lock, which 
> is not allowed from hard interrupt context.
> 

I have no issue in pulling this into stable-rt, but read below.

> cyclictest command line:
> # cyclictest -m -n -q -p99 -l 1000000 -h60  -D 1m
> 
> This patch reduces the average latency in my tests from 14us to 11us.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> ---
>  arch/arm/kvm/arm.c                  |    4 ++--
>  arch/arm/kvm/psci.c                 |    4 ++--
>  arch/mips/kvm/kvm_mips.c            |    8 ++++----
>  arch/powerpc/include/asm/kvm_host.h |    4 ++--
>  arch/powerpc/kvm/book3s_hv.c        |   20 ++++++++++----------
>  arch/s390/include/asm/kvm_host.h    |    2 +-
>  arch/s390/kvm/interrupt.c           |   22 ++++++++++------------
>  arch/s390/kvm/sigp.c                |   16 ++++++++--------
>  arch/x86/kvm/lapic.c                |    6 +++---
>  include/linux/kvm_host.h            |    4 ++--
>  virt/kvm/async_pf.c                 |    4 ++--
>  virt/kvm/kvm_main.c                 |   16 ++++++++--------
>  12 files changed, 54 insertions(+), 56 deletions(-)
> 
> Index: linux-stable-rt/arch/arm/kvm/arm.c
> ===================================================================
> --- linux-stable-rt.orig/arch/arm/kvm/arm.c	2014-11-25 14:13:39.188899952 -0200
> +++ linux-stable-rt/arch/arm/kvm/arm.c	2014-11-25 14:14:38.620810092 -0200
> @@ -495,9 +495,9 @@
>  
>  static void vcpu_pause(struct kvm_vcpu *vcpu)
>  {
> -	wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu);
> +	struct swait_head *wq = kvm_arch_vcpu_wq(vcpu);
>  
> -	wait_event_interruptible(*wq, !vcpu->arch.pause);
> +	swait_event_interruptible(*wq, !vcpu->arch.pause);
>  }
>  
>  static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu)
> Index: linux-stable-rt/arch/arm/kvm/psci.c
> ===================================================================
> --- linux-stable-rt.orig/arch/arm/kvm/psci.c	2014-11-25 14:13:39.189899951 -0200
> +++ linux-stable-rt/arch/arm/kvm/psci.c	2014-11-25 14:14:38.620810092 -0200
> @@ -36,7 +36,7 @@
>  {
>  	struct kvm *kvm = source_vcpu->kvm;
>  	struct kvm_vcpu *vcpu = NULL, *tmp;
> -	wait_queue_head_t *wq;
> +	struct swait_head *wq;
>  	unsigned long cpu_id;
>  	unsigned long mpidr;
>  	phys_addr_t target_pc;
> @@ -80,7 +80,7 @@
>  	smp_mb();		/* Make sure the above is visible */
>  
>  	wq = kvm_arch_vcpu_wq(vcpu);
> -	wake_up_interruptible(wq);
> +	swait_wake_interruptible(wq);
>  
>  	return KVM_PSCI_RET_SUCCESS;
>  }
> Index: linux-stable-rt/arch/mips/kvm/kvm_mips.c
> ===================================================================
> --- linux-stable-rt.orig/arch/mips/kvm/kvm_mips.c	2014-11-25 14:13:39.191899948 -0200
> +++ linux-stable-rt/arch/mips/kvm/kvm_mips.c	2014-11-25 14:14:38.621810091 -0200
> @@ -464,8 +464,8 @@
>  
>  	dvcpu->arch.wait = 0;
>  
> -	if (waitqueue_active(&dvcpu->wq)) {
> -		wake_up_interruptible(&dvcpu->wq);
> +	if (swaitqueue_active(&dvcpu->wq)) {
> +		swait_wake_interruptible(&dvcpu->wq);
>  	}
>  
>  	return 0;
> @@ -971,8 +971,8 @@
>  	kvm_mips_callbacks->queue_timer_int(vcpu);
>  
>  	vcpu->arch.wait = 0;
> -	if (waitqueue_active(&vcpu->wq)) {
> -		wake_up_interruptible(&vcpu->wq);
> +	if (swaitqueue_active(&vcpu->wq)) {
> +		swait_wake_nterruptible(&vcpu->wq);

Sure you compiled this patch?

>  	}
>  }
>  
> Index: linux-stable-rt/arch/powerpc/include/asm/kvm_host.h
> ===================================================================
> --- linux-stable-rt.orig/arch/powerpc/include/asm/kvm_host.h	2014-11-25 14:13:39.193899944 -0200
> +++ linux-stable-rt/arch/powerpc/include/asm/kvm_host.h	2014-11-25 14:14:38.621810091 -0200
> @@ -295,7 +295,7 @@
>  	u8 in_guest;
>  	struct list_head runnable_threads;
>  	spinlock_t lock;
> -	wait_queue_head_t wq;
> +	struct swait_head wq;
>  	u64 stolen_tb;
>  	u64 preempt_tb;
>  	struct kvm_vcpu *runner;
> @@ -612,7 +612,7 @@
>  	u8 prodded;
>  	u32 last_inst;
>  
> -	wait_queue_head_t *wqp;
> +	struct swait_head *wqp;
>  	struct kvmppc_vcore *vcore;
>  	int ret;
>  	int trap;
> Index: linux-stable-rt/arch/powerpc/kvm/book3s_hv.c
> ===================================================================
> --- linux-stable-rt.orig/arch/powerpc/kvm/book3s_hv.c	2014-11-25 14:13:39.195899942 -0200
> +++ linux-stable-rt/arch/powerpc/kvm/book3s_hv.c	2014-11-25 14:14:38.625810085 -0200
> @@ -74,11 +74,11 @@
>  {
>  	int me;
>  	int cpu = vcpu->cpu;
> -	wait_queue_head_t *wqp;
> +	struct swait_head *wqp;
>  
>  	wqp = kvm_arch_vcpu_wq(vcpu);
> -	if (waitqueue_active(wqp)) {
> -		wake_up_interruptible(wqp);
> +	if (swaitqueue_active(wqp)) {
> +		swait_wake_interruptible(wqp);
>  		++vcpu->stat.halt_wakeup;
>  	}
>  
> @@ -583,8 +583,8 @@
>  		tvcpu->arch.prodded = 1;
>  		smp_mb();
>  		if (vcpu->arch.ceded) {
> -			if (waitqueue_active(&vcpu->wq)) {
> -				wake_up_interruptible(&vcpu->wq);
> +			if (swaitqueue_active(&vcpu->wq)) {
> +				swait_wake_interruptible(&vcpu->wq);
>  				vcpu->stat.halt_wakeup++;
>  			}
>  		}
> @@ -1199,7 +1199,7 @@
>  		if (vcore) {
>  			INIT_LIST_HEAD(&vcore->runnable_threads);
>  			spin_lock_init(&vcore->lock);
> -			init_waitqueue_head(&vcore->wq);
> +			init_swait_head(&vcore->wq);
>  			vcore->preempt_tb = TB_NIL;
>  			vcore->lpcr = kvm->arch.lpcr;
>  			vcore->first_vcpuid = core * threads_per_core;
> @@ -1566,13 +1566,13 @@
>   */
>  static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
>  {
> -	DEFINE_WAIT(wait);
> +	DEFINE_SWAITER(wait);
>  
> -	prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> +	swait_prepare(&vc->wq, &wait, TASK_INTERRUPTIBLE);
>  	vc->vcore_state = VCORE_SLEEPING;
>  	spin_unlock(&vc->lock);
>  	schedule();
> -	finish_wait(&vc->wq, &wait);
> +	swait_finish(&vc->wq, &wait);
>  	spin_lock(&vc->lock);
>  	vc->vcore_state = VCORE_INACTIVE;
>  }
> @@ -1613,7 +1613,7 @@
>  			kvmppc_create_dtl_entry(vcpu, vc);
>  			kvmppc_start_thread(vcpu);
>  		} else if (vc->vcore_state == VCORE_SLEEPING) {
> -			wake_up(&vc->wq);
> +			swait_wake(&vc->wq);

I notice everywhere you have a swait_wake_interruptible() but here. Is
there a reason why?

IIRC, Peter wants to make swait wakeup usage homogenous. That is, you
either sleep in an interruptible state, or you don't. You can't mix and
match it.

Peter is that what you plan?

-- Steve


>  		}
>  
>  	}
> Index: linux-stable-rt/arch/s390/include/asm/kvm_host.h
> ===================================================================
> --- linux-stable-rt.orig/arch/s390/include/asm/kvm_host.h	2014-11-25 14:13:39.196899940 -0200
> +++ linux-stable-rt/arch/s390/include/asm/kvm_host.h	2014-11-25 14:14:38.627810082 -0200
> @@ -233,7 +233,7 @@
>  	atomic_t active;
>  	struct kvm_s390_float_interrupt *float_int;
>  	int timer_due; /* event indicator for waitqueue below */
> -	wait_queue_head_t *wq;
> +	struct swait_head *wq;
>  	atomic_t *cpuflags;
>  	unsigned int action_bits;
>  };
> Index: linux-stable-rt/arch/s390/kvm/interrupt.c
> ===================================================================
> --- linux-stable-rt.orig/arch/s390/kvm/interrupt.c	2014-11-25 14:13:39.197899939 -0200
> +++ linux-stable-rt/arch/s390/kvm/interrupt.c	2014-11-25 14:14:38.630810077 -0200
> @@ -403,7 +403,7 @@
>  int kvm_s390_handle_wait(struct kvm_vcpu *vcpu)
>  {
>  	u64 now, sltime;
> -	DECLARE_WAITQUEUE(wait, current);
> +	DECLARE_SWAITER(wait);
>  
>  	vcpu->stat.exit_wait_state++;
>  	if (kvm_cpu_has_interrupt(vcpu))
> @@ -440,12 +440,11 @@
>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
>  	spin_lock(&vcpu->arch.local_int.float_int->lock);
>  	spin_lock_bh(&vcpu->arch.local_int.lock);
> -	add_wait_queue(&vcpu->wq, &wait);
> +	swait_prepare(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
>  	while (list_empty(&vcpu->arch.local_int.list) &&
>  		list_empty(&vcpu->arch.local_int.float_int->list) &&
>  		(!vcpu->arch.local_int.timer_due) &&
>  		!signal_pending(current)) {
> -		set_current_state(TASK_INTERRUPTIBLE);
>  		spin_unlock_bh(&vcpu->arch.local_int.lock);
>  		spin_unlock(&vcpu->arch.local_int.float_int->lock);
>  		schedule();
> @@ -453,8 +452,7 @@
>  		spin_lock_bh(&vcpu->arch.local_int.lock);
>  	}
>  	__unset_cpu_idle(vcpu);
> -	__set_current_state(TASK_RUNNING);
> -	remove_wait_queue(&vcpu->wq, &wait);
> +	swait_finish(&vcpu->wq, &wait);
>  	spin_unlock_bh(&vcpu->arch.local_int.lock);
>  	spin_unlock(&vcpu->arch.local_int.float_int->lock);
>  	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
> @@ -469,8 +467,8 @@
>  
>  	spin_lock(&vcpu->arch.local_int.lock);
>  	vcpu->arch.local_int.timer_due = 1;
> -	if (waitqueue_active(&vcpu->wq))
> -		wake_up_interruptible(&vcpu->wq);
> +	if (swaitqueue_active(&vcpu->wq))
> +		swait_wake_interruptible(&vcpu->wq);
>  	spin_unlock(&vcpu->arch.local_int.lock);
>  }
>  
> @@ -617,7 +615,7 @@
>  	spin_lock_bh(&li->lock);
>  	list_add(&inti->list, &li->list);
>  	atomic_set(&li->active, 1);
> -	BUG_ON(waitqueue_active(li->wq));
> +	BUG_ON(swaitqueue_active(li->wq));
>  	spin_unlock_bh(&li->lock);
>  	return 0;
>  }
> @@ -750,8 +748,8 @@
>  	li = fi->local_int[sigcpu];
>  	spin_lock_bh(&li->lock);
>  	atomic_set_mask(CPUSTAT_EXT_INT, li->cpuflags);
> -	if (waitqueue_active(li->wq))
> -		wake_up_interruptible(li->wq);
> +	if (swaitqueue_active(li->wq))
> +		swait_wake_interruptible(li->wq);
>  	spin_unlock_bh(&li->lock);
>  	spin_unlock(&fi->lock);
>  	mutex_unlock(&kvm->lock);
> @@ -836,8 +834,8 @@
>  	if (inti->type == KVM_S390_SIGP_STOP)
>  		li->action_bits |= ACTION_STOP_ON_STOP;
>  	atomic_set_mask(CPUSTAT_EXT_INT, li->cpuflags);
> -	if (waitqueue_active(&vcpu->wq))
> -		wake_up_interruptible(&vcpu->wq);
> +	if (swaitqueue_active(&vcpu->wq))
> +		swait_wake_interruptible(&vcpu->wq);
>  	spin_unlock_bh(&li->lock);
>  	mutex_unlock(&vcpu->kvm->lock);
>  	return 0;
> Index: linux-stable-rt/arch/s390/kvm/sigp.c
> ===================================================================
> --- linux-stable-rt.orig/arch/s390/kvm/sigp.c	2014-11-25 14:13:39.198899937 -0200
> +++ linux-stable-rt/arch/s390/kvm/sigp.c	2014-11-25 14:14:38.633810073 -0200
> @@ -79,8 +79,8 @@
>  	list_add_tail(&inti->list, &li->list);
>  	atomic_set(&li->active, 1);
>  	atomic_set_mask(CPUSTAT_EXT_INT, li->cpuflags);
> -	if (waitqueue_active(li->wq))
> -		wake_up_interruptible(li->wq);
> +	if (swaitqueue_active(li->wq))
> +		swait_wake_interruptible(li->wq);
>  	spin_unlock_bh(&li->lock);
>  	rc = SIGP_CC_ORDER_CODE_ACCEPTED;
>  	VCPU_EVENT(vcpu, 4, "sent sigp emerg to cpu %x", cpu_addr);
> @@ -148,8 +148,8 @@
>  	list_add_tail(&inti->list, &li->list);
>  	atomic_set(&li->active, 1);
>  	atomic_set_mask(CPUSTAT_EXT_INT, li->cpuflags);
> -	if (waitqueue_active(li->wq))
> -		wake_up_interruptible(li->wq);
> +	if (swaitqueue_active(li->wq))
> +		swait_wake_interruptible(li->wq);
>  	spin_unlock_bh(&li->lock);
>  	rc = SIGP_CC_ORDER_CODE_ACCEPTED;
>  	VCPU_EVENT(vcpu, 4, "sent sigp ext call to cpu %x", cpu_addr);
> @@ -179,8 +179,8 @@
>  	atomic_set(&li->active, 1);
>  	atomic_set_mask(CPUSTAT_STOP_INT, li->cpuflags);
>  	li->action_bits |= action;
> -	if (waitqueue_active(li->wq))
> -		wake_up_interruptible(li->wq);
> +	if (swaitqueue_active(li->wq))
> +		swait_wake_interruptible(li->wq);
>  out:
>  	spin_unlock_bh(&li->lock);
>  
> @@ -288,8 +288,8 @@
>  
>  	list_add_tail(&inti->list, &li->list);
>  	atomic_set(&li->active, 1);
> -	if (waitqueue_active(li->wq))
> -		wake_up_interruptible(li->wq);
> +	if (swaitqueue_active(li->wq))
> +		swait_wake_interruptible(li->wq);
>  	rc = SIGP_CC_ORDER_CODE_ACCEPTED;
>  
>  	VCPU_EVENT(vcpu, 4, "set prefix of cpu %02x to %x", cpu_addr, address);
> Index: linux-stable-rt/arch/x86/kvm/lapic.c
> ===================================================================
> --- linux-stable-rt.orig/arch/x86/kvm/lapic.c	2014-11-25 14:13:39.199899935 -0200
> +++ linux-stable-rt/arch/x86/kvm/lapic.c	2015-01-14 15:02:41.135771065 -0200
> @@ -1533,7 +1533,7 @@
>  	struct kvm_timer *ktimer = container_of(data, struct kvm_timer, timer);
>  	struct kvm_lapic *apic = container_of(ktimer, struct kvm_lapic, lapic_timer);
>  	struct kvm_vcpu *vcpu = apic->vcpu;
> -	wait_queue_head_t *q = &vcpu->wq;
> +	struct swait_head *q = &vcpu->wq;
>  
>  	/*
>  	 * There is a race window between reading and incrementing, but we do
> @@ -1547,8 +1547,8 @@
>  		kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
>  	}
>  
> -	if (waitqueue_active(q))
> -		wake_up_interruptible(q);
> +	if (swaitqueue_active(q))
> +		swait_wake_interruptible(q);
>  
>  	if (lapic_is_periodic(apic)) {
>  		hrtimer_add_expires_ns(&ktimer->timer, ktimer->period);
> Index: linux-stable-rt/include/linux/kvm_host.h
> ===================================================================
> --- linux-stable-rt.orig/include/linux/kvm_host.h	2014-11-25 14:13:39.201899932 -0200
> +++ linux-stable-rt/include/linux/kvm_host.h	2014-11-25 14:14:38.638810065 -0200
> @@ -231,7 +231,7 @@
>  
>  	int fpu_active;
>  	int guest_fpu_loaded, guest_xcr0_loaded;
> -	wait_queue_head_t wq;
> +	struct swait_head wq;
>  	struct pid *pid;
>  	int sigset_active;
>  	sigset_t sigset;
> @@ -677,7 +677,7 @@
>  }
>  #endif
>  
> -static inline wait_queue_head_t *kvm_arch_vcpu_wq(struct kvm_vcpu *vcpu)
> +static inline struct swait_head *kvm_arch_vcpu_wq(struct kvm_vcpu *vcpu)
>  {
>  #ifdef __KVM_HAVE_ARCH_WQP
>  	return vcpu->arch.wqp;
> Index: linux-stable-rt/virt/kvm/async_pf.c
> ===================================================================
> --- linux-stable-rt.orig/virt/kvm/async_pf.c	2014-11-25 14:13:39.202899931 -0200
> +++ linux-stable-rt/virt/kvm/async_pf.c	2014-11-25 14:14:38.639810063 -0200
> @@ -82,8 +82,8 @@
>  
>  	trace_kvm_async_pf_completed(addr, gva);
>  
> -	if (waitqueue_active(&vcpu->wq))
> -		wake_up_interruptible(&vcpu->wq);
> +	if (swaitqueue_active(&vcpu->wq))
> +		swait_wake_interruptible(&vcpu->wq);
>  
>  	mmput(mm);
>  	kvm_put_kvm(vcpu->kvm);
> Index: linux-stable-rt/virt/kvm/kvm_main.c
> ===================================================================
> --- linux-stable-rt.orig/virt/kvm/kvm_main.c	2014-11-25 14:13:39.204899928 -0200
> +++ linux-stable-rt/virt/kvm/kvm_main.c	2014-11-25 14:14:38.641810060 -0200
> @@ -219,7 +219,7 @@
>  	vcpu->kvm = kvm;
>  	vcpu->vcpu_id = id;
>  	vcpu->pid = NULL;
> -	init_waitqueue_head(&vcpu->wq);
> +	init_swait_head(&vcpu->wq);
>  	kvm_async_pf_vcpu_init(vcpu);
>  
>  	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> @@ -1675,10 +1675,10 @@
>   */
>  void kvm_vcpu_block(struct kvm_vcpu *vcpu)
>  {
> -	DEFINE_WAIT(wait);
> +	DEFINE_SWAITER(wait);
>  
>  	for (;;) {
> -		prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
> +		swait_prepare(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
>  
>  		if (kvm_arch_vcpu_runnable(vcpu)) {
>  			kvm_make_request(KVM_REQ_UNHALT, vcpu);
> @@ -1692,7 +1692,7 @@
>  		schedule();
>  	}
>  
> -	finish_wait(&vcpu->wq, &wait);
> +	swait_finish(&vcpu->wq, &wait);
>  }
>  EXPORT_SYMBOL_GPL(kvm_vcpu_block);
>  
> @@ -1704,11 +1704,11 @@
>  {
>  	int me;
>  	int cpu = vcpu->cpu;
> -	wait_queue_head_t *wqp;
> +	struct swait_head *wqp;
>  
>  	wqp = kvm_arch_vcpu_wq(vcpu);
> -	if (waitqueue_active(wqp)) {
> -		wake_up_interruptible(wqp);
> +	if (swaitqueue_active(wqp)) {
> +		swait_wake_interruptible(wqp);
>  		++vcpu->stat.halt_wakeup;
>  	}
>  
> @@ -1814,7 +1814,7 @@
>  				continue;
>  			if (vcpu == me)
>  				continue;
> -			if (waitqueue_active(&vcpu->wq))
> +			if (swaitqueue_active(&vcpu->wq))
>  				continue;
>  			if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
>  				continue;
> 

--
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

Steven Rostedt Jan. 16, 2015, 4:56 p.m. UTC | #3

On Fri, 16 Jan 2015 11:48:46 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> >  static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
> >  {
> > -	DEFINE_WAIT(wait);
> > +	DEFINE_SWAITER(wait);
> >  
> > -	prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> > +	swait_prepare(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> >  	vc->vcore_state = VCORE_SLEEPING;
> >  	spin_unlock(&vc->lock);
> >  	schedule();
> > -	finish_wait(&vc->wq, &wait);
> > +	swait_finish(&vc->wq, &wait);
> >  	spin_lock(&vc->lock);
> >  	vc->vcore_state = VCORE_INACTIVE;
> >  }
> > @@ -1613,7 +1613,7 @@
> >  			kvmppc_create_dtl_entry(vcpu, vc);
> >  			kvmppc_start_thread(vcpu);
> >  		} else if (vc->vcore_state == VCORE_SLEEPING) {
> > -			wake_up(&vc->wq);
> > +			swait_wake(&vc->wq);
> 
> I notice everywhere you have a swait_wake_interruptible() but here. Is
> there a reason why?
> 

Of course I could have misread this patch, and this specific work queue
will only be woken up by swait_wake() and not
swait_wake_interruptible(). If that's the case, then its fine as is
(except for that typo I pointed out before).

-- Steve
--
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

Peter Zijlstra Jan. 17, 2015, 7:57 a.m. UTC | #4

On Fri, Jan 16, 2015 at 11:48:46AM -0500, Steven Rostedt wrote:
> I notice everywhere you have a swait_wake_interruptible() but here. Is
> there a reason why?
> 
> IIRC, Peter wants to make swait wakeup usage homogenous. That is, you
> either sleep in an interruptible state, or you don't. You can't mix and
> match it.
> 
> Peter is that what you plan?

Yes, this is required for the single wakeup case to be bounded.

If you have both INTERRUPTIBLE and UNINTERRUPTIBLE waiters on a list,
and you were allowed to do INTERRUPTIBLE/UNINTERRUPTIBLE wakeups, the
wakeup would have to do O(n) iteration to find a matching waiter.

Seeing that the entire purpose of simple wait queues was to retain RT
properties, that's entirely retarded ;-)
--
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

Marcelo Tosatti Jan. 19, 2015, 2:41 p.m. UTC | #5

On Fri, Jan 16, 2015 at 11:48:46AM -0500, Steven Rostedt wrote:
> > @@ -971,8 +971,8 @@
> >  	kvm_mips_callbacks->queue_timer_int(vcpu);
> >  
> >  	vcpu->arch.wait = 0;
> > -	if (waitqueue_active(&vcpu->wq)) {
> > -		wake_up_interruptible(&vcpu->wq);
> > +	if (swaitqueue_active(&vcpu->wq)) {
> > +		swait_wake_nterruptible(&vcpu->wq);
> 
> Sure you compiled this patch?

Only x86.

> > Index: linux-stable-rt/arch/powerpc/include/asm/kvm_host.h
> > ===================================================================
> > --- linux-stable-rt.orig/arch/powerpc/include/asm/kvm_host.h	2014-11-25 14:13:39.193899944 -0200
> > +++ linux-stable-rt/arch/powerpc/include/asm/kvm_host.h	2014-11-25 14:14:38.621810091 -0200
> > @@ -295,7 +295,7 @@
> >  	u8 in_guest;
> >  	struct list_head runnable_threads;
> >  	spinlock_t lock;
> > -	wait_queue_head_t wq;
> > +	struct swait_head wq;
> >  	u64 stolen_tb;
> >  	u64 preempt_tb;
> >  	struct kvm_vcpu *runner;
> > @@ -612,7 +612,7 @@
> >  	u8 prodded;
> >  	u32 last_inst;
> >  
> > -	wait_queue_head_t *wqp;
> > +	struct swait_head *wqp;
> >  	struct kvmppc_vcore *vcore;
> >  	int ret;
> >  	int trap;
> > Index: linux-stable-rt/arch/powerpc/kvm/book3s_hv.c
> > ===================================================================
> > --- linux-stable-rt.orig/arch/powerpc/kvm/book3s_hv.c	2014-11-25 14:13:39.195899942 -0200
> > +++ linux-stable-rt/arch/powerpc/kvm/book3s_hv.c	2014-11-25 14:14:38.625810085 -0200
> > @@ -74,11 +74,11 @@
> >  {
> >  	int me;
> >  	int cpu = vcpu->cpu;
> > -	wait_queue_head_t *wqp;
> > +	struct swait_head *wqp;
> >  
> >  	wqp = kvm_arch_vcpu_wq(vcpu);
> > -	if (waitqueue_active(wqp)) {
> > -		wake_up_interruptible(wqp);
> > +	if (swaitqueue_active(wqp)) {
> > +		swait_wake_interruptible(wqp);
> >  		++vcpu->stat.halt_wakeup;
> >  	}
> >  
> > @@ -583,8 +583,8 @@
> >  		tvcpu->arch.prodded = 1;
> >  		smp_mb();
> >  		if (vcpu->arch.ceded) {
> > -			if (waitqueue_active(&vcpu->wq)) {
> > -				wake_up_interruptible(&vcpu->wq);
> > +			if (swaitqueue_active(&vcpu->wq)) {
> > +				swait_wake_interruptible(&vcpu->wq);
> >  				vcpu->stat.halt_wakeup++;
> >  			}
> >  		}
> > @@ -1199,7 +1199,7 @@
> >  		if (vcore) {
> >  			INIT_LIST_HEAD(&vcore->runnable_threads);
> >  			spin_lock_init(&vcore->lock);
> > -			init_waitqueue_head(&vcore->wq);
> > +			init_swait_head(&vcore->wq);
> >  			vcore->preempt_tb = TB_NIL;
> >  			vcore->lpcr = kvm->arch.lpcr;
> >  			vcore->first_vcpuid = core * threads_per_core;
> > @@ -1566,13 +1566,13 @@
> >   */
> >  static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
> >  {
> > -	DEFINE_WAIT(wait);
> > +	DEFINE_SWAITER(wait);
> >  
> > -	prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> > +	swait_prepare(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> >  	vc->vcore_state = VCORE_SLEEPING;
> >  	spin_unlock(&vc->lock);
> >  	schedule();
> > -	finish_wait(&vc->wq, &wait);
> > +	swait_finish(&vc->wq, &wait);
> >  	spin_lock(&vc->lock);
> >  	vc->vcore_state = VCORE_INACTIVE;
> >  }
> > @@ -1613,7 +1613,7 @@
> >  			kvmppc_create_dtl_entry(vcpu, vc);
> >  			kvmppc_start_thread(vcpu);
> >  		} else if (vc->vcore_state == VCORE_SLEEPING) {
> > -			wake_up(&vc->wq);
> > +			swait_wake(&vc->wq);
> 
> I notice everywhere you have a swait_wake_interruptible() but here. Is
> there a reason why?
> 
> IIRC, Peter wants to make swait wakeup usage homogenous. That is, you
> either sleep in an interruptible state, or you don't. You can't mix and
> match it.

IIUC there is only one waiter on this waitqueue at any given time.

Paul is that correct?

--
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

Paul Mackerras Jan. 20, 2015, 5:46 a.m. UTC | #6

On Mon, Jan 19, 2015 at 12:41:00PM -0200, Marcelo Tosatti wrote:
> On Fri, Jan 16, 2015 at 11:48:46AM -0500, Steven Rostedt wrote:
> > >  static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
> > >  {
> > > -	DEFINE_WAIT(wait);
> > > +	DEFINE_SWAITER(wait);
> > >  
> > > -	prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> > > +	swait_prepare(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> > >  	vc->vcore_state = VCORE_SLEEPING;
> > >  	spin_unlock(&vc->lock);
> > >  	schedule();
> > > -	finish_wait(&vc->wq, &wait);
> > > +	swait_finish(&vc->wq, &wait);
> > >  	spin_lock(&vc->lock);
> > >  	vc->vcore_state = VCORE_INACTIVE;
> > >  }
> > > @@ -1613,7 +1613,7 @@
> > >  			kvmppc_create_dtl_entry(vcpu, vc);
> > >  			kvmppc_start_thread(vcpu);
> > >  		} else if (vc->vcore_state == VCORE_SLEEPING) {
> > > -			wake_up(&vc->wq);
> > > +			swait_wake(&vc->wq);
> > 
> > I notice everywhere you have a swait_wake_interruptible() but here. Is
> > there a reason why?
> > 
> > IIRC, Peter wants to make swait wakeup usage homogenous. That is, you
> > either sleep in an interruptible state, or you don't. You can't mix and
> > match it.
> 
> IIUC there is only one waiter on this waitqueue at any given time.
> 
> Paul is that correct?

Yes, that's right.  It's only the task that has taken the
responsibility for running the virtual core that would be waiting on
that wait queue.

Paul.
--
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

Steven Rostedt Jan. 20, 2015, 6:16 p.m. UTC | #7

On Tue, 20 Jan 2015 16:46:53 +1100
Paul Mackerras <paulus@samba.org> wrote:

> On Mon, Jan 19, 2015 at 12:41:00PM -0200, Marcelo Tosatti wrote:
> > On Fri, Jan 16, 2015 at 11:48:46AM -0500, Steven Rostedt wrote:
> > > >  static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
> > > >  {
> > > > -	DEFINE_WAIT(wait);
> > > > +	DEFINE_SWAITER(wait);
> > > >  
> > > > -	prepare_to_wait(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> > > > +	swait_prepare(&vc->wq, &wait, TASK_INTERRUPTIBLE);
> > > >  	vc->vcore_state = VCORE_SLEEPING;
> > > >  	spin_unlock(&vc->lock);
> > > >  	schedule();
> > > > -	finish_wait(&vc->wq, &wait);
> > > > +	swait_finish(&vc->wq, &wait);
> > > >  	spin_lock(&vc->lock);
> > > >  	vc->vcore_state = VCORE_INACTIVE;
> > > >  }
> > > > @@ -1613,7 +1613,7 @@
> > > >  			kvmppc_create_dtl_entry(vcpu, vc);
> > > >  			kvmppc_start_thread(vcpu);
> > > >  		} else if (vc->vcore_state == VCORE_SLEEPING) {
> > > > -			wake_up(&vc->wq);
> > > > +			swait_wake(&vc->wq);
> > > 
> > > I notice everywhere you have a swait_wake_interruptible() but here. Is
> > > there a reason why?
> > > 
> > > IIRC, Peter wants to make swait wakeup usage homogenous. That is, you
> > > either sleep in an interruptible state, or you don't. You can't mix and
> > > match it.
> > 
> > IIUC there is only one waiter on this waitqueue at any given time.
> > 
> > Paul is that correct?
> 
> Yes, that's right.  It's only the task that has taken the
> responsibility for running the virtual core that would be waiting on
> that wait queue.

Thanks Paul, but it still makes me nervious.

I'm actually wondering if we should just nuke the _interruptible()
version of swait. As it should only be all interruptible or all not
interruptible, that the swait_wake() should just do the wake up
regardless. In which case, swait_wake() is good enough. No need to have
different versions where people may think do something special.

Peter?

-- Steve
--
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

Peter Zijlstra Jan. 21, 2015, 3:07 p.m. UTC | #8

On Tue, Jan 20, 2015 at 01:16:13PM -0500, Steven Rostedt wrote:
> I'm actually wondering if we should just nuke the _interruptible()
> version of swait. As it should only be all interruptible or all not
> interruptible, that the swait_wake() should just do the wake up
> regardless. In which case, swait_wake() is good enough. No need to have
> different versions where people may think do something special.
> 
> Peter?

Yeah, I think the lastest thing I have sitting here on my disk only has
the swake_up() which does TASK_NORMAL, no choice there.
--
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

Sebastian Sewior Feb. 17, 2015, 5:44 p.m. UTC | #9

* Peter Zijlstra | 2015-01-21 16:07:16 [+0100]:

>On Tue, Jan 20, 2015 at 01:16:13PM -0500, Steven Rostedt wrote:
>> I'm actually wondering if we should just nuke the _interruptible()
>> version of swait. As it should only be all interruptible or all not
>> interruptible, that the swait_wake() should just do the wake up
>> regardless. In which case, swait_wake() is good enough. No need to have
>> different versions where people may think do something special.
>> 
>> Peter?
>
>Yeah, I think the lastest thing I have sitting here on my disk only has
>the swake_up() which does TASK_NORMAL, no choice there.

what is the swait status in terms of mainline? This sounds like it
beeing worked on.
I could take the series but then I would drop it again if the mainline
implementation changes…

Sebastian
--
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

Marcelo Tosatti Feb. 27, 2015, 12:23 a.m. UTC | #10

On Tue, Feb 17, 2015 at 06:44:19PM +0100, Sebastian Andrzej Siewior wrote:
> * Peter Zijlstra | 2015-01-21 16:07:16 [+0100]:
> 
> >On Tue, Jan 20, 2015 at 01:16:13PM -0500, Steven Rostedt wrote:
> >> I'm actually wondering if we should just nuke the _interruptible()
> >> version of swait. As it should only be all interruptible or all not
> >> interruptible, that the swait_wake() should just do the wake up
> >> regardless. In which case, swait_wake() is good enough. No need to have
> >> different versions where people may think do something special.
> >> 
> >> Peter?
> >
> >Yeah, I think the lastest thing I have sitting here on my disk only has
> >the swake_up() which does TASK_NORMAL, no choice there.
> 
> what is the swait status in terms of mainline? This sounds like it
> beeing worked on.
> I could take the series but then I would drop it again if the mainline
> implementation changes…

Hi Sebastian,

No, you would just adjust it to the upstream kernel interfaces, as the rest of
the -rt users of the swait interfaces.

Can you please include the series?

Thanks

--
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

Marcelo Tosatti March 5, 2015, 1:09 a.m. UTC | #11

On Thu, Feb 26, 2015 at 09:23:57PM -0300, Marcelo Tosatti wrote:
> On Tue, Feb 17, 2015 at 06:44:19PM +0100, Sebastian Andrzej Siewior wrote:
> > * Peter Zijlstra | 2015-01-21 16:07:16 [+0100]:
> > 
> > >On Tue, Jan 20, 2015 at 01:16:13PM -0500, Steven Rostedt wrote:
> > >> I'm actually wondering if we should just nuke the _interruptible()
> > >> version of swait. As it should only be all interruptible or all not
> > >> interruptible, that the swait_wake() should just do the wake up
> > >> regardless. In which case, swait_wake() is good enough. No need to have
> > >> different versions where people may think do something special.
> > >> 
> > >> Peter?
> > >
> > >Yeah, I think the lastest thing I have sitting here on my disk only has
> > >the swake_up() which does TASK_NORMAL, no choice there.
> > 
> > what is the swait status in terms of mainline? This sounds like it
> > beeing worked on.
> > I could take the series but then I would drop it again if the mainline
> > implementation changes…
> 
> Hi Sebastian,
> 
> No, you would just adjust it to the upstream kernel interfaces, as the rest of
> the -rt users of the swait interfaces.
> 
> Can you please include the series?
> 
> Thanks

Sebastian?

--
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

Sebastian Sewior March 5, 2015, 7:42 a.m. UTC | #12

On 03/05/2015 02:09 AM, Marcelo Tosatti wrote:
>> Can you please include the series?
>>
>> Thanks
> 
> Sebastian?

I will pick it up, don't worry. I think I do my -RT day tomorrow.

Sebastian
--
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

Sebastian Sewior March 6, 2015, 1:54 p.m. UTC | #13

* Marcelo Tosatti | 2015-01-14 15:12:52 [-0200]:

Against which tree was this prepared? Could please rebase it against
v3.18.7-rt2? Because a I see "fuzz 2", the mips file is gone and
s390 rejects almost every chunk.
And there was that mips chunk Steven noticed.
Patch #2 seems to apply but since it requires #1 I wait for the update.

Sebastian
--
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

[-rt,1/2] KVM: use simple waitqueue for vcpu->wq

Commit Message

Comments

Patch