* [PATCH v2 01/10] xen: sched: harmonize debug dump output among schedulers.
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
@ 2017-02-09 13:58 ` Dario Faggioli
2017-02-15 10:17 ` George Dunlap
2017-02-09 13:58 ` [PATCH v2 02/10] xen: credit2: clear bit instead of skip step in runq_tickle() Dario Faggioli
` (10 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:58 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar, Meng Xu
Information we currently print for idle pCPUs is
rather useless. Credit2 already stopped showing that,
do the same for Credit and RTDS.
Also, define a new CPU status dump hook, which is
not defined by those schedulers which already dump
such info in other ways (e.g., Credit2, which does
that while dumping runqueue information).
This also means that, still in Credit2, we can keep
the runqueue and pCPU info closer together.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: Meng Xu <mengxu@cis.upenn.edu>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
This is basically the rebase of "xen: sched: improve debug dump output.", on
top of "xen: credit2: improve debug dump output." (i.e., commit 3af86727b8204).
Sorry again, George, for the mess... I was sure I hadn't sent the first one
out yet, when I sended it out what turned out to be the second time (and,
even worse, slightly reworked! :-( ).
I'm keeping Meng's ack, as I did not touch the RTDS part, wrt the patch he sent
it against.
---
xen/common/sched_credit.c | 6 +++---
xen/common/sched_credit2.c | 34 +++++++++++-----------------------
xen/common/sched_rt.c | 9 ++++++++-
xen/common/schedule.c | 8 ++++----
4 files changed, 26 insertions(+), 31 deletions(-)
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index ad20819..7c0ff47 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1988,13 +1988,13 @@ csched_dump_pcpu(const struct scheduler *ops, int cpu)
runq = &spc->runq;
cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_sibling_mask, cpu));
- printk(" sort=%d, sibling=%s, ", spc->runq_sort_last, cpustr);
+ printk("CPU[%02d] sort=%d, sibling=%s, ", cpu, spc->runq_sort_last, cpustr);
cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
printk("core=%s\n", cpustr);
- /* current VCPU */
+ /* current VCPU (nothing to say if that's the idle vcpu). */
svc = CSCHED_VCPU(curr_on_cpu(cpu));
- if ( svc )
+ if ( svc && !is_idle_vcpu(svc->vcpu) )
{
printk("\trun: ");
csched_dump_vcpu(svc);
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 84ee015..741d372 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -2627,28 +2627,15 @@ csched2_dump_vcpu(struct csched2_private *prv, struct csched2_vcpu *svc)
printk("\n");
}
-static void
-csched2_dump_pcpu(const struct scheduler *ops, int cpu)
+static inline void
+dump_pcpu(const struct scheduler *ops, int cpu)
{
struct csched2_private *prv = CSCHED2_PRIV(ops);
struct csched2_vcpu *svc;
- unsigned long flags;
- spinlock_t *lock;
#define cpustr keyhandler_scratch
- /*
- * We need both locks:
- * - we print current, so we need the runqueue lock for this
- * cpu (the one of the runqueue this cpu is associated to);
- * - csched2_dump_vcpu() wants to access domains' weights,
- * which are protected by the private scheduler lock.
- */
- read_lock_irqsave(&prv->lock, flags);
- lock = per_cpu(schedule_data, cpu).schedule_lock;
- spin_lock(lock);
-
cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_sibling_mask, cpu));
- printk(" runq=%d, sibling=%s, ", c2r(ops, cpu), cpustr);
+ printk("CPU[%02d] runq=%d, sibling=%s, ", cpu, c2r(ops, cpu), cpustr);
cpumask_scnprintf(cpustr, sizeof(cpustr), per_cpu(cpu_core_mask, cpu));
printk("core=%s\n", cpustr);
@@ -2659,9 +2646,6 @@ csched2_dump_pcpu(const struct scheduler *ops, int cpu)
printk("\trun: ");
csched2_dump_vcpu(prv, svc);
}
-
- spin_unlock(lock);
- read_unlock_irqrestore(&prv->lock, flags);
#undef cpustr
}
@@ -2671,7 +2655,7 @@ csched2_dump(const struct scheduler *ops)
struct list_head *iter_sdom;
struct csched2_private *prv = CSCHED2_PRIV(ops);
unsigned long flags;
- int i, loop;
+ unsigned int i, j, loop;
#define cpustr keyhandler_scratch
/*
@@ -2741,7 +2725,6 @@ csched2_dump(const struct scheduler *ops)
}
}
- printk("Runqueue info:\n");
for_each_cpu(i, &prv->active_queues)
{
struct csched2_runqueue_data *rqd = prv->rqd + i;
@@ -2750,7 +2733,13 @@ csched2_dump(const struct scheduler *ops)
/* We need the lock to scan the runqueue. */
spin_lock(&rqd->lock);
- printk("runqueue %d:\n", i);
+
+ printk("Runqueue %d:\n", i);
+
+ for_each_cpu(j, &rqd->active)
+ dump_pcpu(ops, j);
+
+ printk("RUNQ:\n");
list_for_each( iter, runq )
{
struct csched2_vcpu *svc = __runq_elem(iter);
@@ -3108,7 +3097,6 @@ static const struct scheduler sched_credit2_def = {
.do_schedule = csched2_schedule,
.context_saved = csched2_context_saved,
- .dump_cpu_state = csched2_dump_pcpu,
.dump_settings = csched2_dump,
.init = csched2_init,
.deinit = csched2_deinit,
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 24b4b22..f2d979c 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -320,10 +320,17 @@ static void
rt_dump_pcpu(const struct scheduler *ops, int cpu)
{
struct rt_private *prv = rt_priv(ops);
+ struct rt_vcpu *svc;
unsigned long flags;
spin_lock_irqsave(&prv->lock, flags);
- rt_dump_vcpu(ops, rt_vcpu(curr_on_cpu(cpu)));
+ printk("CPU[%02d]\n", cpu);
+ /* current VCPU (nothing to say if that's the idle vcpu). */
+ svc = rt_vcpu(curr_on_cpu(cpu));
+ if ( svc && !is_idle_vcpu(svc->vcpu) )
+ {
+ rt_dump_vcpu(ops, svc);
+ }
spin_unlock_irqrestore(&prv->lock, flags);
}
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index ed77990..e4320f3 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1844,11 +1844,11 @@ void schedule_dump(struct cpupool *c)
cpus = &cpupool_free_cpus;
}
- printk("CPUs info:\n");
- for_each_cpu (i, cpus)
+ if ( sched->dump_cpu_state != NULL )
{
- printk("CPU[%02d] ", i);
- SCHED_OP(sched, dump_cpu_state, i);
+ printk("CPUs info:\n");
+ for_each_cpu (i, cpus)
+ SCHED_OP(sched, dump_cpu_state, i);
}
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 01/10] xen: sched: harmonize debug dump output among schedulers.
2017-02-09 13:58 ` [PATCH v2 01/10] xen: sched: harmonize debug dump output among schedulers Dario Faggioli
@ 2017-02-15 10:17 ` George Dunlap
2017-02-15 10:31 ` Dario Faggioli
0 siblings, 1 reply; 35+ messages in thread
From: George Dunlap @ 2017-02-15 10:17 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Anshul Makkar, Meng Xu
On Thu, Feb 9, 2017 at 1:58 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> Information we currently print for idle pCPUs is
I take it you meant "idle vCPUs here"? If so I can fix that up on check-in.
Other than that:
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 01/10] xen: sched: harmonize debug dump output among schedulers.
2017-02-15 10:17 ` George Dunlap
@ 2017-02-15 10:31 ` Dario Faggioli
0 siblings, 0 replies; 35+ messages in thread
From: Dario Faggioli @ 2017-02-15 10:31 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel, Anshul Makkar, Meng Xu
[-- Attachment #1.1: Type: text/plain, Size: 1099 bytes --]
On Wed, 2017-02-15 at 10:17 +0000, George Dunlap wrote:
> On Thu, Feb 9, 2017 at 1:58 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> >
> > Information we currently print for idle pCPUs is
>
> I take it you meant "idle vCPUs here"?
>
Sort-of. What I actually meant is that we print info about the idle
vCPUs for all pCPUs that happen to be idle during the dump, which is
redundant bcause such info about the status of idle vCPUs are not very
interesting or useful, and never change.
So, basically, yes, I agree that switching from 'pCPUs' to 'vCPUs' in
that sentence makes what I tried to explain above much easier to
understand.
> If so I can fix that up on check-in.
>
Great, go ahead.
> Other than that:
>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
>
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 02/10] xen: credit2: clear bit instead of skip step in runq_tickle()
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
2017-02-09 13:58 ` [PATCH v2 01/10] xen: sched: harmonize debug dump output among schedulers Dario Faggioli
@ 2017-02-09 13:58 ` Dario Faggioli
2017-02-15 10:21 ` George Dunlap
2017-02-09 13:58 ` [PATCH v2 03/10] xen: credit2: improve comments' style and definition of CSFLAG-s Dario Faggioli
` (9 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:58 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
Since we are doing cpumask manipulation already, clear a bit
in the mask at once. Doing that will save us an if, later in
the code.
No functional change intended.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes from v1:
* rebased on current staging.
---
xen/common/sched_credit2.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 741d372..920a7ce 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -991,7 +991,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
cpumask_andnot(&mask, &rqd->active, &rqd->idle);
cpumask_andnot(&mask, &mask, &rqd->tickled);
cpumask_and(&mask, &mask, cpumask_scratch_cpu(cpu));
- if ( cpumask_test_cpu(cpu, &mask) )
+ if ( __cpumask_test_and_clear_cpu(cpu, &mask) )
{
cur = CSCHED2_VCPU(curr_on_cpu(cpu));
burn_credits(rqd, cur, now);
@@ -1007,8 +1007,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
for_each_cpu(i, &mask)
{
/* Already looked at this one above */
- if ( i == cpu )
- continue;
+ ASSERT(i != cpu);
cur = CSCHED2_VCPU(curr_on_cpu(i));
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 02/10] xen: credit2: clear bit instead of skip step in runq_tickle()
2017-02-09 13:58 ` [PATCH v2 02/10] xen: credit2: clear bit instead of skip step in runq_tickle() Dario Faggioli
@ 2017-02-15 10:21 ` George Dunlap
0 siblings, 0 replies; 35+ messages in thread
From: George Dunlap @ 2017-02-15 10:21 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel
On Thu, Feb 9, 2017 at 1:58 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> Since we are doing cpumask manipulation already, clear a bit
> in the mask at once. Doing that will save us an if, later in
> the code.
>
> No functional change intended.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Changes from v1:
> * rebased on current staging.
> ---
> xen/common/sched_credit2.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 741d372..920a7ce 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -991,7 +991,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
> cpumask_andnot(&mask, &rqd->active, &rqd->idle);
> cpumask_andnot(&mask, &mask, &rqd->tickled);
> cpumask_and(&mask, &mask, cpumask_scratch_cpu(cpu));
> - if ( cpumask_test_cpu(cpu, &mask) )
> + if ( __cpumask_test_and_clear_cpu(cpu, &mask) )
> {
> cur = CSCHED2_VCPU(curr_on_cpu(cpu));
> burn_credits(rqd, cur, now);
> @@ -1007,8 +1007,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
> for_each_cpu(i, &mask)
> {
> /* Already looked at this one above */
> - if ( i == cpu )
> - continue;
> + ASSERT(i != cpu);
>
> cur = CSCHED2_VCPU(curr_on_cpu(i));
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 03/10] xen: credit2: improve comments' style and definition of CSFLAG-s
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
2017-02-09 13:58 ` [PATCH v2 01/10] xen: sched: harmonize debug dump output among schedulers Dario Faggioli
2017-02-09 13:58 ` [PATCH v2 02/10] xen: credit2: clear bit instead of skip step in runq_tickle() Dario Faggioli
@ 2017-02-09 13:58 ` Dario Faggioli
2017-02-15 10:44 ` George Dunlap
2017-02-09 13:58 ` [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros Dario Faggioli
` (8 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:58 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar
Most of the comments describing the meaning of the
vCPU flags used by the scheduler miss the 'wings' (or
have other minor style issues).
Also, use 1U (instead of 1) as the base of shiftings.
No functional change intended.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
xen/common/sched_credit2.c | 54 ++++++++++++++++++++++++++++----------------
1 file changed, 34 insertions(+), 20 deletions(-)
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 920a7ce..b482990 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -129,35 +129,47 @@
/*
* Basic constants
*/
-/* Default weight: How much a new domain starts with */
+/* Default weight: How much a new domain starts with. */
#define CSCHED2_DEFAULT_WEIGHT 256
-/* Min timer: Minimum length a timer will be set, to
- * achieve efficiency */
+/*
+ * Min timer: Minimum length a timer will be set, to
+ * achieve efficiency.
+ */
#define CSCHED2_MIN_TIMER MICROSECS(500)
-/* Amount of credit VMs begin with, and are reset to.
+/*
+ * Amount of credit VMs begin with, and are reset to.
* ATM, set so that highest-weight VMs can only run for 10ms
- * before a reset event. */
+ * before a reset event.
+ */
#define CSCHED2_CREDIT_INIT MILLISECS(10)
-/* Carryover: How much "extra" credit may be carried over after
- * a reset. */
+/*
+ * Amount of credit the idle vcpus have. It never changes, as idle
+ * vcpus does not consume credits, and it must be lower than whatever
+ * amount of credit 'regular' vcpu would end up with.
+ */
+#define CSCHED2_IDLE_CREDIT (-(1U<<30))
+/*
+ * Carryover: How much "extra" credit may be carried over after
+ * a reset.
+ */
#define CSCHED2_CARRYOVER_MAX CSCHED2_MIN_TIMER
-/* Stickiness: Cross-L2 migration resistance. Should be less than
- * MIN_TIMER. */
+/*
+ * Stickiness: Cross-L2 migration resistance. Should be less than
+ * MIN_TIMER.
+ */
#define CSCHED2_MIGRATE_RESIST ((opt_migrate_resist)*MICROSECS(1))
-/* How much to "compensate" a vcpu for L2 migration */
+/* How much to "compensate" a vcpu for L2 migration. */
#define CSCHED2_MIGRATE_COMPENSATION MICROSECS(50)
/* Reset: Value below which credit will be reset. */
#define CSCHED2_CREDIT_RESET 0
/* Max timer: Maximum time a guest can be run for. */
#define CSCHED2_MAX_TIMER CSCHED2_CREDIT_INIT
-
-#define CSCHED2_IDLE_CREDIT (-(1<<30))
-
/*
* Flags
*/
-/* CSFLAG_scheduled: Is this vcpu either running on, or context-switching off,
+/*
+ * CSFLAG_scheduled: Is this vcpu either running on, or context-switching off,
* a physical cpu?
* + Accessed only with runqueue lock held
* + Set when chosen as next in csched2_schedule().
@@ -167,8 +179,9 @@
* + Checked to be false in runq_insert.
*/
#define __CSFLAG_scheduled 1
-#define CSFLAG_scheduled (1<<__CSFLAG_scheduled)
-/* CSFLAG_delayed_runq_add: Do we need to add this to the runqueue once it'd done
+#define CSFLAG_scheduled (1U<<__CSFLAG_scheduled)
+/*
+ * CSFLAG_delayed_runq_add: Do we need to add this to the runqueue once it'd done
* being context switched out?
* + Set when scheduling out in csched2_schedule() if prev is runnable
* + Set in csched2_vcpu_wake if it finds CSFLAG_scheduled set
@@ -176,20 +189,21 @@
* clears the bit.
*/
#define __CSFLAG_delayed_runq_add 2
-#define CSFLAG_delayed_runq_add (1<<__CSFLAG_delayed_runq_add)
-/* CSFLAG_runq_migrate_request: This vcpu is being migrated as a result of a
+#define CSFLAG_delayed_runq_add (1U<<__CSFLAG_delayed_runq_add)
+/*
+ * CSFLAG_runq_migrate_request: This vcpu is being migrated as a result of a
* credit2-initiated runq migrate request; migrate it to the runqueue indicated
* in the svc struct.
*/
#define __CSFLAG_runq_migrate_request 3
-#define CSFLAG_runq_migrate_request (1<<__CSFLAG_runq_migrate_request)
+#define CSFLAG_runq_migrate_request (1U<<__CSFLAG_runq_migrate_request)
/*
* CSFLAG_vcpu_yield: this vcpu was running, and has called vcpu_yield(). The
* scheduler is invoked to see if we can give the cpu to someone else, and
* get back to the yielding vcpu in a while.
*/
#define __CSFLAG_vcpu_yield 4
-#define CSFLAG_vcpu_yield (1<<__CSFLAG_vcpu_yield)
+#define CSFLAG_vcpu_yield (1U<<__CSFLAG_vcpu_yield)
static unsigned int __read_mostly opt_migrate_resist = 500;
integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
` (2 preceding siblings ...)
2017-02-09 13:58 ` [PATCH v2 03/10] xen: credit2: improve comments' style and definition of CSFLAG-s Dario Faggioli
@ 2017-02-09 13:58 ` Dario Faggioli
2017-02-09 14:14 ` Andrew Cooper
2017-02-09 14:36 ` Jan Beulich
2017-02-09 13:58 ` [PATCH v2 05/10] xen: credit2: tidy up functions names by removing leading '__' Dario Faggioli
` (7 subsequent siblings)
11 siblings, 2 replies; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:58 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar
There isn't any particular reason for the accessor helpers
to be macro, so turn them into 'static inline'-s, which are
better.
Note that it is necessary to move the function definitions
below the structure declarations.
No functional change intended.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
xen/common/sched_credit2.c | 156 +++++++++++++++++++++++++-------------------
1 file changed, 89 insertions(+), 67 deletions(-)
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index b482990..786dcca 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -209,18 +209,6 @@ static unsigned int __read_mostly opt_migrate_resist = 500;
integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
/*
- * Useful macros
- */
-#define CSCHED2_PRIV(_ops) \
- ((struct csched2_private *)((_ops)->sched_data))
-#define CSCHED2_VCPU(_vcpu) ((struct csched2_vcpu *) (_vcpu)->sched_priv)
-#define CSCHED2_DOM(_dom) ((struct csched2_dom *) (_dom)->sched_priv)
-/* CPU to runq_id macro */
-#define c2r(_ops, _cpu) (CSCHED2_PRIV(_ops)->runq_map[(_cpu)])
-/* CPU to runqueue struct macro */
-#define RQD(_ops, _cpu) (&CSCHED2_PRIV(_ops)->rqd[c2r(_ops, _cpu)])
-
-/*
* Load tracking and load balancing
*
* Load history of runqueues and vcpus is accounted for by using an
@@ -441,6 +429,40 @@ struct csched2_dom {
};
/*
+ * Accessor helpers functions.
+ */
+static always_inline
+struct csched2_private *csched2_priv(const struct scheduler *ops)
+{
+ return ops->sched_data;
+}
+
+static always_inline
+struct csched2_vcpu *csched2_vcpu(struct vcpu *v)
+{
+ return v->sched_priv;
+}
+
+static always_inline
+struct csched2_dom *csched2_dom(struct domain *d)
+{
+ return d->sched_priv;
+}
+
+/* CPU to runq_id macro */
+static always_inline int c2r(const struct scheduler *ops, unsigned cpu)
+{
+ return (csched2_priv(ops))->runq_map[(cpu)];
+}
+
+/* CPU to runqueue struct macro */
+static always_inline
+struct csched2_runqueue_data *c2rqd(const struct scheduler *ops, unsigned cpu)
+{
+ return &csched2_priv(ops)->rqd[c2r(ops, cpu)];
+}
+
+/*
* Hyperthreading (SMT) support.
*
* We use a special per-runq mask (smt_idle) and update it according to the
@@ -694,7 +716,7 @@ static void
__update_runq_load(const struct scheduler *ops,
struct csched2_runqueue_data *rqd, int change, s_time_t now)
{
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
s_time_t delta, load = rqd->load;
unsigned int P, W;
@@ -781,7 +803,7 @@ static void
__update_svc_load(const struct scheduler *ops,
struct csched2_vcpu *svc, int change, s_time_t now)
{
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
s_time_t delta, vcpu_load;
unsigned int P, W;
@@ -878,7 +900,7 @@ static void
runq_insert(const struct scheduler *ops, struct csched2_vcpu *svc)
{
unsigned int cpu = svc->vcpu->processor;
- struct list_head * runq = &RQD(ops, cpu)->runq;
+ struct list_head * runq = &c2rqd(ops, cpu)->runq;
int pos = 0;
ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock));
@@ -936,7 +958,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
int i, ipid = -1;
s_time_t lowest = (1<<30);
unsigned int cpu = new->vcpu->processor;
- struct csched2_runqueue_data *rqd = RQD(ops, cpu);
+ struct csched2_runqueue_data *rqd = c2rqd(ops, cpu);
cpumask_t mask;
struct csched2_vcpu * cur;
@@ -1007,7 +1029,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
cpumask_and(&mask, &mask, cpumask_scratch_cpu(cpu));
if ( __cpumask_test_and_clear_cpu(cpu, &mask) )
{
- cur = CSCHED2_VCPU(curr_on_cpu(cpu));
+ cur = csched2_vcpu(curr_on_cpu(cpu));
burn_credits(rqd, cur, now);
if ( cur->credit < new->credit )
@@ -1023,7 +1045,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
/* Already looked at this one above */
ASSERT(i != cpu);
- cur = CSCHED2_VCPU(curr_on_cpu(i));
+ cur = csched2_vcpu(curr_on_cpu(i));
/*
* Even if the cpu is not in rqd->idle, it may be running the
@@ -1096,7 +1118,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
static void reset_credit(const struct scheduler *ops, int cpu, s_time_t now,
struct csched2_vcpu *snext)
{
- struct csched2_runqueue_data *rqd = RQD(ops, cpu);
+ struct csched2_runqueue_data *rqd = c2rqd(ops, cpu);
struct list_head *iter;
int m;
@@ -1174,7 +1196,7 @@ void burn_credits(struct csched2_runqueue_data *rqd,
{
s_time_t delta;
- ASSERT(svc == CSCHED2_VCPU(curr_on_cpu(svc->vcpu->processor)));
+ ASSERT(svc == csched2_vcpu(curr_on_cpu(svc->vcpu->processor)));
if ( unlikely(is_idle_vcpu(svc->vcpu)) )
{
@@ -1261,11 +1283,11 @@ static void update_max_weight(struct csched2_runqueue_data *rqd, int new_weight,
static /*inline*/ void
__csched2_vcpu_check(struct vcpu *vc)
{
- struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
+ struct csched2_vcpu * const svc = csched2_vcpu(vc);
struct csched2_dom * const sdom = svc->sdom;
BUG_ON( svc->vcpu != vc );
- BUG_ON( sdom != CSCHED2_DOM(vc->domain) );
+ BUG_ON( sdom != csched2_dom(vc->domain) );
if ( sdom )
{
BUG_ON( is_idle_vcpu(vc) );
@@ -1305,7 +1327,7 @@ csched2_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
svc->credit = CSCHED2_CREDIT_INIT;
svc->weight = svc->sdom->weight;
/* Starting load of 50% */
- svc->avgload = 1ULL << (CSCHED2_PRIV(ops)->load_precision_shift - 1);
+ svc->avgload = 1ULL << (csched2_priv(ops)->load_precision_shift - 1);
svc->load_last_update = NOW() >> LOADAVG_GRANULARITY_SHIFT;
}
else
@@ -1357,7 +1379,7 @@ runq_assign(const struct scheduler *ops, struct vcpu *vc)
ASSERT(svc->rqd == NULL);
- __runq_assign(svc, RQD(ops, vc->processor));
+ __runq_assign(svc, c2rqd(ops, vc->processor));
}
static void
@@ -1382,7 +1404,7 @@ runq_deassign(const struct scheduler *ops, struct vcpu *vc)
{
struct csched2_vcpu *svc = vc->sched_priv;
- ASSERT(svc->rqd == RQD(ops, vc->processor));
+ ASSERT(svc->rqd == c2rqd(ops, vc->processor));
__runq_deassign(svc);
}
@@ -1390,7 +1412,7 @@ runq_deassign(const struct scheduler *ops, struct vcpu *vc)
static void
csched2_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
{
- struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
+ struct csched2_vcpu * const svc = csched2_vcpu(vc);
ASSERT(!is_idle_vcpu(vc));
SCHED_STAT_CRANK(vcpu_sleep);
@@ -1399,7 +1421,7 @@ csched2_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
cpu_raise_softirq(vc->processor, SCHEDULE_SOFTIRQ);
else if ( __vcpu_on_runq(svc) )
{
- ASSERT(svc->rqd == RQD(ops, vc->processor));
+ ASSERT(svc->rqd == c2rqd(ops, vc->processor));
update_load(ops, svc->rqd, svc, -1, NOW());
__runq_remove(svc);
}
@@ -1410,7 +1432,7 @@ csched2_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
static void
csched2_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
{
- struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
+ struct csched2_vcpu * const svc = csched2_vcpu(vc);
unsigned int cpu = vc->processor;
s_time_t now;
@@ -1448,7 +1470,7 @@ csched2_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
if ( svc->rqd == NULL )
runq_assign(ops, vc);
else
- ASSERT(RQD(ops, vc->processor) == svc->rqd );
+ ASSERT(c2rqd(ops, vc->processor) == svc->rqd );
now = NOW();
@@ -1465,7 +1487,7 @@ out:
static void
csched2_vcpu_yield(const struct scheduler *ops, struct vcpu *v)
{
- struct csched2_vcpu * const svc = CSCHED2_VCPU(v);
+ struct csched2_vcpu * const svc = csched2_vcpu(v);
__set_bit(__CSFLAG_vcpu_yield, &svc->flags);
}
@@ -1473,12 +1495,12 @@ csched2_vcpu_yield(const struct scheduler *ops, struct vcpu *v)
static void
csched2_context_saved(const struct scheduler *ops, struct vcpu *vc)
{
- struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
+ struct csched2_vcpu * const svc = csched2_vcpu(vc);
spinlock_t *lock = vcpu_schedule_lock_irq(vc);
s_time_t now = NOW();
- BUG_ON( !is_idle_vcpu(vc) && svc->rqd != RQD(ops, vc->processor));
- ASSERT(is_idle_vcpu(vc) || svc->rqd == RQD(ops, vc->processor));
+ BUG_ON( !is_idle_vcpu(vc) && svc->rqd != c2rqd(ops, vc->processor));
+ ASSERT(is_idle_vcpu(vc) || svc->rqd == c2rqd(ops, vc->processor));
/* This vcpu is now eligible to be put on the runqueue again */
__clear_bit(__CSFLAG_scheduled, &svc->flags);
@@ -1509,9 +1531,9 @@ csched2_context_saved(const struct scheduler *ops, struct vcpu *vc)
static int
csched2_cpu_pick(const struct scheduler *ops, struct vcpu *vc)
{
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
int i, min_rqi = -1, new_cpu, cpu = vc->processor;
- struct csched2_vcpu *svc = CSCHED2_VCPU(vc);
+ struct csched2_vcpu *svc = csched2_vcpu(vc);
s_time_t min_avgload = MAX_LOAD;
ASSERT(!cpumask_empty(&prv->active_queues));
@@ -1774,7 +1796,7 @@ static bool_t vcpu_is_migrateable(struct csched2_vcpu *svc,
static void balance_load(const struct scheduler *ops, int cpu, s_time_t now)
{
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
int i, max_delta_rqi = -1;
struct list_head *push_iter, *pull_iter;
bool_t inner_load_updated = 0;
@@ -1789,7 +1811,7 @@ static void balance_load(const struct scheduler *ops, int cpu, s_time_t now)
*/
ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock));
- st.lrqd = RQD(ops, cpu);
+ st.lrqd = c2rqd(ops, cpu);
__update_runq_load(ops, st.lrqd, 0, now);
@@ -1962,7 +1984,7 @@ csched2_vcpu_migrate(
const struct scheduler *ops, struct vcpu *vc, unsigned int new_cpu)
{
struct domain *d = vc->domain;
- struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
+ struct csched2_vcpu * const svc = csched2_vcpu(vc);
struct csched2_runqueue_data *trqd;
s_time_t now = NOW();
@@ -1995,10 +2017,10 @@ csched2_vcpu_migrate(
}
/* If here, new_cpu must be a valid Credit2 pCPU, and in our affinity. */
- ASSERT(cpumask_test_cpu(new_cpu, &CSCHED2_PRIV(ops)->initialized));
+ ASSERT(cpumask_test_cpu(new_cpu, &csched2_priv(ops)->initialized));
ASSERT(cpumask_test_cpu(new_cpu, vc->cpu_hard_affinity));
- trqd = RQD(ops, new_cpu);
+ trqd = c2rqd(ops, new_cpu);
/*
* Do the actual movement toward new_cpu, and update vc->processor.
@@ -2020,8 +2042,8 @@ csched2_dom_cntl(
struct domain *d,
struct xen_domctl_scheduler_op *op)
{
- struct csched2_dom * const sdom = CSCHED2_DOM(d);
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_dom * const sdom = csched2_dom(d);
+ struct csched2_private *prv = csched2_priv(ops);
unsigned long flags;
int rc = 0;
@@ -2054,10 +2076,10 @@ csched2_dom_cntl(
/* Update weights for vcpus, and max_weight for runqueues on which they reside */
for_each_vcpu ( d, v )
{
- struct csched2_vcpu *svc = CSCHED2_VCPU(v);
+ struct csched2_vcpu *svc = csched2_vcpu(v);
spinlock_t *lock = vcpu_schedule_lock(svc->vcpu);
- ASSERT(svc->rqd == RQD(ops, svc->vcpu->processor));
+ ASSERT(svc->rqd == c2rqd(ops, svc->vcpu->processor));
svc->weight = sdom->weight;
update_max_weight(svc->rqd, svc->weight, old_weight);
@@ -2081,7 +2103,7 @@ static int csched2_sys_cntl(const struct scheduler *ops,
struct xen_sysctl_scheduler_op *sc)
{
xen_sysctl_credit2_schedule_t *params = &sc->u.sched_credit2;
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
unsigned long flags;
switch (sc->cmd )
@@ -2112,7 +2134,7 @@ static int csched2_sys_cntl(const struct scheduler *ops,
static void *
csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
{
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
struct csched2_dom *sdom;
unsigned long flags;
@@ -2128,7 +2150,7 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom)
write_lock_irqsave(&prv->lock, flags);
- list_add_tail(&sdom->sdom_elem, &CSCHED2_PRIV(ops)->sdom);
+ list_add_tail(&sdom->sdom_elem, &csched2_priv(ops)->sdom);
write_unlock_irqrestore(&prv->lock, flags);
@@ -2157,7 +2179,7 @@ csched2_free_domdata(const struct scheduler *ops, void *data)
{
unsigned long flags;
struct csched2_dom *sdom = data;
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
write_lock_irqsave(&prv->lock, flags);
@@ -2171,9 +2193,9 @@ csched2_free_domdata(const struct scheduler *ops, void *data)
static void
csched2_dom_destroy(const struct scheduler *ops, struct domain *dom)
{
- ASSERT(CSCHED2_DOM(dom)->nr_vcpus == 0);
+ ASSERT(csched2_dom(dom)->nr_vcpus == 0);
- csched2_free_domdata(ops, CSCHED2_DOM(dom));
+ csched2_free_domdata(ops, csched2_dom(dom));
}
static void
@@ -2218,7 +2240,7 @@ csched2_free_vdata(const struct scheduler *ops, void *priv)
static void
csched2_vcpu_remove(const struct scheduler *ops, struct vcpu *vc)
{
- struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
+ struct csched2_vcpu * const svc = csched2_vcpu(vc);
spinlock_t *lock;
ASSERT(!is_idle_vcpu(vc));
@@ -2243,9 +2265,9 @@ csched2_runtime(const struct scheduler *ops, int cpu,
{
s_time_t time, min_time;
int rt_credit; /* Proposed runtime measured in credits */
- struct csched2_runqueue_data *rqd = RQD(ops, cpu);
+ struct csched2_runqueue_data *rqd = c2rqd(ops, cpu);
struct list_head *runq = &rqd->runq;
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
/*
* If we're idle, just stay so. Others (or external events)
@@ -2334,7 +2356,7 @@ runq_candidate(struct csched2_runqueue_data *rqd,
{
struct list_head *iter;
struct csched2_vcpu *snext = NULL;
- struct csched2_private *prv = CSCHED2_PRIV(per_cpu(scheduler, cpu));
+ struct csched2_private *prv = csched2_priv(per_cpu(scheduler, cpu));
bool yield = __test_and_clear_bit(__CSFLAG_vcpu_yield, &scurr->flags);
*skipped = 0;
@@ -2373,7 +2395,7 @@ runq_candidate(struct csched2_runqueue_data *rqd,
if ( vcpu_runnable(scurr->vcpu) )
snext = scurr;
else
- snext = CSCHED2_VCPU(idle_vcpu[cpu]);
+ snext = csched2_vcpu(idle_vcpu[cpu]);
list_for_each( iter, &rqd->runq )
{
@@ -2453,7 +2475,7 @@ csched2_schedule(
{
const int cpu = smp_processor_id();
struct csched2_runqueue_data *rqd;
- struct csched2_vcpu * const scurr = CSCHED2_VCPU(current);
+ struct csched2_vcpu * const scurr = csched2_vcpu(current);
struct csched2_vcpu *snext = NULL;
unsigned int skipped_vcpus = 0;
struct task_slice ret;
@@ -2462,9 +2484,9 @@ csched2_schedule(
SCHED_STAT_CRANK(schedule);
CSCHED2_VCPU_CHECK(current);
- BUG_ON(!cpumask_test_cpu(cpu, &CSCHED2_PRIV(ops)->initialized));
+ BUG_ON(!cpumask_test_cpu(cpu, &csched2_priv(ops)->initialized));
- rqd = RQD(ops, cpu);
+ rqd = c2rqd(ops, cpu);
BUG_ON(!cpumask_test_cpu(cpu, &rqd->active));
ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock));
@@ -2522,7 +2544,7 @@ csched2_schedule(
{
__clear_bit(__CSFLAG_vcpu_yield, &scurr->flags);
trace_var(TRC_CSCHED2_SCHED_TASKLET, 1, 0, NULL);
- snext = CSCHED2_VCPU(idle_vcpu[cpu]);
+ snext = csched2_vcpu(idle_vcpu[cpu]);
}
else
snext = runq_candidate(rqd, scurr, cpu, now, &skipped_vcpus);
@@ -2643,7 +2665,7 @@ csched2_dump_vcpu(struct csched2_private *prv, struct csched2_vcpu *svc)
static inline void
dump_pcpu(const struct scheduler *ops, int cpu)
{
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
struct csched2_vcpu *svc;
#define cpustr keyhandler_scratch
@@ -2653,7 +2675,7 @@ dump_pcpu(const struct scheduler *ops, int cpu)
printk("core=%s\n", cpustr);
/* current VCPU (nothing to say if that's the idle vcpu) */
- svc = CSCHED2_VCPU(curr_on_cpu(cpu));
+ svc = csched2_vcpu(curr_on_cpu(cpu));
if ( svc && !is_idle_vcpu(svc->vcpu) )
{
printk("\trun: ");
@@ -2666,7 +2688,7 @@ static void
csched2_dump(const struct scheduler *ops)
{
struct list_head *iter_sdom;
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
unsigned long flags;
unsigned int i, j, loop;
#define cpustr keyhandler_scratch
@@ -2726,7 +2748,7 @@ csched2_dump(const struct scheduler *ops)
for_each_vcpu( sdom->dom, v )
{
- struct csched2_vcpu * const svc = CSCHED2_VCPU(v);
+ struct csched2_vcpu * const svc = csched2_vcpu(v);
spinlock_t *lock;
lock = vcpu_schedule_lock(svc->vcpu);
@@ -2897,7 +2919,7 @@ init_pdata(struct csched2_private *prv, unsigned int cpu)
static void
csched2_init_pdata(const struct scheduler *ops, void *pdata, int cpu)
{
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
spinlock_t *old_lock;
unsigned long flags;
unsigned rqi;
@@ -2925,7 +2947,7 @@ static void
csched2_switch_sched(struct scheduler *new_ops, unsigned int cpu,
void *pdata, void *vdata)
{
- struct csched2_private *prv = CSCHED2_PRIV(new_ops);
+ struct csched2_private *prv = csched2_priv(new_ops);
struct csched2_vcpu *svc = vdata;
unsigned rqi;
@@ -2972,7 +2994,7 @@ static void
csched2_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu)
{
unsigned long flags;
- struct csched2_private *prv = CSCHED2_PRIV(ops);
+ struct csched2_private *prv = csched2_priv(ops);
struct csched2_runqueue_data *rqd;
int rqi;
@@ -3081,7 +3103,7 @@ csched2_deinit(struct scheduler *ops)
{
struct csched2_private *prv;
- prv = CSCHED2_PRIV(ops);
+ prv = csched2_priv(ops);
ops->sched_data = NULL;
xfree(prv);
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros
2017-02-09 13:58 ` [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros Dario Faggioli
@ 2017-02-09 14:14 ` Andrew Cooper
2017-02-09 14:34 ` Jan Beulich
2017-02-09 14:36 ` Jan Beulich
1 sibling, 1 reply; 35+ messages in thread
From: Andrew Cooper @ 2017-02-09 14:14 UTC (permalink / raw)
To: Dario Faggioli, xen-devel; +Cc: George Dunlap, Anshul Makkar
On 09/02/17 13:58, Dario Faggioli wrote:
> @@ -441,6 +429,40 @@ struct csched2_dom {
> };
>
> /*
> + * Accessor helpers functions.
> + */
> +static always_inline
The always_inline isn't necessary. For one, the compiler is always
going to inline these, and IMO should be reserved for when you are
purposefully overriding what the compiler wants to do. As far as I can
tell, its only use thus far in tree is to override what the compiler
tries to do wrt inline assembly.
> +struct csched2_private *csched2_priv(const struct scheduler *ops)
You should either return a const csched2_private *, or not take a const
ops. (Your choice.)
Despite being allowed by the C typesystem, it is type (ab)use as it
changes the expectation of of what is meant by passing a const ops in
the first place.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros
2017-02-09 14:14 ` Andrew Cooper
@ 2017-02-09 14:34 ` Jan Beulich
0 siblings, 0 replies; 35+ messages in thread
From: Jan Beulich @ 2017-02-09 14:34 UTC (permalink / raw)
To: Andrew Cooper, Dario Faggioli; +Cc: George Dunlap, xen-devel, Anshul Makkar
>>> On 09.02.17 at 15:14, <andrew.cooper3@citrix.com> wrote:
> On 09/02/17 13:58, Dario Faggioli wrote:
>> +struct csched2_private *csched2_priv(const struct scheduler *ops)
>
> You should either return a const csched2_private *, or not take a const
> ops. (Your choice.)
>
> Despite being allowed by the C typesystem, it is type (ab)use as it
> changes the expectation of of what is meant by passing a const ops in
> the first place.
That's debatable. I'd rather see csched2_vcpu() and csched2_dom()
also be changed to match the above.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros
2017-02-09 13:58 ` [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros Dario Faggioli
2017-02-09 14:14 ` Andrew Cooper
@ 2017-02-09 14:36 ` Jan Beulich
2017-02-09 15:33 ` Dario Faggioli
1 sibling, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2017-02-09 14:36 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, xen-devel, Anshul Makkar
>>> On 09.02.17 at 14:58, <dario.faggioli@citrix.com> wrote:
> +/* CPU to runq_id macro */
> +static always_inline int c2r(const struct scheduler *ops, unsigned cpu)
> +{
> + return (csched2_priv(ops))->runq_map[(cpu)];
Any reason for having the (pointless) parentheses here but not ...
> +}
> +
> +/* CPU to runqueue struct macro */
> +static always_inline
> +struct csched2_runqueue_data *c2rqd(const struct scheduler *ops, unsigned cpu)
> +{
> + return &csched2_priv(ops)->rqd[c2r(ops, cpu)];
... here?
Also I'm not sure whether the sched*.c files are an exception, but
generally we don't use plain "unsigned" but always "unsigned int".
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros
2017-02-09 14:36 ` Jan Beulich
@ 2017-02-09 15:33 ` Dario Faggioli
2017-02-15 10:49 ` George Dunlap
0 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 15:33 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, xen-devel, Anshul Makkar
[-- Attachment #1.1: Type: text/plain, Size: 1237 bytes --]
On Thu, 2017-02-09 at 07:36 -0700, Jan Beulich wrote:
> > > > On 09.02.17 at 14:58, <dario.faggioli@citrix.com> wrote:
> > +/* CPU to runq_id macro */
> > +static always_inline int c2r(const struct scheduler *ops, unsigned
> > cpu)
> > +{
> > + return (csched2_priv(ops))->runq_map[(cpu)];
>
> Any reason for having the (pointless) parentheses here but not ...
> > +/* CPU to runqueue struct macro */
> > +static always_inline
> > +struct csched2_runqueue_data *c2rqd(const struct scheduler *ops,
> > unsigned cpu)
> > +{
> > + return &csched2_priv(ops)->rqd[c2r(ops, cpu)];
>
> ... here?
>
Nope. I'll get rid of them from above.
> Also I'm not sure whether the sched*.c files are an exception, but
> generally we don't use plain "unsigned" but always "unsigned int".
>
Yes, in sched_credit.c and sched_credit2.c there are a few unsigned.
But I'm happy to use "unsigned int" for new code, such as here.
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros
2017-02-09 15:33 ` Dario Faggioli
@ 2017-02-15 10:49 ` George Dunlap
2017-02-24 18:26 ` Dario Faggioli
0 siblings, 1 reply; 35+ messages in thread
From: George Dunlap @ 2017-02-15 10:49 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Anshul Makkar, Jan Beulich
On Thu, Feb 9, 2017 at 3:33 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Thu, 2017-02-09 at 07:36 -0700, Jan Beulich wrote:
>> > > > On 09.02.17 at 14:58, <dario.faggioli@citrix.com> wrote:
>> > +/* CPU to runq_id macro */
>> > +static always_inline int c2r(const struct scheduler *ops, unsigned
>> > cpu)
>> > +{
>> > + return (csched2_priv(ops))->runq_map[(cpu)];
>>
>> Any reason for having the (pointless) parentheses here but not ...
>
>> > +/* CPU to runqueue struct macro */
>> > +static always_inline
>> > +struct csched2_runqueue_data *c2rqd(const struct scheduler *ops,
>> > unsigned cpu)
>> > +{
>> > + return &csched2_priv(ops)->rqd[c2r(ops, cpu)];
>>
>> ... here?
>>
> Nope. I'll get rid of them from above.
>
>> Also I'm not sure whether the sched*.c files are an exception, but
>> generally we don't use plain "unsigned" but always "unsigned int".
>>
> Yes, in sched_credit.c and sched_credit2.c there are a few unsigned.
> But I'm happy to use "unsigned int" for new code, such as here.
FWIW I agree with Andy and Jan's comments. Everything else looks good to me.
Thanks,
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros
2017-02-15 10:49 ` George Dunlap
@ 2017-02-24 18:26 ` Dario Faggioli
0 siblings, 0 replies; 35+ messages in thread
From: Dario Faggioli @ 2017-02-24 18:26 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel, Anshul Makkar, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 1289 bytes --]
On Wed, 2017-02-15 at 10:49 +0000, George Dunlap wrote:
> On Thu, Feb 9, 2017 at 3:33 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > On Thu, 2017-02-09 at 07:36 -0700, Jan Beulich wrote:
> > Nope. I'll get rid of them from above.
> > > Also I'm not sure whether the sched*.c files are an exception,
> > > but
> > > generally we don't use plain "unsigned" but always "unsigned
> > > int".
> > >
> > Yes, in sched_credit.c and sched_credit2.c there are a few
> > unsigned.
> > But I'm happy to use "unsigned int" for new code, such as here.
>
> FWIW I agree with Andy and Jan's comments. Everything else looks
> good to me.
>
Err... So, I removed the pointless parentheses and I turned "unsigned"
to "unsigned int".
As per consts, I agree with Jan, and hence I added the qualifier to the
pointer arguments of csched2_dom() and csched2_vcpu(), as he suggested,
without changing the return type.
And I'm also using plain 'inline' rather than 'always_inline'.
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 05/10] xen: credit2: tidy up functions names by removing leading '__'.
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
` (3 preceding siblings ...)
2017-02-09 13:58 ` [PATCH v2 04/10] xen: credit2: make accessor helpers inline functions instead of macros Dario Faggioli
@ 2017-02-09 13:58 ` Dario Faggioli
2017-02-15 13:57 ` George Dunlap
2017-02-09 13:59 ` [PATCH v2 06/10] xen: credit2: group the runq manipulating functions Dario Faggioli
` (6 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:58 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar
There is no reason for having pretty much all of the
functions whose names begin with double underscores
('__') to actually look like that.
In fact, that is misleading and makes the code hard
to read and understand. So, remove the '__'-s.
The only two that we keep are __runq_assign() and
__runq_deassign() (althought they're converted to
single underscore). In fact, in those cases, it is
indeed useful to have those sort of a "raw" variants.
In case of __runq_insert(), which is only called
once, by runq_insert(), merge the two functions.
No functional change intended.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
xen/common/sched_credit2.c | 114 +++++++++++++++++++-------------------------
1 file changed, 49 insertions(+), 65 deletions(-)
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 786dcca..4b4f4f8 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -221,7 +221,7 @@ integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
* shift all time samples to the right.
*
* The details of the formulas used for load tracking are explained close to
- * __update_runq_load(). Let's just say here that, with full nanosecond time
+ * update_runq_load(). Let's just say here that, with full nanosecond time
* granularity, a 30 bits wide 'decaying window' is ~1 second long.
*
* We want to consider the following equations:
@@ -233,7 +233,7 @@ integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
* Q-format fixed point arithmetic and load is the instantaneous load of a
* runqueue, which basically is the number of runnable vcpus there are on the
* runqueue (for the meaning of the other terms, look at the doc comment to
- * __update_runq_load()).
+ * update_runq_load()).
*
* So, again, with full nanosecond granularity, and 1 second window, we have:
*
@@ -594,14 +594,12 @@ static s_time_t c2t(struct csched2_runqueue_data *rqd, s_time_t credit, struct c
* Runqueue related code
*/
-static /*inline*/ int
-__vcpu_on_runq(struct csched2_vcpu *svc)
+static inline int vcpu_on_runq(struct csched2_vcpu *svc)
{
return !list_empty(&svc->runq_elem);
}
-static /*inline*/ struct csched2_vcpu *
-__runq_elem(struct list_head *elem)
+static struct csched2_vcpu * runq_elem(struct list_head *elem)
{
return list_entry(elem, struct csched2_vcpu, runq_elem);
}
@@ -713,8 +711,8 @@ __runq_elem(struct list_head *elem)
* Which, in both cases, is what we expect.
*/
static void
-__update_runq_load(const struct scheduler *ops,
- struct csched2_runqueue_data *rqd, int change, s_time_t now)
+update_runq_load(const struct scheduler *ops,
+ struct csched2_runqueue_data *rqd, int change, s_time_t now)
{
struct csched2_private *prv = csched2_priv(ops);
s_time_t delta, load = rqd->load;
@@ -800,8 +798,8 @@ __update_runq_load(const struct scheduler *ops,
}
static void
-__update_svc_load(const struct scheduler *ops,
- struct csched2_vcpu *svc, int change, s_time_t now)
+update_svc_load(const struct scheduler *ops,
+ struct csched2_vcpu *svc, int change, s_time_t now)
{
struct csched2_private *prv = csched2_priv(ops);
s_time_t delta, vcpu_load;
@@ -865,17 +863,24 @@ update_load(const struct scheduler *ops,
{
trace_var(TRC_CSCHED2_UPDATE_LOAD, 1, 0, NULL);
- __update_runq_load(ops, rqd, change, now);
+ update_runq_load(ops, rqd, change, now);
if ( svc )
- __update_svc_load(ops, svc, change, now);
+ update_svc_load(ops, svc, change, now);
}
-static int
-__runq_insert(struct list_head *runq, struct csched2_vcpu *svc)
+static void
+runq_insert(const struct scheduler *ops, struct csched2_vcpu *svc)
{
struct list_head *iter;
+ unsigned int cpu = svc->vcpu->processor;
+ struct list_head * runq = &c2rqd(ops, cpu)->runq;
int pos = 0;
+ ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock));
+
+ ASSERT(!vcpu_on_runq(svc));
+ ASSERT(c2r(ops, cpu) == c2r(ops, svc->vcpu->processor));
+
ASSERT(&svc->rqd->runq == runq);
ASSERT(!is_idle_vcpu(svc->vcpu));
ASSERT(!svc->vcpu->is_running);
@@ -883,33 +888,15 @@ __runq_insert(struct list_head *runq, struct csched2_vcpu *svc)
list_for_each( iter, runq )
{
- struct csched2_vcpu * iter_svc = __runq_elem(iter);
+ struct csched2_vcpu * iter_svc = runq_elem(iter);
if ( svc->credit > iter_svc->credit )
break;
pos++;
}
-
list_add_tail(&svc->runq_elem, iter);
- return pos;
-}
-
-static void
-runq_insert(const struct scheduler *ops, struct csched2_vcpu *svc)
-{
- unsigned int cpu = svc->vcpu->processor;
- struct list_head * runq = &c2rqd(ops, cpu)->runq;
- int pos = 0;
-
- ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock));
-
- ASSERT(!__vcpu_on_runq(svc));
- ASSERT(c2r(ops, cpu) == c2r(ops, svc->vcpu->processor));
-
- pos = __runq_insert(runq, svc);
-
if ( unlikely(tb_init_done) )
{
struct {
@@ -923,14 +910,11 @@ runq_insert(const struct scheduler *ops, struct csched2_vcpu *svc)
sizeof(d),
(unsigned char *)&d);
}
-
- return;
}
-static inline void
-__runq_remove(struct csched2_vcpu *svc)
+static inline void runq_remove(struct csched2_vcpu *svc)
{
- ASSERT(__vcpu_on_runq(svc));
+ ASSERT(vcpu_on_runq(svc));
list_del_init(&svc->runq_elem);
}
@@ -1280,8 +1264,8 @@ static void update_max_weight(struct csched2_runqueue_data *rqd, int new_weight,
}
#ifndef NDEBUG
-static /*inline*/ void
-__csched2_vcpu_check(struct vcpu *vc)
+static inline void
+csched2_vcpu_check(struct vcpu *vc)
{
struct csched2_vcpu * const svc = csched2_vcpu(vc);
struct csched2_dom * const sdom = svc->sdom;
@@ -1299,7 +1283,7 @@ __csched2_vcpu_check(struct vcpu *vc)
}
SCHED_STAT_CRANK(vcpu_check);
}
-#define CSCHED2_VCPU_CHECK(_vc) (__csched2_vcpu_check(_vc))
+#define CSCHED2_VCPU_CHECK(_vc) (csched2_vcpu_check(_vc))
#else
#define CSCHED2_VCPU_CHECK(_vc)
#endif
@@ -1345,7 +1329,7 @@ csched2_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
/* Add and remove from runqueue assignment (not active run queue) */
static void
-__runq_assign(struct csched2_vcpu *svc, struct csched2_runqueue_data *rqd)
+_runq_assign(struct csched2_vcpu *svc, struct csched2_runqueue_data *rqd)
{
svc->rqd = rqd;
@@ -1379,15 +1363,15 @@ runq_assign(const struct scheduler *ops, struct vcpu *vc)
ASSERT(svc->rqd == NULL);
- __runq_assign(svc, c2rqd(ops, vc->processor));
+ _runq_assign(svc, c2rqd(ops, vc->processor));
}
static void
-__runq_deassign(struct csched2_vcpu *svc)
+_runq_deassign(struct csched2_vcpu *svc)
{
struct csched2_runqueue_data *rqd = svc->rqd;
- ASSERT(!__vcpu_on_runq(svc));
+ ASSERT(!vcpu_on_runq(svc));
ASSERT(!(svc->flags & CSFLAG_scheduled));
list_del_init(&svc->rqd_elem);
@@ -1406,7 +1390,7 @@ runq_deassign(const struct scheduler *ops, struct vcpu *vc)
ASSERT(svc->rqd == c2rqd(ops, vc->processor));
- __runq_deassign(svc);
+ _runq_deassign(svc);
}
static void
@@ -1419,11 +1403,11 @@ csched2_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
if ( curr_on_cpu(vc->processor) == vc )
cpu_raise_softirq(vc->processor, SCHEDULE_SOFTIRQ);
- else if ( __vcpu_on_runq(svc) )
+ else if ( vcpu_on_runq(svc) )
{
ASSERT(svc->rqd == c2rqd(ops, vc->processor));
update_load(ops, svc->rqd, svc, -1, NOW());
- __runq_remove(svc);
+ runq_remove(svc);
}
else if ( svc->flags & CSFLAG_delayed_runq_add )
__clear_bit(__CSFLAG_delayed_runq_add, &svc->flags);
@@ -1446,7 +1430,7 @@ csched2_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
goto out;
}
- if ( unlikely(__vcpu_on_runq(svc)) )
+ if ( unlikely(vcpu_on_runq(svc)) )
{
SCHED_STAT_CRANK(vcpu_wake_onrunq);
goto out;
@@ -1508,7 +1492,7 @@ csched2_context_saved(const struct scheduler *ops, struct vcpu *vc)
/* If someone wants it on the runqueue, put it there. */
/*
* NB: We can get rid of CSFLAG_scheduled by checking for
- * vc->is_running and __vcpu_on_runq(svc) here. However,
+ * vc->is_running and vcpu_on_runq(svc) here. However,
* since we're accessing the flags cacheline anyway,
* it seems a bit pointless; especially as we have plenty of
* bits free.
@@ -1516,7 +1500,7 @@ csched2_context_saved(const struct scheduler *ops, struct vcpu *vc)
if ( __test_and_clear_bit(__CSFLAG_delayed_runq_add, &svc->flags)
&& likely(vcpu_runnable(vc)) )
{
- ASSERT(!__vcpu_on_runq(svc));
+ ASSERT(!vcpu_on_runq(svc));
runq_insert(ops, svc);
runq_tickle(ops, svc, now);
@@ -1748,13 +1732,13 @@ static void migrate(const struct scheduler *ops,
{
int on_runq = 0;
/* It's not running; just move it */
- if ( __vcpu_on_runq(svc) )
+ if ( vcpu_on_runq(svc) )
{
- __runq_remove(svc);
+ runq_remove(svc);
update_load(ops, svc->rqd, NULL, -1, now);
on_runq = 1;
}
- __runq_deassign(svc);
+ _runq_deassign(svc);
cpumask_and(cpumask_scratch_cpu(cpu), svc->vcpu->cpu_hard_affinity,
cpupool_domain_cpumask(svc->vcpu->domain));
@@ -1763,7 +1747,7 @@ static void migrate(const struct scheduler *ops,
svc->vcpu->processor = cpumask_any(cpumask_scratch_cpu(cpu));
ASSERT(svc->vcpu->processor < nr_cpu_ids);
- __runq_assign(svc, trqd);
+ _runq_assign(svc, trqd);
if ( on_runq )
{
update_load(ops, svc->rqd, NULL, 1, now);
@@ -1813,7 +1797,7 @@ static void balance_load(const struct scheduler *ops, int cpu, s_time_t now)
ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock));
st.lrqd = c2rqd(ops, cpu);
- __update_runq_load(ops, st.lrqd, 0, now);
+ update_runq_load(ops, st.lrqd, 0, now);
retry:
if ( !read_trylock(&prv->lock) )
@@ -1831,7 +1815,7 @@ retry:
|| !spin_trylock(&st.orqd->lock) )
continue;
- __update_runq_load(ops, st.orqd, 0, now);
+ update_runq_load(ops, st.orqd, 0, now);
delta = st.lrqd->b_avgload - st.orqd->b_avgload;
if ( delta < 0 )
@@ -1932,7 +1916,7 @@ retry:
{
struct csched2_vcpu * push_svc = list_entry(push_iter, struct csched2_vcpu, rqd_elem);
- __update_svc_load(ops, push_svc, 0, now);
+ update_svc_load(ops, push_svc, 0, now);
if ( !vcpu_is_migrateable(push_svc, st.orqd) )
continue;
@@ -1942,7 +1926,7 @@ retry:
struct csched2_vcpu * pull_svc = list_entry(pull_iter, struct csched2_vcpu, rqd_elem);
if ( !inner_load_updated )
- __update_svc_load(ops, pull_svc, 0, now);
+ update_svc_load(ops, pull_svc, 0, now);
if ( !vcpu_is_migrateable(pull_svc, st.lrqd) )
continue;
@@ -2006,12 +1990,12 @@ csched2_vcpu_migrate(
if ( unlikely(!cpumask_test_cpu(new_cpu, cpupool_domain_cpumask(d))) )
{
ASSERT(system_state == SYS_STATE_suspend);
- if ( __vcpu_on_runq(svc) )
+ if ( vcpu_on_runq(svc) )
{
- __runq_remove(svc);
+ runq_remove(svc);
update_load(ops, svc->rqd, NULL, -1, now);
}
- __runq_deassign(svc);
+ _runq_deassign(svc);
vc->processor = new_cpu;
return;
}
@@ -2303,7 +2287,7 @@ csched2_runtime(const struct scheduler *ops, int cpu,
* run until your credit ~= his */
if ( ! list_empty(runq) )
{
- struct csched2_vcpu *swait = __runq_elem(runq->next);
+ struct csched2_vcpu *swait = runq_elem(runq->next);
if ( ! is_idle_vcpu(swait->vcpu)
&& swait->credit > 0 )
@@ -2567,7 +2551,7 @@ csched2_schedule(
ASSERT(snext->rqd == rqd);
ASSERT(!snext->vcpu->is_running);
- __runq_remove(snext);
+ runq_remove(snext);
__set_bit(__CSFLAG_scheduled, &snext->flags);
}
@@ -2777,7 +2761,7 @@ csched2_dump(const struct scheduler *ops)
printk("RUNQ:\n");
list_for_each( iter, runq )
{
- struct csched2_vcpu *svc = __runq_elem(iter);
+ struct csched2_vcpu *svc = runq_elem(iter);
if ( svc )
{
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 05/10] xen: credit2: tidy up functions names by removing leading '__'.
2017-02-09 13:58 ` [PATCH v2 05/10] xen: credit2: tidy up functions names by removing leading '__' Dario Faggioli
@ 2017-02-15 13:57 ` George Dunlap
2017-02-24 18:32 ` Dario Faggioli
0 siblings, 1 reply; 35+ messages in thread
From: George Dunlap @ 2017-02-15 13:57 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Anshul Makkar
On Thu, Feb 9, 2017 at 1:58 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> There is no reason for having pretty much all of the
> functions whose names begin with double underscores
> ('__') to actually look like that.
>
> In fact, that is misleading and makes the code hard
> to read and understand. So, remove the '__'-s.
>
> The only two that we keep are __runq_assign() and
> __runq_deassign() (althought they're converted to
> single underscore). In fact, in those cases, it is
> indeed useful to have those sort of a "raw" variants.
>
> In case of __runq_insert(), which is only called
> once, by runq_insert(), merge the two functions.
>
> No functional change intended.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
With one comment...
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> ---
> xen/common/sched_credit2.c | 114 +++++++++++++++++++-------------------------
> 1 file changed, 49 insertions(+), 65 deletions(-)
>
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 786dcca..4b4f4f8 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -221,7 +221,7 @@ integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
> * shift all time samples to the right.
> *
> * The details of the formulas used for load tracking are explained close to
> - * __update_runq_load(). Let's just say here that, with full nanosecond time
> + * update_runq_load(). Let's just say here that, with full nanosecond time
> * granularity, a 30 bits wide 'decaying window' is ~1 second long.
> *
> * We want to consider the following equations:
> @@ -233,7 +233,7 @@ integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
> * Q-format fixed point arithmetic and load is the instantaneous load of a
> * runqueue, which basically is the number of runnable vcpus there are on the
> * runqueue (for the meaning of the other terms, look at the doc comment to
> - * __update_runq_load()).
> + * update_runq_load()).
> *
> * So, again, with full nanosecond granularity, and 1 second window, we have:
> *
> @@ -594,14 +594,12 @@ static s_time_t c2t(struct csched2_runqueue_data *rqd, s_time_t credit, struct c
> * Runqueue related code
> */
>
> -static /*inline*/ int
> -__vcpu_on_runq(struct csched2_vcpu *svc)
> +static inline int vcpu_on_runq(struct csched2_vcpu *svc)
> {
> return !list_empty(&svc->runq_elem);
> }
>
> -static /*inline*/ struct csched2_vcpu *
> -__runq_elem(struct list_head *elem)
> +static struct csched2_vcpu * runq_elem(struct list_head *elem)
> {
> return list_entry(elem, struct csched2_vcpu, runq_elem);
> }
Would it make sense to make this inline as well?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 05/10] xen: credit2: tidy up functions names by removing leading '__'.
2017-02-15 13:57 ` George Dunlap
@ 2017-02-24 18:32 ` Dario Faggioli
0 siblings, 0 replies; 35+ messages in thread
From: Dario Faggioli @ 2017-02-24 18:32 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel, Anshul Makkar
[-- Attachment #1.1: Type: text/plain, Size: 1518 bytes --]
On Wed, 2017-02-15 at 13:57 +0000, George Dunlap wrote:
> On Thu, Feb 9, 2017 at 1:58 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> >
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
>
> Acked-by: George Dunlap <george.dunlap@citrix.com>
>
Thanks.
> With one comment...
>
> > diff --git a/xen/common/sched_credit2.c
> > b/xen/common/sched_credit2.c
> > index 786dcca..4b4f4f8 100644
> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -594,14 +594,12 @@ static s_time_t c2t(struct
> > csched2_runqueue_data *rqd, s_time_t credit, struct c
> > * Runqueue related code
> > */
> >
> > -static /*inline*/ int
> > -__vcpu_on_runq(struct csched2_vcpu *svc)
> > +static inline int vcpu_on_runq(struct csched2_vcpu *svc)
> > {
> > return !list_empty(&svc->runq_elem);
> > }
> >
> > -static /*inline*/ struct csched2_vcpu *
> > -__runq_elem(struct list_head *elem)
> > +static struct csched2_vcpu * runq_elem(struct list_head *elem)
> > {
> > return list_entry(elem, struct csched2_vcpu, runq_elem);
> > }
>
> Would it make sense to make this inline as well?
>
Absolutely (not sure why it wasn't! :-P)
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 06/10] xen: credit2: group the runq manipulating functions.
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
` (4 preceding siblings ...)
2017-02-09 13:58 ` [PATCH v2 05/10] xen: credit2: tidy up functions names by removing leading '__' Dario Faggioli
@ 2017-02-09 13:59 ` Dario Faggioli
2017-02-15 14:42 ` George Dunlap
2017-02-09 13:59 ` [PATCH v2 07/10] xen: credit2: always mark a tickled pCPU as... tickled! Dario Faggioli
` (5 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:59 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar
So that they're all close among each other, and
also near to the comment describind the runqueue
organization (which is also moved).
No functional change intended.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
xen/common/sched_credit2.c | 572 ++++++++++++++++++++++----------------------
1 file changed, 286 insertions(+), 286 deletions(-)
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 4b4f4f8..addee7b 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -295,63 +295,6 @@ static int __read_mostly opt_overload_balance_tolerance = -3;
integer_param("credit2_balance_over", opt_overload_balance_tolerance);
/*
- * Runqueue organization.
- *
- * The various cpus are to be assigned each one to a runqueue, and we
- * want that to happen basing on topology. At the moment, it is possible
- * to choose to arrange runqueues to be:
- *
- * - per-core: meaning that there will be one runqueue per each physical
- * core of the host. This will happen if the opt_runqueue
- * parameter is set to 'core';
- *
- * - per-socket: meaning that there will be one runqueue per each physical
- * socket (AKA package, which often, but not always, also
- * matches a NUMA node) of the host; This will happen if
- * the opt_runqueue parameter is set to 'socket';
- *
- * - per-node: meaning that there will be one runqueue per each physical
- * NUMA node of the host. This will happen if the opt_runqueue
- * parameter is set to 'node';
- *
- * - global: meaning that there will be only one runqueue to which all the
- * (logical) processors of the host belong. This will happen if
- * the opt_runqueue parameter is set to 'all'.
- *
- * Depending on the value of opt_runqueue, therefore, cpus that are part of
- * either the same physical core, the same physical socket, the same NUMA
- * node, or just all of them, will be put together to form runqueues.
- */
-#define OPT_RUNQUEUE_CORE 0
-#define OPT_RUNQUEUE_SOCKET 1
-#define OPT_RUNQUEUE_NODE 2
-#define OPT_RUNQUEUE_ALL 3
-static const char *const opt_runqueue_str[] = {
- [OPT_RUNQUEUE_CORE] = "core",
- [OPT_RUNQUEUE_SOCKET] = "socket",
- [OPT_RUNQUEUE_NODE] = "node",
- [OPT_RUNQUEUE_ALL] = "all"
-};
-static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
-
-static void parse_credit2_runqueue(const char *s)
-{
- unsigned int i;
-
- for ( i = 0; i < ARRAY_SIZE(opt_runqueue_str); i++ )
- {
- if ( !strcmp(s, opt_runqueue_str[i]) )
- {
- opt_runqueue = i;
- return;
- }
- }
-
- printk("WARNING, unrecognized value of credit2_runqueue option!\n");
-}
-custom_param("credit2_runqueue", parse_credit2_runqueue);
-
-/*
* Per-runqueue data
*/
struct csched2_runqueue_data {
@@ -563,45 +506,304 @@ static int get_fallback_cpu(struct csched2_vcpu *svc)
return cpumask_first(cpumask_scratch_cpu(cpu));
}
- ASSERT(!cpumask_empty(cpumask_scratch_cpu(cpu)));
-
- return cpumask_first(cpumask_scratch_cpu(cpu));
+ ASSERT(!cpumask_empty(cpumask_scratch_cpu(cpu)));
+
+ return cpumask_first(cpumask_scratch_cpu(cpu));
+}
+
+/*
+ * Time-to-credit, credit-to-time.
+ *
+ * We keep track of the "residual" time to make sure that frequent short
+ * schedules still get accounted for in the end.
+ *
+ * FIXME: Do pre-calculated division?
+ */
+static void t2c_update(struct csched2_runqueue_data *rqd, s_time_t time,
+ struct csched2_vcpu *svc)
+{
+ uint64_t val = time * rqd->max_weight + svc->residual;
+
+ svc->residual = do_div(val, svc->weight);
+ svc->credit -= val;
+}
+
+static s_time_t c2t(struct csched2_runqueue_data *rqd, s_time_t credit, struct csched2_vcpu *svc)
+{
+ return credit * svc->weight / rqd->max_weight;
+}
+
+/*
+ * Runqueue related code.
+ */
+
+/*
+ * Runqueue organization.
+ *
+ * The various cpus are to be assigned each one to a runqueue, and we
+ * want that to happen basing on topology. At the moment, it is possible
+ * to choose to arrange runqueues to be:
+ *
+ * - per-core: meaning that there will be one runqueue per each physical
+ * core of the host. This will happen if the opt_runqueue
+ * parameter is set to 'core';
+ *
+ * - per-socket: meaning that there will be one runqueue per each physical
+ * socket (AKA package, which often, but not always, also
+ * matches a NUMA node) of the host; This will happen if
+ * the opt_runqueue parameter is set to 'socket';
+ *
+ * - per-node: meaning that there will be one runqueue per each physical
+ * NUMA node of the host. This will happen if the opt_runqueue
+ * parameter is set to 'node';
+ *
+ * - global: meaning that there will be only one runqueue to which all the
+ * (logical) processors of the host belong. This will happen if
+ * the opt_runqueue parameter is set to 'all'.
+ *
+ * Depending on the value of opt_runqueue, therefore, cpus that are part of
+ * either the same physical core, the same physical socket, the same NUMA
+ * node, or just all of them, will be put together to form runqueues.
+ */
+#define OPT_RUNQUEUE_CORE 0
+#define OPT_RUNQUEUE_SOCKET 1
+#define OPT_RUNQUEUE_NODE 2
+#define OPT_RUNQUEUE_ALL 3
+static const char *const opt_runqueue_str[] = {
+ [OPT_RUNQUEUE_CORE] = "core",
+ [OPT_RUNQUEUE_SOCKET] = "socket",
+ [OPT_RUNQUEUE_NODE] = "node",
+ [OPT_RUNQUEUE_ALL] = "all"
+};
+static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
+
+static void parse_credit2_runqueue(const char *s)
+{
+ unsigned int i;
+
+ for ( i = 0; i < ARRAY_SIZE(opt_runqueue_str); i++ )
+ {
+ if ( !strcmp(s, opt_runqueue_str[i]) )
+ {
+ opt_runqueue = i;
+ return;
+ }
+ }
+
+ printk("WARNING, unrecognized value of credit2_runqueue option!\n");
+}
+custom_param("credit2_runqueue", parse_credit2_runqueue);
+
+static inline int vcpu_on_runq(struct csched2_vcpu *svc)
+{
+ return !list_empty(&svc->runq_elem);
+}
+
+static struct csched2_vcpu * runq_elem(struct list_head *elem)
+{
+ return list_entry(elem, struct csched2_vcpu, runq_elem);
+}
+
+static void activate_runqueue(struct csched2_private *prv, int rqi)
+{
+ struct csched2_runqueue_data *rqd;
+
+ rqd = prv->rqd + rqi;
+
+ BUG_ON(!cpumask_empty(&rqd->active));
+
+ rqd->max_weight = 1;
+ rqd->id = rqi;
+ INIT_LIST_HEAD(&rqd->svc);
+ INIT_LIST_HEAD(&rqd->runq);
+ spin_lock_init(&rqd->lock);
+
+ __cpumask_set_cpu(rqi, &prv->active_queues);
+}
+
+static void deactivate_runqueue(struct csched2_private *prv, int rqi)
+{
+ struct csched2_runqueue_data *rqd;
+
+ rqd = prv->rqd + rqi;
+
+ BUG_ON(!cpumask_empty(&rqd->active));
+
+ rqd->id = -1;
+
+ __cpumask_clear_cpu(rqi, &prv->active_queues);
+}
+
+static inline bool_t same_node(unsigned int cpua, unsigned int cpub)
+{
+ return cpu_to_node(cpua) == cpu_to_node(cpub);
+}
+
+static inline bool_t same_socket(unsigned int cpua, unsigned int cpub)
+{
+ return cpu_to_socket(cpua) == cpu_to_socket(cpub);
+}
+
+static inline bool_t same_core(unsigned int cpua, unsigned int cpub)
+{
+ return same_socket(cpua, cpub) &&
+ cpu_to_core(cpua) == cpu_to_core(cpub);
+}
+
+static unsigned int
+cpu_to_runqueue(struct csched2_private *prv, unsigned int cpu)
+{
+ struct csched2_runqueue_data *rqd;
+ unsigned int rqi;
+
+ for ( rqi = 0; rqi < nr_cpu_ids; rqi++ )
+ {
+ unsigned int peer_cpu;
+
+ /*
+ * As soon as we come across an uninitialized runqueue, use it.
+ * In fact, either:
+ * - we are initializing the first cpu, and we assign it to
+ * runqueue 0. This is handy, especially if we are dealing
+ * with the boot cpu (if credit2 is the default scheduler),
+ * as we would not be able to use cpu_to_socket() and similar
+ * helpers anyway (they're result of which is not reliable yet);
+ * - we have gone through all the active runqueues, and have not
+ * found anyone whose cpus' topology matches the one we are
+ * dealing with, so activating a new runqueue is what we want.
+ */
+ if ( prv->rqd[rqi].id == -1 )
+ break;
+
+ rqd = prv->rqd + rqi;
+ BUG_ON(cpumask_empty(&rqd->active));
+
+ peer_cpu = cpumask_first(&rqd->active);
+ BUG_ON(cpu_to_socket(cpu) == XEN_INVALID_SOCKET_ID ||
+ cpu_to_socket(peer_cpu) == XEN_INVALID_SOCKET_ID);
+
+ if ( opt_runqueue == OPT_RUNQUEUE_ALL ||
+ (opt_runqueue == OPT_RUNQUEUE_CORE && same_core(peer_cpu, cpu)) ||
+ (opt_runqueue == OPT_RUNQUEUE_SOCKET && same_socket(peer_cpu, cpu)) ||
+ (opt_runqueue == OPT_RUNQUEUE_NODE && same_node(peer_cpu, cpu)) )
+ break;
+ }
+
+ /* We really expect to be able to assign each cpu to a runqueue. */
+ BUG_ON(rqi >= nr_cpu_ids);
+
+ return rqi;
+}
+
+/* Find the domain with the highest weight. */
+static void update_max_weight(struct csched2_runqueue_data *rqd, int new_weight,
+ int old_weight)
+{
+ /* Try to avoid brute-force search:
+ * - If new_weight is larger, max_weigth <- new_weight
+ * - If old_weight != max_weight, someone else is still max_weight
+ * (No action required)
+ * - If old_weight == max_weight, brute-force search for max weight
+ */
+ if ( new_weight > rqd->max_weight )
+ {
+ rqd->max_weight = new_weight;
+ SCHED_STAT_CRANK(upd_max_weight_quick);
+ }
+ else if ( old_weight == rqd->max_weight )
+ {
+ struct list_head *iter;
+ int max_weight = 1;
+
+ list_for_each( iter, &rqd->svc )
+ {
+ struct csched2_vcpu * svc = list_entry(iter, struct csched2_vcpu, rqd_elem);
+
+ if ( svc->weight > max_weight )
+ max_weight = svc->weight;
+ }
+
+ rqd->max_weight = max_weight;
+ SCHED_STAT_CRANK(upd_max_weight_full);
+ }
+
+ if ( unlikely(tb_init_done) )
+ {
+ struct {
+ unsigned rqi:16, max_weight:16;
+ } d;
+ d.rqi = rqd->id;
+ d.max_weight = rqd->max_weight;
+ __trace_var(TRC_CSCHED2_RUNQ_MAX_WEIGHT, 1,
+ sizeof(d),
+ (unsigned char *)&d);
+ }
+}
+
+/* Add and remove from runqueue assignment (not active run queue) */
+static void
+_runq_assign(struct csched2_vcpu *svc, struct csched2_runqueue_data *rqd)
+{
+
+ svc->rqd = rqd;
+ list_add_tail(&svc->rqd_elem, &svc->rqd->svc);
+
+ update_max_weight(svc->rqd, svc->weight, 0);
+
+ /* Expected new load based on adding this vcpu */
+ rqd->b_avgload += svc->avgload;
+
+ if ( unlikely(tb_init_done) )
+ {
+ struct {
+ unsigned vcpu:16, dom:16;
+ unsigned rqi:16;
+ } d;
+ d.dom = svc->vcpu->domain->domain_id;
+ d.vcpu = svc->vcpu->vcpu_id;
+ d.rqi=rqd->id;
+ __trace_var(TRC_CSCHED2_RUNQ_ASSIGN, 1,
+ sizeof(d),
+ (unsigned char *)&d);
+ }
+
}
-/*
- * Time-to-credit, credit-to-time.
- *
- * We keep track of the "residual" time to make sure that frequent short
- * schedules still get accounted for in the end.
- *
- * FIXME: Do pre-calculated division?
- */
-static void t2c_update(struct csched2_runqueue_data *rqd, s_time_t time,
- struct csched2_vcpu *svc)
+static void
+runq_assign(const struct scheduler *ops, struct vcpu *vc)
{
- uint64_t val = time * rqd->max_weight + svc->residual;
+ struct csched2_vcpu *svc = vc->sched_priv;
- svc->residual = do_div(val, svc->weight);
- svc->credit -= val;
+ ASSERT(svc->rqd == NULL);
+
+ _runq_assign(svc, c2rqd(ops, vc->processor));
}
-static s_time_t c2t(struct csched2_runqueue_data *rqd, s_time_t credit, struct csched2_vcpu *svc)
+static void
+_runq_deassign(struct csched2_vcpu *svc)
{
- return credit * svc->weight / rqd->max_weight;
-}
+ struct csched2_runqueue_data *rqd = svc->rqd;
-/*
- * Runqueue related code
- */
+ ASSERT(!vcpu_on_runq(svc));
+ ASSERT(!(svc->flags & CSFLAG_scheduled));
-static inline int vcpu_on_runq(struct csched2_vcpu *svc)
-{
- return !list_empty(&svc->runq_elem);
+ list_del_init(&svc->rqd_elem);
+ update_max_weight(rqd, 0, svc->weight);
+
+ /* Expected new load based on removing this vcpu */
+ rqd->b_avgload = max_t(s_time_t, rqd->b_avgload - svc->avgload, 0);
+
+ svc->rqd = NULL;
}
-static struct csched2_vcpu * runq_elem(struct list_head *elem)
+static void
+runq_deassign(const struct scheduler *ops, struct vcpu *vc)
{
- return list_entry(elem, struct csched2_vcpu, runq_elem);
+ struct csched2_vcpu *svc = vc->sched_priv;
+
+ ASSERT(svc->rqd == c2rqd(ops, vc->processor));
+
+ _runq_deassign(svc);
}
/*
@@ -1218,51 +1420,6 @@ void burn_credits(struct csched2_runqueue_data *rqd,
}
}
-/* Find the domain with the highest weight. */
-static void update_max_weight(struct csched2_runqueue_data *rqd, int new_weight,
- int old_weight)
-{
- /* Try to avoid brute-force search:
- * - If new_weight is larger, max_weigth <- new_weight
- * - If old_weight != max_weight, someone else is still max_weight
- * (No action required)
- * - If old_weight == max_weight, brute-force search for max weight
- */
- if ( new_weight > rqd->max_weight )
- {
- rqd->max_weight = new_weight;
- SCHED_STAT_CRANK(upd_max_weight_quick);
- }
- else if ( old_weight == rqd->max_weight )
- {
- struct list_head *iter;
- int max_weight = 1;
-
- list_for_each( iter, &rqd->svc )
- {
- struct csched2_vcpu * svc = list_entry(iter, struct csched2_vcpu, rqd_elem);
-
- if ( svc->weight > max_weight )
- max_weight = svc->weight;
- }
-
- rqd->max_weight = max_weight;
- SCHED_STAT_CRANK(upd_max_weight_full);
- }
-
- if ( unlikely(tb_init_done) )
- {
- struct {
- unsigned rqi:16, max_weight:16;
- } d;
- d.rqi = rqd->id;
- d.max_weight = rqd->max_weight;
- __trace_var(TRC_CSCHED2_RUNQ_MAX_WEIGHT, 1,
- sizeof(d),
- (unsigned char *)&d);
- }
-}
-
#ifndef NDEBUG
static inline void
csched2_vcpu_check(struct vcpu *vc)
@@ -1327,72 +1484,6 @@ csched2_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
return svc;
}
-/* Add and remove from runqueue assignment (not active run queue) */
-static void
-_runq_assign(struct csched2_vcpu *svc, struct csched2_runqueue_data *rqd)
-{
-
- svc->rqd = rqd;
- list_add_tail(&svc->rqd_elem, &svc->rqd->svc);
-
- update_max_weight(svc->rqd, svc->weight, 0);
-
- /* Expected new load based on adding this vcpu */
- rqd->b_avgload += svc->avgload;
-
- if ( unlikely(tb_init_done) )
- {
- struct {
- unsigned vcpu:16, dom:16;
- unsigned rqi:16;
- } d;
- d.dom = svc->vcpu->domain->domain_id;
- d.vcpu = svc->vcpu->vcpu_id;
- d.rqi=rqd->id;
- __trace_var(TRC_CSCHED2_RUNQ_ASSIGN, 1,
- sizeof(d),
- (unsigned char *)&d);
- }
-
-}
-
-static void
-runq_assign(const struct scheduler *ops, struct vcpu *vc)
-{
- struct csched2_vcpu *svc = vc->sched_priv;
-
- ASSERT(svc->rqd == NULL);
-
- _runq_assign(svc, c2rqd(ops, vc->processor));
-}
-
-static void
-_runq_deassign(struct csched2_vcpu *svc)
-{
- struct csched2_runqueue_data *rqd = svc->rqd;
-
- ASSERT(!vcpu_on_runq(svc));
- ASSERT(!(svc->flags & CSFLAG_scheduled));
-
- list_del_init(&svc->rqd_elem);
- update_max_weight(rqd, 0, svc->weight);
-
- /* Expected new load based on removing this vcpu */
- rqd->b_avgload = max_t(s_time_t, rqd->b_avgload - svc->avgload, 0);
-
- svc->rqd = NULL;
-}
-
-static void
-runq_deassign(const struct scheduler *ops, struct vcpu *vc)
-{
- struct csched2_vcpu *svc = vc->sched_priv;
-
- ASSERT(svc->rqd == c2rqd(ops, vc->processor));
-
- _runq_deassign(svc);
-}
-
static void
csched2_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
{
@@ -2776,97 +2867,6 @@ csched2_dump(const struct scheduler *ops)
#undef cpustr
}
-static void activate_runqueue(struct csched2_private *prv, int rqi)
-{
- struct csched2_runqueue_data *rqd;
-
- rqd = prv->rqd + rqi;
-
- BUG_ON(!cpumask_empty(&rqd->active));
-
- rqd->max_weight = 1;
- rqd->id = rqi;
- INIT_LIST_HEAD(&rqd->svc);
- INIT_LIST_HEAD(&rqd->runq);
- spin_lock_init(&rqd->lock);
-
- __cpumask_set_cpu(rqi, &prv->active_queues);
-}
-
-static void deactivate_runqueue(struct csched2_private *prv, int rqi)
-{
- struct csched2_runqueue_data *rqd;
-
- rqd = prv->rqd + rqi;
-
- BUG_ON(!cpumask_empty(&rqd->active));
-
- rqd->id = -1;
-
- __cpumask_clear_cpu(rqi, &prv->active_queues);
-}
-
-static inline bool_t same_node(unsigned int cpua, unsigned int cpub)
-{
- return cpu_to_node(cpua) == cpu_to_node(cpub);
-}
-
-static inline bool_t same_socket(unsigned int cpua, unsigned int cpub)
-{
- return cpu_to_socket(cpua) == cpu_to_socket(cpub);
-}
-
-static inline bool_t same_core(unsigned int cpua, unsigned int cpub)
-{
- return same_socket(cpua, cpub) &&
- cpu_to_core(cpua) == cpu_to_core(cpub);
-}
-
-static unsigned int
-cpu_to_runqueue(struct csched2_private *prv, unsigned int cpu)
-{
- struct csched2_runqueue_data *rqd;
- unsigned int rqi;
-
- for ( rqi = 0; rqi < nr_cpu_ids; rqi++ )
- {
- unsigned int peer_cpu;
-
- /*
- * As soon as we come across an uninitialized runqueue, use it.
- * In fact, either:
- * - we are initializing the first cpu, and we assign it to
- * runqueue 0. This is handy, especially if we are dealing
- * with the boot cpu (if credit2 is the default scheduler),
- * as we would not be able to use cpu_to_socket() and similar
- * helpers anyway (they're result of which is not reliable yet);
- * - we have gone through all the active runqueues, and have not
- * found anyone whose cpus' topology matches the one we are
- * dealing with, so activating a new runqueue is what we want.
- */
- if ( prv->rqd[rqi].id == -1 )
- break;
-
- rqd = prv->rqd + rqi;
- BUG_ON(cpumask_empty(&rqd->active));
-
- peer_cpu = cpumask_first(&rqd->active);
- BUG_ON(cpu_to_socket(cpu) == XEN_INVALID_SOCKET_ID ||
- cpu_to_socket(peer_cpu) == XEN_INVALID_SOCKET_ID);
-
- if ( opt_runqueue == OPT_RUNQUEUE_ALL ||
- (opt_runqueue == OPT_RUNQUEUE_CORE && same_core(peer_cpu, cpu)) ||
- (opt_runqueue == OPT_RUNQUEUE_SOCKET && same_socket(peer_cpu, cpu)) ||
- (opt_runqueue == OPT_RUNQUEUE_NODE && same_node(peer_cpu, cpu)) )
- break;
- }
-
- /* We really expect to be able to assign each cpu to a runqueue. */
- BUG_ON(rqi >= nr_cpu_ids);
-
- return rqi;
-}
-
/* Returns the ID of the runqueue the cpu is assigned to. */
static unsigned
init_pdata(struct csched2_private *prv, unsigned int cpu)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 06/10] xen: credit2: group the runq manipulating functions.
2017-02-09 13:59 ` [PATCH v2 06/10] xen: credit2: group the runq manipulating functions Dario Faggioli
@ 2017-02-15 14:42 ` George Dunlap
2017-02-27 18:25 ` Dario Faggioli
0 siblings, 1 reply; 35+ messages in thread
From: George Dunlap @ 2017-02-15 14:42 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Anshul Makkar
On Thu, Feb 9, 2017 at 1:59 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> So that they're all close among each other, and
> also near to the comment describind the runqueue
> organization (which is also moved).
>
> No functional change intended.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> ---
> xen/common/sched_credit2.c | 572 ++++++++++++++++++++++----------------------
> 1 file changed, 286 insertions(+), 286 deletions(-)
>
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 4b4f4f8..addee7b 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -295,63 +295,6 @@ static int __read_mostly opt_overload_balance_tolerance = -3;
> integer_param("credit2_balance_over", opt_overload_balance_tolerance);
>
> /*
> - * Runqueue organization.
> - *
> - * The various cpus are to be assigned each one to a runqueue, and we
> - * want that to happen basing on topology. At the moment, it is possible
> - * to choose to arrange runqueues to be:
> - *
> - * - per-core: meaning that there will be one runqueue per each physical
> - * core of the host. This will happen if the opt_runqueue
> - * parameter is set to 'core';
> - *
> - * - per-socket: meaning that there will be one runqueue per each physical
> - * socket (AKA package, which often, but not always, also
> - * matches a NUMA node) of the host; This will happen if
> - * the opt_runqueue parameter is set to 'socket';
> - *
> - * - per-node: meaning that there will be one runqueue per each physical
> - * NUMA node of the host. This will happen if the opt_runqueue
> - * parameter is set to 'node';
> - *
> - * - global: meaning that there will be only one runqueue to which all the
> - * (logical) processors of the host belong. This will happen if
> - * the opt_runqueue parameter is set to 'all'.
> - *
> - * Depending on the value of opt_runqueue, therefore, cpus that are part of
> - * either the same physical core, the same physical socket, the same NUMA
> - * node, or just all of them, will be put together to form runqueues.
> - */
> -#define OPT_RUNQUEUE_CORE 0
> -#define OPT_RUNQUEUE_SOCKET 1
> -#define OPT_RUNQUEUE_NODE 2
> -#define OPT_RUNQUEUE_ALL 3
> -static const char *const opt_runqueue_str[] = {
> - [OPT_RUNQUEUE_CORE] = "core",
> - [OPT_RUNQUEUE_SOCKET] = "socket",
> - [OPT_RUNQUEUE_NODE] = "node",
> - [OPT_RUNQUEUE_ALL] = "all"
> -};
> -static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
> -
> -static void parse_credit2_runqueue(const char *s)
> -{
> - unsigned int i;
> -
> - for ( i = 0; i < ARRAY_SIZE(opt_runqueue_str); i++ )
> - {
> - if ( !strcmp(s, opt_runqueue_str[i]) )
> - {
> - opt_runqueue = i;
> - return;
> - }
> - }
> -
> - printk("WARNING, unrecognized value of credit2_runqueue option!\n");
> -}
> -custom_param("credit2_runqueue", parse_credit2_runqueue);
Most of the motion makes sense, but moving the option parser along too
seems a bit strange. Wouldn't it make more sense to leave it with the
other option parsers? (Perhaps with a "pointer" comment if you want
to move the main bulk of the comment?)
Everything else looks good.
-George
> -
> -/*
> * Per-runqueue data
> */
> struct csched2_runqueue_data {
> @@ -563,45 +506,304 @@ static int get_fallback_cpu(struct csched2_vcpu *svc)
> return cpumask_first(cpumask_scratch_cpu(cpu));
> }
>
> - ASSERT(!cpumask_empty(cpumask_scratch_cpu(cpu)));
> -
> - return cpumask_first(cpumask_scratch_cpu(cpu));
> + ASSERT(!cpumask_empty(cpumask_scratch_cpu(cpu)));
> +
> + return cpumask_first(cpumask_scratch_cpu(cpu));
> +}
> +
> +/*
> + * Time-to-credit, credit-to-time.
> + *
> + * We keep track of the "residual" time to make sure that frequent short
> + * schedules still get accounted for in the end.
> + *
> + * FIXME: Do pre-calculated division?
> + */
> +static void t2c_update(struct csched2_runqueue_data *rqd, s_time_t time,
> + struct csched2_vcpu *svc)
> +{
> + uint64_t val = time * rqd->max_weight + svc->residual;
> +
> + svc->residual = do_div(val, svc->weight);
> + svc->credit -= val;
> +}
> +
> +static s_time_t c2t(struct csched2_runqueue_data *rqd, s_time_t credit, struct csched2_vcpu *svc)
> +{
> + return credit * svc->weight / rqd->max_weight;
> +}
> +
> +/*
> + * Runqueue related code.
> + */
> +
> +/*
> + * Runqueue organization.
> + *
> + * The various cpus are to be assigned each one to a runqueue, and we
> + * want that to happen basing on topology. At the moment, it is possible
> + * to choose to arrange runqueues to be:
> + *
> + * - per-core: meaning that there will be one runqueue per each physical
> + * core of the host. This will happen if the opt_runqueue
> + * parameter is set to 'core';
> + *
> + * - per-socket: meaning that there will be one runqueue per each physical
> + * socket (AKA package, which often, but not always, also
> + * matches a NUMA node) of the host; This will happen if
> + * the opt_runqueue parameter is set to 'socket';
> + *
> + * - per-node: meaning that there will be one runqueue per each physical
> + * NUMA node of the host. This will happen if the opt_runqueue
> + * parameter is set to 'node';
> + *
> + * - global: meaning that there will be only one runqueue to which all the
> + * (logical) processors of the host belong. This will happen if
> + * the opt_runqueue parameter is set to 'all'.
> + *
> + * Depending on the value of opt_runqueue, therefore, cpus that are part of
> + * either the same physical core, the same physical socket, the same NUMA
> + * node, or just all of them, will be put together to form runqueues.
> + */
> +#define OPT_RUNQUEUE_CORE 0
> +#define OPT_RUNQUEUE_SOCKET 1
> +#define OPT_RUNQUEUE_NODE 2
> +#define OPT_RUNQUEUE_ALL 3
> +static const char *const opt_runqueue_str[] = {
> + [OPT_RUNQUEUE_CORE] = "core",
> + [OPT_RUNQUEUE_SOCKET] = "socket",
> + [OPT_RUNQUEUE_NODE] = "node",
> + [OPT_RUNQUEUE_ALL] = "all"
> +};
> +static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
> +
> +static void parse_credit2_runqueue(const char *s)
> +{
> + unsigned int i;
> +
> + for ( i = 0; i < ARRAY_SIZE(opt_runqueue_str); i++ )
> + {
> + if ( !strcmp(s, opt_runqueue_str[i]) )
> + {
> + opt_runqueue = i;
> + return;
> + }
> + }
> +
> + printk("WARNING, unrecognized value of credit2_runqueue option!\n");
> +}
> +custom_param("credit2_runqueue", parse_credit2_runqueue);
> +
> +static inline int vcpu_on_runq(struct csched2_vcpu *svc)
> +{
> + return !list_empty(&svc->runq_elem);
> +}
> +
> +static struct csched2_vcpu * runq_elem(struct list_head *elem)
> +{
> + return list_entry(elem, struct csched2_vcpu, runq_elem);
> +}
> +
> +static void activate_runqueue(struct csched2_private *prv, int rqi)
> +{
> + struct csched2_runqueue_data *rqd;
> +
> + rqd = prv->rqd + rqi;
> +
> + BUG_ON(!cpumask_empty(&rqd->active));
> +
> + rqd->max_weight = 1;
> + rqd->id = rqi;
> + INIT_LIST_HEAD(&rqd->svc);
> + INIT_LIST_HEAD(&rqd->runq);
> + spin_lock_init(&rqd->lock);
> +
> + __cpumask_set_cpu(rqi, &prv->active_queues);
> +}
> +
> +static void deactivate_runqueue(struct csched2_private *prv, int rqi)
> +{
> + struct csched2_runqueue_data *rqd;
> +
> + rqd = prv->rqd + rqi;
> +
> + BUG_ON(!cpumask_empty(&rqd->active));
> +
> + rqd->id = -1;
> +
> + __cpumask_clear_cpu(rqi, &prv->active_queues);
> +}
> +
> +static inline bool_t same_node(unsigned int cpua, unsigned int cpub)
> +{
> + return cpu_to_node(cpua) == cpu_to_node(cpub);
> +}
> +
> +static inline bool_t same_socket(unsigned int cpua, unsigned int cpub)
> +{
> + return cpu_to_socket(cpua) == cpu_to_socket(cpub);
> +}
> +
> +static inline bool_t same_core(unsigned int cpua, unsigned int cpub)
> +{
> + return same_socket(cpua, cpub) &&
> + cpu_to_core(cpua) == cpu_to_core(cpub);
> +}
> +
> +static unsigned int
> +cpu_to_runqueue(struct csched2_private *prv, unsigned int cpu)
> +{
> + struct csched2_runqueue_data *rqd;
> + unsigned int rqi;
> +
> + for ( rqi = 0; rqi < nr_cpu_ids; rqi++ )
> + {
> + unsigned int peer_cpu;
> +
> + /*
> + * As soon as we come across an uninitialized runqueue, use it.
> + * In fact, either:
> + * - we are initializing the first cpu, and we assign it to
> + * runqueue 0. This is handy, especially if we are dealing
> + * with the boot cpu (if credit2 is the default scheduler),
> + * as we would not be able to use cpu_to_socket() and similar
> + * helpers anyway (they're result of which is not reliable yet);
> + * - we have gone through all the active runqueues, and have not
> + * found anyone whose cpus' topology matches the one we are
> + * dealing with, so activating a new runqueue is what we want.
> + */
> + if ( prv->rqd[rqi].id == -1 )
> + break;
> +
> + rqd = prv->rqd + rqi;
> + BUG_ON(cpumask_empty(&rqd->active));
> +
> + peer_cpu = cpumask_first(&rqd->active);
> + BUG_ON(cpu_to_socket(cpu) == XEN_INVALID_SOCKET_ID ||
> + cpu_to_socket(peer_cpu) == XEN_INVALID_SOCKET_ID);
> +
> + if ( opt_runqueue == OPT_RUNQUEUE_ALL ||
> + (opt_runqueue == OPT_RUNQUEUE_CORE && same_core(peer_cpu, cpu)) ||
> + (opt_runqueue == OPT_RUNQUEUE_SOCKET && same_socket(peer_cpu, cpu)) ||
> + (opt_runqueue == OPT_RUNQUEUE_NODE && same_node(peer_cpu, cpu)) )
> + break;
> + }
> +
> + /* We really expect to be able to assign each cpu to a runqueue. */
> + BUG_ON(rqi >= nr_cpu_ids);
> +
> + return rqi;
> +}
> +
> +/* Find the domain with the highest weight. */
> +static void update_max_weight(struct csched2_runqueue_data *rqd, int new_weight,
> + int old_weight)
> +{
> + /* Try to avoid brute-force search:
> + * - If new_weight is larger, max_weigth <- new_weight
> + * - If old_weight != max_weight, someone else is still max_weight
> + * (No action required)
> + * - If old_weight == max_weight, brute-force search for max weight
> + */
> + if ( new_weight > rqd->max_weight )
> + {
> + rqd->max_weight = new_weight;
> + SCHED_STAT_CRANK(upd_max_weight_quick);
> + }
> + else if ( old_weight == rqd->max_weight )
> + {
> + struct list_head *iter;
> + int max_weight = 1;
> +
> + list_for_each( iter, &rqd->svc )
> + {
> + struct csched2_vcpu * svc = list_entry(iter, struct csched2_vcpu, rqd_elem);
> +
> + if ( svc->weight > max_weight )
> + max_weight = svc->weight;
> + }
> +
> + rqd->max_weight = max_weight;
> + SCHED_STAT_CRANK(upd_max_weight_full);
> + }
> +
> + if ( unlikely(tb_init_done) )
> + {
> + struct {
> + unsigned rqi:16, max_weight:16;
> + } d;
> + d.rqi = rqd->id;
> + d.max_weight = rqd->max_weight;
> + __trace_var(TRC_CSCHED2_RUNQ_MAX_WEIGHT, 1,
> + sizeof(d),
> + (unsigned char *)&d);
> + }
> +}
> +
> +/* Add and remove from runqueue assignment (not active run queue) */
> +static void
> +_runq_assign(struct csched2_vcpu *svc, struct csched2_runqueue_data *rqd)
> +{
> +
> + svc->rqd = rqd;
> + list_add_tail(&svc->rqd_elem, &svc->rqd->svc);
> +
> + update_max_weight(svc->rqd, svc->weight, 0);
> +
> + /* Expected new load based on adding this vcpu */
> + rqd->b_avgload += svc->avgload;
> +
> + if ( unlikely(tb_init_done) )
> + {
> + struct {
> + unsigned vcpu:16, dom:16;
> + unsigned rqi:16;
> + } d;
> + d.dom = svc->vcpu->domain->domain_id;
> + d.vcpu = svc->vcpu->vcpu_id;
> + d.rqi=rqd->id;
> + __trace_var(TRC_CSCHED2_RUNQ_ASSIGN, 1,
> + sizeof(d),
> + (unsigned char *)&d);
> + }
> +
> }
>
> -/*
> - * Time-to-credit, credit-to-time.
> - *
> - * We keep track of the "residual" time to make sure that frequent short
> - * schedules still get accounted for in the end.
> - *
> - * FIXME: Do pre-calculated division?
> - */
> -static void t2c_update(struct csched2_runqueue_data *rqd, s_time_t time,
> - struct csched2_vcpu *svc)
> +static void
> +runq_assign(const struct scheduler *ops, struct vcpu *vc)
> {
> - uint64_t val = time * rqd->max_weight + svc->residual;
> + struct csched2_vcpu *svc = vc->sched_priv;
>
> - svc->residual = do_div(val, svc->weight);
> - svc->credit -= val;
> + ASSERT(svc->rqd == NULL);
> +
> + _runq_assign(svc, c2rqd(ops, vc->processor));
> }
>
> -static s_time_t c2t(struct csched2_runqueue_data *rqd, s_time_t credit, struct csched2_vcpu *svc)
> +static void
> +_runq_deassign(struct csched2_vcpu *svc)
> {
> - return credit * svc->weight / rqd->max_weight;
> -}
> + struct csched2_runqueue_data *rqd = svc->rqd;
>
> -/*
> - * Runqueue related code
> - */
> + ASSERT(!vcpu_on_runq(svc));
> + ASSERT(!(svc->flags & CSFLAG_scheduled));
>
> -static inline int vcpu_on_runq(struct csched2_vcpu *svc)
> -{
> - return !list_empty(&svc->runq_elem);
> + list_del_init(&svc->rqd_elem);
> + update_max_weight(rqd, 0, svc->weight);
> +
> + /* Expected new load based on removing this vcpu */
> + rqd->b_avgload = max_t(s_time_t, rqd->b_avgload - svc->avgload, 0);
> +
> + svc->rqd = NULL;
> }
>
> -static struct csched2_vcpu * runq_elem(struct list_head *elem)
> +static void
> +runq_deassign(const struct scheduler *ops, struct vcpu *vc)
> {
> - return list_entry(elem, struct csched2_vcpu, runq_elem);
> + struct csched2_vcpu *svc = vc->sched_priv;
> +
> + ASSERT(svc->rqd == c2rqd(ops, vc->processor));
> +
> + _runq_deassign(svc);
> }
>
> /*
> @@ -1218,51 +1420,6 @@ void burn_credits(struct csched2_runqueue_data *rqd,
> }
> }
>
> -/* Find the domain with the highest weight. */
> -static void update_max_weight(struct csched2_runqueue_data *rqd, int new_weight,
> - int old_weight)
> -{
> - /* Try to avoid brute-force search:
> - * - If new_weight is larger, max_weigth <- new_weight
> - * - If old_weight != max_weight, someone else is still max_weight
> - * (No action required)
> - * - If old_weight == max_weight, brute-force search for max weight
> - */
> - if ( new_weight > rqd->max_weight )
> - {
> - rqd->max_weight = new_weight;
> - SCHED_STAT_CRANK(upd_max_weight_quick);
> - }
> - else if ( old_weight == rqd->max_weight )
> - {
> - struct list_head *iter;
> - int max_weight = 1;
> -
> - list_for_each( iter, &rqd->svc )
> - {
> - struct csched2_vcpu * svc = list_entry(iter, struct csched2_vcpu, rqd_elem);
> -
> - if ( svc->weight > max_weight )
> - max_weight = svc->weight;
> - }
> -
> - rqd->max_weight = max_weight;
> - SCHED_STAT_CRANK(upd_max_weight_full);
> - }
> -
> - if ( unlikely(tb_init_done) )
> - {
> - struct {
> - unsigned rqi:16, max_weight:16;
> - } d;
> - d.rqi = rqd->id;
> - d.max_weight = rqd->max_weight;
> - __trace_var(TRC_CSCHED2_RUNQ_MAX_WEIGHT, 1,
> - sizeof(d),
> - (unsigned char *)&d);
> - }
> -}
> -
> #ifndef NDEBUG
> static inline void
> csched2_vcpu_check(struct vcpu *vc)
> @@ -1327,72 +1484,6 @@ csched2_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
> return svc;
> }
>
> -/* Add and remove from runqueue assignment (not active run queue) */
> -static void
> -_runq_assign(struct csched2_vcpu *svc, struct csched2_runqueue_data *rqd)
> -{
> -
> - svc->rqd = rqd;
> - list_add_tail(&svc->rqd_elem, &svc->rqd->svc);
> -
> - update_max_weight(svc->rqd, svc->weight, 0);
> -
> - /* Expected new load based on adding this vcpu */
> - rqd->b_avgload += svc->avgload;
> -
> - if ( unlikely(tb_init_done) )
> - {
> - struct {
> - unsigned vcpu:16, dom:16;
> - unsigned rqi:16;
> - } d;
> - d.dom = svc->vcpu->domain->domain_id;
> - d.vcpu = svc->vcpu->vcpu_id;
> - d.rqi=rqd->id;
> - __trace_var(TRC_CSCHED2_RUNQ_ASSIGN, 1,
> - sizeof(d),
> - (unsigned char *)&d);
> - }
> -
> -}
> -
> -static void
> -runq_assign(const struct scheduler *ops, struct vcpu *vc)
> -{
> - struct csched2_vcpu *svc = vc->sched_priv;
> -
> - ASSERT(svc->rqd == NULL);
> -
> - _runq_assign(svc, c2rqd(ops, vc->processor));
> -}
> -
> -static void
> -_runq_deassign(struct csched2_vcpu *svc)
> -{
> - struct csched2_runqueue_data *rqd = svc->rqd;
> -
> - ASSERT(!vcpu_on_runq(svc));
> - ASSERT(!(svc->flags & CSFLAG_scheduled));
> -
> - list_del_init(&svc->rqd_elem);
> - update_max_weight(rqd, 0, svc->weight);
> -
> - /* Expected new load based on removing this vcpu */
> - rqd->b_avgload = max_t(s_time_t, rqd->b_avgload - svc->avgload, 0);
> -
> - svc->rqd = NULL;
> -}
> -
> -static void
> -runq_deassign(const struct scheduler *ops, struct vcpu *vc)
> -{
> - struct csched2_vcpu *svc = vc->sched_priv;
> -
> - ASSERT(svc->rqd == c2rqd(ops, vc->processor));
> -
> - _runq_deassign(svc);
> -}
> -
> static void
> csched2_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
> {
> @@ -2776,97 +2867,6 @@ csched2_dump(const struct scheduler *ops)
> #undef cpustr
> }
>
> -static void activate_runqueue(struct csched2_private *prv, int rqi)
> -{
> - struct csched2_runqueue_data *rqd;
> -
> - rqd = prv->rqd + rqi;
> -
> - BUG_ON(!cpumask_empty(&rqd->active));
> -
> - rqd->max_weight = 1;
> - rqd->id = rqi;
> - INIT_LIST_HEAD(&rqd->svc);
> - INIT_LIST_HEAD(&rqd->runq);
> - spin_lock_init(&rqd->lock);
> -
> - __cpumask_set_cpu(rqi, &prv->active_queues);
> -}
> -
> -static void deactivate_runqueue(struct csched2_private *prv, int rqi)
> -{
> - struct csched2_runqueue_data *rqd;
> -
> - rqd = prv->rqd + rqi;
> -
> - BUG_ON(!cpumask_empty(&rqd->active));
> -
> - rqd->id = -1;
> -
> - __cpumask_clear_cpu(rqi, &prv->active_queues);
> -}
> -
> -static inline bool_t same_node(unsigned int cpua, unsigned int cpub)
> -{
> - return cpu_to_node(cpua) == cpu_to_node(cpub);
> -}
> -
> -static inline bool_t same_socket(unsigned int cpua, unsigned int cpub)
> -{
> - return cpu_to_socket(cpua) == cpu_to_socket(cpub);
> -}
> -
> -static inline bool_t same_core(unsigned int cpua, unsigned int cpub)
> -{
> - return same_socket(cpua, cpub) &&
> - cpu_to_core(cpua) == cpu_to_core(cpub);
> -}
> -
> -static unsigned int
> -cpu_to_runqueue(struct csched2_private *prv, unsigned int cpu)
> -{
> - struct csched2_runqueue_data *rqd;
> - unsigned int rqi;
> -
> - for ( rqi = 0; rqi < nr_cpu_ids; rqi++ )
> - {
> - unsigned int peer_cpu;
> -
> - /*
> - * As soon as we come across an uninitialized runqueue, use it.
> - * In fact, either:
> - * - we are initializing the first cpu, and we assign it to
> - * runqueue 0. This is handy, especially if we are dealing
> - * with the boot cpu (if credit2 is the default scheduler),
> - * as we would not be able to use cpu_to_socket() and similar
> - * helpers anyway (they're result of which is not reliable yet);
> - * - we have gone through all the active runqueues, and have not
> - * found anyone whose cpus' topology matches the one we are
> - * dealing with, so activating a new runqueue is what we want.
> - */
> - if ( prv->rqd[rqi].id == -1 )
> - break;
> -
> - rqd = prv->rqd + rqi;
> - BUG_ON(cpumask_empty(&rqd->active));
> -
> - peer_cpu = cpumask_first(&rqd->active);
> - BUG_ON(cpu_to_socket(cpu) == XEN_INVALID_SOCKET_ID ||
> - cpu_to_socket(peer_cpu) == XEN_INVALID_SOCKET_ID);
> -
> - if ( opt_runqueue == OPT_RUNQUEUE_ALL ||
> - (opt_runqueue == OPT_RUNQUEUE_CORE && same_core(peer_cpu, cpu)) ||
> - (opt_runqueue == OPT_RUNQUEUE_SOCKET && same_socket(peer_cpu, cpu)) ||
> - (opt_runqueue == OPT_RUNQUEUE_NODE && same_node(peer_cpu, cpu)) )
> - break;
> - }
> -
> - /* We really expect to be able to assign each cpu to a runqueue. */
> - BUG_ON(rqi >= nr_cpu_ids);
> -
> - return rqi;
> -}
> -
> /* Returns the ID of the runqueue the cpu is assigned to. */
> static unsigned
> init_pdata(struct csched2_private *prv, unsigned int cpu)
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 06/10] xen: credit2: group the runq manipulating functions.
2017-02-15 14:42 ` George Dunlap
@ 2017-02-27 18:25 ` Dario Faggioli
0 siblings, 0 replies; 35+ messages in thread
From: Dario Faggioli @ 2017-02-27 18:25 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel, Anshul Makkar
[-- Attachment #1.1: Type: text/plain, Size: 1004 bytes --]
On Wed, 2017-02-15 at 14:42 +0000, George Dunlap wrote:
> Most of the motion makes sense, but moving the option parser along
> too
> seems a bit strange. Wouldn't it make more sense to leave it with
> the
> other option parsers?
>
Well, it's a runqueue related parameter, so it's runqueue related code,
and this is why I moved it.
_BUT_ after all, I think I agree with you. More specifically, I think
that either all the option parser are in "sections" where they belong,
depending on the option they take care of, or they all live together.
So, yes, let's keep them together. I'll see about moving each one in a
more specific place in the file and, if I decide to go for it, send a
patch.
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 07/10] xen: credit2: always mark a tickled pCPU as... tickled!
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
` (5 preceding siblings ...)
2017-02-09 13:59 ` [PATCH v2 06/10] xen: credit2: group the runq manipulating functions Dario Faggioli
@ 2017-02-09 13:59 ` Dario Faggioli
2017-02-09 23:48 ` Dario Faggioli
2017-02-09 13:59 ` [PATCH v2 08/10] xen: credit2: don't miss accounting while doing a credit reset Dario Faggioli
` (4 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:59 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar
In fact, whether or not a pCPU has been tickled, and is
therefore about to re-schedule, is something we look at
and base decisions on in various places.
So, let's make sure that we do that basing on accurate
information.
While there, also tweak a little bit smt_idle_mask_clear()
(used for implementing SMT support), so that it only alter
the relevant cpumask when there is the actual need for this.
(This is only for reduced overhead, behavior remains the
same).
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
xen/common/sched_credit2.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index addee7b..bfb4891 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -470,12 +470,15 @@ void smt_idle_mask_set(unsigned int cpu, const cpumask_t *idlers,
}
/*
- * Clear the bits of all the siblings of cpu from mask.
+ * Clear the bits of all the siblings of cpu from mask (if necessary).
*/
static inline
void smt_idle_mask_clear(unsigned int cpu, cpumask_t *mask)
{
- cpumask_andnot(mask, mask, per_cpu(cpu_sibling_mask, cpu));
+ const cpumask_t *cpu_siblings = per_cpu(cpu_sibling_mask, cpu);
+
+ if ( cpumask_subset(cpu_siblings, mask) )
+ cpumask_andnot(mask, mask, per_cpu(cpu_sibling_mask, cpu));
}
/*
@@ -1122,6 +1125,14 @@ static inline void runq_remove(struct csched2_vcpu *svc)
void burn_credits(struct csched2_runqueue_data *rqd, struct csched2_vcpu *, s_time_t);
+static inline void
+tickle_cpu(unsigned int cpu, struct csched2_runqueue_data *rqd)
+{
+ __cpumask_set_cpu(cpu, &rqd->tickled);
+ smt_idle_mask_clear(cpu, &rqd->smt_idle);
+ cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
+}
+
/*
* Check what processor it is best to 'wake', for picking up a vcpu that has
* just been put (back) in the runqueue. Logic is as follows:
@@ -1289,9 +1300,8 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
sizeof(d),
(unsigned char *)&d);
}
- __cpumask_set_cpu(ipid, &rqd->tickled);
- smt_idle_mask_clear(ipid, &rqd->smt_idle);
- cpu_raise_softirq(ipid, SCHEDULE_SOFTIRQ);
+
+ tickle_cpu(cpu, rqd);
if ( unlikely(new->tickled_cpu != -1) )
SCHED_STAT_CRANK(tickled_cpu_overwritten);
@@ -1493,7 +1503,9 @@ csched2_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc)
SCHED_STAT_CRANK(vcpu_sleep);
if ( curr_on_cpu(vc->processor) == vc )
- cpu_raise_softirq(vc->processor, SCHEDULE_SOFTIRQ);
+ {
+ tickle_cpu(vc->processor, svc->rqd);
+ }
else if ( vcpu_on_runq(svc) )
{
ASSERT(svc->rqd == c2rqd(ops, vc->processor));
@@ -1816,8 +1828,8 @@ static void migrate(const struct scheduler *ops,
svc->migrate_rqd = trqd;
__set_bit(_VPF_migrating, &svc->vcpu->pause_flags);
__set_bit(__CSFLAG_runq_migrate_request, &svc->flags);
- cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
SCHED_STAT_CRANK(migrate_requested);
+ tickle_cpu(cpu, svc->rqd);
}
else
{
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 07/10] xen: credit2: always mark a tickled pCPU as... tickled!
2017-02-09 13:59 ` [PATCH v2 07/10] xen: credit2: always mark a tickled pCPU as... tickled! Dario Faggioli
@ 2017-02-09 23:48 ` Dario Faggioli
2017-02-15 14:55 ` George Dunlap
0 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 23:48 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar
[-- Attachment #1.1: Type: text/plain, Size: 1667 bytes --]
On Thu, 2017-02-09 at 14:59 +0100, Dario Faggioli wrote:
> In fact, whether or not a pCPU has been tickled, and is
> therefore about to re-schedule, is something we look at
> and base decisions on in various places.
>
> So, let's make sure that we do that basing on accurate
> information.
>
> While there, also tweak a little bit smt_idle_mask_clear()
> (used for implementing SMT support), so that it only alter
> the relevant cpumask when there is the actual need for this.
> (This is only for reduced overhead, behavior remains the
> same).
>
So, while working on other things with this series applied, I noticed
some strange behavior, for which it turned out this patch is
responsible.
More specifically...
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index addee7b..bfb4891 100644
> @@ -1289,9 +1300,8 @@ runq_tickle(const struct scheduler *ops, struct
> csched2_vcpu *new, s_time_t now)
> sizeof(d),
> (unsigned char *)&d);
> }
> - __cpumask_set_cpu(ipid, &rqd->tickled);
> - smt_idle_mask_clear(ipid, &rqd->smt_idle);
> - cpu_raise_softirq(ipid, SCHEDULE_SOFTIRQ);
> +
> + tickle_cpu(cpu, rqd);
>
... this, quite obviously, wants to be:
tickle_cpu(ipid, rqd);
I'll fix this in v2.
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 07/10] xen: credit2: always mark a tickled pCPU as... tickled!
2017-02-09 23:48 ` Dario Faggioli
@ 2017-02-15 14:55 ` George Dunlap
0 siblings, 0 replies; 35+ messages in thread
From: George Dunlap @ 2017-02-15 14:55 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Anshul Makkar
On Thu, Feb 9, 2017 at 11:48 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Thu, 2017-02-09 at 14:59 +0100, Dario Faggioli wrote:
>> In fact, whether or not a pCPU has been tickled, and is
>> therefore about to re-schedule, is something we look at
>> and base decisions on in various places.
>>
>> So, let's make sure that we do that basing on accurate
>> information.
>>
>> While there, also tweak a little bit smt_idle_mask_clear()
>> (used for implementing SMT support), so that it only alter
>> the relevant cpumask when there is the actual need for this.
>> (This is only for reduced overhead, behavior remains the
>> same).
>>
> So, while working on other things with this series applied, I noticed
> some strange behavior, for which it turned out this patch is
> responsible.
>
> More specifically...
>
>> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
>> index addee7b..bfb4891 100644
>> @@ -1289,9 +1300,8 @@ runq_tickle(const struct scheduler *ops, struct
>> csched2_vcpu *new, s_time_t now)
>> sizeof(d),
>> (unsigned char *)&d);
>> }
>> - __cpumask_set_cpu(ipid, &rqd->tickled);
>> - smt_idle_mask_clear(ipid, &rqd->smt_idle);
>> - cpu_raise_softirq(ipid, SCHEDULE_SOFTIRQ);
>> +
>> + tickle_cpu(cpu, rqd);
>>
> ... this, quite obviously, wants to be:
>
> tickle_cpu(ipid, rqd);
>
> I'll fix this in v2.
You mean v3? :-)
You can add:
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 08/10] xen: credit2: don't miss accounting while doing a credit reset.
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
` (6 preceding siblings ...)
2017-02-09 13:59 ` [PATCH v2 07/10] xen: credit2: always mark a tickled pCPU as... tickled! Dario Faggioli
@ 2017-02-09 13:59 ` Dario Faggioli
2017-02-15 15:07 ` George Dunlap
2017-02-09 13:59 ` [PATCH v2 09/10] xen/tools: tracing: trace (Credit2) runq traversal Dario Faggioli
` (3 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:59 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar
A credit reset basically means going through all the
vCPUs of a runqueue and altering their credits, as a
consequence of a 'scheduling epoch' having come to an
end.
Blocked or runnable vCPUs are fine, all the credits
they've spent running so far have been accounted to
them when they were scheduled out.
But if a vCPU is running on a pCPU, when a reset event
occurs (on another pCPU), that does not get properly
accounted. Let's therefore begin to do so, for better
accuracy and fairness.
In fact, after this patch, we see this in a trace:
csched2:schedule cpu 10, rq# 1, busy, not tickled
csched2:burn_credits d1v5, credit = 9998353, delta = 202996
runstate_continue d1v5 running->running
...
csched2:schedule cpu 12, rq# 1, busy, not tickled
csched2:burn_credits d1v6, credit = -1327, delta = 9999544
csched2:reset_credits d0v13, credit_start = 10500000, credit_end = 10500000, mult = 1
csched2:reset_credits d0v14, credit_start = 10500000, credit_end = 10500000, mult = 1
csched2:reset_credits d0v7, credit_start = 10500000, credit_end = 10500000, mult = 1
csched2:burn_credits d1v5, credit = 201805, delta = 9796548
csched2:reset_credits d1v5, credit_start = 201805, credit_end = 10201805, mult = 1
csched2:burn_credits d1v6, credit = -1327, delta = 0
csched2:reset_credits d1v6, credit_start = -1327, credit_end = 9998673, mult = 1
Which shows how d1v5 actually executed for ~9.796 ms,
on pCPU 10, when reset_credit() is executed, on pCPU
12, because of d1v6's credits going below 0.
Without this patch, this 9.796ms are not accounted
to anyone. With this patch, d1v5 is charged for that,
and its credits drop down from 9796548 to 201805.
And this is important, as it means that it will
begin the new epoch with 10201805 credits, instead
of 10500000 (which he would have, before this patch).
Basically, we were forgetting one round of accounting
in epoch x, for the vCPUs that are running at the time
the epoch ends. And this meant favouring a little bit
these same vCPUs, in epoch x+1, providing them with
the chance of execute longer than their fair share.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
xen/common/sched_credit2.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index bfb4891..8057abf 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1341,18 +1341,28 @@ static void reset_credit(const struct scheduler *ops, int cpu, s_time_t now,
list_for_each( iter, &rqd->svc )
{
+ unsigned int svc_cpu;
struct csched2_vcpu * svc;
int start_credit;
svc = list_entry(iter, struct csched2_vcpu, rqd_elem);
+ svc_cpu = svc->vcpu->processor;
ASSERT(!is_idle_vcpu(svc->vcpu));
ASSERT(svc->rqd == rqd);
+ /*
+ * If svc is running, it is our responsibility to make sure, here,
+ * that the credit it has spent so far get accounted.
+ */
+ if ( svc->vcpu == curr_on_cpu(svc_cpu) )
+ burn_credits(rqd, svc, now);
+
start_credit = svc->credit;
- /* And add INIT * m, avoiding integer multiplication in the
- * common case. */
+ /*
+ * Add INIT * m, avoiding integer multiplication in the common case.
+ */
if ( likely(m==1) )
svc->credit += CSCHED2_CREDIT_INIT;
else
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 08/10] xen: credit2: don't miss accounting while doing a credit reset.
2017-02-09 13:59 ` [PATCH v2 08/10] xen: credit2: don't miss accounting while doing a credit reset Dario Faggioli
@ 2017-02-15 15:07 ` George Dunlap
0 siblings, 0 replies; 35+ messages in thread
From: George Dunlap @ 2017-02-15 15:07 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Anshul Makkar
On Thu, Feb 9, 2017 at 1:59 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> A credit reset basically means going through all the
> vCPUs of a runqueue and altering their credits, as a
> consequence of a 'scheduling epoch' having come to an
> end.
>
> Blocked or runnable vCPUs are fine, all the credits
> they've spent running so far have been accounted to
> them when they were scheduled out.
>
> But if a vCPU is running on a pCPU, when a reset event
> occurs (on another pCPU), that does not get properly
> accounted. Let's therefore begin to do so, for better
> accuracy and fairness.
>
> In fact, after this patch, we see this in a trace:
>
> csched2:schedule cpu 10, rq# 1, busy, not tickled
> csched2:burn_credits d1v5, credit = 9998353, delta = 202996
> runstate_continue d1v5 running->running
> ...
> csched2:schedule cpu 12, rq# 1, busy, not tickled
> csched2:burn_credits d1v6, credit = -1327, delta = 9999544
> csched2:reset_credits d0v13, credit_start = 10500000, credit_end = 10500000, mult = 1
> csched2:reset_credits d0v14, credit_start = 10500000, credit_end = 10500000, mult = 1
> csched2:reset_credits d0v7, credit_start = 10500000, credit_end = 10500000, mult = 1
> csched2:burn_credits d1v5, credit = 201805, delta = 9796548
> csched2:reset_credits d1v5, credit_start = 201805, credit_end = 10201805, mult = 1
> csched2:burn_credits d1v6, credit = -1327, delta = 0
> csched2:reset_credits d1v6, credit_start = -1327, credit_end = 9998673, mult = 1
>
> Which shows how d1v5 actually executed for ~9.796 ms,
> on pCPU 10, when reset_credit() is executed, on pCPU
> 12, because of d1v6's credits going below 0.
>
> Without this patch, this 9.796ms are not accounted
> to anyone. With this patch, d1v5 is charged for that,
> and its credits drop down from 9796548 to 201805.
>
> And this is important, as it means that it will
> begin the new epoch with 10201805 credits, instead
> of 10500000 (which he would have, before this patch).
>
> Basically, we were forgetting one round of accounting
> in epoch x, for the vCPUs that are running at the time
> the epoch ends. And this meant favouring a little bit
> these same vCPUs, in epoch x+1, providing them with
> the chance of execute longer than their fair share.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Hmm, not sure how I missed that -- I feel like I was calling
"burn_credits()" for other vcpus all the time. Must have missed one.
Anyway:
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 09/10] xen/tools: tracing: trace (Credit2) runq traversal.
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
` (7 preceding siblings ...)
2017-02-09 13:59 ` [PATCH v2 08/10] xen: credit2: don't miss accounting while doing a credit reset Dario Faggioli
@ 2017-02-09 13:59 ` Dario Faggioli
2017-02-15 15:31 ` George Dunlap
2017-02-09 13:59 ` [PATCH v2 10/10] xen/tools: tracing: always report how long next slice will be Dario Faggioli
` (2 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:59 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
When traversing a Credit2 runqueue to select the
best candidate vCPU to be run next, show in the
trace which vCPUs we consider.
A bit verbose, but quite useful, considering that
we may end up looking at, but then discarding, one
of more vCPU. This will help understand which ones
are skipped and why.
Also, add how much credits the chosen vCPU has
(in the TRC_CSCHED2_RUNQ_CANDIDATE record). And,
while there, fix a bug in tools/xentrace/formats
(still in the output of TRC_CSCHED2_RUNQ_CANDIDATE).
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
tools/xentrace/formats | 3 ++-
tools/xentrace/xenalyze.c | 15 +++++++++++++--
xen/common/sched_credit2.c | 15 +++++++++++++++
3 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index db89f92..72c0b24 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -65,9 +65,10 @@
0x00022210 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:load_check [ lrq_id[16]:orq_id[16] = 0x%(1)08x, delta = %(2)d ]
0x00022211 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:load_balance [ l_bavgload = 0x%(2)08x%(1)08x, o_bavgload = 0x%(4)08x%(3)08x, lrq_id[16]:orq_id[16] = 0x%(5)08x ]
0x00022212 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:pick_cpu [ b_avgload = 0x%(2)08x%(1)08x, dom:vcpu = 0x%(3)08x, rq_id[16]:new_cpu[16] = %(4)d ]
-0x00022213 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:runq_candidate [ dom:vcpu = 0x%(1)08x, skipped_vcpus = %(2)d tickled_cpu = %(3)d ]
+0x00022213 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:runq_candidate [ dom:vcpu = 0x%(1)08x, credit = %(4)d, skipped_vcpus = %(3)d, tickled_cpu = %(2)d ]
0x00022214 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:schedule [ rq:cpu = 0x%(1)08x, tasklet[8]:idle[8]:smt_idle[8]:tickled[8] = %(2)08x ]
0x00022215 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:ratelimit [ dom:vcpu = 0x%(1)08x, runtime = %(2)d ]
+0x00022216 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:runq_cand_chk [ dom:vcpu = 0x%(1)08x ]
0x00022801 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:tickle [ cpu = %(1)d ]
0x00022802 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:runq_pick [ dom:vcpu = 0x%(1)08x, cur_deadline = 0x%(3)08x%(2)08x, cur_budget = 0x%(5)08x%(4)08x ]
diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index a90da20..2678e2a 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -7825,12 +7825,13 @@ void sched_process(struct pcpu_info *p)
struct {
unsigned vcpuid:16, domid:16;
unsigned tickled_cpu, skipped;
+ int credit;
} *r = (typeof(r))ri->d;
- printf(" %s csched2:runq_candidate d%uv%u, "
+ printf(" %s csched2:runq_candidate d%uv%u, credit = %d, "
"%u vcpus skipped, ",
ri->dump_header, r->domid, r->vcpuid,
- r->skipped);
+ r->credit, r->skipped);
if (r->tickled_cpu == (unsigned)-1)
printf("no cpu was tickled\n");
else
@@ -7864,6 +7865,16 @@ void sched_process(struct pcpu_info *p)
r->runtime / 1000, r->runtime % 1000);
}
break;
+ case TRC_SCHED_CLASS_EVT(CSCHED2, 23): /* RUNQ_CAND_CHECK */
+ if(opt.dump_all) {
+ struct {
+ unsigned int vcpuid:16, domid:16;
+ } *r = (typeof(r))ri->d;
+
+ printf(" %s csched2:runq_cand_check d%uv%u\n",
+ ri->dump_header, r->domid, r->vcpuid);
+ }
+ break;
/* RTDS (TRC_RTDS_xxx) */
case TRC_SCHED_CLASS_EVT(RTDS, 1): /* TICKLE */
if(opt.dump_all) {
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 8057abf..35d6988 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -57,6 +57,7 @@
#define TRC_CSCHED2_RUNQ_CANDIDATE TRC_SCHED_CLASS_EVT(CSCHED2, 20)
#define TRC_CSCHED2_SCHEDULE TRC_SCHED_CLASS_EVT(CSCHED2, 21)
#define TRC_CSCHED2_RATELIMIT TRC_SCHED_CLASS_EVT(CSCHED2, 22)
+#define TRC_CSCHED2_RUNQ_CAND_CHECK TRC_SCHED_CLASS_EVT(CSCHED2, 23)
/*
* WARNING: This is still in an experimental phase. Status and work can be found at the
@@ -2498,6 +2499,18 @@ runq_candidate(struct csched2_runqueue_data *rqd,
{
struct csched2_vcpu * svc = list_entry(iter, struct csched2_vcpu, runq_elem);
+ if ( unlikely(tb_init_done) )
+ {
+ struct {
+ unsigned vcpu:16, dom:16;
+ } d;
+ d.dom = svc->vcpu->domain->domain_id;
+ d.vcpu = svc->vcpu->vcpu_id;
+ __trace_var(TRC_CSCHED2_RUNQ_CAND_CHECK, 1,
+ sizeof(d),
+ (unsigned char *)&d);
+ }
+
/* Only consider vcpus that are allowed to run on this processor. */
if ( !cpumask_test_cpu(cpu, svc->vcpu->cpu_hard_affinity) )
{
@@ -2546,9 +2559,11 @@ runq_candidate(struct csched2_runqueue_data *rqd,
struct {
unsigned vcpu:16, dom:16;
unsigned tickled_cpu, skipped;
+ int credit;
} d;
d.dom = snext->vcpu->domain->domain_id;
d.vcpu = snext->vcpu->vcpu_id;
+ d.credit = snext->credit;
d.tickled_cpu = snext->tickled_cpu;
d.skipped = *skipped;
__trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 09/10] xen/tools: tracing: trace (Credit2) runq traversal.
2017-02-09 13:59 ` [PATCH v2 09/10] xen/tools: tracing: trace (Credit2) runq traversal Dario Faggioli
@ 2017-02-15 15:31 ` George Dunlap
2017-02-24 18:48 ` Dario Faggioli
0 siblings, 1 reply; 35+ messages in thread
From: George Dunlap @ 2017-02-15 15:31 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel
On Thu, Feb 9, 2017 at 1:59 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> When traversing a Credit2 runqueue to select the
> best candidate vCPU to be run next, show in the
> trace which vCPUs we consider.
>
> A bit verbose, but quite useful, considering that
> we may end up looking at, but then discarding, one
> of more vCPU. This will help understand which ones
> are skipped and why.
This sounds useful -- but what I don't quite understand is why it's
useful simply to know that a vcpu was considered, and not also know
specifically why it was decided against. (I'm sure you've found it
useful or you wouldn't have submitted the patch.)
Anyway, I can check it in as-is (if it applies).
Acked-by: George Dunlap <george.dunlap@citrix.com>
>
> Also, add how much credits the chosen vCPU has
> (in the TRC_CSCHED2_RUNQ_CANDIDATE record). And,
> while there, fix a bug in tools/xentrace/formats
> (still in the output of TRC_CSCHED2_RUNQ_CANDIDATE).
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> tools/xentrace/formats | 3 ++-
> tools/xentrace/xenalyze.c | 15 +++++++++++++--
> xen/common/sched_credit2.c | 15 +++++++++++++++
> 3 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/tools/xentrace/formats b/tools/xentrace/formats
> index db89f92..72c0b24 100644
> --- a/tools/xentrace/formats
> +++ b/tools/xentrace/formats
> @@ -65,9 +65,10 @@
> 0x00022210 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:load_check [ lrq_id[16]:orq_id[16] = 0x%(1)08x, delta = %(2)d ]
> 0x00022211 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:load_balance [ l_bavgload = 0x%(2)08x%(1)08x, o_bavgload = 0x%(4)08x%(3)08x, lrq_id[16]:orq_id[16] = 0x%(5)08x ]
> 0x00022212 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:pick_cpu [ b_avgload = 0x%(2)08x%(1)08x, dom:vcpu = 0x%(3)08x, rq_id[16]:new_cpu[16] = %(4)d ]
> -0x00022213 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:runq_candidate [ dom:vcpu = 0x%(1)08x, skipped_vcpus = %(2)d tickled_cpu = %(3)d ]
> +0x00022213 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:runq_candidate [ dom:vcpu = 0x%(1)08x, credit = %(4)d, skipped_vcpus = %(3)d, tickled_cpu = %(2)d ]
> 0x00022214 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:schedule [ rq:cpu = 0x%(1)08x, tasklet[8]:idle[8]:smt_idle[8]:tickled[8] = %(2)08x ]
> 0x00022215 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:ratelimit [ dom:vcpu = 0x%(1)08x, runtime = %(2)d ]
> +0x00022216 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:runq_cand_chk [ dom:vcpu = 0x%(1)08x ]
>
> 0x00022801 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:tickle [ cpu = %(1)d ]
> 0x00022802 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:runq_pick [ dom:vcpu = 0x%(1)08x, cur_deadline = 0x%(3)08x%(2)08x, cur_budget = 0x%(5)08x%(4)08x ]
> diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
> index a90da20..2678e2a 100644
> --- a/tools/xentrace/xenalyze.c
> +++ b/tools/xentrace/xenalyze.c
> @@ -7825,12 +7825,13 @@ void sched_process(struct pcpu_info *p)
> struct {
> unsigned vcpuid:16, domid:16;
> unsigned tickled_cpu, skipped;
> + int credit;
> } *r = (typeof(r))ri->d;
>
> - printf(" %s csched2:runq_candidate d%uv%u, "
> + printf(" %s csched2:runq_candidate d%uv%u, credit = %d, "
> "%u vcpus skipped, ",
> ri->dump_header, r->domid, r->vcpuid,
> - r->skipped);
> + r->credit, r->skipped);
> if (r->tickled_cpu == (unsigned)-1)
> printf("no cpu was tickled\n");
> else
> @@ -7864,6 +7865,16 @@ void sched_process(struct pcpu_info *p)
> r->runtime / 1000, r->runtime % 1000);
> }
> break;
> + case TRC_SCHED_CLASS_EVT(CSCHED2, 23): /* RUNQ_CAND_CHECK */
> + if(opt.dump_all) {
> + struct {
> + unsigned int vcpuid:16, domid:16;
> + } *r = (typeof(r))ri->d;
> +
> + printf(" %s csched2:runq_cand_check d%uv%u\n",
> + ri->dump_header, r->domid, r->vcpuid);
> + }
> + break;
> /* RTDS (TRC_RTDS_xxx) */
> case TRC_SCHED_CLASS_EVT(RTDS, 1): /* TICKLE */
> if(opt.dump_all) {
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 8057abf..35d6988 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -57,6 +57,7 @@
> #define TRC_CSCHED2_RUNQ_CANDIDATE TRC_SCHED_CLASS_EVT(CSCHED2, 20)
> #define TRC_CSCHED2_SCHEDULE TRC_SCHED_CLASS_EVT(CSCHED2, 21)
> #define TRC_CSCHED2_RATELIMIT TRC_SCHED_CLASS_EVT(CSCHED2, 22)
> +#define TRC_CSCHED2_RUNQ_CAND_CHECK TRC_SCHED_CLASS_EVT(CSCHED2, 23)
>
> /*
> * WARNING: This is still in an experimental phase. Status and work can be found at the
> @@ -2498,6 +2499,18 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> {
> struct csched2_vcpu * svc = list_entry(iter, struct csched2_vcpu, runq_elem);
>
> + if ( unlikely(tb_init_done) )
> + {
> + struct {
> + unsigned vcpu:16, dom:16;
> + } d;
> + d.dom = svc->vcpu->domain->domain_id;
> + d.vcpu = svc->vcpu->vcpu_id;
> + __trace_var(TRC_CSCHED2_RUNQ_CAND_CHECK, 1,
> + sizeof(d),
> + (unsigned char *)&d);
> + }
> +
> /* Only consider vcpus that are allowed to run on this processor. */
> if ( !cpumask_test_cpu(cpu, svc->vcpu->cpu_hard_affinity) )
> {
> @@ -2546,9 +2559,11 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> struct {
> unsigned vcpu:16, dom:16;
> unsigned tickled_cpu, skipped;
> + int credit;
> } d;
> d.dom = snext->vcpu->domain->domain_id;
> d.vcpu = snext->vcpu->vcpu_id;
> + d.credit = snext->credit;
> d.tickled_cpu = snext->tickled_cpu;
> d.skipped = *skipped;
> __trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1,
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 09/10] xen/tools: tracing: trace (Credit2) runq traversal.
2017-02-15 15:31 ` George Dunlap
@ 2017-02-24 18:48 ` Dario Faggioli
0 siblings, 0 replies; 35+ messages in thread
From: Dario Faggioli @ 2017-02-24 18:48 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 2016 bytes --]
On Wed, 2017-02-15 at 15:31 +0000, George Dunlap wrote:
> On Thu, Feb 9, 2017 at 1:59 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> >
> > When traversing a Credit2 runqueue to select the
> > best candidate vCPU to be run next, show in the
> > trace which vCPUs we consider.
> >
> > A bit verbose, but quite useful, considering that
> > we may end up looking at, but then discarding, one
> > of more vCPU. This will help understand which ones
> > are skipped and why.
>
> This sounds useful -- but what I don't quite understand is why it's
> useful simply to know that a vcpu was considered, and not also know
> specifically why it was decided against. (I'm sure you've found it
> useful or you wouldn't have submitted the patch.)
>
Yeah, I see what you mean.
Of course, the more info, the better. But knowing which vcpus have been
skipped, is the real hard thing here. Meaning that it, in theory, was
possible to reconstruct that, but only with a very tedious and error
prone process of staring and noting down previous records.
OTOH, once that you see which ones have been skipped --considering that
there are not too many reasons for that to happen, yet-- it's fairly
straightforward to guess why.
And yes, as you say, I so far have found more useful to know which
ones, while the why hasn't been critical for the trace analysis I've
done so far.
> Anyway, I can check it in as-is (if it applies).
>
> Acked-by: George Dunlap <george.dunlap@citrix.com>
>
Thanks, and fear not: I actually may end up sending a follow up patch
for spitting out the reason why we skipped someone too... As you
probably have understood by this point, I like a lot adding
tracepoints! :-P :-P
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v2 10/10] xen/tools: tracing: always report how long next slice will be.
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
` (8 preceding siblings ...)
2017-02-09 13:59 ` [PATCH v2 09/10] xen/tools: tracing: trace (Credit2) runq traversal Dario Faggioli
@ 2017-02-09 13:59 ` Dario Faggioli
2017-02-15 15:40 ` George Dunlap
2017-02-09 14:37 ` [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Jan Beulich
2017-02-15 16:03 ` George Dunlap
11 siblings, 1 reply; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 13:59 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
In fact, it is quite useful a pice of information,
to know how long it is the "next time slice" a vCPU
has been granted. It allows one to assume (and then
check) when the scheduler is supposed to run next,
and other things.
While this information is indeed reported when a
context switch happens, it is not when the same vCPU
that is running, continues to run.
Fix that, with the following outcome.
Before this change:
csched2:schedule cpu 9, rq# 1, idle, SMT idle, tickled
csched2:runq_candidate d0v3, 0 vcpus skipped, cpu 9 was tickled
sched_switch prev d32767v9, run for 991.186us
sched_switch next d0v3, was runnable for 2.515us, next slice 10000.0us
sched_switch prev d32767v9 next d0v3 ^^^^^^^^^^^^^^^^^^^^
runstate_change d32767v9 running->runnable
...
csched2:schedule cpu 2, rq# 0, busy, not tickled
csched2:burn_credits d1v5, credit = 9996950, delta = 502913
csched2:runq_candidate d1v5, 0 vcpus skipped, no cpu was tickled
runstate_continue d1v5 running->running
?????????????
That is, in the second block, it is shown that d1v5
was already running and will continue to do so,
but we have no idea for how long.
OTOH, after the change:
csched2:schedule cpu 1, rq# 0, busy, not tickled
csched2:burn_credits d0v8, credit = 9998645, delta = 12104
csched2:runq_candidate d0v8, credit = 9998645, 0 vcpus skipped, no cpu was tickled
sched_switch continue d0v8, run for 1125.820us, next slice 9998.645us
runstate_continue d0v8 running->running ^^^^^^^^^^^^^^^^^^^^^
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
tools/xentrace/formats | 1 +
tools/xentrace/xenalyze.c | 17 +++++++++++++++++
xen/common/schedule.c | 4 ++++
xen/include/public/trace.h | 1 +
4 files changed, 23 insertions(+)
diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index 72c0b24..a055231 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -35,6 +35,7 @@
0x0002800e CPU%(cpu)d %(tsc)d (+%(reltsc)8d) switch_infprev [ dom:vcpu = 0x%(1)04x%(2)04x, runtime = %(3)d ]
0x0002800f CPU%(cpu)d %(tsc)d (+%(reltsc)8d) switch_infnext [ new_dom:vcpu = 0x%(1)04x%(2)04x, time = %(3)d, r_time = %(4)d ]
0x00028010 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) domain_shutdown_code [ dom:vcpu = 0x%(1)04x%(2)04x, reason = 0x%(3)08x ]
+0x00028011 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) switch_infcont [ dom:vcpu = 0x%(1)04x%(2)04x, runtime = %(3)d, r_time = %(4)d ]
0x00022001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:sched_tasklet
0x00022002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:account_start [ dom:vcpu = 0x%(1)04x%(2)04x, active = %(3)d ]
diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 2678e2a..68ffcc2 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -7528,6 +7528,23 @@ void sched_process(struct pcpu_info *p)
printf("\n");
}
break;
+ case TRC_SCHED_SWITCH_INFCONT:
+ if(opt.dump_all)
+ {
+ struct {
+ unsigned int domid, vcpuid, rsince;
+ int slice;
+ } *r = (typeof(r))ri->d;
+
+ printf(" %s sched_switch continue d%uv%u, run for %u.%uus",
+ ri->dump_header, r->domid, r->vcpuid,
+ r->rsince / 1000, r->rsince % 1000);
+ if ( r->slice > 0 )
+ printf(", next slice %u.%uus", r->slice / 1000,
+ r->slice % 1000);
+ printf("\n");
+ }
+ break;
case TRC_SCHED_CTL:
case TRC_SCHED_S_TIMER_FN:
case TRC_SCHED_T_TIMER_FN:
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index e4320f3..495f138 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -1398,6 +1398,10 @@ static void schedule(void)
if ( unlikely(prev == next) )
{
pcpu_schedule_unlock_irq(lock, cpu);
+ TRACE_4D(TRC_SCHED_SWITCH_INFCONT,
+ next->domain->domain_id, next->vcpu_id,
+ now - prev->runstate.state_entry_time,
+ next_slice.time);
trace_continue_running(next);
return continue_running(prev);
}
diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
index 5ef9c37..7f2e891 100644
--- a/xen/include/public/trace.h
+++ b/xen/include/public/trace.h
@@ -115,6 +115,7 @@
#define TRC_SCHED_SWITCH_INFPREV (TRC_SCHED_VERBOSE + 14)
#define TRC_SCHED_SWITCH_INFNEXT (TRC_SCHED_VERBOSE + 15)
#define TRC_SCHED_SHUTDOWN_CODE (TRC_SCHED_VERBOSE + 16)
+#define TRC_SCHED_SWITCH_INFCONT (TRC_SCHED_VERBOSE + 17)
#define TRC_DOM0_DOM_ADD (TRC_DOM0_DOMOPS + 1)
#define TRC_DOM0_DOM_REM (TRC_DOM0_DOMOPS + 2)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v2 10/10] xen/tools: tracing: always report how long next slice will be.
2017-02-09 13:59 ` [PATCH v2 10/10] xen/tools: tracing: always report how long next slice will be Dario Faggioli
@ 2017-02-15 15:40 ` George Dunlap
2017-02-27 18:12 ` Dario Faggioli
0 siblings, 1 reply; 35+ messages in thread
From: George Dunlap @ 2017-02-15 15:40 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel
On Thu, Feb 9, 2017 at 1:59 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> In fact, it is quite useful a pice of information,
> to know how long it is the "next time slice" a vCPU
> has been granted. It allows one to assume (and then
> check) when the scheduler is supposed to run next,
> and other things.
>
> While this information is indeed reported when a
> context switch happens, it is not when the same vCPU
> that is running, continues to run.
The patch looks fine; but the description seems a bit round-about. I
think I would have said something like:
---
xen: Report next slice time when continuing as well as switching
We record trace information about the next timeslice when switching to
a different vcpu, but not when continuing to run the same cpu. This
information
is quite useful; so add a trace including that information on the
'continue_running' path as well.
---
What do you think?
-George
>
> Fix that, with the following outcome.
>
> Before this change:
>
> csched2:schedule cpu 9, rq# 1, idle, SMT idle, tickled
> csched2:runq_candidate d0v3, 0 vcpus skipped, cpu 9 was tickled
> sched_switch prev d32767v9, run for 991.186us
> sched_switch next d0v3, was runnable for 2.515us, next slice 10000.0us
> sched_switch prev d32767v9 next d0v3 ^^^^^^^^^^^^^^^^^^^^
> runstate_change d32767v9 running->runnable
> ...
> csched2:schedule cpu 2, rq# 0, busy, not tickled
> csched2:burn_credits d1v5, credit = 9996950, delta = 502913
> csched2:runq_candidate d1v5, 0 vcpus skipped, no cpu was tickled
> runstate_continue d1v5 running->running
> ?????????????
>
> That is, in the second block, it is shown that d1v5
> was already running and will continue to do so,
> but we have no idea for how long.
>
> OTOH, after the change:
>
> csched2:schedule cpu 1, rq# 0, busy, not tickled
> csched2:burn_credits d0v8, credit = 9998645, delta = 12104
> csched2:runq_candidate d0v8, credit = 9998645, 0 vcpus skipped, no cpu was tickled
> sched_switch continue d0v8, run for 1125.820us, next slice 9998.645us
> runstate_continue d0v8 running->running ^^^^^^^^^^^^^^^^^^^^^
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> tools/xentrace/formats | 1 +
> tools/xentrace/xenalyze.c | 17 +++++++++++++++++
> xen/common/schedule.c | 4 ++++
> xen/include/public/trace.h | 1 +
> 4 files changed, 23 insertions(+)
>
> diff --git a/tools/xentrace/formats b/tools/xentrace/formats
> index 72c0b24..a055231 100644
> --- a/tools/xentrace/formats
> +++ b/tools/xentrace/formats
> @@ -35,6 +35,7 @@
> 0x0002800e CPU%(cpu)d %(tsc)d (+%(reltsc)8d) switch_infprev [ dom:vcpu = 0x%(1)04x%(2)04x, runtime = %(3)d ]
> 0x0002800f CPU%(cpu)d %(tsc)d (+%(reltsc)8d) switch_infnext [ new_dom:vcpu = 0x%(1)04x%(2)04x, time = %(3)d, r_time = %(4)d ]
> 0x00028010 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) domain_shutdown_code [ dom:vcpu = 0x%(1)04x%(2)04x, reason = 0x%(3)08x ]
> +0x00028011 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) switch_infcont [ dom:vcpu = 0x%(1)04x%(2)04x, runtime = %(3)d, r_time = %(4)d ]
>
> 0x00022001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:sched_tasklet
> 0x00022002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:account_start [ dom:vcpu = 0x%(1)04x%(2)04x, active = %(3)d ]
> diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
> index 2678e2a..68ffcc2 100644
> --- a/tools/xentrace/xenalyze.c
> +++ b/tools/xentrace/xenalyze.c
> @@ -7528,6 +7528,23 @@ void sched_process(struct pcpu_info *p)
> printf("\n");
> }
> break;
> + case TRC_SCHED_SWITCH_INFCONT:
> + if(opt.dump_all)
> + {
> + struct {
> + unsigned int domid, vcpuid, rsince;
> + int slice;
> + } *r = (typeof(r))ri->d;
> +
> + printf(" %s sched_switch continue d%uv%u, run for %u.%uus",
> + ri->dump_header, r->domid, r->vcpuid,
> + r->rsince / 1000, r->rsince % 1000);
> + if ( r->slice > 0 )
> + printf(", next slice %u.%uus", r->slice / 1000,
> + r->slice % 1000);
> + printf("\n");
> + }
> + break;
> case TRC_SCHED_CTL:
> case TRC_SCHED_S_TIMER_FN:
> case TRC_SCHED_T_TIMER_FN:
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index e4320f3..495f138 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1398,6 +1398,10 @@ static void schedule(void)
> if ( unlikely(prev == next) )
> {
> pcpu_schedule_unlock_irq(lock, cpu);
> + TRACE_4D(TRC_SCHED_SWITCH_INFCONT,
> + next->domain->domain_id, next->vcpu_id,
> + now - prev->runstate.state_entry_time,
> + next_slice.time);
> trace_continue_running(next);
> return continue_running(prev);
> }
> diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
> index 5ef9c37..7f2e891 100644
> --- a/xen/include/public/trace.h
> +++ b/xen/include/public/trace.h
> @@ -115,6 +115,7 @@
> #define TRC_SCHED_SWITCH_INFPREV (TRC_SCHED_VERBOSE + 14)
> #define TRC_SCHED_SWITCH_INFNEXT (TRC_SCHED_VERBOSE + 15)
> #define TRC_SCHED_SHUTDOWN_CODE (TRC_SCHED_VERBOSE + 16)
> +#define TRC_SCHED_SWITCH_INFCONT (TRC_SCHED_VERBOSE + 17)
>
> #define TRC_DOM0_DOM_ADD (TRC_DOM0_DOMOPS + 1)
> #define TRC_DOM0_DOM_REM (TRC_DOM0_DOMOPS + 2)
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 10/10] xen/tools: tracing: always report how long next slice will be.
2017-02-15 15:40 ` George Dunlap
@ 2017-02-27 18:12 ` Dario Faggioli
0 siblings, 0 replies; 35+ messages in thread
From: Dario Faggioli @ 2017-02-27 18:12 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1125 bytes --]
On Wed, 2017-02-15 at 15:40 +0000, George Dunlap wrote:
> On Thu, Feb 9, 2017 at 1:59 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> xen: Report next slice time when continuing as well as switching
>
> We record trace information about the next timeslice when switching
> to
> a different vcpu, but not when continuing to run the same cpu. This
> information
> is quite useful; so add a trace including that information on the
> 'continue_running' path as well.
>
> ---
>
> What do you think?
>
Ok, I like it.
I did replace my changelog with your proposed one, but I've left the
example output from xenalyze in place.
Not sure whether you were suggesting to also kill that part. I do think
it's useful so I left it there. But if you want it removed too, I can
live with that, so just ask. :-)
Thanks and Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
` (9 preceding siblings ...)
2017-02-09 13:59 ` [PATCH v2 10/10] xen/tools: tracing: always report how long next slice will be Dario Faggioli
@ 2017-02-09 14:37 ` Jan Beulich
2017-02-09 15:29 ` Dario Faggioli
2017-02-15 16:03 ` George Dunlap
11 siblings, 1 reply; 35+ messages in thread
From: Jan Beulich @ 2017-02-09 14:37 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, xen-devel, Anshul Makkar
>>> On 09.02.17 at 14:58, <dario.faggioli@citrix.com> wrote:
> This series contains mostly style or cosmetic fixes for Credit2, with the
> following two exceptions:
> - 2 actual fixes for (not so severe) behavioral bugs (patches 5 and 6);
That's not really in line with patch 5 saying "No functional change
intended."
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs
2017-02-09 14:37 ` [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Jan Beulich
@ 2017-02-09 15:29 ` Dario Faggioli
0 siblings, 0 replies; 35+ messages in thread
From: Dario Faggioli @ 2017-02-09 15:29 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, xen-devel, Anshul Makkar
[-- Attachment #1.1: Type: text/plain, Size: 822 bytes --]
On Thu, 2017-02-09 at 07:37 -0700, Jan Beulich wrote:
> > > > On 09.02.17 at 14:58, <dario.faggioli@citrix.com> wrote:
> > This series contains mostly style or cosmetic fixes for Credit2,
> > with the
> > following two exceptions:
> > - 2 actual fixes for (not so severe) behavioral bugs (patches 5
> > and 6);
>
> That's not really in line with patch 5 saying "No functional change
> intended."
>
Ah, yes, sorry, it is actually patches 7 and 8 that contains the fixes
now, as I added two patches at the beginning of the series.
Regards,
Dario
---
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs
2017-02-09 13:58 [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Dario Faggioli
` (10 preceding siblings ...)
2017-02-09 14:37 ` [PATCH v2 00/10] xen: credit2: improve style, and tracing; fix two bugs Jan Beulich
@ 2017-02-15 16:03 ` George Dunlap
11 siblings, 0 replies; 35+ messages in thread
From: George Dunlap @ 2017-02-15 16:03 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Anshul Makkar
On Thu, Feb 9, 2017 at 1:58 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> Hello,
>
> This series contains mostly style or cosmetic fixes for Credit2, with the
> following two exceptions:
> - 2 actual fixes for (not so severe) behavioral bugs (patches 5 and 6);
> - some tracing improvements (last 3 patches).
>
> More info on the specific changelogs.
>
> This is basically a resubmission of
> https://lists.xen.org/archives/html/xen-devel/2017-01/msg02837.html
>
> but with:
> - 2 patches (the first two) that were not here before, and are now
> upon request from George of rebasind and resending all the scheduler
> patches I have outstanding in one series;
> - all rebased on current staging.
>
> The series is also available as a git tree here:
>
> git://xenbits.xen.org/people/dariof/xen.git rel/sched/credit2-style-tracing-accounting-v
> https://travis-ci.org/xen-project/xen/builds/199958381
> (there are some clang failures, but they look unrelated).
>
> Thanks and Regards,
> Dario
>
> ---
> Dario Faggioli (10):
> xen: sched: harmonize debug dump output among schedulers.
> xen: credit2: clear bit instead of skip step in runq_tickle()
> xen: credit2: improve comments' style and definition of CSFLAG-s
I've pushed these three. I would have pushed patch 7 and 8 as well
(with the fix to patch 7), but they didn't apply without the other
patches.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 35+ messages in thread