All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting
@ 2011-01-26 21:56 Rik van Riel
  2011-01-26 22:19 ` [RFC -v7 PATCH 2/7] sched: limit the scope of clear_buddies Rik van Riel
                   ` (7 more replies)
  0 siblings, 8 replies; 17+ messages in thread
From: Rik van Riel @ 2011-01-26 21:56 UTC (permalink / raw)
  To: kvm
  Cc: linux-kernel, Avi Kiviti, Srivatsa Vaddagiri, Peter Zijlstra,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

When running SMP virtual machines, it is possible for one VCPU to be
spinning on a spinlock, while the VCPU that holds the spinlock is not
currently running, because the host scheduler preempted it to run
something else.

Both Intel and AMD CPUs have a feature that detects when a virtual
CPU is spinning on a lock and will trap to the host.

The current KVM code sleeps for a bit whenever that happens, which
results in eg. a 64 VCPU Windows guest taking forever and a bit to
boot up.  This is because the VCPU holding the lock is actually
running and not sleeping, so the pause is counter-productive.

In other workloads a pause can also be counter-productive, with
spinlock detection resulting in one guest giving up its CPU time
to the others.  Instead of spinning, it ends up simply not running
much at all.

This patch series aims to fix that, by having a VCPU that spins
give the remainder of its timeslice to another VCPU in the same
guest before yielding the CPU - one that is runnable but got 
preempted, hopefully the lock holder.

v7:
- move the vcpu to pid mapping to inside the vcpu->mutex
- rename ->yield to ->skip
- merge patch 5 into patch 4
v6:
- implement yield_task_fair in a way that works with task groups,
  this allows me to actually get a performance improvement!
- fix another race Avi pointed out, the code should be good now
v5:
- fix the race condition Avi pointed out, by tracking vcpu->pid
- also allows us to yield to vcpu tasks that got preempted while in qemu
  userspace
v4:
- change to newer version of Mike Galbraith's yield_to implementation
- chainsaw out some code from Mike that looked like a great idea, but
  turned out to give weird interactions in practice
v3:
- more cleanups
- change to Mike Galbraith's yield_to implementation
- yield to spinning VCPUs, this seems to work better in some
  situations and has little downside potential
v2:
- make lots of cleanups and improvements suggested
- do not implement timeslice scheduling or fairness stuff
  yet, since it is not entirely clear how to do that right
  (suggestions welcome)


Benchmark results:

Two 4-CPU KVM guests are pinned to the same 4 physical CPUs.

One guest runs the AMQP performance test, the other guest runs
0, 2 or 4 infinite loops, for CPU overcommit factors of 0, 1.5
and 4.

The AMQP perftest is run 30 times, with message payloads of 8 and 16 bytes.

size8	no overcommit	1.5x overcommit		2x overcommit

no PLE	223801		135137			104951
PLE	224135		141105			118744

size16	no overcommit	1.5x overcommit		2x overcommit

no PLE	222424		126175			105299
PLE	222534		138082			132945

Note: this is with the KVM guests NOT running inside cgroups.  There
seems to be a CPU load balancing issue with cgroup fair group scheduling,
which often results in one guest getting only 80% CPU time and the other
guest 320%.  That will have to be fixed to get meaningful results with
cgroups.

CPU time division between the AMQP guest and the infinite loop guest
were not exactly fair, but the guests got close to the same amount
of CPU time in each test run.

There is a substantial amount of randomness in CPU time division between
guests, but the performance improvement is consistent between multiple
runs.

-- 
All rights reversed.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [RFC -v7 PATCH 2/7] sched: limit the scope of clear_buddies
  2011-01-26 21:56 [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Rik van Riel
@ 2011-01-26 22:19 ` Rik van Riel
  2011-01-26 22:19 ` [RFC -v7 PATCH 1/7] sched: check the right ->nr_running in yield_task_fair Rik van Riel
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Rik van Riel @ 2011-01-26 22:19 UTC (permalink / raw)
  To: kvm
  Cc: linux-kernel, Avi Kiviti, Srivatsa Vaddagiri, Peter Zijlstra,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

The clear_buddies function does not seem to play well with the concept
of hierarchical runqueues.  In the following tree, task groups are
represented by 'G', tasks by 'T', next by 'n' and last by 'l'.

     (nl)
    /    \
   G(nl)  G
   / \     \
 T(l) T(n)  T

This situation can arise when a task is woken up T(n), and the previously
running task T(l) is marked last.

When clear_buddies is called from either T(l) or T(n), the next and last
buddies of the group G(nl) will be cleared.  This is not the desired
result, since we would like to be able to find the other type of buddy
in many cases.

This especially a worry when implementing yield_task_fair through the
buddy system.

The fix is simple: only clear the buddy type that the task itself
is indicated to be.  As an added bonus, we stop walking up the tree
when the buddy has already been cleared or pointed elsewhere.

Signed-off-by: Rik van Riel <riel@redhat.coM>
---
 kernel/sched_fair.c |   30 +++++++++++++++++++++++-------
 1 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index f4ee445..0321473 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -784,19 +784,35 @@ enqueue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 		__enqueue_entity(cfs_rq, se);
 }
 
-static void __clear_buddies(struct cfs_rq *cfs_rq, struct sched_entity *se)
+static void __clear_buddies_last(struct sched_entity *se)
 {
-	if (!se || cfs_rq->last == se)
-		cfs_rq->last = NULL;
+	for_each_sched_entity(se) {
+		struct cfs_rq *cfs_rq = cfs_rq_of(se);
+		if (cfs_rq->last == se)
+			cfs_rq->last = NULL;
+		else
+			break;
+	}
+}
 
-	if (!se || cfs_rq->next == se)
-		cfs_rq->next = NULL;
+static void __clear_buddies_next(struct sched_entity *se)
+{
+	for_each_sched_entity(se) {
+		struct cfs_rq *cfs_rq = cfs_rq_of(se);
+		if (cfs_rq->next == se)
+			cfs_rq->next = NULL;
+		else
+			break;
+	}
 }
 
 static void clear_buddies(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
-	for_each_sched_entity(se)
-		__clear_buddies(cfs_rq_of(se), se);
+	if (cfs_rq->last == se)
+		__clear_buddies_last(se);
+
+	if (cfs_rq->next == se)
+		__clear_buddies_next(se);
 }
 
 static void

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RFC -v7 PATCH 1/7] sched: check the right ->nr_running in yield_task_fair
  2011-01-26 21:56 [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Rik van Riel
  2011-01-26 22:19 ` [RFC -v7 PATCH 2/7] sched: limit the scope of clear_buddies Rik van Riel
@ 2011-01-26 22:19 ` Rik van Riel
  2011-01-26 22:21 ` [RFC -v7 PATCH 3/7] sched: use a buddy to implement yield_task_fair Rik van Riel
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 17+ messages in thread
From: Rik van Riel @ 2011-01-26 22:19 UTC (permalink / raw)
  To: kvm
  Cc: linux-kernel, Avi Kiviti, Srivatsa Vaddagiri, Peter Zijlstra,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

With CONFIG_FAIR_GROUP_SCHED, each task_group has its own cfs_rq.
Yielding to a task from another cfs_rq may be worthwhile, since
a process calling yield typically cannot use the CPU right now.

Therefor, we want to check the per-cpu nr_running, not the
cgroup local one.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 kernel/sched_fair.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index c62ebae..7b338ac 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1304,7 +1304,7 @@ static void yield_task_fair(struct rq *rq)
 	/*
 	 * Are we the only task in the tree?
 	 */
-	if (unlikely(cfs_rq->nr_running == 1))
+	if (unlikely(rq->nr_running == 1))
 		return;
 
 	clear_buddies(cfs_rq, se);

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RFC -v7 PATCH 3/7] sched: use a buddy to implement yield_task_fair
  2011-01-26 21:56 [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Rik van Riel
  2011-01-26 22:19 ` [RFC -v7 PATCH 2/7] sched: limit the scope of clear_buddies Rik van Riel
  2011-01-26 22:19 ` [RFC -v7 PATCH 1/7] sched: check the right ->nr_running in yield_task_fair Rik van Riel
@ 2011-01-26 22:21 ` Rik van Riel
  2011-01-31 11:47   ` Peter Zijlstra
  2011-01-26 22:21 ` [RFC -v7 PATCH 4/7] Add yield_to(task, preempt) functionality Rik van Riel
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Rik van Riel @ 2011-01-26 22:21 UTC (permalink / raw)
  To: kvm
  Cc: linux-kernel, Avi Kiviti, Srivatsa Vaddagiri, Peter Zijlstra,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

Use the buddy mechanism to implement yield_task_fair.  This
allows us to skip onto the next highest priority se at every
level in the CFS tree, unless doing so would introduce gross
unfairness in CPU time distribution.

We order the buddy selection in pick_next_entity to check
yield first, then last, then next.  We need next to be able
to override yield, because it is possible for the "next" and
"yield" task to be different processen in the same sub-tree
of the CFS tree.  When they are, we need to go into that
sub-tree regardless of the "yield" hint, and pick the correct
entity once we get to the right level.

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

diff --git a/kernel/sched.c b/kernel/sched.c
index dc91a4d..7ff53e2 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -327,7 +327,7 @@ struct cfs_rq {
 	 * 'curr' points to currently running entity on this cfs_rq.
 	 * It is set to NULL otherwise (i.e when none are currently running).
 	 */
-	struct sched_entity *curr, *next, *last;
+	struct sched_entity *curr, *next, *last, *skip;
 
 	unsigned int nr_spread_over;
 
diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index ad946fd..a56d410 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -384,6 +384,22 @@ static struct sched_entity *__pick_next_entity(struct cfs_rq *cfs_rq)
 	return rb_entry(left, struct sched_entity, run_node);
 }
 
+static struct sched_entity *__pick_second_entity(struct cfs_rq *cfs_rq)
+{
+	struct rb_node *left = cfs_rq->rb_leftmost;
+	struct rb_node *second;
+
+	if (!left)
+		return NULL;
+
+	second = rb_next(left);
+
+	if (!second)
+		second = left;
+
+	return rb_entry(second, struct sched_entity, run_node);
+}
+
 static struct sched_entity *__pick_last_entity(struct cfs_rq *cfs_rq)
 {
 	struct rb_node *last = rb_last(&cfs_rq->tasks_timeline);
@@ -806,6 +822,17 @@ static void __clear_buddies_next(struct sched_entity *se)
 	}
 }
 
+static void __clear_buddies_skip(struct sched_entity *se)
+{
+	for_each_sched_entity(se) {
+		struct cfs_rq *cfs_rq = cfs_rq_of(se);
+		if (cfs_rq->skip == se)
+			cfs_rq->skip = NULL;
+		else
+			break;
+	}
+}
+
 static void clear_buddies(struct cfs_rq *cfs_rq, struct sched_entity *se)
 {
 	if (cfs_rq->last == se)
@@ -813,6 +840,9 @@ static void clear_buddies(struct cfs_rq *cfs_rq, struct sched_entity *se)
 
 	if (cfs_rq->next == se)
 		__clear_buddies_next(se);
+
+	if (cfs_rq->skip == se)
+		__clear_buddies_skip(se);
 }
 
 static void
@@ -926,13 +956,27 @@ set_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *se)
 static int
 wakeup_preempt_entity(struct sched_entity *curr, struct sched_entity *se);
 
+/*
+ * Pick the next process, keeping these things in mind, in this order:
+ * 1) keep things fair between processes/task groups
+ * 2) pick the "next" process, since someone really wants that to run
+ * 3) pick the "last" process, for cache locality
+ * 4) do not run the "skip" process, if something else is available
+ */
 static struct sched_entity *pick_next_entity(struct cfs_rq *cfs_rq)
 {
 	struct sched_entity *se = __pick_next_entity(cfs_rq);
 	struct sched_entity *left = se;
 
-	if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
-		se = cfs_rq->next;
+	/*
+	 * Avoid running the skip buddy, if running something else can
+	 * be done without getting too unfair.
+	 */
+	if (cfs_rq->skip == se) {
+		struct sched_entity *second = __pick_second_entity(cfs_rq);
+		if (wakeup_preempt_entity(second, left) < 1)
+			se = second;
+	}
 
 	/*
 	 * Prefer last buddy, try to return the CPU to a preempted task.
@@ -940,6 +984,12 @@ static struct sched_entity *pick_next_entity(struct cfs_rq *cfs_rq)
 	if (cfs_rq->last && wakeup_preempt_entity(cfs_rq->last, left) < 1)
 		se = cfs_rq->last;
 
+	/*
+	 * Someone really wants this to run. If it's not unfair, run it.
+	 */
+	if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
+		se = cfs_rq->next;
+
 	clear_buddies(cfs_rq, se);
 
 	return se;
@@ -1096,52 +1146,6 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 	hrtick_update(rq);
 }
 
-/*
- * sched_yield() support is very simple - we dequeue and enqueue.
- *
- * If compat_yield is turned on then we requeue to the end of the tree.
- */
-static void yield_task_fair(struct rq *rq)
-{
-	struct task_struct *curr = rq->curr;
-	struct cfs_rq *cfs_rq = task_cfs_rq(curr);
-	struct sched_entity *rightmost, *se = &curr->se;
-
-	/*
-	 * Are we the only task in the tree?
-	 */
-	if (unlikely(rq->nr_running == 1))
-		return;
-
-	clear_buddies(cfs_rq, se);
-
-	if (likely(!sysctl_sched_compat_yield) && curr->policy != SCHED_BATCH) {
-		update_rq_clock(rq);
-		/*
-		 * Update run-time statistics of the 'current'.
-		 */
-		update_curr(cfs_rq);
-
-		return;
-	}
-	/*
-	 * Find the rightmost entry in the rbtree:
-	 */
-	rightmost = __pick_last_entity(cfs_rq);
-	/*
-	 * Already in the rightmost position?
-	 */
-	if (unlikely(!rightmost || entity_before(rightmost, se)))
-		return;
-
-	/*
-	 * Minimally necessary key value to be last in the tree:
-	 * Upon rescheduling, sched_class::put_prev_task() will place
-	 * 'current' within the tree based on its new key value.
-	 */
-	se->vruntime = rightmost->vruntime + 1;
-}
-
 #ifdef CONFIG_SMP
 
 static void task_waking_fair(struct rq *rq, struct task_struct *p)
@@ -1660,6 +1664,14 @@ static void set_next_buddy(struct sched_entity *se)
 	}
 }
 
+static void set_skip_buddy(struct sched_entity *se)
+{
+	if (likely(task_of(se)->policy != SCHED_IDLE)) {
+		for_each_sched_entity(se)
+			cfs_rq_of(se)->skip = se;
+	}
+}
+
 /*
  * Preempt the current task with a newly woken task if needed:
  */
@@ -1758,6 +1770,36 @@ static void put_prev_task_fair(struct rq *rq, struct task_struct *prev)
 	}
 }
 
+/*
+ * sched_yield() is very simple
+ *
+ * The magic of dealing with the ->skip buddy is in pick_next_entity.
+ */
+static void yield_task_fair(struct rq *rq)
+{
+	struct task_struct *curr = rq->curr;
+	struct cfs_rq *cfs_rq = task_cfs_rq(curr);
+	struct sched_entity *se = &curr->se;
+
+	/*
+	 * Are we the only task in the tree?
+	 */
+	if (unlikely(rq->nr_running == 1))
+		return;
+
+	clear_buddies(cfs_rq, se);
+
+	if (curr->policy != SCHED_BATCH) {
+		update_rq_clock(rq);
+		/*
+		 * Update run-time statistics of the 'current'.
+		 */
+		update_curr(cfs_rq);
+	}
+
+	set_skip_buddy(se);
+}
+
 #ifdef CONFIG_SMP
 /**************************************************
  * Fair scheduling class load-balancing methods:

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RFC -v7 PATCH 4/7] Add yield_to(task, preempt) functionality.
  2011-01-26 21:56 [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Rik van Riel
                   ` (2 preceding siblings ...)
  2011-01-26 22:21 ` [RFC -v7 PATCH 3/7] sched: use a buddy to implement yield_task_fair Rik van Riel
@ 2011-01-26 22:21 ` Rik van Riel
  2011-01-31 11:49   ` Peter Zijlstra
  2011-01-26 22:23 ` [RFC -v7 PATCH 5/7] export pid symbols needed for kvm_vcpu_on_spin Rik van Riel
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Rik van Riel @ 2011-01-26 22:21 UTC (permalink / raw)
  To: kvm
  Cc: linux-kernel, Avi Kiviti, Srivatsa Vaddagiri, Peter Zijlstra,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

From: Mike Galbraith <efault@gmx.de>

Currently only implemented for fair class tasks.

Add a yield_to_task method() to the fair scheduling class. allowing the
caller of yield_to() to accelerate another thread in it's thread group,
task group.

Implemented via a scheduler hint, using cfs_rq->next to encourage the
target being selected.  We can rely on pick_next_entity to keep things
fair, so noone can accelerate a thread that has already used its fair
share of CPU time.

This also means callers should only call yield_to when they really
mean it.  Calling it too often can result in the scheduler just
ignoring the hint.

Signed-off-by: Rik van Riel <riel@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Mike Galbraith <efault@gmx.de>

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2c79e92..6c43fc4 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1047,6 +1047,7 @@ struct sched_class {
 	void (*enqueue_task) (struct rq *rq, struct task_struct *p, int flags);
 	void (*dequeue_task) (struct rq *rq, struct task_struct *p, int flags);
 	void (*yield_task) (struct rq *rq);
+	bool (*yield_to_task) (struct rq *rq, struct task_struct *p, bool preempt);
 
 	void (*check_preempt_curr) (struct rq *rq, struct task_struct *p, int flags);
 
@@ -1943,6 +1944,7 @@ static inline int rt_mutex_getprio(struct task_struct *p)
 # define rt_mutex_adjust_pi(p)		do { } while (0)
 #endif
 
+extern bool yield_to(struct task_struct *p, bool preempt);
 extern void set_user_nice(struct task_struct *p, long nice);
 extern int task_prio(const struct task_struct *p);
 extern int task_nice(const struct task_struct *p);
diff --git a/kernel/sched.c b/kernel/sched.c
index 7ff53e2..5e70156 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -5270,6 +5270,56 @@ void __sched yield(void)
 }
 EXPORT_SYMBOL(yield);
 
+/**
+ * yield_to - yield the current processor to another thread in
+ * your thread group, or accelerate that thread toward the
+ * processor it's on.
+ *
+ * It's the caller's job to ensure that the target task struct
+ * can't go away on us before we can do any checks.
+ *
+ * Returns true if we indeed boosted the target task.
+ */
+bool __sched yield_to(struct task_struct *p, bool preempt)
+{
+	struct task_struct *curr = current;
+	struct rq *rq, *p_rq;
+	unsigned long flags;
+	bool yielded = 0;
+
+	local_irq_save(flags);
+	rq = this_rq();
+
+again:
+	p_rq = task_rq(p);
+	double_rq_lock(rq, p_rq);
+	while (task_rq(p) != p_rq) {
+		double_rq_unlock(rq, p_rq);
+		goto again;
+	}
+
+	if (!curr->sched_class->yield_to_task)
+		goto out;
+
+	if (curr->sched_class != p->sched_class)
+		goto out;
+
+	if (task_running(p_rq, p) || p->state)
+		goto out;
+
+	yielded = curr->sched_class->yield_to_task(rq, p, preempt);
+
+out:
+	double_rq_unlock(rq, p_rq);
+	local_irq_restore(flags);
+
+	if (yielded)
+		yield();
+
+	return yielded;
+}
+EXPORT_SYMBOL_GPL(yield_to);
+
 /*
  * This task is about to go to sleep on IO. Increment rq->nr_iowait so
  * that process accounting knows that this is a task in IO wait state.
diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index a56d410..be729d7 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1800,6 +1800,23 @@ static void yield_task_fair(struct rq *rq)
 	set_skip_buddy(se);
 }
 
+static bool yield_to_task_fair(struct rq *rq, struct task_struct *p, bool preempt)
+{
+	struct sched_entity *se = &p->se;
+
+	if (!se->on_rq)
+		return false;
+
+	/* Tell the scheduler that we'd really like pse to run next. */
+	set_next_buddy(se);
+
+	/* Make p's CPU reschedule; pick_next_entity takes care of fairness. */
+	if (preempt)
+		resched_task(rq->curr);
+
+	return true;
+}
+
 #ifdef CONFIG_SMP
 /**************************************************
  * Fair scheduling class load-balancing methods:
@@ -3993,6 +4010,7 @@ static const struct sched_class fair_sched_class = {
 	.enqueue_task		= enqueue_task_fair,
 	.dequeue_task		= dequeue_task_fair,
 	.yield_task		= yield_task_fair,
+	.yield_to_task		= yield_to_task_fair,
 
 	.check_preempt_curr	= check_preempt_wakeup,
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RFC -v7 PATCH 5/7] export pid symbols needed for kvm_vcpu_on_spin
  2011-01-26 21:56 [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Rik van Riel
                   ` (3 preceding siblings ...)
  2011-01-26 22:21 ` [RFC -v7 PATCH 4/7] Add yield_to(task, preempt) functionality Rik van Riel
@ 2011-01-26 22:23 ` Rik van Riel
  2011-01-31 11:51   ` Peter Zijlstra
  2011-01-26 22:24 ` [RFC -v7 PATCH 6/7] kvm: keep track of which task is running a KVM vcpu Rik van Riel
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 17+ messages in thread
From: Rik van Riel @ 2011-01-26 22:23 UTC (permalink / raw)
  To: kvm
  Cc: linux-kernel, Avi Kiviti, Srivatsa Vaddagiri, Peter Zijlstra,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

Export the symbols required for a race-free kvm_vcpu_on_spin.

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

diff --git a/kernel/fork.c b/kernel/fork.c
index 3b159c5..adc8f47 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -191,6 +191,7 @@ void __put_task_struct(struct task_struct *tsk)
 	if (!profile_handoff_task(tsk))
 		free_task(tsk);
 }
+EXPORT_SYMBOL_GPL(__put_task_struct);
 
 /*
  * macro override instead of weak attribute alias, to workaround
diff --git a/kernel/pid.c b/kernel/pid.c
index 39b65b6..02f2212 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -435,6 +435,7 @@ struct pid *get_task_pid(struct task_struct *task, enum pid_type type)
 	rcu_read_unlock();
 	return pid;
 }
+EXPORT_SYMBOL_GPL(get_task_pid);
 
 struct task_struct *get_pid_task(struct pid *pid, enum pid_type type)
 {
@@ -446,6 +447,7 @@ struct task_struct *get_pid_task(struct pid *pid, enum pid_type type)
 	rcu_read_unlock();
 	return result;
 }
+EXPORT_SYMBOL_GPL(get_pid_task);
 
 struct pid *find_get_pid(pid_t nr)
 {

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RFC -v7 PATCH 6/7] kvm: keep track of which task is running a KVM vcpu
  2011-01-26 21:56 [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Rik van Riel
                   ` (4 preceding siblings ...)
  2011-01-26 22:23 ` [RFC -v7 PATCH 5/7] export pid symbols needed for kvm_vcpu_on_spin Rik van Riel
@ 2011-01-26 22:24 ` Rik van Riel
  2011-01-26 22:25 ` [RFC -v7 PATCH 7/7] kvm: use yield_to instead of sleep in kvm_vcpu_on_spin Rik van Riel
  2011-01-27 13:20 ` [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Avi Kivity
  7 siblings, 0 replies; 17+ messages in thread
From: Rik van Riel @ 2011-01-26 22:24 UTC (permalink / raw)
  To: kvm
  Cc: linux-kernel, Avi Kiviti, Srivatsa Vaddagiri, Peter Zijlstra,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

Keep track of which task is running a KVM vcpu.  This helps us
figure out later what task to wake up if we want to boost a
vcpu that got preempted.

Unfortunately there are no guarantees that the same task
always keeps the same vcpu, so we can only track the task
across a single "run" of the vcpu.

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

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index a055742..9d56ed5 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -81,6 +81,7 @@ struct kvm_vcpu {
 #endif
 	int vcpu_id;
 	struct mutex mutex;
+	struct pid *pid;
 	int   cpu;
 	atomic_t guest_mode;
 	struct kvm_run *run;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 5225052..0fa9a48 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -117,6 +117,14 @@ void vcpu_load(struct kvm_vcpu *vcpu)
 	int cpu;
 
 	mutex_lock(&vcpu->mutex);
+	if (unlikely(vcpu->pid != current->pids[PIDTYPE_PID].pid)) {
+		/* The thread running this VCPU changed. */
+		struct pid *oldpid = vcpu->pid;
+		struct pid *newpid = get_task_pid(current, PIDTYPE_PID);
+		rcu_assign_pointer(vcpu->pid, newpid);
+		synchronize_rcu();
+		put_pid(oldpid);
+	}
 	cpu = get_cpu();
 	preempt_notifier_register(&vcpu->preempt_notifier);
 	kvm_arch_vcpu_load(vcpu, cpu);
@@ -185,6 +193,7 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	vcpu->cpu = -1;
 	vcpu->kvm = kvm;
 	vcpu->vcpu_id = id;
+	vcpu->pid = NULL;
 	init_waitqueue_head(&vcpu->wq);
 
 	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
@@ -208,6 +217,7 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_init);
 
 void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
 {
+	put_pid(vcpu->pid);
 	kvm_arch_vcpu_uninit(vcpu);
 	free_page((unsigned long)vcpu->run);
 }


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [RFC -v7 PATCH 7/7] kvm: use yield_to instead of sleep in kvm_vcpu_on_spin
  2011-01-26 21:56 [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Rik van Riel
                   ` (5 preceding siblings ...)
  2011-01-26 22:24 ` [RFC -v7 PATCH 6/7] kvm: keep track of which task is running a KVM vcpu Rik van Riel
@ 2011-01-26 22:25 ` Rik van Riel
  2011-01-27 13:20 ` [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Avi Kivity
  7 siblings, 0 replies; 17+ messages in thread
From: Rik van Riel @ 2011-01-26 22:25 UTC (permalink / raw)
  To: kvm
  Cc: linux-kernel, Avi Kiviti, Srivatsa Vaddagiri, Peter Zijlstra,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

Instead of sleeping in kvm_vcpu_on_spin, which can cause gigantic
slowdowns of certain workloads, we instead use yield_to to get
another VCPU in the same KVM guest to run sooner.

This seems to give a 10-15% speedup in certain workloads.

Signed-off-by: Rik van Riel <riel@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 9d56ed5..fab2250 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -187,6 +187,7 @@ struct kvm {
 #endif
 	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
 	atomic_t online_vcpus;
+	int last_boosted_vcpu;
 	struct list_head vm_list;
 	struct mutex lock;
 	struct kvm_io_bus *buses[KVM_NR_BUSES];
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 86c4905..8b761ba 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1292,18 +1292,55 @@ void kvm_resched(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_resched);
 
-void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu)
+void kvm_vcpu_on_spin(struct kvm_vcpu *me)
 {
-	ktime_t expires;
-	DEFINE_WAIT(wait);
-
-	prepare_to_wait(&vcpu->wq, &wait, TASK_INTERRUPTIBLE);
-
-	/* Sleep for 100 us, and hope lock-holder got scheduled */
-	expires = ktime_add_ns(ktime_get(), 100000UL);
-	schedule_hrtimeout(&expires, HRTIMER_MODE_ABS);
+	struct kvm *kvm = me->kvm;
+	struct kvm_vcpu *vcpu;
+	int last_boosted_vcpu = me->kvm->last_boosted_vcpu;
+	int yielded = 0;
+	int pass;
+	int i;
 
-	finish_wait(&vcpu->wq, &wait);
+	/*
+	 * We boost the priority of a VCPU that is runnable but not
+	 * currently running, because it got preempted by something
+	 * else and called schedule in __vcpu_run.  Hopefully that
+	 * VCPU is holding the lock that we need and will release it.
+	 * We approximate round-robin by starting at the last boosted VCPU.
+	 */
+	for (pass = 0; pass < 2 && !yielded; pass++) {
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			struct task_struct *task = NULL;
+			struct pid *pid;
+			if (!pass && i < last_boosted_vcpu) {
+				i = last_boosted_vcpu;
+				continue;
+			} else if (pass && i > last_boosted_vcpu)
+				break;
+			if (vcpu == me)
+				continue;
+			if (waitqueue_active(&vcpu->wq))
+				continue;
+			rcu_read_lock();
+			pid = rcu_dereference(vcpu->pid);
+			if (pid)
+				task = get_pid_task(vcpu->pid, PIDTYPE_PID);
+			rcu_read_unlock();
+			if (!task)
+				continue;
+			if (task->flags & PF_VCPU) {
+				put_task_struct(task);
+				continue;
+			}
+			if (yield_to(task, 1)) {
+				put_task_struct(task);
+				kvm->last_boosted_vcpu = i;
+				yielded = 1;
+				break;
+			}
+			put_task_struct(task);
+		}
+	}
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_on_spin);
 


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting
  2011-01-26 21:56 [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Rik van Riel
                   ` (6 preceding siblings ...)
  2011-01-26 22:25 ` [RFC -v7 PATCH 7/7] kvm: use yield_to instead of sleep in kvm_vcpu_on_spin Rik van Riel
@ 2011-01-27 13:20 ` Avi Kivity
  7 siblings, 0 replies; 17+ messages in thread
From: Avi Kivity @ 2011-01-27 13:20 UTC (permalink / raw)
  To: Rik van Riel
  Cc: kvm, linux-kernel, Srivatsa Vaddagiri, Peter Zijlstra,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

On 01/26/2011 11:56 PM, Rik van Riel wrote:
> When running SMP virtual machines, it is possible for one VCPU to be
> spinning on a spinlock, while the VCPU that holds the spinlock is not
> currently running, because the host scheduler preempted it to run
> something else.
>
> Both Intel and AMD CPUs have a feature that detects when a virtual
> CPU is spinning on a lock and will trap to the host.
>
> The current KVM code sleeps for a bit whenever that happens, which
> results in eg. a 64 VCPU Windows guest taking forever and a bit to
> boot up.  This is because the VCPU holding the lock is actually
> running and not sleeping, so the pause is counter-productive.
>
> In other workloads a pause can also be counter-productive, with
> spinlock detection resulting in one guest giving up its CPU time
> to the others.  Instead of spinning, it ends up simply not running
> much at all.
>
> This patch series aims to fix that, by having a VCPU that spins
> give the remainder of its timeslice to another VCPU in the same
> guest before yielding the CPU - one that is runnable but got
> preempted, hopefully the lock holder.
>
> v7:
> - move the vcpu to pid mapping to inside the vcpu->mutex
> - rename ->yield to ->skip
> - merge patch 5 into patch 4
> v6:
> - implement yield_task_fair in a way that works with task groups,
>    this allows me to actually get a performance improvement!
> - fix another race Avi pointed out, the code should be good now
> v5:
> - fix the race condition Avi pointed out, by tracking vcpu->pid
> - also allows us to yield to vcpu tasks that got preempted while in qemu
>    userspace
> v4:
> - change to newer version of Mike Galbraith's yield_to implementation
> - chainsaw out some code from Mike that looked like a great idea, but
>    turned out to give weird interactions in practice
> v3:
> - more cleanups
> - change to Mike Galbraith's yield_to implementation
> - yield to spinning VCPUs, this seems to work better in some
>    situations and has little downside potential
> v2:
> - make lots of cleanups and improvements suggested
> - do not implement timeslice scheduling or fairness stuff
>    yet, since it is not entirely clear how to do that right
>    (suggestions welcome)
>
>
> Benchmark results:
>
> Two 4-CPU KVM guests are pinned to the same 4 physical CPUs.
>
> One guest runs the AMQP performance test, the other guest runs
> 0, 2 or 4 infinite loops, for CPU overcommit factors of 0, 1.5
> and 4.
>
> The AMQP perftest is run 30 times, with message payloads of 8 and 16 bytes.
>
> size8	no overcommit	1.5x overcommit		2x overcommit
>
> no PLE	223801		135137			104951
> PLE	224135		141105			118744
>
> size16	no overcommit	1.5x overcommit		2x overcommit
>
> no PLE	222424		126175			105299
> PLE	222534		138082			132945
>
> Note: this is with the KVM guests NOT running inside cgroups.  There
> seems to be a CPU load balancing issue with cgroup fair group scheduling,
> which often results in one guest getting only 80% CPU time and the other
> guest 320%.  That will have to be fixed to get meaningful results with
> cgroups.
>
> CPU time division between the AMQP guest and the infinite loop guest
> were not exactly fair, but the guests got close to the same amount
> of CPU time in each test run.
>
> There is a substantial amount of randomness in CPU time division between
> guests, but the performance improvement is consistent between multiple
> runs.

The kvm bits look fine; I'll be happy to apply them after the scheduler 
changes are accepted into tip.git.

-- 
error compiling committee.c: too many arguments to function


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC -v7 PATCH 3/7] sched: use a buddy to implement yield_task_fair
  2011-01-26 22:21 ` [RFC -v7 PATCH 3/7] sched: use a buddy to implement yield_task_fair Rik van Riel
@ 2011-01-31 11:47   ` Peter Zijlstra
  2011-01-31 15:02     ` Rik van Riel
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2011-01-31 11:47 UTC (permalink / raw)
  To: Rik van Riel
  Cc: kvm, linux-kernel, Avi Kiviti, Srivatsa Vaddagiri,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

On Wed, 2011-01-26 at 17:21 -0500, Rik van Riel wrote:

> +static struct sched_entity *__pick_second_entity(struct cfs_rq *cfs_rq)
> +{
> +	struct rb_node *left = cfs_rq->rb_leftmost;
> +	struct rb_node *second;
> +
> +	if (!left)
> +		return NULL;
> +
> +	second = rb_next(left);
> +
> +	if (!second)
> +		second = left;
> +
> +	return rb_entry(second, struct sched_entity, run_node);
> +}

I would still prefer:

sed -i 's/__pick_next_entity/__pick_first_entity/g' kernel/sched*.[ch]

and

static struct sched_entity *__pick_next_entity(struct sched_entity *se)
{
	struct rb_node *next = rb_next(&se->run_node);

	if (!next)
		return NULL;

	return rb_entry(next, struct sched_entity, run_node);
}

Its simpler and more versatile.

>  static struct sched_entity *pick_next_entity(struct cfs_rq *cfs_rq)
>  {
>  	struct sched_entity *se = __pick_next_entity(cfs_rq);
>  	struct sched_entity *left = se;
>  
> -	if (cfs_rq->next && wakeup_preempt_entity(cfs_rq->next, left) < 1)
> -		se = cfs_rq->next;
> +	/*
> +	 * Avoid running the skip buddy, if running something else can
> +	 * be done without getting too unfair.
> +	 */
> +	if (cfs_rq->skip == se) {
> +		struct sched_entity *second = __pick_second_entity(cfs_rq);

which then becomes *next = __pick_next_entity(se);

> +		if (wakeup_preempt_entity(second, left) < 1)

oops when !second. (!next)

> +			se = second;
> +	}

> -/*
> - * sched_yield() support is very simple - we dequeue and enqueue.
> - *
> - * If compat_yield is turned on then we requeue to the end of the tree.
> - */
> -static void yield_task_fair(struct rq *rq)
> -{
> -	struct task_struct *curr = rq->curr;
> -	struct cfs_rq *cfs_rq = task_cfs_rq(curr);
> -	struct sched_entity *rightmost, *se = &curr->se;
> -
> -	/*
> -	 * Are we the only task in the tree?
> -	 */
> -	if (unlikely(rq->nr_running == 1))
> -		return;
> -
> -	clear_buddies(cfs_rq, se);
> -
> -	if (likely(!sysctl_sched_compat_yield) && curr->policy != SCHED_BATCH) {
> -		update_rq_clock(rq);
> -		/*
> -		 * Update run-time statistics of the 'current'.
> -		 */
> -		update_curr(cfs_rq);
> -
> -		return;
> -	}
> -	/*
> -	 * Find the rightmost entry in the rbtree:
> -	 */
> -	rightmost = __pick_last_entity(cfs_rq);
> -	/*
> -	 * Already in the rightmost position?
> -	 */
> -	if (unlikely(!rightmost || entity_before(rightmost, se)))
> -		return;
> -
> -	/*
> -	 * Minimally necessary key value to be last in the tree:
> -	 * Upon rescheduling, sched_class::put_prev_task() will place
> -	 * 'current' within the tree based on its new key value.
> -	 */
> -	se->vruntime = rightmost->vruntime + 1;
> -}

You seem to have lost the hunk removing sysctl_sched_compat_yield from
kernel/sysctl.c :-)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC -v7 PATCH 4/7] Add yield_to(task, preempt) functionality.
  2011-01-26 22:21 ` [RFC -v7 PATCH 4/7] Add yield_to(task, preempt) functionality Rik van Riel
@ 2011-01-31 11:49   ` Peter Zijlstra
  2011-01-31 18:11     ` Rik van Riel
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2011-01-31 11:49 UTC (permalink / raw)
  To: Rik van Riel
  Cc: kvm, linux-kernel, Avi Kiviti, Srivatsa Vaddagiri,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

On Wed, 2011-01-26 at 17:21 -0500, Rik van Riel wrote:
> +bool __sched yield_to(struct task_struct *p, bool preempt)
> +{
> +       struct task_struct *curr = current;
> +       struct rq *rq, *p_rq;
> +       unsigned long flags;
> +       bool yielded = 0;
> +
> +       local_irq_save(flags);
> +       rq = this_rq();
> +
> +again:
> +       p_rq = task_rq(p);
> +       double_rq_lock(rq, p_rq);
> +       while (task_rq(p) != p_rq) {
> +               double_rq_unlock(rq, p_rq);
> +               goto again;
> +       }
> +
> +       if (!curr->sched_class->yield_to_task)
> +               goto out;
> +
> +       if (curr->sched_class != p->sched_class)
> +               goto out;
> +
> +       if (task_running(p_rq, p) || p->state)
> +               goto out;
> +
> +       yielded = curr->sched_class->yield_to_task(rq, p, preempt);
> +
> +out:
> +       double_rq_unlock(rq, p_rq);
> +       local_irq_restore(flags);
> +
> +       if (yielded)
> +               yield();
> +
> +       return yielded;
> +}
> +EXPORT_SYMBOL_GPL(yield_to); 

yield() will again acquire rq->lock.. not not simply have
->yield_to_task() do everything required and make that an unconditional
schedule()?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC -v7 PATCH 5/7] export pid symbols needed for kvm_vcpu_on_spin
  2011-01-26 22:23 ` [RFC -v7 PATCH 5/7] export pid symbols needed for kvm_vcpu_on_spin Rik van Riel
@ 2011-01-31 11:51   ` Peter Zijlstra
  2011-01-31 13:26     ` Avi Kivity
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2011-01-31 11:51 UTC (permalink / raw)
  To: Rik van Riel
  Cc: kvm, linux-kernel, Avi Kiviti, Srivatsa Vaddagiri,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

On Wed, 2011-01-26 at 17:23 -0500, Rik van Riel wrote:
> Export the symbols required for a race-free kvm_vcpu_on_spin.

Avi, you asked for an example of why I hated KVM as a module :-)

> Signed-off-by: Rik van Riel <riel@redhat.com>
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 3b159c5..adc8f47 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -191,6 +191,7 @@ void __put_task_struct(struct task_struct *tsk)
>  	if (!profile_handoff_task(tsk))
>  		free_task(tsk);
>  }
> +EXPORT_SYMBOL_GPL(__put_task_struct);
>  
>  /*
>   * macro override instead of weak attribute alias, to workaround
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 39b65b6..02f2212 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -435,6 +435,7 @@ struct pid *get_task_pid(struct task_struct *task, enum pid_type type)
>  	rcu_read_unlock();
>  	return pid;
>  }
> +EXPORT_SYMBOL_GPL(get_task_pid);
>  
>  struct task_struct *get_pid_task(struct pid *pid, enum pid_type type)
>  {
> @@ -446,6 +447,7 @@ struct task_struct *get_pid_task(struct pid *pid, enum pid_type type)
>  	rcu_read_unlock();
>  	return result;
>  }
> +EXPORT_SYMBOL_GPL(get_pid_task);
>  
>  struct pid *find_get_pid(pid_t nr)
>  {



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC -v7 PATCH 5/7] export pid symbols needed for kvm_vcpu_on_spin
  2011-01-31 11:51   ` Peter Zijlstra
@ 2011-01-31 13:26     ` Avi Kivity
  2011-01-31 13:43       ` Peter Zijlstra
  0 siblings, 1 reply; 17+ messages in thread
From: Avi Kivity @ 2011-01-31 13:26 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Rik van Riel, kvm, linux-kernel, Srivatsa Vaddagiri,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

On 01/31/2011 01:51 PM, Peter Zijlstra wrote:
> On Wed, 2011-01-26 at 17:23 -0500, Rik van Riel wrote:
> >  Export the symbols required for a race-free kvm_vcpu_on_spin.
>
> Avi, you asked for an example of why I hated KVM as a module :-)

Why do you dislike exports so much?

-- 
error compiling committee.c: too many arguments to function


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC -v7 PATCH 5/7] export pid symbols needed for kvm_vcpu_on_spin
  2011-01-31 13:26     ` Avi Kivity
@ 2011-01-31 13:43       ` Peter Zijlstra
  2011-01-31 13:45         ` Avi Kivity
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2011-01-31 13:43 UTC (permalink / raw)
  To: Avi Kivity
  Cc: Rik van Riel, kvm, linux-kernel, Srivatsa Vaddagiri,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

On Mon, 2011-01-31 at 15:26 +0200, Avi Kivity wrote:
> On 01/31/2011 01:51 PM, Peter Zijlstra wrote:
> > On Wed, 2011-01-26 at 17:23 -0500, Rik van Riel wrote:
> > >  Export the symbols required for a race-free kvm_vcpu_on_spin.
> >
> > Avi, you asked for an example of why I hated KVM as a module :-)
> 
> Why do you dislike exports so much?

Because I hate modules [*], since we sadly cannot undo those, limiting
the module interface is all we've got, and KVM is one that pushes the
module interface further and further.

Many people see an EXPORT as a acknowledgement that its OK to use the
interface, even when its not.

[*] they enable the binary junk thing.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC -v7 PATCH 5/7] export pid symbols needed for kvm_vcpu_on_spin
  2011-01-31 13:43       ` Peter Zijlstra
@ 2011-01-31 13:45         ` Avi Kivity
  0 siblings, 0 replies; 17+ messages in thread
From: Avi Kivity @ 2011-01-31 13:45 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Rik van Riel, kvm, linux-kernel, Srivatsa Vaddagiri,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

On 01/31/2011 03:43 PM, Peter Zijlstra wrote:
> On Mon, 2011-01-31 at 15:26 +0200, Avi Kivity wrote:
> >  On 01/31/2011 01:51 PM, Peter Zijlstra wrote:
> >  >  On Wed, 2011-01-26 at 17:23 -0500, Rik van Riel wrote:
> >  >  >   Export the symbols required for a race-free kvm_vcpu_on_spin.
> >  >
> >  >  Avi, you asked for an example of why I hated KVM as a module :-)
> >
> >  Why do you dislike exports so much?
>
> Because I hate modules [*],

Well, you could have just said you hate KVM as a module because it is a 
module, and saved one indirection.

> since we sadly cannot undo those, limiting
> the module interface is all we've got, and KVM is one that pushes the
> module interface further and further.
>
> Many people see an EXPORT as a acknowledgement that its OK to use the
> interface, even when its not.
>
> [*] they enable the binary junk thing.

Then we need to stop binary modules, not castrate modules as a whole.

-- 
error compiling committee.c: too many arguments to function


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC -v7 PATCH 3/7] sched: use a buddy to implement yield_task_fair
  2011-01-31 11:47   ` Peter Zijlstra
@ 2011-01-31 15:02     ` Rik van Riel
  0 siblings, 0 replies; 17+ messages in thread
From: Rik van Riel @ 2011-01-31 15:02 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: kvm, linux-kernel, Avi Kiviti, Srivatsa Vaddagiri,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

On 01/31/2011 06:47 AM, Peter Zijlstra wrote:
> On Wed, 2011-01-26 at 17:21 -0500, Rik van Riel wrote:
>
>> +static struct sched_entity *__pick_second_entity(struct cfs_rq *cfs_rq)
>> +{
>> +	struct rb_node *left = cfs_rq->rb_leftmost;
>> +	struct rb_node *second;
>> +
>> +	if (!left)
>> +		return NULL;
>> +
>> +	second = rb_next(left);
>> +
>> +	if (!second)
>> +		second = left;
>> +
>> +	return rb_entry(second, struct sched_entity, run_node);
>> +}
>
> I would still prefer:
>
> sed -i 's/__pick_next_entity/__pick_first_entity/g' kernel/sched*.[ch]

Ahhh, now I understand what you want.  I'll get right on it.

> You seem to have lost the hunk removing sysctl_sched_compat_yield from
> kernel/sysctl.c :-)

Let me take care of that, too.

-- 
All rights reversed

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [RFC -v7 PATCH 4/7] Add yield_to(task, preempt) functionality.
  2011-01-31 11:49   ` Peter Zijlstra
@ 2011-01-31 18:11     ` Rik van Riel
  0 siblings, 0 replies; 17+ messages in thread
From: Rik van Riel @ 2011-01-31 18:11 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: kvm, linux-kernel, Avi Kiviti, Srivatsa Vaddagiri,
	Mike Galbraith, Chris Wright, ttracy, Nakajima, Jun

On 01/31/2011 06:49 AM, Peter Zijlstra wrote:
> On Wed, 2011-01-26 at 17:21 -0500, Rik van Riel wrote:

>> +       if (yielded)
>> +               yield();
>> +
>> +       return yielded;
>> +}
>> +EXPORT_SYMBOL_GPL(yield_to);
>
> yield() will again acquire rq->lock.. not not simply have
> ->yield_to_task() do everything required and make that an unconditional
> schedule()?

I wanted to reduce code duplication, since this code path
should not be taken all that frequently.  But hey, you're
the maintainer, so I'll implement it your way :)

The patches should be on the way later today :)

-- 
All rights reversed

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2011-01-31 18:12 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-26 21:56 [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Rik van Riel
2011-01-26 22:19 ` [RFC -v7 PATCH 2/7] sched: limit the scope of clear_buddies Rik van Riel
2011-01-26 22:19 ` [RFC -v7 PATCH 1/7] sched: check the right ->nr_running in yield_task_fair Rik van Riel
2011-01-26 22:21 ` [RFC -v7 PATCH 3/7] sched: use a buddy to implement yield_task_fair Rik van Riel
2011-01-31 11:47   ` Peter Zijlstra
2011-01-31 15:02     ` Rik van Riel
2011-01-26 22:21 ` [RFC -v7 PATCH 4/7] Add yield_to(task, preempt) functionality Rik van Riel
2011-01-31 11:49   ` Peter Zijlstra
2011-01-31 18:11     ` Rik van Riel
2011-01-26 22:23 ` [RFC -v7 PATCH 5/7] export pid symbols needed for kvm_vcpu_on_spin Rik van Riel
2011-01-31 11:51   ` Peter Zijlstra
2011-01-31 13:26     ` Avi Kivity
2011-01-31 13:43       ` Peter Zijlstra
2011-01-31 13:45         ` Avi Kivity
2011-01-26 22:24 ` [RFC -v7 PATCH 6/7] kvm: keep track of which task is running a KVM vcpu Rik van Riel
2011-01-26 22:25 ` [RFC -v7 PATCH 7/7] kvm: use yield_to instead of sleep in kvm_vcpu_on_spin Rik van Riel
2011-01-27 13:20 ` [RFC -v7 PATCH 0/7] directed yield for Pause Loop Exiting Avi Kivity

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.