* [PATCH v2 01/10] xen: credit1: return the 'time remaining to the limit' as next timeslice.
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
@ 2016-09-30 2:53 ` Dario Faggioli
2016-09-30 11:16 ` George Dunlap
2016-09-30 2:53 ` [PATCH v2 02/10] xen: credit1: don't rate limit context switches in case of yields Dario Faggioli
` (9 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:53 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
If vcpu x has run for 200us, and sched_ratelimit_us is
1000us, continue running x _but_ return 1000us-200us as
the next time slice. This way, next scheduling point will
happen in 800us, i.e., exactly at the point when x crosses
the threshold, and can be descheduled (if appropriate).
Right now (without this patch), we're always returning
sched_ratelimit_us (1000us, in the example above), which
means we're (potentially) allowing x to run more than
it should have been able to.
Note that, however, in order to avoid setting timers to very
short intervals, which is part of the purpose of rate limiting,
we never use a time slice smaller than a well defined threshold.
Such threshold (CSCHED_MIN_TIMER defined in this patch) is, in
general independent from rate limiting, but it looks a good idea
to set it to the minimum possible ratelimiting value.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes from v1:
* introduce CSCHED_MIN_TIMER, as agreed during review.
---
xen/common/sched_credit.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index b63efac..4d84b5f 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -51,6 +51,8 @@
/* Default timeslice: 30ms */
#define CSCHED_DEFAULT_TSLICE_MS 30
#define CSCHED_CREDITS_PER_MSEC 10
+/* Never set a timer shorter than this value. */
+#define CSCHED_MIN_TIMER XEN_SYSCTL_SCHED_RATELIMIT_MIN
/*
@@ -1811,7 +1813,15 @@ csched_schedule(
snext = scurr;
snext->start_time += now;
perfc_incr(delay_ms);
- tslice = MICROSECS(prv->ratelimit_us);
+ /*
+ * Next timeslice must last just until we'll have executed for
+ * ratelimit_us. However, to avoid setting a really short timer, which
+ * will most likely be inaccurate and counterproductive, we never go
+ * below CSCHED_MIN_TIMER.
+ */
+ tslice = MICROSECS(prv->ratelimit_us) - runtime;
+ if ( unlikely(runtime < CSCHED_MIN_TIMER) )
+ tslice = CSCHED_MIN_TIMER;
ret.migrated = 0;
goto out;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 01/10] xen: credit1: return the 'time remaining to the limit' as next timeslice.
2016-09-30 2:53 ` [PATCH v2 01/10] xen: credit1: return the 'time remaining to the limit' as next timeslice Dario Faggioli
@ 2016-09-30 11:16 ` George Dunlap
0 siblings, 0 replies; 38+ messages in thread
From: George Dunlap @ 2016-09-30 11:16 UTC (permalink / raw)
To: Dario Faggioli, xen-devel; +Cc: George Dunlap
On 30/09/16 03:53, Dario Faggioli wrote:
> If vcpu x has run for 200us, and sched_ratelimit_us is
> 1000us, continue running x _but_ return 1000us-200us as
> the next time slice. This way, next scheduling point will
> happen in 800us, i.e., exactly at the point when x crosses
> the threshold, and can be descheduled (if appropriate).
>
> Right now (without this patch), we're always returning
> sched_ratelimit_us (1000us, in the example above), which
> means we're (potentially) allowing x to run more than
> it should have been able to.
>
> Note that, however, in order to avoid setting timers to very
> short intervals, which is part of the purpose of rate limiting,
> we never use a time slice smaller than a well defined threshold.
> Such threshold (CSCHED_MIN_TIMER defined in this patch) is, in
> general independent from rate limiting, but it looks a good idea
> to set it to the minimum possible ratelimiting value.
>
> 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:
> * introduce CSCHED_MIN_TIMER, as agreed during review.
> ---
> xen/common/sched_credit.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index b63efac..4d84b5f 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -51,6 +51,8 @@
> /* Default timeslice: 30ms */
> #define CSCHED_DEFAULT_TSLICE_MS 30
> #define CSCHED_CREDITS_PER_MSEC 10
> +/* Never set a timer shorter than this value. */
> +#define CSCHED_MIN_TIMER XEN_SYSCTL_SCHED_RATELIMIT_MIN
>
>
> /*
> @@ -1811,7 +1813,15 @@ csched_schedule(
> snext = scurr;
> snext->start_time += now;
> perfc_incr(delay_ms);
> - tslice = MICROSECS(prv->ratelimit_us);
> + /*
> + * Next timeslice must last just until we'll have executed for
> + * ratelimit_us. However, to avoid setting a really short timer, which
> + * will most likely be inaccurate and counterproductive, we never go
> + * below CSCHED_MIN_TIMER.
> + */
> + tslice = MICROSECS(prv->ratelimit_us) - runtime;
> + if ( unlikely(runtime < CSCHED_MIN_TIMER) )
> + tslice = CSCHED_MIN_TIMER;
> ret.migrated = 0;
> goto out;
> }
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 02/10] xen: credit1: don't rate limit context switches in case of yields
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
2016-09-30 2:53 ` [PATCH v2 01/10] xen: credit1: return the 'time remaining to the limit' as next timeslice Dario Faggioli
@ 2016-09-30 2:53 ` Dario Faggioli
2016-09-30 11:18 ` George Dunlap
2016-09-30 2:53 ` [PATCH v2 03/10] xen: credit2: make tickling more deterministic Dario Faggioli
` (8 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:53 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar
Rate limiting has been primarily introduced to avoid too
heavy context switch rate due to interrupts, and, in
general, asynchronous events.
If a vcpu "voluntarily" yields, we really should let it
give up the cpu for a while.
In fact, it may be that it is yielding because it's about
to start spinning, and there's few point in forcing a vcpu
to spin for (potentially) the entire rate-limiting period.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
Changes from v1:
* move this patch up in the series, and remove the Credit2 bits, as suggested
during review;
---
xen/common/sched_credit.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 4d84b5f..5700763 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1802,9 +1802,16 @@ csched_schedule(
* cpu and steal it.
*/
- /* If we have schedule rate limiting enabled, check to see
- * how long we've run for. */
- if ( !tasklet_work_scheduled
+ /*
+ * If we have schedule rate limiting enabled, check to see
+ * how long we've run for.
+ *
+ * If scurr is yielding, however, we don't let rate limiting kick in.
+ * In fact, it may be the case that scurr is about to spin, and there's
+ * no point forcing it to do so until rate limiting expires.
+ */
+ if ( !test_bit(CSCHED_FLAG_VCPU_YIELD, &scurr->flags)
+ && !tasklet_work_scheduled
&& prv->ratelimit_us
&& vcpu_runnable(current)
&& !is_idle_vcpu(current)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 02/10] xen: credit1: don't rate limit context switches in case of yields
2016-09-30 2:53 ` [PATCH v2 02/10] xen: credit1: don't rate limit context switches in case of yields Dario Faggioli
@ 2016-09-30 11:18 ` George Dunlap
0 siblings, 0 replies; 38+ messages in thread
From: George Dunlap @ 2016-09-30 11:18 UTC (permalink / raw)
To: Dario Faggioli, xen-devel; +Cc: George Dunlap, Anshul Makkar
On 30/09/16 03:53, Dario Faggioli wrote:
> Rate limiting has been primarily introduced to avoid too
> heavy context switch rate due to interrupts, and, in
> general, asynchronous events.
>
> If a vcpu "voluntarily" yields, we really should let it
> give up the cpu for a while.
>
> In fact, it may be that it is yielding because it's about
> to start spinning, and there's few point in forcing a vcpu
> to spin for (potentially) the entire rate-limiting period.
>
> 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>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> ---
> Changes from v1:
> * move this patch up in the series, and remove the Credit2 bits, as suggested
> during review;
> ---
> xen/common/sched_credit.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 4d84b5f..5700763 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1802,9 +1802,16 @@ csched_schedule(
> * cpu and steal it.
> */
>
> - /* If we have schedule rate limiting enabled, check to see
> - * how long we've run for. */
> - if ( !tasklet_work_scheduled
> + /*
> + * If we have schedule rate limiting enabled, check to see
> + * how long we've run for.
> + *
> + * If scurr is yielding, however, we don't let rate limiting kick in.
> + * In fact, it may be the case that scurr is about to spin, and there's
> + * no point forcing it to do so until rate limiting expires.
> + */
> + if ( !test_bit(CSCHED_FLAG_VCPU_YIELD, &scurr->flags)
> + && !tasklet_work_scheduled
> && prv->ratelimit_us
> && vcpu_runnable(current)
> && !is_idle_vcpu(current)
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 03/10] xen: credit2: make tickling more deterministic
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
2016-09-30 2:53 ` [PATCH v2 01/10] xen: credit1: return the 'time remaining to the limit' as next timeslice Dario Faggioli
2016-09-30 2:53 ` [PATCH v2 02/10] xen: credit1: don't rate limit context switches in case of yields Dario Faggioli
@ 2016-09-30 2:53 ` Dario Faggioli
2016-09-30 11:25 ` George Dunlap
2016-09-30 2:53 ` [PATCH v2 04/10] xen: credit2: only reset credit on reset condition Dario Faggioli
` (7 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:53 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Anshul Makkar, George Dunlap, Jan Beulich
Right now, the following scenario can occurr:
- upon vcpu v wakeup, v itself is put in the runqueue,
and pcpu X is tickled;
- pcpu Y schedules (for whatever reason), sees v in
the runqueue and picks it up.
This may seem ok (or even a good thing), but it's not.
In fact, if runq_tickle() decided X is where v should
run, it did it for a reason (load distribution, SMT
support, cache hotness, affinity, etc), and we really
should try as hard as possible to stick to that.
Of course, we can't be too strict, or we risk leaving
vcpus in the runqueue while there is available CPU
capacity. So, we only leave v in runqueue --for X to
pick it up-- if we see that X has been tickled and
has not scheduled yet, i.e., it will have a real chance
of actually select and schedule v.
If that is not the case, we schedule it on Y (or, at
least, we consider that), as running somewhere non-ideal
is better than not running at all.
The commit also adds performance counters for each of
the possible situations.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes from v1:
* always initialize tickled_cpu to -1, also for idle vcpus (in which cases, it
just won't ever change to anything else than that), for improved readability
and understandability;
* logic for reporting back to csched_schedule() whether any vcpu was skipped,
within runq_candidate(), and to only reset the credits if that did not
happen moved out from here, to another patch.
---
xen/common/sched_credit2.c | 37 ++++++++++++++++++++++++++++++++++++-
xen/include/xen/perfc_defn.h | 3 +++
2 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 5c7d0dc..3986441 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -54,6 +54,7 @@
#define TRC_CSCHED2_LOAD_CHECK TRC_SCHED_CLASS_EVT(CSCHED2, 16)
#define TRC_CSCHED2_LOAD_BALANCE TRC_SCHED_CLASS_EVT(CSCHED2, 17)
#define TRC_CSCHED2_PICKED_CPU TRC_SCHED_CLASS_EVT(CSCHED2, 19)
+#define TRC_CSCHED2_RUNQ_CANDIDATE TRC_SCHED_CLASS_EVT(CSCHED2, 20)
/*
* WARNING: This is still in an experimental phase. Status and work can be found at the
@@ -398,6 +399,7 @@ struct csched2_vcpu {
int credit;
s_time_t start_time; /* When we were scheduled (used for credit) */
unsigned flags; /* 16 bits doesn't seem to play well with clear_bit() */
+ int tickled_cpu; /* cpu tickled for picking us up (-1 if none) */
/* Individual contribution to load */
s_time_t load_last_update; /* Last time average was updated */
@@ -1049,6 +1051,10 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
__cpumask_set_cpu(ipid, &rqd->tickled);
smt_idle_mask_clear(ipid, &rqd->smt_idle);
cpu_raise_softirq(ipid, SCHEDULE_SOFTIRQ);
+
+ if ( unlikely(new->tickled_cpu != -1) )
+ SCHED_STAT_CRANK(tickled_cpu_overwritten);
+ new->tickled_cpu = ipid;
}
/*
@@ -1276,6 +1282,7 @@ csched2_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
svc->credit = CSCHED2_IDLE_CREDIT;
svc->weight = 0;
}
+ svc->tickled_cpu = -1;
SCHED_STAT_CRANK(vcpu_alloc);
@@ -2268,6 +2275,17 @@ runq_candidate(struct csched2_runqueue_data *rqd,
if ( !cpumask_test_cpu(cpu, svc->vcpu->cpu_hard_affinity) )
continue;
+ /*
+ * If a vcpu is meant to be picked up by another processor, and such
+ * processor has not scheduled yet, leave it in the runqueue for him.
+ */
+ if ( svc->tickled_cpu != -1 && svc->tickled_cpu != cpu &&
+ cpumask_test_cpu(svc->tickled_cpu, &rqd->tickled) )
+ {
+ SCHED_STAT_CRANK(deferred_to_tickled_cpu);
+ continue;
+ }
+
/* If this is on a different processor, don't pull it unless
* its credit is at least CSCHED2_MIGRATE_RESIST higher. */
if ( svc->vcpu->processor != cpu
@@ -2284,9 +2302,25 @@ runq_candidate(struct csched2_runqueue_data *rqd,
/* In any case, if we got this far, break. */
break;
+ }
+ if ( unlikely(tb_init_done) )
+ {
+ struct {
+ unsigned vcpu:16, dom:16;
+ unsigned tickled_cpu;
+ } d;
+ d.dom = snext->vcpu->domain->domain_id;
+ d.vcpu = snext->vcpu->vcpu_id;
+ d.tickled_cpu = snext->tickled_cpu;
+ __trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1,
+ sizeof(d),
+ (unsigned char *)&d);
}
+ if ( unlikely(snext->tickled_cpu != -1 && snext->tickled_cpu != cpu) )
+ SCHED_STAT_CRANK(tickled_cpu_overridden);
+
return snext;
}
@@ -2351,7 +2385,7 @@ csched2_schedule(
snext = CSCHED2_VCPU(idle_vcpu[cpu]);
}
else
- snext=runq_candidate(rqd, scurr, cpu, now);
+ snext = runq_candidate(rqd, scurr, cpu, now);
/* If switching from a non-idle runnable vcpu, put it
* back on the runqueue. */
@@ -2390,6 +2424,7 @@ csched2_schedule(
}
snext->start_time = now;
+ snext->tickled_cpu = -1;
/* Safe because lock for old processor is held */
if ( snext->vcpu->processor != cpu )
diff --git a/xen/include/xen/perfc_defn.h b/xen/include/xen/perfc_defn.h
index a336c71..4a835b8 100644
--- a/xen/include/xen/perfc_defn.h
+++ b/xen/include/xen/perfc_defn.h
@@ -66,6 +66,9 @@ PERFCOUNTER(runtime_max_timer, "csched2: runtime_max_timer")
PERFCOUNTER(migrated, "csched2: migrated")
PERFCOUNTER(migrate_resisted, "csched2: migrate_resisted")
PERFCOUNTER(credit_reset, "csched2: credit_reset")
+PERFCOUNTER(deferred_to_tickled_cpu,"csched2: deferred_to_tickled_cpu")
+PERFCOUNTER(tickled_cpu_overwritten,"csched2: tickled_cpu_overwritten")
+PERFCOUNTER(tickled_cpu_overridden, "csched2: tickled_cpu_overridden")
PERFCOUNTER(need_flush_tlb_flush, "PG_need_flush tlb flushes")
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 03/10] xen: credit2: make tickling more deterministic
2016-09-30 2:53 ` [PATCH v2 03/10] xen: credit2: make tickling more deterministic Dario Faggioli
@ 2016-09-30 11:25 ` George Dunlap
0 siblings, 0 replies; 38+ messages in thread
From: George Dunlap @ 2016-09-30 11:25 UTC (permalink / raw)
To: Dario Faggioli, xen-devel; +Cc: Andrew Cooper, Anshul Makkar, Jan Beulich
On 30/09/16 03:53, Dario Faggioli wrote:
> Right now, the following scenario can occurr:
> - upon vcpu v wakeup, v itself is put in the runqueue,
> and pcpu X is tickled;
> - pcpu Y schedules (for whatever reason), sees v in
> the runqueue and picks it up.
>
> This may seem ok (or even a good thing), but it's not.
> In fact, if runq_tickle() decided X is where v should
> run, it did it for a reason (load distribution, SMT
> support, cache hotness, affinity, etc), and we really
> should try as hard as possible to stick to that.
>
> Of course, we can't be too strict, or we risk leaving
> vcpus in the runqueue while there is available CPU
> capacity. So, we only leave v in runqueue --for X to
> pick it up-- if we see that X has been tickled and
> has not scheduled yet, i.e., it will have a real chance
> of actually select and schedule v.
>
> If that is not the case, we schedule it on Y (or, at
> least, we consider that), as running somewhere non-ideal
> is better than not running at all.
>
> The commit also adds performance counters for each of
> the possible situations.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@citrix.com>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> Changes from v1:
> * always initialize tickled_cpu to -1, also for idle vcpus (in which cases, it
> just won't ever change to anything else than that), for improved readability
> and understandability;
> * logic for reporting back to csched_schedule() whether any vcpu was skipped,
> within runq_candidate(), and to only reset the credits if that did not
> happen moved out from here, to another patch.
> ---
> xen/common/sched_credit2.c | 37 ++++++++++++++++++++++++++++++++++++-
> xen/include/xen/perfc_defn.h | 3 +++
> 2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 5c7d0dc..3986441 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -54,6 +54,7 @@
> #define TRC_CSCHED2_LOAD_CHECK TRC_SCHED_CLASS_EVT(CSCHED2, 16)
> #define TRC_CSCHED2_LOAD_BALANCE TRC_SCHED_CLASS_EVT(CSCHED2, 17)
> #define TRC_CSCHED2_PICKED_CPU TRC_SCHED_CLASS_EVT(CSCHED2, 19)
> +#define TRC_CSCHED2_RUNQ_CANDIDATE TRC_SCHED_CLASS_EVT(CSCHED2, 20)
>
> /*
> * WARNING: This is still in an experimental phase. Status and work can be found at the
> @@ -398,6 +399,7 @@ struct csched2_vcpu {
> int credit;
> s_time_t start_time; /* When we were scheduled (used for credit) */
> unsigned flags; /* 16 bits doesn't seem to play well with clear_bit() */
> + int tickled_cpu; /* cpu tickled for picking us up (-1 if none) */
>
> /* Individual contribution to load */
> s_time_t load_last_update; /* Last time average was updated */
> @@ -1049,6 +1051,10 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now)
> __cpumask_set_cpu(ipid, &rqd->tickled);
> smt_idle_mask_clear(ipid, &rqd->smt_idle);
> cpu_raise_softirq(ipid, SCHEDULE_SOFTIRQ);
> +
> + if ( unlikely(new->tickled_cpu != -1) )
> + SCHED_STAT_CRANK(tickled_cpu_overwritten);
> + new->tickled_cpu = ipid;
> }
>
> /*
> @@ -1276,6 +1282,7 @@ csched2_alloc_vdata(const struct scheduler *ops, struct vcpu *vc, void *dd)
> svc->credit = CSCHED2_IDLE_CREDIT;
> svc->weight = 0;
> }
> + svc->tickled_cpu = -1;
>
> SCHED_STAT_CRANK(vcpu_alloc);
>
> @@ -2268,6 +2275,17 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> if ( !cpumask_test_cpu(cpu, svc->vcpu->cpu_hard_affinity) )
> continue;
>
> + /*
> + * If a vcpu is meant to be picked up by another processor, and such
> + * processor has not scheduled yet, leave it in the runqueue for him.
> + */
> + if ( svc->tickled_cpu != -1 && svc->tickled_cpu != cpu &&
> + cpumask_test_cpu(svc->tickled_cpu, &rqd->tickled) )
> + {
> + SCHED_STAT_CRANK(deferred_to_tickled_cpu);
> + continue;
> + }
> +
> /* If this is on a different processor, don't pull it unless
> * its credit is at least CSCHED2_MIGRATE_RESIST higher. */
> if ( svc->vcpu->processor != cpu
> @@ -2284,9 +2302,25 @@ runq_candidate(struct csched2_runqueue_data *rqd,
>
> /* In any case, if we got this far, break. */
> break;
> + }
>
> + if ( unlikely(tb_init_done) )
> + {
> + struct {
> + unsigned vcpu:16, dom:16;
> + unsigned tickled_cpu;
> + } d;
> + d.dom = snext->vcpu->domain->domain_id;
> + d.vcpu = snext->vcpu->vcpu_id;
> + d.tickled_cpu = snext->tickled_cpu;
> + __trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1,
> + sizeof(d),
> + (unsigned char *)&d);
> }
>
> + if ( unlikely(snext->tickled_cpu != -1 && snext->tickled_cpu != cpu) )
> + SCHED_STAT_CRANK(tickled_cpu_overridden);
> +
> return snext;
> }
>
> @@ -2351,7 +2385,7 @@ csched2_schedule(
> snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> }
> else
> - snext=runq_candidate(rqd, scurr, cpu, now);
> + snext = runq_candidate(rqd, scurr, cpu, now);
>
> /* If switching from a non-idle runnable vcpu, put it
> * back on the runqueue. */
> @@ -2390,6 +2424,7 @@ csched2_schedule(
> }
>
> snext->start_time = now;
> + snext->tickled_cpu = -1;
>
> /* Safe because lock for old processor is held */
> if ( snext->vcpu->processor != cpu )
> diff --git a/xen/include/xen/perfc_defn.h b/xen/include/xen/perfc_defn.h
> index a336c71..4a835b8 100644
> --- a/xen/include/xen/perfc_defn.h
> +++ b/xen/include/xen/perfc_defn.h
> @@ -66,6 +66,9 @@ PERFCOUNTER(runtime_max_timer, "csched2: runtime_max_timer")
> PERFCOUNTER(migrated, "csched2: migrated")
> PERFCOUNTER(migrate_resisted, "csched2: migrate_resisted")
> PERFCOUNTER(credit_reset, "csched2: credit_reset")
> +PERFCOUNTER(deferred_to_tickled_cpu,"csched2: deferred_to_tickled_cpu")
> +PERFCOUNTER(tickled_cpu_overwritten,"csched2: tickled_cpu_overwritten")
> +PERFCOUNTER(tickled_cpu_overridden, "csched2: tickled_cpu_overridden")
>
> PERFCOUNTER(need_flush_tlb_flush, "PG_need_flush tlb flushes")
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 04/10] xen: credit2: only reset credit on reset condition
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
` (2 preceding siblings ...)
2016-09-30 2:53 ` [PATCH v2 03/10] xen: credit2: make tickling more deterministic Dario Faggioli
@ 2016-09-30 2:53 ` Dario Faggioli
2016-09-30 11:28 ` George Dunlap
2016-09-30 12:25 ` anshul makkar
2016-09-30 2:53 ` [PATCH v2 05/10] xen: credit2: implement yield() Dario Faggioli
` (6 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:53 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar
The condition for a Credit2 scheduling epoch coming to an
end is that the vcpu at the front of the runqueue has negative
credits. However, it is possible, that runq_candidate() does
not actually return to the scheduler the first vcpu in the
runqueue (e.g., because such vcpu can't run on the cpu that
is going through the scheduler, because of hard-affinity).
If that happens, we should not trigger a credit reset, or we
risk altering the lenght of a scheduler epoch, wrt what the
original idea of the algorithm was.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
Changes from v1:
* new patch, containing part of what was in patch 5;
* (wrt v1 patch 5) 'pos' parameter to runq_candidate renamed 'skipped', as
requested during review.
---
xen/common/sched_credit2.c | 33 ++++++++++++++++++++++++++++-----
1 file changed, 28 insertions(+), 5 deletions(-)
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 3986441..72e31b5 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -2244,12 +2244,15 @@ void __dump_execstate(void *unused);
static struct csched2_vcpu *
runq_candidate(struct csched2_runqueue_data *rqd,
struct csched2_vcpu *scurr,
- int cpu, s_time_t now)
+ int cpu, s_time_t now,
+ unsigned int *skipped)
{
struct list_head *iter;
struct csched2_vcpu *snext = NULL;
struct csched2_private *prv = CSCHED2_PRIV(per_cpu(scheduler, cpu));
+ *skipped = 0;
+
/* Default to current if runnable, idle otherwise */
if ( vcpu_runnable(scurr->vcpu) )
snext = scurr;
@@ -2273,7 +2276,10 @@ runq_candidate(struct csched2_runqueue_data *rqd,
/* Only consider vcpus that are allowed to run on this processor. */
if ( !cpumask_test_cpu(cpu, svc->vcpu->cpu_hard_affinity) )
+ {
+ (*skipped)++;
continue;
+ }
/*
* If a vcpu is meant to be picked up by another processor, and such
@@ -2282,6 +2288,7 @@ runq_candidate(struct csched2_runqueue_data *rqd,
if ( svc->tickled_cpu != -1 && svc->tickled_cpu != cpu &&
cpumask_test_cpu(svc->tickled_cpu, &rqd->tickled) )
{
+ (*skipped)++;
SCHED_STAT_CRANK(deferred_to_tickled_cpu);
continue;
}
@@ -2291,6 +2298,7 @@ runq_candidate(struct csched2_runqueue_data *rqd,
if ( svc->vcpu->processor != cpu
&& snext->credit + CSCHED2_MIGRATE_RESIST > svc->credit )
{
+ (*skipped)++;
SCHED_STAT_CRANK(migrate_resisted);
continue;
}
@@ -2308,11 +2316,12 @@ runq_candidate(struct csched2_runqueue_data *rqd,
{
struct {
unsigned vcpu:16, dom:16;
- unsigned tickled_cpu;
+ unsigned tickled_cpu, skipped;
} d;
d.dom = snext->vcpu->domain->domain_id;
d.vcpu = snext->vcpu->vcpu_id;
d.tickled_cpu = snext->tickled_cpu;
+ d.skipped = *skipped;
__trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1,
sizeof(d),
(unsigned char *)&d);
@@ -2336,6 +2345,7 @@ csched2_schedule(
struct csched2_runqueue_data *rqd;
struct csched2_vcpu * const scurr = CSCHED2_VCPU(current);
struct csched2_vcpu *snext = NULL;
+ unsigned int skipped_vcpus = 0;
struct task_slice ret;
SCHED_STAT_CRANK(schedule);
@@ -2385,7 +2395,7 @@ csched2_schedule(
snext = CSCHED2_VCPU(idle_vcpu[cpu]);
}
else
- snext = runq_candidate(rqd, scurr, cpu, now);
+ snext = runq_candidate(rqd, scurr, cpu, now, &skipped_vcpus);
/* If switching from a non-idle runnable vcpu, put it
* back on the runqueue. */
@@ -2409,8 +2419,21 @@ csched2_schedule(
__set_bit(__CSFLAG_scheduled, &snext->flags);
}
- /* Check for the reset condition */
- if ( snext->credit <= CSCHED2_CREDIT_RESET )
+ /*
+ * The reset condition is "has a scheduler epoch come to an end?".
+ * The way this is enforced is checking whether the vcpu at the top
+ * of the runqueue has negative credits. This means the epochs have
+ * variable lenght, as in one epoch expores when:
+ * 1) the vcpu at the top of the runqueue has executed for
+ * around 10 ms (with default parameters);
+ * 2) no other vcpu with higher credits wants to run.
+ *
+ * Here, where we want to check for reset, we need to make sure the
+ * proper vcpu is being used. In fact, runqueue_candidate() may have
+ * not returned the first vcpu in the runqueue, for various reasons
+ * (e.g., affinity). Only trigger a reset when it does.
+ */
+ if ( skipped_vcpus == 0 && snext->credit <= CSCHED2_CREDIT_RESET )
{
reset_credit(ops, cpu, now, snext);
balance_load(ops, cpu, now);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 04/10] xen: credit2: only reset credit on reset condition
2016-09-30 2:53 ` [PATCH v2 04/10] xen: credit2: only reset credit on reset condition Dario Faggioli
@ 2016-09-30 11:28 ` George Dunlap
2016-09-30 12:25 ` anshul makkar
1 sibling, 0 replies; 38+ messages in thread
From: George Dunlap @ 2016-09-30 11:28 UTC (permalink / raw)
To: Dario Faggioli, xen-devel; +Cc: George Dunlap, Anshul Makkar
On 30/09/16 03:53, Dario Faggioli wrote:
> The condition for a Credit2 scheduling epoch coming to an
> end is that the vcpu at the front of the runqueue has negative
> credits. However, it is possible, that runq_candidate() does
> not actually return to the scheduler the first vcpu in the
> runqueue (e.g., because such vcpu can't run on the cpu that
> is going through the scheduler, because of hard-affinity).
>
> If that happens, we should not trigger a credit reset, or we
> risk altering the lenght of a scheduler epoch, wrt what the
> original idea of the algorithm was.
>
> 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>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> ---
> Changes from v1:
> * new patch, containing part of what was in patch 5;
> * (wrt v1 patch 5) 'pos' parameter to runq_candidate renamed 'skipped', as
> requested during review.
> ---
> xen/common/sched_credit2.c | 33 ++++++++++++++++++++++++++++-----
> 1 file changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 3986441..72e31b5 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -2244,12 +2244,15 @@ void __dump_execstate(void *unused);
> static struct csched2_vcpu *
> runq_candidate(struct csched2_runqueue_data *rqd,
> struct csched2_vcpu *scurr,
> - int cpu, s_time_t now)
> + int cpu, s_time_t now,
> + unsigned int *skipped)
> {
> struct list_head *iter;
> struct csched2_vcpu *snext = NULL;
> struct csched2_private *prv = CSCHED2_PRIV(per_cpu(scheduler, cpu));
>
> + *skipped = 0;
> +
> /* Default to current if runnable, idle otherwise */
> if ( vcpu_runnable(scurr->vcpu) )
> snext = scurr;
> @@ -2273,7 +2276,10 @@ runq_candidate(struct csched2_runqueue_data *rqd,
>
> /* Only consider vcpus that are allowed to run on this processor. */
> if ( !cpumask_test_cpu(cpu, svc->vcpu->cpu_hard_affinity) )
> + {
> + (*skipped)++;
> continue;
> + }
>
> /*
> * If a vcpu is meant to be picked up by another processor, and such
> @@ -2282,6 +2288,7 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> if ( svc->tickled_cpu != -1 && svc->tickled_cpu != cpu &&
> cpumask_test_cpu(svc->tickled_cpu, &rqd->tickled) )
> {
> + (*skipped)++;
> SCHED_STAT_CRANK(deferred_to_tickled_cpu);
> continue;
> }
> @@ -2291,6 +2298,7 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> if ( svc->vcpu->processor != cpu
> && snext->credit + CSCHED2_MIGRATE_RESIST > svc->credit )
> {
> + (*skipped)++;
> SCHED_STAT_CRANK(migrate_resisted);
> continue;
> }
> @@ -2308,11 +2316,12 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> {
> struct {
> unsigned vcpu:16, dom:16;
> - unsigned tickled_cpu;
> + unsigned tickled_cpu, skipped;
> } d;
> d.dom = snext->vcpu->domain->domain_id;
> d.vcpu = snext->vcpu->vcpu_id;
> d.tickled_cpu = snext->tickled_cpu;
> + d.skipped = *skipped;
> __trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1,
> sizeof(d),
> (unsigned char *)&d);
> @@ -2336,6 +2345,7 @@ csched2_schedule(
> struct csched2_runqueue_data *rqd;
> struct csched2_vcpu * const scurr = CSCHED2_VCPU(current);
> struct csched2_vcpu *snext = NULL;
> + unsigned int skipped_vcpus = 0;
> struct task_slice ret;
>
> SCHED_STAT_CRANK(schedule);
> @@ -2385,7 +2395,7 @@ csched2_schedule(
> snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> }
> else
> - snext = runq_candidate(rqd, scurr, cpu, now);
> + snext = runq_candidate(rqd, scurr, cpu, now, &skipped_vcpus);
>
> /* If switching from a non-idle runnable vcpu, put it
> * back on the runqueue. */
> @@ -2409,8 +2419,21 @@ csched2_schedule(
> __set_bit(__CSFLAG_scheduled, &snext->flags);
> }
>
> - /* Check for the reset condition */
> - if ( snext->credit <= CSCHED2_CREDIT_RESET )
> + /*
> + * The reset condition is "has a scheduler epoch come to an end?".
> + * The way this is enforced is checking whether the vcpu at the top
> + * of the runqueue has negative credits. This means the epochs have
> + * variable lenght, as in one epoch expores when:
> + * 1) the vcpu at the top of the runqueue has executed for
> + * around 10 ms (with default parameters);
> + * 2) no other vcpu with higher credits wants to run.
> + *
> + * Here, where we want to check for reset, we need to make sure the
> + * proper vcpu is being used. In fact, runqueue_candidate() may have
> + * not returned the first vcpu in the runqueue, for various reasons
> + * (e.g., affinity). Only trigger a reset when it does.
> + */
> + if ( skipped_vcpus == 0 && snext->credit <= CSCHED2_CREDIT_RESET )
> {
> reset_credit(ops, cpu, now, snext);
> balance_load(ops, cpu, now);
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 04/10] xen: credit2: only reset credit on reset condition
2016-09-30 2:53 ` [PATCH v2 04/10] xen: credit2: only reset credit on reset condition Dario Faggioli
2016-09-30 11:28 ` George Dunlap
@ 2016-09-30 12:25 ` anshul makkar
2016-09-30 12:57 ` Dario Faggioli
1 sibling, 1 reply; 38+ messages in thread
From: anshul makkar @ 2016-09-30 12:25 UTC (permalink / raw)
To: Dario Faggioli, xen-devel; +Cc: George Dunlap
On 30/09/16 03:53, Dario Faggioli wrote:
> The condition for a Credit2 scheduling epoch coming to an
> end is that the vcpu at the front of the runqueue has negative
> credits. However, it is possible, that runq_candidate() does
> not actually return to the scheduler the first vcpu in the
> runqueue (e.g., because such vcpu can't run on the cpu that
> is going through the scheduler, because of hard-affinity).
>
> If that happens, we should not trigger a credit reset, or we
> risk altering the lenght of a scheduler epoch, wrt what the
> original idea of the algorithm was.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> ---
>
> /*
> * If a vcpu is meant to be picked up by another processor, and such
> @@ -2282,6 +2288,7 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> if ( svc->tickled_cpu != -1 && svc->tickled_cpu != cpu &&
> cpumask_test_cpu(svc->tickled_cpu, &rqd->tickled) )
> {
> + (*skipped)++;
> SCHED_STAT_CRANK(deferred_to_tickled_cpu);
> continue;
> }
> @@ -2291,6 +2298,7 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> if ( svc->vcpu->processor != cpu
> && snext->credit + CSCHED2_MIGRATE_RESIST > svc->credit )
> {
> + (*skipped)++;
> SCHED_STAT_CRANK(migrate_resisted);
> continue;
> }
> @@ -2308,11 +2316,12 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> {
> struct {
> unsigned vcpu:16, dom:16;
> - unsigned tickled_cpu;
> + unsigned tickled_cpu, skipped;
> } d;
> d.dom = snext->vcpu->domain->domain_id;
> d.vcpu = snext->vcpu->vcpu_id;
> d.tickled_cpu = snext->tickled_cpu;
> + d.skipped = *skipped;
> __trace_var(TRC_CSCHED2_RUNQ_CANDIDATE, 1,
> sizeof(d),
> (unsigned char *)&d);
> @@ -2336,6 +2345,7 @@ csched2_schedule(
> struct csched2_runqueue_data *rqd;
> struct csched2_vcpu * const scurr = CSCHED2_VCPU(current);
> struct csched2_vcpu *snext = NULL;
> + unsigned int skipped_vcpus = 0;
> struct task_slice ret;
>
> SCHED_STAT_CRANK(schedule);
> @@ -2385,7 +2395,7 @@ csched2_schedule(
> snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> }
> else
> - snext = runq_candidate(rqd, scurr, cpu, now);
> + snext = runq_candidate(rqd, scurr, cpu, now, &skipped_vcpus);
>
> /* If switching from a non-idle runnable vcpu, put it
> * back on the runqueue. */
> @@ -2409,8 +2419,21 @@ csched2_schedule(
> __set_bit(__CSFLAG_scheduled, &snext->flags);
> }
>
> - /* Check for the reset condition */
> - if ( snext->credit <= CSCHED2_CREDIT_RESET )
> + /*
> + * The reset condition is "has a scheduler epoch come to an end?".
> + * The way this is enforced is checking whether the vcpu at the top
> + * of the runqueue has negative credits. This means the epochs have
> + * variable lenght, as in one epoch expores when:
> + * 1) the vcpu at the top of the runqueue has executed for
> + * around 10 ms (with default parameters);
> + * 2) no other vcpu with higher credits wants to run.
> + *
> + * Here, where we want to check for reset, we need to make sure the
> + * proper vcpu is being used. In fact, runqueue_candidate() may have
> + * not returned the first vcpu in the runqueue, for various reasons
> + * (e.g., affinity). Only trigger a reset when it does.
> + */
> + if ( skipped_vcpus == 0 && snext->credit <= CSCHED2_CREDIT_RESET )
> {
Is there no other way of checking that the returned vcpu from
runqueue_candidate is the first one apart from using "skipped_vcpu"
extra variable. Looks a bit ugly.
> reset_credit(ops, cpu, now, snext);
> balance_load(ops, cpu, now);
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 04/10] xen: credit2: only reset credit on reset condition
2016-09-30 12:25 ` anshul makkar
@ 2016-09-30 12:57 ` Dario Faggioli
0 siblings, 0 replies; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 12:57 UTC (permalink / raw)
To: anshul makkar, xen-devel; +Cc: George Dunlap
[-- Attachment #1.1: Type: text/plain, Size: 4078 bytes --]
On Fri, 2016-09-30 at 13:25 +0100, anshul makkar wrote:
> On 30/09/16 03:53, Dario Faggioli wrote:
> > @@ -2336,6 +2345,7 @@ csched2_schedule(
> > struct csched2_runqueue_data *rqd;
> > struct csched2_vcpu * const scurr = CSCHED2_VCPU(current);
> > struct csched2_vcpu *snext = NULL;
> > + unsigned int skipped_vcpus = 0;
> > struct task_slice ret;
> >
> > SCHED_STAT_CRANK(schedule);
> > @@ -2385,7 +2395,7 @@ csched2_schedule(
> > snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> > }
> > else
> > - snext = runq_candidate(rqd, scurr, cpu, now);
> > + snext = runq_candidate(rqd, scurr, cpu, now,
> > &skipped_vcpus);
> >
> > /* If switching from a non-idle runnable vcpu, put it
> > * back on the runqueue. */
> > @@ -2409,8 +2419,21 @@ csched2_schedule(
> > __set_bit(__CSFLAG_scheduled, &snext->flags);
> > }
> >
> > - /* Check for the reset condition */
> > - if ( snext->credit <= CSCHED2_CREDIT_RESET )
> > + /*
> > + * The reset condition is "has a scheduler epoch come to
> > an end?".
> > + * The way this is enforced is checking whether the vcpu
> > at the top
> > + * of the runqueue has negative credits. This means the
> > epochs have
> > + * variable lenght, as in one epoch expores when:
> > + * 1) the vcpu at the top of the runqueue has executed
> > for
> > + * around 10 ms (with default parameters);
> > + * 2) no other vcpu with higher credits wants to run.
> > + *
> > + * Here, where we want to check for reset, we need to make
> > sure the
> > + * proper vcpu is being used. In fact,
> > runqueue_candidate() may have
> > + * not returned the first vcpu in the runqueue, for
> > various reasons
> > + * (e.g., affinity). Only trigger a reset when it does.
> > + */
> > + if ( skipped_vcpus == 0 && snext->credit <=
> > CSCHED2_CREDIT_RESET )
> > {
> Is there no other way of checking that the returned vcpu from
> runqueue_candidate is the first one apart from using "skipped_vcpu"
> extra variable. Looks a bit ugly.
>
Well, I guess we can try to use list_first_entry() on the runqueue, and
compare it with snext.
However, that "leaks" the bit of information that the runqueue is
implemented as a list, and would break if at some point we decide to
use something different. TBF, this is not a big problem, as we're
inside the scheduler implementation anyway (so not really too big of a
leak), and we'd most likely notice the breakage at build time... but
still.
More important, I don't think that "just" doing that would work. In
fact, if scurr is running, and there is no-one in the runqueue, and
scurr's credits are negative, right now, we reset. So, to catch that
case the condition would need to become something like this:
if ( (list_empty(&RQD(ops, cpu)->runq) ||
list_first_entry(&RQD(ops, cpu)->runq) == snext) &&
snext->credit <= CSCHED2_CREDIT_RESET )
I agree that counting is a bit ugly, but this one here above does not
look much prettier to me.
Also, the number of skipped vcpus, if reported via tracing, does convey
some information on the status of the runqueue. Nothing crucial, but
something that could be useful (at least, it was to me, during
developing and debugging).
So, for these reasons, I'd be inclined to leave this as it is.
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] 38+ messages in thread
* [PATCH v2 05/10] xen: credit2: implement yield()
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
` (3 preceding siblings ...)
2016-09-30 2:53 ` [PATCH v2 04/10] xen: credit2: only reset credit on reset condition Dario Faggioli
@ 2016-09-30 2:53 ` Dario Faggioli
2016-09-30 12:52 ` George Dunlap
2016-09-30 2:54 ` [PATCH v2 06/10] xen: tracing: add trace records for schedule and rate-limiting Dario Faggioli
` (5 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:53 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, Anshul Makkar, Jan Beulich
When a vcpu explicitly yields it is usually giving
us an advice of "let someone else run and come back
to me in a bit."
Credit2 isn't, so far, doing anything when a vcpu
yields, which means an yield is basically a NOP (well,
actually, it's pure overhead, as it causes the scheduler
kick in, but the result is --at least 99% of the time--
that the very same vcpu that yielded continues to run).
Implement a "preempt bias", to be applied to yielding
vcpus. Basically when evaluating what vcpu to run next,
if a vcpu that has just yielded is encountered, we give
it a credit penalty, and check whether there is anyone
else that would better take over the cpu (of course,
if there isn't the yielding vcpu will continue).
The value of this bias can be configured with a boot
time parameter, and the default is set to 1 ms.
Also, add an yield performance counter, and fix the
style of a couple of comments.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes from v1:
* add _us to the parameter name, as suggested during review;
* get rid of the minimum value for the yield bias;
* apply the idle bias via subtraction of credits to the yielding vcpu, rather
than via addition to all the others;
* merge the Credit2 bits of what was patch 7 here, as suggested during review.
---
docs/misc/xen-command-line.markdown | 10 +++++
xen/common/sched_credit2.c | 76 +++++++++++++++++++++++++++--------
xen/common/schedule.c | 2 +
xen/include/xen/perfc_defn.h | 1
4 files changed, 71 insertions(+), 18 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 8ff57fa..4fd3460 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1395,6 +1395,16 @@ Choose the default scheduler.
### sched\_credit2\_migrate\_resist
> `= <integer>`
+### sched\_credit2\_yield\_bias\_us
+> `= <integer>`
+
+> Default: `1000`
+
+Set how much a yielding vcpu will be penalized, in order to actually
+give a chance to run to some other vcpu. This is basically a bias, in
+favour of the non-yielding vcpus, expressed in microseconds (default
+is 1ms).
+
### sched\_credit\_tslice\_ms
> `= <integer>`
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 72e31b5..fde61ef 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -144,6 +144,8 @@
#define CSCHED2_MIGRATE_RESIST ((opt_migrate_resist)*MICROSECS(1))
/* How much to "compensate" a vcpu for L2 migration */
#define CSCHED2_MIGRATE_COMPENSATION MICROSECS(50)
+/* How big of a bias we should have against a yielding vcpu */
+#define CSCHED2_YIELD_BIAS ((opt_yield_bias)*MICROSECS(1))
/* Reset: Value below which credit will be reset. */
#define CSCHED2_CREDIT_RESET 0
/* Max timer: Maximum time a guest can be run for. */
@@ -181,11 +183,20 @@
*/
#define __CSFLAG_runq_migrate_request 3
#define CSFLAG_runq_migrate_request (1<<__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)
static unsigned int __read_mostly opt_migrate_resist = 500;
integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
+static unsigned int __read_mostly opt_yield_bias = 1000;
+integer_param("sched_credit2_yield_bias_us", opt_yield_bias);
+
/*
* Useful macros
*/
@@ -1431,6 +1442,14 @@ out:
}
static void
+csched2_vcpu_yield(const struct scheduler *ops, struct vcpu *v)
+{
+ struct csched2_vcpu * const svc = CSCHED2_VCPU(v);
+
+ __set_bit(__CSFLAG_vcpu_yield, &svc->flags);
+}
+
+static void
csched2_context_saved(const struct scheduler *ops, struct vcpu *vc)
{
struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
@@ -2250,26 +2269,39 @@ 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));
+ /*
+ * If scurr is yielding, temporarily subtract CSCHED2_YIELD_BIAS
+ * credits to it (where "temporarily" means "for the sake of just
+ * this scheduling decision).
+ */
+ int yield_bias = 0, snext_credit;
*skipped = 0;
- /* Default to current if runnable, idle otherwise */
- if ( vcpu_runnable(scurr->vcpu) )
- snext = scurr;
- else
- snext = CSCHED2_VCPU(idle_vcpu[cpu]);
-
/*
* Return the current vcpu if it has executed for less than ratelimit.
* Adjuststment for the selected vcpu's credit and decision
* for how long it will run will be taken in csched2_runtime.
+ *
+ * Note that, if scurr is yielding, we don't let rate limiting kick in.
+ * In fact, it may be the case that scurr is about to spin, and there's
+ * no point forcing it to do so until rate limiting expires.
*/
- if ( prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
- vcpu_runnable(scurr->vcpu) &&
- (now - scurr->vcpu->runstate.state_entry_time) <
- MICROSECS(prv->ratelimit_us) )
+ if ( __test_and_clear_bit(__CSFLAG_vcpu_yield, &scurr->flags) )
+ yield_bias = CSCHED2_YIELD_BIAS;
+ else if ( prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
+ vcpu_runnable(scurr->vcpu) &&
+ (now - scurr->vcpu->runstate.state_entry_time) <
+ MICROSECS(prv->ratelimit_us) )
return scurr;
+ /* Default to current if runnable, idle otherwise */
+ if ( vcpu_runnable(scurr->vcpu) )
+ snext = scurr;
+ else
+ snext = CSCHED2_VCPU(idle_vcpu[cpu]);
+
+ snext_credit = snext->credit - yield_bias;
list_for_each( iter, &rqd->runq )
{
struct csched2_vcpu * svc = list_entry(iter, struct csched2_vcpu, runq_elem);
@@ -2293,19 +2325,23 @@ runq_candidate(struct csched2_runqueue_data *rqd,
continue;
}
- /* If this is on a different processor, don't pull it unless
- * its credit is at least CSCHED2_MIGRATE_RESIST higher. */
+ /*
+ * If this is on a different processor, don't pull it unless
+ * its credit is at least CSCHED2_MIGRATE_RESIST higher.
+ */
if ( svc->vcpu->processor != cpu
- && snext->credit + CSCHED2_MIGRATE_RESIST > svc->credit )
+ && snext_credit + CSCHED2_MIGRATE_RESIST > svc->credit )
{
(*skipped)++;
SCHED_STAT_CRANK(migrate_resisted);
continue;
}
- /* If the next one on the list has more credit than current
- * (or idle, if current is not runnable), choose it. */
- if ( svc->credit > snext->credit )
+ /*
+ * If the next one on the list has more credit than current
+ * (or idle, if current is not runnable), choose it.
+ */
+ if ( svc->credit > snext_credit )
snext = svc;
/* In any case, if we got this far, break. */
@@ -2391,7 +2427,8 @@ csched2_schedule(
*/
if ( tasklet_work_scheduled )
{
- trace_var(TRC_CSCHED2_SCHED_TASKLET, 1, 0, NULL);
+ __clear_bit(__CSFLAG_vcpu_yield, &scurr->flags);
+ trace_var(TRC_CSCHED2_SCHED_TASKLET, 1, 0, NULL);
snext = CSCHED2_VCPU(idle_vcpu[cpu]);
}
else
@@ -2923,6 +2960,8 @@ csched2_init(struct scheduler *ops)
printk(XENLOG_INFO "load tracking window lenght %llu ns\n",
1ULL << opt_load_window_shift);
+ printk(XENLOG_INFO "yield bias value %d us\n", opt_yield_bias);
+
/* Basically no CPU information is available at this point; just
* set up basic structures, and a callback when the CPU info is
* available. */
@@ -2975,6 +3014,7 @@ static const struct scheduler sched_credit2_def = {
.sleep = csched2_vcpu_sleep,
.wake = csched2_vcpu_wake,
+ .yield = csched2_vcpu_yield,
.adjust = csched2_dom_cntl,
.adjust_global = csched2_sys_cntl,
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 104d203..5b444c4 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -947,6 +947,8 @@ long vcpu_yield(void)
SCHED_OP(VCPU2OP(v), yield, v);
vcpu_schedule_unlock_irq(lock, v);
+ SCHED_STAT_CRANK(vcpu_yield);
+
TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
raise_softirq(SCHEDULE_SOFTIRQ);
return 0;
diff --git a/xen/include/xen/perfc_defn.h b/xen/include/xen/perfc_defn.h
index 4a835b8..900fddd 100644
--- a/xen/include/xen/perfc_defn.h
+++ b/xen/include/xen/perfc_defn.h
@@ -23,6 +23,7 @@ PERFCOUNTER(vcpu_alloc, "sched: vcpu_alloc")
PERFCOUNTER(vcpu_insert, "sched: vcpu_insert")
PERFCOUNTER(vcpu_remove, "sched: vcpu_remove")
PERFCOUNTER(vcpu_sleep, "sched: vcpu_sleep")
+PERFCOUNTER(vcpu_yield, "sched: vcpu_yield")
PERFCOUNTER(vcpu_wake_running, "sched: vcpu_wake_running")
PERFCOUNTER(vcpu_wake_onrunq, "sched: vcpu_wake_onrunq")
PERFCOUNTER(vcpu_wake_runnable, "sched: vcpu_wake_runnable")
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 05/10] xen: credit2: implement yield()
2016-09-30 2:53 ` [PATCH v2 05/10] xen: credit2: implement yield() Dario Faggioli
@ 2016-09-30 12:52 ` George Dunlap
2016-09-30 14:01 ` Dario Faggioli
0 siblings, 1 reply; 38+ messages in thread
From: George Dunlap @ 2016-09-30 12:52 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: George Dunlap, Andrew Cooper, Anshul Makkar, Jan Beulich
On 30/09/16 03:53, Dario Faggioli wrote:
> When a vcpu explicitly yields it is usually giving
> us an advice of "let someone else run and come back
> to me in a bit."
>
> Credit2 isn't, so far, doing anything when a vcpu
> yields, which means an yield is basically a NOP (well,
> actually, it's pure overhead, as it causes the scheduler
> kick in, but the result is --at least 99% of the time--
> that the very same vcpu that yielded continues to run).
>
> Implement a "preempt bias", to be applied to yielding
> vcpus. Basically when evaluating what vcpu to run next,
> if a vcpu that has just yielded is encountered, we give
> it a credit penalty, and check whether there is anyone
> else that would better take over the cpu (of course,
> if there isn't the yielding vcpu will continue).
>
> The value of this bias can be configured with a boot
> time parameter, and the default is set to 1 ms.
>
> Also, add an yield performance counter, and fix the
> style of a couple of comments.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Hmm, I'm sorry for not asking this earlier -- but what's the rationale
for having a "yield bias", rather than just choosing the next runnable
guy in the queue on yield, regardless of what his credits are?
If snext has 9ms of credit and the top runnable guy on the runqueue has
7.8ms of credit, doesn't it make sense to run him instead of making
snext run again and burn his credit?
Couldn't this be done by changing yield_bias to a boolean yield, and...
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> Changes from v1:
> * add _us to the parameter name, as suggested during review;
> * get rid of the minimum value for the yield bias;
> * apply the idle bias via subtraction of credits to the yielding vcpu, rather
> than via addition to all the others;
> * merge the Credit2 bits of what was patch 7 here, as suggested during review.
> ---
> docs/misc/xen-command-line.markdown | 10 +++++
> xen/common/sched_credit2.c | 76 +++++++++++++++++++++++++++--------
> xen/common/schedule.c | 2 +
> xen/include/xen/perfc_defn.h | 1
> 4 files changed, 71 insertions(+), 18 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 8ff57fa..4fd3460 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1395,6 +1395,16 @@ Choose the default scheduler.
> ### sched\_credit2\_migrate\_resist
> > `= <integer>`
>
> +### sched\_credit2\_yield\_bias\_us
> +> `= <integer>`
> +
> +> Default: `1000`
> +
> +Set how much a yielding vcpu will be penalized, in order to actually
> +give a chance to run to some other vcpu. This is basically a bias, in
> +favour of the non-yielding vcpus, expressed in microseconds (default
> +is 1ms).
> +
> ### sched\_credit\_tslice\_ms
> > `= <integer>`
>
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 72e31b5..fde61ef 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -144,6 +144,8 @@
> #define CSCHED2_MIGRATE_RESIST ((opt_migrate_resist)*MICROSECS(1))
> /* How much to "compensate" a vcpu for L2 migration */
> #define CSCHED2_MIGRATE_COMPENSATION MICROSECS(50)
> +/* How big of a bias we should have against a yielding vcpu */
> +#define CSCHED2_YIELD_BIAS ((opt_yield_bias)*MICROSECS(1))
> /* Reset: Value below which credit will be reset. */
> #define CSCHED2_CREDIT_RESET 0
> /* Max timer: Maximum time a guest can be run for. */
> @@ -181,11 +183,20 @@
> */
> #define __CSFLAG_runq_migrate_request 3
> #define CSFLAG_runq_migrate_request (1<<__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)
>
> static unsigned int __read_mostly opt_migrate_resist = 500;
> integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
>
> +static unsigned int __read_mostly opt_yield_bias = 1000;
> +integer_param("sched_credit2_yield_bias_us", opt_yield_bias);
> +
> /*
> * Useful macros
> */
> @@ -1431,6 +1442,14 @@ out:
> }
>
> static void
> +csched2_vcpu_yield(const struct scheduler *ops, struct vcpu *v)
> +{
> + struct csched2_vcpu * const svc = CSCHED2_VCPU(v);
> +
> + __set_bit(__CSFLAG_vcpu_yield, &svc->flags);
> +}
> +
> +static void
> csched2_context_saved(const struct scheduler *ops, struct vcpu *vc)
> {
> struct csched2_vcpu * const svc = CSCHED2_VCPU(vc);
> @@ -2250,26 +2269,39 @@ 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));
> + /*
> + * If scurr is yielding, temporarily subtract CSCHED2_YIELD_BIAS
> + * credits to it (where "temporarily" means "for the sake of just
> + * this scheduling decision).
> + */
> + int yield_bias = 0, snext_credit;
>
> *skipped = 0;
>
> - /* Default to current if runnable, idle otherwise */
> - if ( vcpu_runnable(scurr->vcpu) )
> - snext = scurr;
> - else
> - snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> -
> /*
> * Return the current vcpu if it has executed for less than ratelimit.
> * Adjuststment for the selected vcpu's credit and decision
> * for how long it will run will be taken in csched2_runtime.
> + *
> + * Note that, if scurr is yielding, we don't let rate limiting kick in.
> + * In fact, it may be the case that scurr is about to spin, and there's
> + * no point forcing it to do so until rate limiting expires.
> */
> - if ( prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
> - vcpu_runnable(scurr->vcpu) &&
> - (now - scurr->vcpu->runstate.state_entry_time) <
> - MICROSECS(prv->ratelimit_us) )
> + if ( __test_and_clear_bit(__CSFLAG_vcpu_yield, &scurr->flags) )
> + yield_bias = CSCHED2_YIELD_BIAS;
> + else if ( prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
> + vcpu_runnable(scurr->vcpu) &&
> + (now - scurr->vcpu->runstate.state_entry_time) <
> + MICROSECS(prv->ratelimit_us) )
> return scurr;
>
> + /* Default to current if runnable, idle otherwise */
> + if ( vcpu_runnable(scurr->vcpu) )
> + snext = scurr;
> + else
> + snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> +
> + snext_credit = snext->credit - yield_bias;
> list_for_each( iter, &rqd->runq )
> {
> struct csched2_vcpu * svc = list_entry(iter, struct csched2_vcpu, runq_elem);
> @@ -2293,19 +2325,23 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> continue;
> }
>
> - /* If this is on a different processor, don't pull it unless
> - * its credit is at least CSCHED2_MIGRATE_RESIST higher. */
> + /*
> + * If this is on a different processor, don't pull it unless
> + * its credit is at least CSCHED2_MIGRATE_RESIST higher.
> + */
> if ( svc->vcpu->processor != cpu
> - && snext->credit + CSCHED2_MIGRATE_RESIST > svc->credit )
> + && snext_credit + CSCHED2_MIGRATE_RESIST > svc->credit )
> {
> (*skipped)++;
> SCHED_STAT_CRANK(migrate_resisted);
> continue;
> }
>
> - /* If the next one on the list has more credit than current
> - * (or idle, if current is not runnable), choose it. */
> - if ( svc->credit > snext->credit )
> + /*
> + * If the next one on the list has more credit than current
> + * (or idle, if current is not runnable), choose it.
> + */
> + if ( svc->credit > snext_credit )
> snext = svc;
...changing this conditional to the following?
if ( yield || svc->credit > snext->credit )
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 05/10] xen: credit2: implement yield()
2016-09-30 12:52 ` George Dunlap
@ 2016-09-30 14:01 ` Dario Faggioli
0 siblings, 0 replies; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 14:01 UTC (permalink / raw)
To: George Dunlap, xen-devel
Cc: George Dunlap, Andrew Cooper, Anshul Makkar, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 3788 bytes --]
On Fri, 2016-09-30 at 13:52 +0100, George Dunlap wrote:
> On 30/09/16 03:53, Dario Faggioli wrote:
> >
> > When a vcpu explicitly yields it is usually giving
> > us an advice of "let someone else run and come back
> > to me in a bit."
> >
> > Credit2 isn't, so far, doing anything when a vcpu
> > yields, which means an yield is basically a NOP (well,
> > actually, it's pure overhead, as it causes the scheduler
> > kick in, but the result is --at least 99% of the time--
> > that the very same vcpu that yielded continues to run).
> >
> > Implement a "preempt bias", to be applied to yielding
> > vcpus. Basically when evaluating what vcpu to run next,
> > if a vcpu that has just yielded is encountered, we give
> > it a credit penalty, and check whether there is anyone
> > else that would better take over the cpu (of course,
> > if there isn't the yielding vcpu will continue).
> >
> > The value of this bias can be configured with a boot
> > time parameter, and the default is set to 1 ms.
> >
> > Also, add an yield performance counter, and fix the
> > style of a couple of comments.
> >
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
>
> Hmm, I'm sorry for not asking this earlier -- but what's the
> rationale
> for having a "yield bias", rather than just choosing the next
> runnable
> guy in the queue on yield, regardless of what his credits are?
>
Flexibility, I'd say. Flexibility of deciding 'how strong' an yield
would be and, e.g., have two different values for two cpupools (not
possible with just this patch, but not a big deal to do in future).
Sure, if we only think at the spinlock case, that's not really
important (if good at all). OTOH, if you think at yielding as a way of
saying: <<hey, I've still got things to do, and I could go on, but if
there's anyone that has something more important, I'm fine letting him
run for a while>>. Well, this implementation gives you a way of
quantifying that "while".
But of course, more flexibility often means more complexity. And in
this case, rather than complexity in the code, what would be hard is to
come up with a good value for a specific workload.
IOW, right now we have no yield. Instead of adding a "yield switch",
it's implemented as a "yield knob", which has its up and down sides. I
personally like knobs a lot more than switches... But I see the risk of
people starting to turn the knob, expecting wonders, and being
disappointed (and complaining!) if things don't improve for them! :-P
> If snext has 9ms of credit and the top runnable guy on the runqueue
> has
> 7.8ms of credit, doesn't it make sense to run him instead of making
> snext run again and burn his credit?
>
Again, in the one use case for which yield is most popular (the
spinlock one) this that you say totally makes sense. Which makes me
think that, even if we were to keep (or go back to) using the bias, I'd
probably go with a default value higher than 1ms worth of credits.
*ANYWAY*, you asked for a rationale, this is mine. But all this being
said, though, I honestly think the simple solution you're hinting at is
better, at least for now. Also, I've just tried to implement it, and
yes, it works by doing as you suggest here, and the code is simpler.
Therefore, I'm going for that. :-)
I've just seen you have applied most of the series already. I'll send a
v3 consisting only of the remaining patches, with this one modified as
suggested.
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] 38+ messages in thread
* [PATCH v2 06/10] xen: tracing: add trace records for schedule and rate-limiting.
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
` (4 preceding siblings ...)
2016-09-30 2:53 ` [PATCH v2 05/10] xen: credit2: implement yield() Dario Faggioli
@ 2016-09-30 2:54 ` Dario Faggioli
2016-09-30 13:16 ` George Dunlap
2016-10-01 0:18 ` Meng Xu
2016-09-30 2:54 ` [PATCH v2 07/10] tools: tracing: handle more scheduling related events Dario Faggioli
` (4 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:54 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Anshul Makkar, Meng Xu
As far as {csched, csched2, rt}_schedule() are concerned,
an "empty" event, would already make it easier to read and
understand a trace.
But while there, add a few useful information, like
if the cpu that is going through the scheduler has
been tickled or not, if it is currently idle, etc
(they vary, on a per-scheduler basis).
For Credit1 and Credit2, add a record about when
rate-limiting kicks in too.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Meng Xu <mengxu@cis.upenn.edu>
Cc: Anshul Makkar <anshul.makkar@citrix.com>
---
Changes from v1:
* corrected the schedule record for sched_rt.c, as pointed out during review;
* pack the Credit1 records as well, as requested during review.
---
xen/common/sched_credit.c | 32 ++++++++++++++++++++++++++++++++
xen/common/sched_credit2.c | 40 +++++++++++++++++++++++++++++++++++++++-
xen/common/sched_rt.c | 15 +++++++++++++++
3 files changed, 86 insertions(+), 1 deletion(-)
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 5700763..fc3a321 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -133,6 +133,8 @@
#define TRC_CSCHED_TICKLE TRC_SCHED_CLASS_EVT(CSCHED, 6)
#define TRC_CSCHED_BOOST_START TRC_SCHED_CLASS_EVT(CSCHED, 7)
#define TRC_CSCHED_BOOST_END TRC_SCHED_CLASS_EVT(CSCHED, 8)
+#define TRC_CSCHED_SCHEDULE TRC_SCHED_CLASS_EVT(CSCHED, 9)
+#define TRC_CSCHED_RATELIMIT TRC_SCHED_CLASS_EVT(CSCHED, 10)
/*
@@ -1774,6 +1776,23 @@ csched_schedule(
SCHED_STAT_CRANK(schedule);
CSCHED_VCPU_CHECK(current);
+ /*
+ * Here in Credit1 code, we usually just call TRACE_nD() helpers, and
+ * don't care about packing. But scheduling happens very often, so it
+ * actually is important that the record is as small as possible.
+ */
+ if ( unlikely(tb_init_done) )
+ {
+ struct {
+ unsigned cpu:16, tasklet:8, idle:8;
+ } d;
+ d.cpu = cpu;
+ d.tasklet = tasklet_work_scheduled;
+ d.idle = is_idle_vcpu(current);
+ __trace_var(TRC_CSCHED_SCHEDULE, 1, sizeof(d),
+ (unsigned char *)&d);
+ }
+
runtime = now - current->runstate.state_entry_time;
if ( runtime < 0 ) /* Does this ever happen? */
runtime = 0;
@@ -1829,6 +1848,19 @@ csched_schedule(
tslice = MICROSECS(prv->ratelimit_us) - runtime;
if ( unlikely(runtime < CSCHED_MIN_TIMER) )
tslice = CSCHED_MIN_TIMER;
+ if ( unlikely(tb_init_done) )
+ {
+ struct {
+ unsigned vcpu:16, dom:16;
+ unsigned runtime;
+ } d;
+ d.dom = scurr->vcpu->domain->domain_id;
+ d.vcpu = scurr->vcpu->vcpu_id;
+ d.runtime = runtime;
+ __trace_var(TRC_CSCHED_RATELIMIT, 1, sizeof(d),
+ (unsigned char *)&d);
+ }
+
ret.migrated = 0;
goto out;
}
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index fde61ef..5cf3f16 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -55,6 +55,8 @@
#define TRC_CSCHED2_LOAD_BALANCE TRC_SCHED_CLASS_EVT(CSCHED2, 17)
#define TRC_CSCHED2_PICKED_CPU TRC_SCHED_CLASS_EVT(CSCHED2, 19)
#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)
/*
* WARNING: This is still in an experimental phase. Status and work can be found at the
@@ -2288,12 +2290,29 @@ runq_candidate(struct csched2_runqueue_data *rqd,
* no point forcing it to do so until rate limiting expires.
*/
if ( __test_and_clear_bit(__CSFLAG_vcpu_yield, &scurr->flags) )
+ {
yield_bias = CSCHED2_YIELD_BIAS;
+ }
else if ( prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
vcpu_runnable(scurr->vcpu) &&
(now - scurr->vcpu->runstate.state_entry_time) <
MICROSECS(prv->ratelimit_us) )
+ {
+ if ( unlikely(tb_init_done) )
+ {
+ struct {
+ unsigned vcpu:16, dom:16;
+ unsigned runtime;
+ } d;
+ d.dom = scurr->vcpu->domain->domain_id;
+ d.vcpu = scurr->vcpu->vcpu_id;
+ d.runtime = now - scurr->vcpu->runstate.state_entry_time;
+ __trace_var(TRC_CSCHED2_RATELIMIT, 1,
+ sizeof(d),
+ (unsigned char *)&d);
+ }
return scurr;
+ }
/* Default to current if runnable, idle otherwise */
if ( vcpu_runnable(scurr->vcpu) )
@@ -2383,6 +2402,7 @@ csched2_schedule(
struct csched2_vcpu *snext = NULL;
unsigned int skipped_vcpus = 0;
struct task_slice ret;
+ bool_t tickled;
SCHED_STAT_CRANK(schedule);
CSCHED2_VCPU_CHECK(current);
@@ -2397,13 +2417,31 @@ csched2_schedule(
BUG_ON(!is_idle_vcpu(scurr->vcpu) && scurr->rqd != rqd);
/* Clear "tickled" bit now that we've been scheduled */
- if ( cpumask_test_cpu(cpu, &rqd->tickled) )
+ tickled = cpumask_test_cpu(cpu, &rqd->tickled);
+ if ( tickled )
{
__cpumask_clear_cpu(cpu, &rqd->tickled);
cpumask_andnot(cpumask_scratch, &rqd->idle, &rqd->tickled);
smt_idle_mask_set(cpu, cpumask_scratch, &rqd->smt_idle);
}
+ if ( unlikely(tb_init_done) )
+ {
+ struct {
+ unsigned cpu:16, rq_id:16;
+ unsigned tasklet:8, idle:8, smt_idle:8, tickled:8;
+ } d;
+ d.cpu = cpu;
+ d.rq_id = c2r(ops, cpu);
+ d.tasklet = tasklet_work_scheduled;
+ d.idle = is_idle_vcpu(current);
+ d.smt_idle = cpumask_test_cpu(cpu, &rqd->smt_idle);
+ d.tickled = tickled;
+ __trace_var(TRC_CSCHED2_SCHEDULE, 1,
+ sizeof(d),
+ (unsigned char *)&d);
+ }
+
/* Update credits */
burn_credits(rqd, scurr, now);
diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 41c61a7..d95f798 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -160,6 +160,7 @@
#define TRC_RTDS_BUDGET_BURN TRC_SCHED_CLASS_EVT(RTDS, 3)
#define TRC_RTDS_BUDGET_REPLENISH TRC_SCHED_CLASS_EVT(RTDS, 4)
#define TRC_RTDS_SCHED_TASKLET TRC_SCHED_CLASS_EVT(RTDS, 5)
+#define TRC_RTDS_SCHEDULE TRC_SCHED_CLASS_EVT(RTDS, 6)
static void repl_timer_handler(void *data);
@@ -1035,6 +1036,20 @@ rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched
struct rt_vcpu *snext = NULL;
struct task_slice ret = { .migrated = 0 };
+ /* TRACE */
+ {
+ struct __packed {
+ unsigned cpu:16, tasklet:8, tickled:4, idle:4;
+ } d;
+ d.cpu = cpu;
+ d.tasklet = tasklet_work_scheduled;
+ d.tickled = cpumask_test_cpu(cpu, &prv->tickled);
+ d.idle = is_idle_vcpu(current);
+ trace_var(TRC_RTDS_SCHEDULE, 1,
+ sizeof(d),
+ (unsigned char *)&d);
+ }
+
/* clear ticked bit now that we've been scheduled */
cpumask_clear_cpu(cpu, &prv->tickled);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 06/10] xen: tracing: add trace records for schedule and rate-limiting.
2016-09-30 2:54 ` [PATCH v2 06/10] xen: tracing: add trace records for schedule and rate-limiting Dario Faggioli
@ 2016-09-30 13:16 ` George Dunlap
2016-10-01 0:18 ` Meng Xu
1 sibling, 0 replies; 38+ messages in thread
From: George Dunlap @ 2016-09-30 13:16 UTC (permalink / raw)
To: Dario Faggioli, xen-devel; +Cc: George Dunlap, Anshul Makkar, Meng Xu
On 30/09/16 03:54, Dario Faggioli wrote:
> As far as {csched, csched2, rt}_schedule() are concerned,
> an "empty" event, would already make it easier to read and
> understand a trace.
>
> But while there, add a few useful information, like
> if the cpu that is going through the scheduler has
> been tickled or not, if it is currently idle, etc
> (they vary, on a per-scheduler basis).
>
> For Credit1 and Credit2, add a record about when
> rate-limiting kicks in too.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Meng Xu <mengxu@cis.upenn.edu>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> ---
> Changes from v1:
> * corrected the schedule record for sched_rt.c, as pointed out during review;
> * pack the Credit1 records as well, as requested during review.
> ---
> xen/common/sched_credit.c | 32 ++++++++++++++++++++++++++++++++
> xen/common/sched_credit2.c | 40 +++++++++++++++++++++++++++++++++++++++-
> xen/common/sched_rt.c | 15 +++++++++++++++
> 3 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 5700763..fc3a321 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -133,6 +133,8 @@
> #define TRC_CSCHED_TICKLE TRC_SCHED_CLASS_EVT(CSCHED, 6)
> #define TRC_CSCHED_BOOST_START TRC_SCHED_CLASS_EVT(CSCHED, 7)
> #define TRC_CSCHED_BOOST_END TRC_SCHED_CLASS_EVT(CSCHED, 8)
> +#define TRC_CSCHED_SCHEDULE TRC_SCHED_CLASS_EVT(CSCHED, 9)
> +#define TRC_CSCHED_RATELIMIT TRC_SCHED_CLASS_EVT(CSCHED, 10)
>
>
> /*
> @@ -1774,6 +1776,23 @@ csched_schedule(
> SCHED_STAT_CRANK(schedule);
> CSCHED_VCPU_CHECK(current);
>
> + /*
> + * Here in Credit1 code, we usually just call TRACE_nD() helpers, and
> + * don't care about packing. But scheduling happens very often, so it
> + * actually is important that the record is as small as possible.
> + */
> + if ( unlikely(tb_init_done) )
> + {
> + struct {
> + unsigned cpu:16, tasklet:8, idle:8;
> + } d;
> + d.cpu = cpu;
> + d.tasklet = tasklet_work_scheduled;
> + d.idle = is_idle_vcpu(current);
> + __trace_var(TRC_CSCHED_SCHEDULE, 1, sizeof(d),
> + (unsigned char *)&d);
> + }
> +
> runtime = now - current->runstate.state_entry_time;
> if ( runtime < 0 ) /* Does this ever happen? */
> runtime = 0;
> @@ -1829,6 +1848,19 @@ csched_schedule(
> tslice = MICROSECS(prv->ratelimit_us) - runtime;
> if ( unlikely(runtime < CSCHED_MIN_TIMER) )
> tslice = CSCHED_MIN_TIMER;
> + if ( unlikely(tb_init_done) )
> + {
> + struct {
> + unsigned vcpu:16, dom:16;
> + unsigned runtime;
> + } d;
> + d.dom = scurr->vcpu->domain->domain_id;
> + d.vcpu = scurr->vcpu->vcpu_id;
> + d.runtime = runtime;
> + __trace_var(TRC_CSCHED_RATELIMIT, 1, sizeof(d),
> + (unsigned char *)&d);
> + }
> +
> ret.migrated = 0;
> goto out;
> }
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index fde61ef..5cf3f16 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -55,6 +55,8 @@
> #define TRC_CSCHED2_LOAD_BALANCE TRC_SCHED_CLASS_EVT(CSCHED2, 17)
> #define TRC_CSCHED2_PICKED_CPU TRC_SCHED_CLASS_EVT(CSCHED2, 19)
> #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)
>
> /*
> * WARNING: This is still in an experimental phase. Status and work can be found at the
> @@ -2288,12 +2290,29 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> * no point forcing it to do so until rate limiting expires.
> */
> if ( __test_and_clear_bit(__CSFLAG_vcpu_yield, &scurr->flags) )
> + {
> yield_bias = CSCHED2_YIELD_BIAS;
> + }
> else if ( prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
> vcpu_runnable(scurr->vcpu) &&
> (now - scurr->vcpu->runstate.state_entry_time) <
> MICROSECS(prv->ratelimit_us) )
> + {
> + if ( unlikely(tb_init_done) )
> + {
> + struct {
> + unsigned vcpu:16, dom:16;
> + unsigned runtime;
> + } d;
> + d.dom = scurr->vcpu->domain->domain_id;
> + d.vcpu = scurr->vcpu->vcpu_id;
> + d.runtime = now - scurr->vcpu->runstate.state_entry_time;
> + __trace_var(TRC_CSCHED2_RATELIMIT, 1,
> + sizeof(d),
> + (unsigned char *)&d);
> + }
> return scurr;
> + }
>
> /* Default to current if runnable, idle otherwise */
> if ( vcpu_runnable(scurr->vcpu) )
> @@ -2383,6 +2402,7 @@ csched2_schedule(
> struct csched2_vcpu *snext = NULL;
> unsigned int skipped_vcpus = 0;
> struct task_slice ret;
> + bool_t tickled;
>
> SCHED_STAT_CRANK(schedule);
> CSCHED2_VCPU_CHECK(current);
> @@ -2397,13 +2417,31 @@ csched2_schedule(
> BUG_ON(!is_idle_vcpu(scurr->vcpu) && scurr->rqd != rqd);
>
> /* Clear "tickled" bit now that we've been scheduled */
> - if ( cpumask_test_cpu(cpu, &rqd->tickled) )
> + tickled = cpumask_test_cpu(cpu, &rqd->tickled);
> + if ( tickled )
> {
> __cpumask_clear_cpu(cpu, &rqd->tickled);
> cpumask_andnot(cpumask_scratch, &rqd->idle, &rqd->tickled);
> smt_idle_mask_set(cpu, cpumask_scratch, &rqd->smt_idle);
> }
>
> + if ( unlikely(tb_init_done) )
> + {
> + struct {
> + unsigned cpu:16, rq_id:16;
> + unsigned tasklet:8, idle:8, smt_idle:8, tickled:8;
> + } d;
> + d.cpu = cpu;
> + d.rq_id = c2r(ops, cpu);
> + d.tasklet = tasklet_work_scheduled;
> + d.idle = is_idle_vcpu(current);
> + d.smt_idle = cpumask_test_cpu(cpu, &rqd->smt_idle);
> + d.tickled = tickled;
> + __trace_var(TRC_CSCHED2_SCHEDULE, 1,
> + sizeof(d),
> + (unsigned char *)&d);
> + }
> +
> /* Update credits */
> burn_credits(rqd, scurr, now);
>
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 41c61a7..d95f798 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -160,6 +160,7 @@
> #define TRC_RTDS_BUDGET_BURN TRC_SCHED_CLASS_EVT(RTDS, 3)
> #define TRC_RTDS_BUDGET_REPLENISH TRC_SCHED_CLASS_EVT(RTDS, 4)
> #define TRC_RTDS_SCHED_TASKLET TRC_SCHED_CLASS_EVT(RTDS, 5)
> +#define TRC_RTDS_SCHEDULE TRC_SCHED_CLASS_EVT(RTDS, 6)
>
> static void repl_timer_handler(void *data);
>
> @@ -1035,6 +1036,20 @@ rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched
> struct rt_vcpu *snext = NULL;
> struct task_slice ret = { .migrated = 0 };
>
> + /* TRACE */
> + {
> + struct __packed {
> + unsigned cpu:16, tasklet:8, tickled:4, idle:4;
> + } d;
> + d.cpu = cpu;
> + d.tasklet = tasklet_work_scheduled;
> + d.tickled = cpumask_test_cpu(cpu, &prv->tickled);
> + d.idle = is_idle_vcpu(current);
> + trace_var(TRC_RTDS_SCHEDULE, 1,
> + sizeof(d),
> + (unsigned char *)&d);
> + }
> +
> /* clear ticked bit now that we've been scheduled */
> cpumask_clear_cpu(cpu, &prv->tickled);
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 06/10] xen: tracing: add trace records for schedule and rate-limiting.
2016-09-30 2:54 ` [PATCH v2 06/10] xen: tracing: add trace records for schedule and rate-limiting Dario Faggioli
2016-09-30 13:16 ` George Dunlap
@ 2016-10-01 0:18 ` Meng Xu
1 sibling, 0 replies; 38+ messages in thread
From: Meng Xu @ 2016-10-01 0:18 UTC (permalink / raw)
To: Dario Faggioli
Cc: George Dunlap, xen-devel@lists.xenproject.org, Anshul Makkar
On Thu, Sep 29, 2016 at 10:54 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> As far as {csched, csched2, rt}_schedule() are concerned,
> an "empty" event, would already make it easier to read and
> understand a trace.
>
> But while there, add a few useful information, like
> if the cpu that is going through the scheduler has
> been tickled or not, if it is currently idle, etc
> (they vary, on a per-scheduler basis).
>
> For Credit1 and Credit2, add a record about when
> rate-limiting kicks in too.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Meng Xu <mengxu@cis.upenn.edu>
> Cc: Anshul Makkar <anshul.makkar@citrix.com>
> ---
> Changes from v1:
> * corrected the schedule record for sched_rt.c, as pointed out during review;
> * pack the Credit1 records as well, as requested during review.
> ---
> xen/common/sched_credit.c | 32 ++++++++++++++++++++++++++++++++
> xen/common/sched_credit2.c | 40 +++++++++++++++++++++++++++++++++++++++-
> xen/common/sched_rt.c | 15 +++++++++++++++
> 3 files changed, 86 insertions(+), 1 deletion(-)
>
As to sched_rt.c,
Reviewed-by: Meng Xu <mengxu@cis.upenn.edu>
Thanks,
Meng
-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 07/10] tools: tracing: handle more scheduling related events.
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
` (5 preceding siblings ...)
2016-09-30 2:54 ` [PATCH v2 06/10] xen: tracing: add trace records for schedule and rate-limiting Dario Faggioli
@ 2016-09-30 2:54 ` Dario Faggioli
2016-09-30 10:22 ` Ian Jackson
2016-09-30 2:54 ` [PATCH v2 08/10] libxl: fix coding style of credit1 parameters related functions Dario Faggioli
` (3 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:54 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Wei Liu, Ian Jackson
There are some scheduling related trace records that
are not being taken care of (and hence only dumped as
raw records).
Some of them are being introduced in this series, while
other were just neglected by previous patches.
Add support for them.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes from v1:
* only the one made necessary by the packing done to Credit1 records. Those
were requested by George himself, and the effect on this patch is small,
and purely mechanic, so I decided to keep his Ack.
---
tools/xentrace/formats | 8 ++++
tools/xentrace/xenalyze.c | 101 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 109 insertions(+)
diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index 0de7990..db89f92 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -42,6 +42,10 @@
0x00022004 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:stolen_vcpu [ dom:vcpu = 0x%(2)04x%(3)04x, from = %(1)d ]
0x00022005 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:picked_cpu [ dom:vcpu = 0x%(1)04x%(2)04x, cpu = %(3)d ]
0x00022006 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:tickle [ cpu = %(1)d ]
+0x00022007 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:boost [ dom:vcpu = 0x%(1)04x%(2)04x ]
+0x00022008 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:unboost [ dom:vcpu = 0x%(1)04x%(2)04x ]
+0x00022009 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:schedule [ cpu[16]:tasklet[8]:idle[8] = %(1)08x ]
+0x0002200A CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched:ratelimit [ dom:vcpu = 0x%(1)08x, runtime = %(2)d ]
0x00022201 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:tick
0x00022202 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) csched2:runq_pos [ dom:vcpu = 0x%(1)08x, pos = %(2)d]
@@ -61,12 +65,16 @@
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 ]
+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 ]
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 ]
0x00022803 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:burn_budget [ dom:vcpu = 0x%(1)08x, cur_budget = 0x%(3)08x%(2)08x, delta = %(4)d ]
0x00022804 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:repl_budget [ dom:vcpu = 0x%(1)08x, cur_deadline = 0x%(3)08x%(2)08x, cur_budget = 0x%(5)08x%(4)08x ]
0x00022805 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:sched_tasklet
+0x00022806 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) rtds:schedule [ cpu[16]:tasklet[8]:idle[4]:tickled[4] = %(1)08x ]
0x00041001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) domain_create [ dom = 0x%(1)08x ]
0x00041002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) domain_destroy [ dom = 0x%(1)08x ]
diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index 0b697d0..f006804 100644
--- a/tools/xentrace/xenalyze.c
+++ b/tools/xentrace/xenalyze.c
@@ -7590,6 +7590,50 @@ void sched_process(struct pcpu_info *p)
ri->dump_header, r->cpu);
}
break;
+ case TRC_SCHED_CLASS_EVT(CSCHED, 7): /* BOOST_START */
+ if(opt.dump_all) {
+ struct {
+ unsigned int domid, vcpuid;
+ } *r = (typeof(r))ri->d;
+
+ printf(" %s csched: d%uv%u boosted\n",
+ ri->dump_header, r->domid, r->vcpuid);
+ }
+ break;
+ case TRC_SCHED_CLASS_EVT(CSCHED, 8): /* BOOST_END */
+ if(opt.dump_all) {
+ struct {
+ unsigned int domid, vcpuid;
+ } *r = (typeof(r))ri->d;
+
+ printf(" %s csched: d%uv%u unboosted\n",
+ ri->dump_header, r->domid, r->vcpuid);
+ }
+ break;
+ case TRC_SCHED_CLASS_EVT(CSCHED, 9): /* SCHEDULE */
+ if(opt.dump_all) {
+ struct {
+ unsigned int cpu:16, tasklet:8, idle:8;
+ } *r = (typeof(r))ri->d;
+
+ printf(" %s csched:schedule cpu %u, %s%s\n",
+ ri->dump_header, r->cpu,
+ r->tasklet ? ", tasklet scheduled" : "",
+ r->idle ? ", idle" : ", busy");
+ }
+ break;
+ case TRC_SCHED_CLASS_EVT(CSCHED, 10): /* RATELIMIT */
+ if(opt.dump_all) {
+ struct {
+ unsigned int vcpuid:16, domid:16;
+ unsigned int runtime;
+ } *r = (typeof(r))ri->d;
+
+ printf(" %s csched:ratelimit, d%uv%u run only %u.%uus\n",
+ ri->dump_header, r->domid, r->vcpuid,
+ r->runtime / 1000, r->runtime % 1000);
+ }
+ break;
/* CREDIT 2 (TRC_CSCHED2_xxx) */
case TRC_SCHED_CLASS_EVT(CSCHED2, 1): /* TICK */
case TRC_SCHED_CLASS_EVT(CSCHED2, 4): /* CREDIT_ADD */
@@ -7776,6 +7820,50 @@ void sched_process(struct pcpu_info *p)
ri->dump_header, r->domid, r->vcpuid, r->rqi, r->cpu);
}
break;
+ case TRC_SCHED_CLASS_EVT(CSCHED2, 20): /* RUNQ_CANDIDATE */
+ if (opt.dump_all) {
+ struct {
+ unsigned vcpuid:16, domid:16;
+ unsigned tickled_cpu, skipped;
+ } *r = (typeof(r))ri->d;
+
+ printf(" %s csched2:runq_candidate d%uv%u, "
+ "%u vcpus skipped, ",
+ ri->dump_header, r->domid, r->vcpuid,
+ r->skipped);
+ if (r->tickled_cpu == (unsigned)-1)
+ printf("no cpu was tickled\n");
+ else
+ printf("cpu %u was tickled\n", r->tickled_cpu);
+ }
+ break;
+ case TRC_SCHED_CLASS_EVT(CSCHED2, 21): /* SCHEDULE */
+ if (opt.dump_all) {
+ struct {
+ unsigned cpu:16, rqi:16;
+ unsigned tasklet:8, idle:8, smt_idle:8, tickled:8;
+ } *r = (typeof(r))ri->d;
+
+ printf(" %s csched2:schedule cpu %u, rq# %u%s%s%s%s\n",
+ ri->dump_header, r->cpu, r->rqi,
+ r->tasklet ? ", tasklet scheduled" : "",
+ r->idle ? ", idle" : ", busy",
+ r->idle ? (r->smt_idle ? ", SMT idle" : ", SMT busy") : "",
+ r->tickled ? ", tickled" : ", not tickled");
+ }
+ break;
+ case TRC_SCHED_CLASS_EVT(CSCHED2, 22): /* RATELIMIT */
+ if (opt.dump_all) {
+ struct {
+ unsigned int vcpuid:16, domid:16;
+ unsigned int runtime;
+ } *r = (typeof(r))ri->d;
+
+ printf(" %s csched2:ratelimit, d%uv%u run only %u.%uus\n",
+ ri->dump_header, r->domid, r->vcpuid,
+ r->runtime / 1000, r->runtime % 1000);
+ }
+ break;
/* RTDS (TRC_RTDS_xxx) */
case TRC_SCHED_CLASS_EVT(RTDS, 1): /* TICKLE */
if(opt.dump_all) {
@@ -7828,6 +7916,19 @@ void sched_process(struct pcpu_info *p)
if(opt.dump_all)
printf(" %s rtds:sched_tasklet\n", ri->dump_header);
break;
+ case TRC_SCHED_CLASS_EVT(RTDS, 6): /* SCHEDULE */
+ if (opt.dump_all) {
+ struct {
+ unsigned cpu:16, tasklet:8, idle:4, tickled:4;
+ } __attribute__((packed)) *r = (typeof(r))ri->d;
+
+ printf(" %s rtds:schedule cpu %u, %s%s%s\n",
+ ri->dump_header, r->cpu,
+ r->tasklet ? ", tasklet scheduled" : "",
+ r->idle ? ", idle" : ", busy",
+ r->tickled ? ", tickled" : ", not tickled");
+ }
+ break;
default:
process_generic(ri);
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 07/10] tools: tracing: handle more scheduling related events.
2016-09-30 2:54 ` [PATCH v2 07/10] tools: tracing: handle more scheduling related events Dario Faggioli
@ 2016-09-30 10:22 ` Ian Jackson
0 siblings, 0 replies; 38+ messages in thread
From: Ian Jackson @ 2016-09-30 10:22 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, xen-devel, Wei Liu
Dario Faggioli writes ("[PATCH v2 07/10] tools: tracing: handle more scheduling related events."):
> There are some scheduling related trace records that
> are not being taken care of (and hence only dumped as
> raw records).
>
> Some of them are being introduced in this series, while
> other were just neglected by previous patches.
>
> Add support for them.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
FAOD I think this is fine with George's ack.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 08/10] libxl: fix coding style of credit1 parameters related functions
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
` (6 preceding siblings ...)
2016-09-30 2:54 ` [PATCH v2 07/10] tools: tracing: handle more scheduling related events Dario Faggioli
@ 2016-09-30 2:54 ` Dario Faggioli
2016-09-30 10:24 ` Ian Jackson
2016-09-30 2:54 ` [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2 Dario Faggioli
` (2 subsequent siblings)
10 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:54 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Wei Liu, Ian Jackson
More specifically, the the error handling path is
made compliant with libxl's codying style.
No functional change intended.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes from v1:
* new patch, containing only the coding style changes from what was patch 14
in v1.
---
tools/libxl/libxl.c | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 997d94c..606d71a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5233,65 +5233,68 @@ int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo)
{
struct xen_sysctl_credit_schedule sparam;
- int rc;
+ int r, rc;
GC_INIT(ctx);
- rc = xc_sched_credit_params_get(ctx->xch, poolid, &sparam);
- if (rc != 0) {
- LOGE(ERROR, "getting sched credit param");
- GC_FREE;
- return ERROR_FAIL;
+ r = xc_sched_credit_params_get(ctx->xch, poolid, &sparam);
+ if (r < 0) {
+ LOGE(ERROR, "getting Credit scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
}
scinfo->tslice_ms = sparam.tslice_ms;
scinfo->ratelimit_us = sparam.ratelimit_us;
+ rc = 0;
+ out:
GC_FREE;
- return 0;
+ return rc;
}
int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo)
{
struct xen_sysctl_credit_schedule sparam;
- int rc=0;
+ int r, rc;
GC_INIT(ctx);
if (scinfo->tslice_ms < XEN_SYSCTL_CSCHED_TSLICE_MIN
|| scinfo->tslice_ms > XEN_SYSCTL_CSCHED_TSLICE_MAX) {
LOG(ERROR, "Time slice out of range, valid range is from %d to %d",
XEN_SYSCTL_CSCHED_TSLICE_MIN, XEN_SYSCTL_CSCHED_TSLICE_MAX);
- GC_FREE;
- return ERROR_INVAL;
+ rc = ERROR_INVAL;
+ goto out;
}
if (scinfo->ratelimit_us < XEN_SYSCTL_SCHED_RATELIMIT_MIN
|| scinfo->ratelimit_us > XEN_SYSCTL_SCHED_RATELIMIT_MAX) {
LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
- GC_FREE;
- return ERROR_INVAL;
+ rc = ERROR_INVAL;
+ goto out;
}
if (scinfo->ratelimit_us > scinfo->tslice_ms*1000) {
LOG(ERROR, "Ratelimit cannot be greater than timeslice");
- GC_FREE;
- return ERROR_INVAL;
+ rc = ERROR_INVAL;
+ goto out;
}
sparam.tslice_ms = scinfo->tslice_ms;
sparam.ratelimit_us = scinfo->ratelimit_us;
- rc = xc_sched_credit_params_set(ctx->xch, poolid, &sparam);
- if ( rc < 0 ) {
- LOGE(ERROR, "setting sched credit param");
- GC_FREE;
- return ERROR_FAIL;
+ r = xc_sched_credit_params_set(ctx->xch, poolid, &sparam);
+ if ( r < 0 ) {
+ LOGE(ERROR, "Setting Credit scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
}
scinfo->tslice_ms = sparam.tslice_ms;
scinfo->ratelimit_us = sparam.ratelimit_us;
+ out:
GC_FREE;
- return 0;
+ return rc;
}
static int sched_credit2_domain_get(libxl__gc *gc, uint32_t domid,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 08/10] libxl: fix coding style of credit1 parameters related functions
2016-09-30 2:54 ` [PATCH v2 08/10] libxl: fix coding style of credit1 parameters related functions Dario Faggioli
@ 2016-09-30 10:24 ` Ian Jackson
2016-09-30 12:04 ` Dario Faggioli
0 siblings, 1 reply; 38+ messages in thread
From: Ian Jackson @ 2016-09-30 10:24 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, xen-devel, Ian Jackson, Wei Liu
Dario Faggioli writes ("[PATCH v2 08/10] libxl: fix coding style of credit1 parameters related functions"):
> More specifically, the the error handling path is
> made compliant with libxl's codying style.
...
> int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
> libxl_sched_credit_params *scinfo)
> {
> struct xen_sysctl_credit_schedule sparam;
> - int rc=0;
> + int r, rc;
...
>
> scinfo->tslice_ms = sparam.tslice_ms;
> scinfo->ratelimit_us = sparam.ratelimit_us;
>
> + out:
> GC_FREE;
> - return 0;
> + return rc;
I think this is missing an assignment
rc = 0;
on the successful exit path, just before out. Am I wrong ?
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 08/10] libxl: fix coding style of credit1 parameters related functions
2016-09-30 10:24 ` Ian Jackson
@ 2016-09-30 12:04 ` Dario Faggioli
2016-09-30 13:25 ` George Dunlap
0 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 12:04 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, xen-devel, Wei Liu
[-- Attachment #1.1.1: Type: text/plain, Size: 1364 bytes --]
On Fri, 2016-09-30 at 11:24 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH v2 08/10] libxl: fix coding style of
> credit1 parameters related functions"):
> > int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
> > libxl_sched_credit_params
> > *scinfo)
> > {
> > struct xen_sysctl_credit_schedule sparam;
> > - int rc=0;
> > + int r, rc;
> ...
> >
> >
> > scinfo->tslice_ms = sparam.tslice_ms;
> > scinfo->ratelimit_us = sparam.ratelimit_us;
> >
> > + out:
> > GC_FREE;
> > - return 0;
> > + return rc;
>
> I think this is missing an assignment
>
> rc = 0;
>
> on the successful exit path, just before out. Am I wrong ?
>
Indeed it's missing. It was not necessary in v1 of this patch, so I
must have failed to notice that it was, when splitting that in two.
Sorry.
Not sure how to proceed, so I'm attaching an updated version of the
patch to this email.
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.1.2: libxl-fix-coding-style-sched-param.patch --]
[-- Type: text/x-patch, Size: 3487 bytes --]
From: Dario Faggioli <dario.faggioli@citrix.com>
libxl: fix coding style of credit1 parameters related functions
More specifically, the the error handling path is
made compliant with libxl's codying style.
No functional change intended.
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes from v2:
* add a missing 'rc = 0', as noted during review.
Changes from v1:
* new patch, containing only the coding style changes from what was patch 14
in v1.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index a46b827..d2552f9 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5260,65 +5260,69 @@ int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo)
{
struct xen_sysctl_credit_schedule sparam;
- int rc;
+ int r, rc;
GC_INIT(ctx);
- rc = xc_sched_credit_params_get(ctx->xch, poolid, &sparam);
- if (rc != 0) {
- LOGE(ERROR, "getting sched credit param");
- GC_FREE;
- return ERROR_FAIL;
+ r = xc_sched_credit_params_get(ctx->xch, poolid, &sparam);
+ if (r < 0) {
+ LOGE(ERROR, "getting Credit scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
}
scinfo->tslice_ms = sparam.tslice_ms;
scinfo->ratelimit_us = sparam.ratelimit_us;
+ rc = 0;
+ out:
GC_FREE;
- return 0;
+ return rc;
}
int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo)
{
struct xen_sysctl_credit_schedule sparam;
- int rc=0;
+ int r, rc;
GC_INIT(ctx);
if (scinfo->tslice_ms < XEN_SYSCTL_CSCHED_TSLICE_MIN
|| scinfo->tslice_ms > XEN_SYSCTL_CSCHED_TSLICE_MAX) {
LOG(ERROR, "Time slice out of range, valid range is from %d to %d",
XEN_SYSCTL_CSCHED_TSLICE_MIN, XEN_SYSCTL_CSCHED_TSLICE_MAX);
- GC_FREE;
- return ERROR_INVAL;
+ rc = ERROR_INVAL;
+ goto out;
}
if (scinfo->ratelimit_us < XEN_SYSCTL_SCHED_RATELIMIT_MIN
|| scinfo->ratelimit_us > XEN_SYSCTL_SCHED_RATELIMIT_MAX) {
LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
- GC_FREE;
- return ERROR_INVAL;
+ rc = ERROR_INVAL;
+ goto out;
}
if (scinfo->ratelimit_us > scinfo->tslice_ms*1000) {
LOG(ERROR, "Ratelimit cannot be greater than timeslice");
- GC_FREE;
- return ERROR_INVAL;
+ rc = ERROR_INVAL;
+ goto out;
}
sparam.tslice_ms = scinfo->tslice_ms;
sparam.ratelimit_us = scinfo->ratelimit_us;
- rc = xc_sched_credit_params_set(ctx->xch, poolid, &sparam);
- if ( rc < 0 ) {
- LOGE(ERROR, "setting sched credit param");
- GC_FREE;
- return ERROR_FAIL;
+ r = xc_sched_credit_params_set(ctx->xch, poolid, &sparam);
+ if ( r < 0 ) {
+ LOGE(ERROR, "Setting Credit scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
}
scinfo->tslice_ms = sparam.tslice_ms;
scinfo->ratelimit_us = sparam.ratelimit_us;
+ rc = 0;
+ out:
GC_FREE;
- return 0;
+ return rc;
}
static int sched_credit2_domain_get(libxl__gc *gc, uint32_t domid,
[-- 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 related [flat|nested] 38+ messages in thread* Re: [PATCH v2 08/10] libxl: fix coding style of credit1 parameters related functions
2016-09-30 12:04 ` Dario Faggioli
@ 2016-09-30 13:25 ` George Dunlap
0 siblings, 0 replies; 38+ messages in thread
From: George Dunlap @ 2016-09-30 13:25 UTC (permalink / raw)
To: Dario Faggioli, Ian Jackson; +Cc: George Dunlap, xen-devel, Wei Liu
On 30/09/16 13:04, Dario Faggioli wrote:
> On Fri, 2016-09-30 at 11:24 +0100, Ian Jackson wrote:
>> Dario Faggioli writes ("[PATCH v2 08/10] libxl: fix coding style of
>> credit1 parameters related functions"):
>>> int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
>>> libxl_sched_credit_params
>>> *scinfo)
>>> {
>>> struct xen_sysctl_credit_schedule sparam;
>>> - int rc=0;
>>> + int r, rc;
>> ...
>>>
>>>
>>> scinfo->tslice_ms = sparam.tslice_ms;
>>> scinfo->ratelimit_us = sparam.ratelimit_us;
>>>
>>> + out:
>>> GC_FREE;
>>> - return 0;
>>> + return rc;
>>
>> I think this is missing an assignment
>>
>> rc = 0;
>>
>> on the successful exit path, just before out. Am I wrong ?
>>
> Indeed it's missing. It was not necessary in v1 of this patch, so I
> must have failed to notice that it was, when splitting that in two.
>
> Sorry.
>
> Not sure how to proceed, so I'm attaching an updated version of the
> patch to this email.
I've fixed it up in my tree, and added your Acked-by, Ian.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
` (7 preceding siblings ...)
2016-09-30 2:54 ` [PATCH v2 08/10] libxl: fix coding style of credit1 parameters related functions Dario Faggioli
@ 2016-09-30 2:54 ` Dario Faggioli
2016-09-30 10:30 ` Ian Jackson
2016-09-30 2:54 ` [PATCH v2 10/10] xl: " Dario Faggioli
2016-09-30 13:51 ` [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! George Dunlap
10 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:54 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Wei Liu, Ian Jackson
This is the remaining part of the plumbing (the libxl
one) necessary to be able to change the value of the
ratelimit_us parameter online, for Credit2 (like it is
already for Credit1).
Note that, so far, we were rejecting (for Credit1) a
new value of zero, despite it is a pretty nice way to
ask for the rate limiting to be disabled, and the
hypervisor is already capable of dealing with it in
that way.
Therefore, we change things so that it is possible to
do so, both for Credit1 and Credit2
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes from v1:
* added the appropriate LIBXL_HAVE_<foo>, as requested during review.
* coding style fixes put in previous patch, as requested during review.
---
tools/libxl/libxl.c | 71 ++++++++++++++++++++++++++++++++++++++++---
tools/libxl/libxl.h | 11 +++++++
tools/libxl/libxl_types.idl | 4 ++
3 files changed, 81 insertions(+), 5 deletions(-)
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 606d71a..5a70564 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5229,6 +5229,19 @@ static int sched_credit_domain_set(libxl__gc *gc, uint32_t domid,
return 0;
}
+static int sched_ratelimit_check(libxl__gc *gc, int ratelimit)
+{
+ if (ratelimit != 0 &&
+ (ratelimit < XEN_SYSCTL_SCHED_RATELIMIT_MIN ||
+ ratelimit > XEN_SYSCTL_SCHED_RATELIMIT_MAX)) {
+ LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
+ XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
+ return ERROR_INVAL;
+ }
+
+ return 0;
+}
+
int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo)
{
@@ -5266,11 +5279,8 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
rc = ERROR_INVAL;
goto out;
}
- if (scinfo->ratelimit_us < XEN_SYSCTL_SCHED_RATELIMIT_MIN
- || scinfo->ratelimit_us > XEN_SYSCTL_SCHED_RATELIMIT_MAX) {
- LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
- XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
- rc = ERROR_INVAL;
+ rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+ if (rc) {
goto out;
}
if (scinfo->ratelimit_us > scinfo->tslice_ms*1000) {
@@ -5297,6 +5307,57 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
return rc;
}
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo)
+{
+ struct xen_sysctl_credit2_schedule sparam;
+ int r, rc;
+ GC_INIT(ctx);
+
+ r = xc_sched_credit2_params_get(ctx->xch, poolid, &sparam);
+ if (r < 0) {
+ LOGE(ERROR, "getting Credit2 scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ scinfo->ratelimit_us = sparam.ratelimit_us;
+
+ rc = 0;
+ out:
+ GC_FREE;
+ return rc;
+}
+
+
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo)
+{
+ struct xen_sysctl_credit2_schedule sparam;
+ int r, rc;
+ GC_INIT(ctx);
+
+ rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+ if (rc) {
+ goto out;
+ }
+
+ sparam.ratelimit_us = scinfo->ratelimit_us;
+
+ r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
+ if ( r < 0 ) {
+ LOGE(ERROR, "Setting Credit2 scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ scinfo->ratelimit_us = sparam.ratelimit_us;
+
+ out:
+ GC_FREE;
+ return rc;
+}
+
static int sched_credit2_domain_get(libxl__gc *gc, uint32_t domid,
libxl_domain_sched_params *scinfo)
{
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7cfa540..969a089 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -281,6 +281,13 @@
#define LIBXL_HAVE_QEMU_MONITOR_COMMAND 1
/*
+ * LIBXL_HAVE_SCHED_CREDIT2_PARAMS indicates the existance of a
+ * libxl_sched_credit2_params structure, containing Credit2 scheduler
+ * wide parameters (i.e., the ratelimiting value).
+ */
+#define LIBXL_HAVE_SCHED_CREDIT2_PARAMS 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
@@ -1992,6 +1999,10 @@ int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo);
int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo);
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo);
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo);
/* Scheduler Per-domain parameters */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 98bfc3a..de5996e 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -834,6 +834,10 @@ libxl_sched_credit_params = Struct("sched_credit_params", [
("ratelimit_us", integer),
], dispose_fn=None)
+libxl_sched_credit2_params = Struct("sched_credit2_params", [
+ ("ratelimit_us", integer),
+ ], dispose_fn=None)
+
libxl_domain_remus_info = Struct("domain_remus_info",[
("interval", integer),
("allow_unsafe", libxl_defbool),
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2
2016-09-30 2:54 ` [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2 Dario Faggioli
@ 2016-09-30 10:30 ` Ian Jackson
2016-09-30 10:33 ` George Dunlap
0 siblings, 1 reply; 38+ messages in thread
From: Ian Jackson @ 2016-09-30 10:30 UTC (permalink / raw)
To: Dario Faggioli; +Cc: George Dunlap, xen-devel, Wei Liu
Dario Faggioli writes ("[PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2"):
> This is the remaining part of the plumbing (the libxl
> one) necessary to be able to change the value of the
> ratelimit_us parameter online, for Credit2 (like it is
> already for Credit1).
Thanks. I have some coding style nits, all but one of which are
utterly trivial. If it hadn't been for the `rc=0' one I would have
acked this patch as-is.
> +int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
> + libxl_sched_credit2_params *scinfo)
> +{
> + struct xen_sysctl_credit2_schedule sparam;
> + int r, rc;
> + GC_INIT(ctx);
> +
> + rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
> + if (rc) {
> + goto out;
> + }
When you respin this, I think IWBNI you could change this to
if (rc)
goto out;
or
if (rc) goto out;
both of which are encouraged by the coding style, and are more common
inside libxl.
An earlier hunk in this patch does not adjust from a similar construct
where the { } are becoming newly unnecessary. You may want to adjust
that too, but I won't object if you don't feel like it.
> +
> + sparam.ratelimit_us = scinfo->ratelimit_us;
> +
> + r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
> + if ( r < 0 ) {
^ ^
Coding style: libxl does not use the spaces just inside the ( ).
> + LOGE(ERROR, "Setting Credit2 scheduler parameters");
> + rc = ERROR_FAIL;
> + goto out;
> + }
> +
> + scinfo->ratelimit_us = sparam.ratelimit_us;
> +
> + out:
> + GC_FREE;
> + return rc;
This should have an explicit assignment rc=0 before the out label. As
it is, it will happen to be 0 anyway, so that's a stylistic fix.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2
2016-09-30 10:30 ` Ian Jackson
@ 2016-09-30 10:33 ` George Dunlap
2016-09-30 10:35 ` Ian Jackson
2016-09-30 12:37 ` Dario Faggioli
0 siblings, 2 replies; 38+ messages in thread
From: George Dunlap @ 2016-09-30 10:33 UTC (permalink / raw)
To: Ian Jackson, Dario Faggioli; +Cc: George Dunlap, xen-devel, Wei Liu
On 30/09/16 11:30, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2"):
>> This is the remaining part of the plumbing (the libxl
>> one) necessary to be able to change the value of the
>> ratelimit_us parameter online, for Credit2 (like it is
>> already for Credit1).
>
> Thanks. I have some coding style nits, all but one of which are
> utterly trivial. If it hadn't been for the `rc=0' one I would have
> acked this patch as-is.
>
>> +int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
>> + libxl_sched_credit2_params *scinfo)
>> +{
>> + struct xen_sysctl_credit2_schedule sparam;
>> + int r, rc;
>> + GC_INIT(ctx);
>> +
>> + rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
>> + if (rc) {
>> + goto out;
>> + }
>
> When you respin this, I think IWBNI you could change this to
> if (rc)
> goto out;
> or
> if (rc) goto out;
> both of which are encouraged by the coding style, and are more common
> inside libxl.
>
> An earlier hunk in this patch does not adjust from a similar construct
> where the { } are becoming newly unnecessary. You may want to adjust
> that too, but I won't object if you don't feel like it.
>
>> +
>> + sparam.ratelimit_us = scinfo->ratelimit_us;
>> +
>> + r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
>> + if ( r < 0 ) {
> ^ ^
> Coding style: libxl does not use the spaces just inside the ( ).
>
>> + LOGE(ERROR, "Setting Credit2 scheduler parameters");
>> + rc = ERROR_FAIL;
>> + goto out;
>> + }
>> +
>> + scinfo->ratelimit_us = sparam.ratelimit_us;
>> +
>> + out:
>> + GC_FREE;
>> + return rc;
>
> This should have an explicit assignment rc=0 before the out label. As
> it is, it will happen to be 0 anyway, so that's a stylistic fix.
If I'm happy with it as-is, and I fix these up before I check them in,
can I put your Ack on it? Or would you rather have Dario re-send it to
make sure the changes get implemented correctly?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2
2016-09-30 10:33 ` George Dunlap
@ 2016-09-30 10:35 ` Ian Jackson
2016-09-30 12:37 ` Dario Faggioli
1 sibling, 0 replies; 38+ messages in thread
From: Ian Jackson @ 2016-09-30 10:35 UTC (permalink / raw)
To: George Dunlap; +Cc: George Dunlap, xen-devel, Dario Faggioli, Wei Liu
George Dunlap writes ("Re: [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2"):
> On 30/09/16 11:30, Ian Jackson wrote:
> >> + out:
> >> + GC_FREE;
> >> + return rc;
> >
> > This should have an explicit assignment rc=0 before the out label. As
> > it is, it will happen to be 0 anyway, so that's a stylistic fix.
>
> If I'm happy with it as-is, and I fix these up before I check them in,
> can I put your Ack on it? Or would you rather have Dario re-send it to
> make sure the changes get implemented correctly?
Please feel free to fix it up during checking, with my ack.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2
2016-09-30 10:33 ` George Dunlap
2016-09-30 10:35 ` Ian Jackson
@ 2016-09-30 12:37 ` Dario Faggioli
1 sibling, 0 replies; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 12:37 UTC (permalink / raw)
To: George Dunlap, Ian Jackson; +Cc: George Dunlap, xen-devel, Wei Liu
[-- Attachment #1.1.1: Type: text/plain, Size: 3855 bytes --]
On Fri, 2016-09-30 at 11:33 +0100, George Dunlap wrote:
> On 30/09/16 11:30, Ian Jackson wrote:
> >
> > Dario Faggioli writes ("[PATCH v2 09/10] libxl: allow to set the
> > ratelimit value online for Credit2"):
> > >
> > > This is the remaining part of the plumbing (the libxl
> > > one) necessary to be able to change the value of the
> > > ratelimit_us parameter online, for Credit2 (like it is
> > > already for Credit1).
> >
> > Thanks. I have some coding style nits, all but one of which are
> > utterly trivial. If it hadn't been for the `rc=0' one I would have
> > acked this patch as-is.
> >
Ok, thanks for looking this quickly. :-)
> > > +int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t
> > > poolid,
> > > + libxl_sched_credit2_params
> > > *scinfo)
> > > +{
> > > + struct xen_sysctl_credit2_schedule sparam;
> > > + int r, rc;
> > > + GC_INIT(ctx);
> > > +
> > > + rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
> > > + if (rc) {
> > > + goto out;
> > > + }
> >
> > When you respin this, I think IWBNI you could change this to
> > if (rc)
> > goto out;
> > or
> > if (rc) goto out;
> > both of which are encouraged by the coding style, and are more
> > common
> > inside libxl.
> >
Right!
> > An earlier hunk in this patch does not adjust from a similar
> > construct
> > where the { } are becoming newly unnecessary. You may want to
> > adjust
> > that too, but I won't object if you don't feel like it.
> >
Sure.
> > > + sparam.ratelimit_us = scinfo->ratelimit_us;
> > > +
> > > + r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
> > > + if ( r < 0 ) {
> > ^ ^
> > Coding style: libxl does not use the spaces just inside the ( ).
> >
Bah, how did this got here! :-/
> > >
> > > + LOGE(ERROR, "Setting Credit2 scheduler parameters");
> > > + rc = ERROR_FAIL;
> > > + goto out;
> > > + }
> > > +
> > > + scinfo->ratelimit_us = sparam.ratelimit_us;
> > > +
> > > + out:
> > > + GC_FREE;
> > > + return rc;
> >
> > This should have an explicit assignment rc=0 before the out
> > label. As
> > it is, it will happen to be 0 anyway, so that's a stylistic fix.
>
> If I'm happy with it as-is, and I fix these up before I check them
> in,
> can I put your Ack on it?
>
So, just FTR, it doesn't just "happen to be 0 anyway", that was on
purpose. :-)
I.e., I think the function is small and simple enough for me to take
advantage of the knowledge that, when we get to the out: label, we've
necessarily called sched_ratelimit_check(), and saved the return code
in 'rc', and that such return code is either 0, or the proper error
value we want to return.
That being said, it's no big deal, and I'm fine whatever the code ends
up looking.
> Or would you rather have Dario re-send it to
> make sure the changes get implemented correctly?
>
Again, I'm fine either way. For your convenience, I'm attaching here
two variants of this patch. Both have the coding style fixes suggested.
One (the one with '-rc' in the name), includes the 'rc = 0', the other
doesn't.
Feel free to pickup whichever you find best/more convenient/etc, or
feel free to ignore them, if they're not useful. I'm fine in any case.
:-)
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.1.2: libxl-allow-disabling-ratelimit-online.patch --]
[-- Type: text/x-patch, Size: 5841 bytes --]
From: Dario Faggioli <dario.faggioli@citrix.com>
libxl: allow to set the ratelimit value online for Credit2
This is the remaining part of the plumbing (the libxl
one) necessary to be able to change the value of the
ratelimit_us parameter online, for Credit2 (like it is
already for Credit1).
Note that, so far, we were rejecting (for Credit1) a
new value of zero, despite it is a pretty nice way to
ask for the rate limiting to be disabled, and the
hypervisor is already capable of dealing with it in
that way.
Therefore, we change things so that it is possible to
do so, both for Credit1 and Credit2
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes from v2:
* coding style, reported during review.
Changes from v1:
* added the appropriate LIBXL_HAVE_<foo>, as requested during review.
* coding style fixes put in previous patch, as requested during review.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d2552f9..57c052c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5256,6 +5256,19 @@ static int sched_credit_domain_set(libxl__gc *gc, uint32_t domid,
return 0;
}
+static int sched_ratelimit_check(libxl__gc *gc, int ratelimit)
+{
+ if (ratelimit != 0 &&
+ (ratelimit < XEN_SYSCTL_SCHED_RATELIMIT_MIN ||
+ ratelimit > XEN_SYSCTL_SCHED_RATELIMIT_MAX)) {
+ LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
+ XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
+ return ERROR_INVAL;
+ }
+
+ return 0;
+}
+
int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo)
{
@@ -5293,13 +5306,9 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
rc = ERROR_INVAL;
goto out;
}
- if (scinfo->ratelimit_us < XEN_SYSCTL_SCHED_RATELIMIT_MIN
- || scinfo->ratelimit_us > XEN_SYSCTL_SCHED_RATELIMIT_MAX) {
- LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
- XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
- rc = ERROR_INVAL;
- goto out;
- }
+ rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+ if (rc) goto out;
+
if (scinfo->ratelimit_us > scinfo->tslice_ms*1000) {
LOG(ERROR, "Ratelimit cannot be greater than timeslice");
rc = ERROR_INVAL;
@@ -5319,12 +5328,60 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
scinfo->tslice_ms = sparam.tslice_ms;
scinfo->ratelimit_us = sparam.ratelimit_us;
+ out:
+ GC_FREE;
+ return rc;
+}
+
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo)
+{
+ struct xen_sysctl_credit2_schedule sparam;
+ int r, rc;
+ GC_INIT(ctx);
+
+ r = xc_sched_credit2_params_get(ctx->xch, poolid, &sparam);
+ if (r < 0) {
+ LOGE(ERROR, "getting Credit2 scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ scinfo->ratelimit_us = sparam.ratelimit_us;
+
rc = 0;
out:
GC_FREE;
return rc;
}
+
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo)
+{
+ struct xen_sysctl_credit2_schedule sparam;
+ int r, rc;
+ GC_INIT(ctx);
+
+ rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+ if (rc) goto out;
+
+ sparam.ratelimit_us = scinfo->ratelimit_us;
+
+ r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
+ if (r < 0) {
+ LOGE(ERROR, "Setting Credit2 scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ scinfo->ratelimit_us = sparam.ratelimit_us;
+
+ out:
+ GC_FREE;
+ return rc;
+}
+
static int sched_credit2_domain_get(libxl__gc *gc, uint32_t domid,
libxl_domain_sched_params *scinfo)
{
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7cfa540..969a089 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -281,6 +281,13 @@
#define LIBXL_HAVE_QEMU_MONITOR_COMMAND 1
/*
+ * LIBXL_HAVE_SCHED_CREDIT2_PARAMS indicates the existance of a
+ * libxl_sched_credit2_params structure, containing Credit2 scheduler
+ * wide parameters (i.e., the ratelimiting value).
+ */
+#define LIBXL_HAVE_SCHED_CREDIT2_PARAMS 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
@@ -1992,6 +1999,10 @@ int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo);
int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo);
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo);
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo);
/* Scheduler Per-domain parameters */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a02446f..a32c751 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -838,6 +838,10 @@ libxl_sched_credit_params = Struct("sched_credit_params", [
("ratelimit_us", integer),
], dispose_fn=None)
+libxl_sched_credit2_params = Struct("sched_credit2_params", [
+ ("ratelimit_us", integer),
+ ], dispose_fn=None)
+
libxl_domain_remus_info = Struct("domain_remus_info",[
("interval", integer),
("allow_unsafe", libxl_defbool),
[-- Attachment #1.1.3: libxl-allow-disabling-ratelimit-online_rc.patch --]
[-- Type: text/x-patch, Size: 5811 bytes --]
From: Dario Faggioli <dario.faggioli@citrix.com>
libxl: allow to set the ratelimit value online for Credit2
This is the remaining part of the plumbing (the libxl
one) necessary to be able to change the value of the
ratelimit_us parameter online, for Credit2 (like it is
already for Credit1).
Note that, so far, we were rejecting (for Credit1) a
new value of zero, despite it is a pretty nice way to
ask for the rate limiting to be disabled, and the
hypervisor is already capable of dealing with it in
that way.
Therefore, we change things so that it is possible to
do so, both for Credit1 and Credit2
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
Changes from v2:
* coding style, reported during review;
* explicitly set rc to 0 on succesful path, as suggested during review.
Changes from v1:
* added the appropriate LIBXL_HAVE_<foo>, as requested during review.
* coding style fixes put in previous patch, as requested during review.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d2552f9..a3b5e6d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5256,6 +5256,19 @@ static int sched_credit_domain_set(libxl__gc *gc, uint32_t domid,
return 0;
}
+static int sched_ratelimit_check(libxl__gc *gc, int ratelimit)
+{
+ if (ratelimit != 0 &&
+ (ratelimit < XEN_SYSCTL_SCHED_RATELIMIT_MIN ||
+ ratelimit > XEN_SYSCTL_SCHED_RATELIMIT_MAX)) {
+ LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
+ XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
+ return ERROR_INVAL;
+ }
+
+ return 0;
+}
+
int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo)
{
@@ -5293,13 +5306,9 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
rc = ERROR_INVAL;
goto out;
}
- if (scinfo->ratelimit_us < XEN_SYSCTL_SCHED_RATELIMIT_MIN
- || scinfo->ratelimit_us > XEN_SYSCTL_SCHED_RATELIMIT_MAX) {
- LOG(ERROR, "Ratelimit out of range, valid range is from %d to %d",
- XEN_SYSCTL_SCHED_RATELIMIT_MIN, XEN_SYSCTL_SCHED_RATELIMIT_MAX);
- rc = ERROR_INVAL;
- goto out;
- }
+ rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+ if (rc) goto out;
+
if (scinfo->ratelimit_us > scinfo->tslice_ms*1000) {
LOG(ERROR, "Ratelimit cannot be greater than timeslice");
rc = ERROR_INVAL;
@@ -5325,6 +5334,56 @@ int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
return rc;
}
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo)
+{
+ struct xen_sysctl_credit2_schedule sparam;
+ int r, rc;
+ GC_INIT(ctx);
+
+ r = xc_sched_credit2_params_get(ctx->xch, poolid, &sparam);
+ if (r < 0) {
+ LOGE(ERROR, "getting Credit2 scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ scinfo->ratelimit_us = sparam.ratelimit_us;
+
+ rc = 0;
+ out:
+ GC_FREE;
+ return rc;
+}
+
+
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo)
+{
+ struct xen_sysctl_credit2_schedule sparam;
+ int r, rc;
+ GC_INIT(ctx);
+
+ rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
+ if (rc) goto out;
+
+ sparam.ratelimit_us = scinfo->ratelimit_us;
+
+ r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
+ if (r < 0) {
+ LOGE(ERROR, "Setting Credit2 scheduler parameters");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ scinfo->ratelimit_us = sparam.ratelimit_us;
+
+ rc = 0;
+ out:
+ GC_FREE;
+ return rc;
+}
+
static int sched_credit2_domain_get(libxl__gc *gc, uint32_t domid,
libxl_domain_sched_params *scinfo)
{
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7cfa540..969a089 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -281,6 +281,13 @@
#define LIBXL_HAVE_QEMU_MONITOR_COMMAND 1
/*
+ * LIBXL_HAVE_SCHED_CREDIT2_PARAMS indicates the existance of a
+ * libxl_sched_credit2_params structure, containing Credit2 scheduler
+ * wide parameters (i.e., the ratelimiting value).
+ */
+#define LIBXL_HAVE_SCHED_CREDIT2_PARAMS 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
@@ -1992,6 +1999,10 @@ int libxl_sched_credit_params_get(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo);
int libxl_sched_credit_params_set(libxl_ctx *ctx, uint32_t poolid,
libxl_sched_credit_params *scinfo);
+int libxl_sched_credit2_params_get(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo);
+int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
+ libxl_sched_credit2_params *scinfo);
/* Scheduler Per-domain parameters */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a02446f..a32c751 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -838,6 +838,10 @@ libxl_sched_credit_params = Struct("sched_credit_params", [
("ratelimit_us", integer),
], dispose_fn=None)
+libxl_sched_credit2_params = Struct("sched_credit2_params", [
+ ("ratelimit_us", integer),
+ ], dispose_fn=None)
+
libxl_domain_remus_info = Struct("domain_remus_info",[
("interval", integer),
("allow_unsafe", libxl_defbool),
[-- 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 related [flat|nested] 38+ messages in thread
* [PATCH v2 10/10] xl: allow to set the ratelimit value online for Credit2
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
` (8 preceding siblings ...)
2016-09-30 2:54 ` [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2 Dario Faggioli
@ 2016-09-30 2:54 ` Dario Faggioli
2016-09-30 10:34 ` Ian Jackson
2016-10-13 22:19 ` Jim Fehlig
2016-09-30 13:51 ` [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! George Dunlap
10 siblings, 2 replies; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 2:54 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Ian Jackson, George Dunlap
Last part of the wiring necessary for allowing to
change the value of the ratelimit_us parameter online,
for Credit2 (like it is already for Credit1).
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
docs/man/xl.pod.1.in | 9 ++++
tools/libxl/xl_cmdimpl.c | 91 +++++++++++++++++++++++++++++++++++++--------
tools/libxl/xl_cmdtable.c | 2 +
3 files changed, 86 insertions(+), 16 deletions(-)
diff --git a/docs/man/xl.pod.1.in b/docs/man/xl.pod.1.in
index a2be541..803c67e 100644
--- a/docs/man/xl.pod.1.in
+++ b/docs/man/xl.pod.1.in
@@ -1089,6 +1089,15 @@ to 65535 and the default is 256.
Restrict output to domains in the specified cpupool.
+=item B<-s>, B<--schedparam>
+
+Specify to list or set pool-wide scheduler parameters.
+
+=item B<-r RLIMIT>, B<--ratelimit_us=RLIMIT>
+
+Attempts to limit the rate of context switching. It is basically the same
+as B<--ratelimit_us> in B<sched-credit>
+
=back
=item B<sched-rtds> [I<OPTIONS>]
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index cb43c00..b317dde 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -6457,8 +6457,29 @@ static int sched_credit_pool_output(uint32_t poolid)
return 0;
}
-static int sched_credit2_domain_output(
- int domid)
+static int sched_credit2_params_set(int poolid,
+ libxl_sched_credit2_params *scinfo)
+{
+ if (libxl_sched_credit2_params_set(ctx, poolid, scinfo)) {
+ fprintf(stderr, "libxl_sched_credit2_params_set failed.\n");
+ return 1;
+ }
+
+ return 0;
+}
+
+static int sched_credit2_params_get(int poolid,
+ libxl_sched_credit2_params *scinfo)
+{
+ if (libxl_sched_credit2_params_get(ctx, poolid, scinfo)) {
+ fprintf(stderr, "libxl_sched_credit2_params_get failed.\n");
+ return 1;
+ }
+
+ return 0;
+}
+
+static int sched_credit2_domain_output(int domid)
{
char *domname;
libxl_domain_sched_params scinfo;
@@ -6483,6 +6504,22 @@ static int sched_credit2_domain_output(
return 0;
}
+static int sched_credit2_pool_output(uint32_t poolid)
+{
+ libxl_sched_credit2_params scparam;
+ char *poolname = libxl_cpupoolid_to_name(ctx, poolid);
+
+ if (sched_credit2_params_get(poolid, &scparam))
+ printf("Cpupool %s: [sched params unavailable]\n", poolname);
+ else
+ printf("Cpupool %s: ratelimit=%dus\n",
+ poolname, scparam.ratelimit_us);
+
+ free(poolname);
+
+ return 0;
+}
+
static int sched_rtds_domain_output(
int domid)
{
@@ -6582,17 +6619,6 @@ static int sched_rtds_pool_output(uint32_t poolid)
return 0;
}
-static int sched_default_pool_output(uint32_t poolid)
-{
- char *poolname;
-
- poolname = libxl_cpupoolid_to_name(ctx, poolid);
- printf("Cpupool %s:\n",
- poolname);
- free(poolname);
- return 0;
-}
-
static int sched_domain_output(libxl_scheduler sched, int (*output)(int),
int (*pooloutput)(uint32_t), const char *cpupool)
{
@@ -6838,17 +6864,22 @@ int main_sched_credit2(int argc, char **argv)
{
const char *dom = NULL;
const char *cpupool = NULL;
+ int ratelimit = 0;
int weight = 256;
+ bool opt_s = false;
+ bool opt_r = false;
bool opt_w = false;
int opt, rc;
static struct option opts[] = {
{"domain", 1, 0, 'd'},
{"weight", 1, 0, 'w'},
+ {"schedparam", 0, 0, 's'},
+ {"ratelimit_us", 1, 0, 'r'},
{"cpupool", 1, 0, 'p'},
COMMON_LONG_OPTS
};
- SWITCH_FOREACH_OPT(opt, "d:w:p:", opts, "sched-credit2", 0) {
+ SWITCH_FOREACH_OPT(opt, "d:w:p:r:s", opts, "sched-credit2", 0) {
case 'd':
dom = optarg;
break;
@@ -6856,6 +6887,13 @@ int main_sched_credit2(int argc, char **argv)
weight = strtol(optarg, NULL, 10);
opt_w = true;
break;
+ case 's':
+ opt_s = true;
+ break;
+ case 'r':
+ ratelimit = strtol(optarg, NULL, 10);
+ opt_r = true;
+ break;
case 'p':
cpupool = optarg;
break;
@@ -6871,10 +6909,31 @@ int main_sched_credit2(int argc, char **argv)
return EXIT_FAILURE;
}
- if (!dom) { /* list all domain's credit scheduler info */
+ if (opt_s) {
+ libxl_sched_credit2_params scparam;
+ uint32_t poolid = 0;
+
+ if (cpupool) {
+ if (libxl_cpupool_qualifier_to_cpupoolid(ctx, cpupool,
+ &poolid, NULL) ||
+ !libxl_cpupoolid_is_valid(ctx, poolid)) {
+ fprintf(stderr, "unknown cpupool \'%s\'\n", cpupool);
+ return EXIT_FAILURE;
+ }
+ }
+
+ if (!opt_r) { /* Output scheduling parameters */
+ if (sched_credit2_pool_output(poolid))
+ return EXIT_FAILURE;
+ } else { /* Set scheduling parameters (so far, just ratelimit) */
+ scparam.ratelimit_us = ratelimit;
+ if (sched_credit2_params_set(poolid, &scparam))
+ return EXIT_FAILURE;
+ }
+ } else if (!dom) { /* list all domain's credit scheduler info */
if (sched_domain_output(LIBXL_SCHEDULER_CREDIT2,
sched_credit2_domain_output,
- sched_default_pool_output,
+ sched_credit2_pool_output,
cpupool))
return EXIT_FAILURE;
} else {
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 78786fe..5a57342 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -265,6 +265,8 @@ struct cmd_spec cmd_table[] = {
"[-d <Domain> [-w[=WEIGHT]]] [-p CPUPOOL]",
"-d DOMAIN, --domain=DOMAIN Domain to modify\n"
"-w WEIGHT, --weight=WEIGHT Weight (int)\n"
+ "-s --schedparam Query / modify scheduler parameters\n"
+ "-r RLIMIT, --ratelimit_us=RLIMIT Set the scheduling rate limit, in microseconds\n"
"-p CPUPOOL, --cpupool=CPUPOOL Restrict output to CPUPOOL"
},
{ "sched-rtds",
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v2 10/10] xl: allow to set the ratelimit value online for Credit2
2016-09-30 2:54 ` [PATCH v2 10/10] xl: " Dario Faggioli
@ 2016-09-30 10:34 ` Ian Jackson
2016-09-30 15:54 ` Dario Faggioli
2016-10-13 22:19 ` Jim Fehlig
1 sibling, 1 reply; 38+ messages in thread
From: Ian Jackson @ 2016-09-30 10:34 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Wei Liu, George Dunlap
Dario Faggioli writes ("[PATCH v2 10/10] xl: allow to set the ratelimit value online for Credit2"):
> Last part of the wiring necessary for allowing to
> change the value of the ratelimit_us parameter online,
> for Credit2 (like it is already for Credit1).
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Although I do have a comment:
> +static int sched_credit2_params_set(int poolid,
> + libxl_sched_credit2_params *scinfo)
Each of this and the corresponding _get calls the corresponding libxl
function, prints a message, and adjusts the return value.
But: AFAICT each of these has only one call site; and each of the
underlying libxl functions logs an error too; and each of the call
sites prints a message too.
So I question whether these functions are valuable.
At this stage of the release cycle, and the maturity of this series, I
guess you probably don't want to start messing with the error paths.
The messages that will come out will be overly verbose rather than
inadequate, so the code as it is fine if not as good as it could be.
But it was a thing I noticed which I wanted to write down and/or have
your opinion on.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 10/10] xl: allow to set the ratelimit value online for Credit2
2016-09-30 10:34 ` Ian Jackson
@ 2016-09-30 15:54 ` Dario Faggioli
2016-09-30 16:02 ` Ian Jackson
0 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 15:54 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel, Wei Liu, George Dunlap
[-- Attachment #1.1: Type: text/plain, Size: 2697 bytes --]
On Fri, 2016-09-30 at 11:34 +0100, Ian Jackson wrote:
> Dario Faggioli writes ("[PATCH v2 10/10] xl: allow to set
> theratelimit value online for Credit2"):
> >
> > +static int sched_credit2_params_set(int poolid,
> > + libxl_sched_credit2_params
> > *scinfo)
>
> Each of this and the corresponding _get calls the corresponding libxl
> function, prints a message, and adjusts the return value.
>
> But: AFAICT each of these has only one call site; and each of the
> underlying libxl functions logs an error too; and each of the call
> sites prints a message too.
>
> So I question whether these functions are valuable.
>
So, about this last point (utility of sched_*_params_{get,set}() ), I
totally agree. I think the reason why the functions exist is for the
most part related to convenience and style. E.g., maybe
indentation/nesting level was getting too deep within the various
main_sched_*().
As said in another email, I don't especially like those functions, and
I have the feeling they're a better way to write them, but I haven't
really try. I'll put having a look at that in my todo list, but not at
super high priority. :-P
> At this stage of the release cycle, and the maturity of this series,
> I
> guess you probably don't want to start messing with the error paths.
>
Yeah, indeed, thanks for understanding! :-P
Although...
> The messages that will come out will be overly verbose rather than
> inadequate, so the code as it is fine if not as good as it could be.
>
...I'm not entirely sure I get what you're saying here.
I mean, AFAICT, there's xl calling a libxl function and printing an
error if it fails. The libxl function also logs an error --true-- but
this seems rather common to me, and apart from that, it looks
reasonable as well, isn't it so?
Avoiding double/overly verbose logging from either within xl or within
libxl is something I totally understand and agree. But when the
'border' is crossed, is it that terrible that both do log? After all,
how can xl be sure that libxl will log something? And even if he could,
I see the two as different kind and different level of logging.
Or was it something else that you were referring to?
> But it was a thing I noticed which I wanted to write down and/or have
> your opinion on.
>
Sure! :-)
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] 38+ messages in thread* Re: [PATCH v2 10/10] xl: allow to set the ratelimit value online for Credit2
2016-09-30 15:54 ` Dario Faggioli
@ 2016-09-30 16:02 ` Ian Jackson
0 siblings, 0 replies; 38+ messages in thread
From: Ian Jackson @ 2016-09-30 16:02 UTC (permalink / raw)
To: Dario Faggioli; +Cc: xen-devel, Wei Liu, George Dunlap
Dario Faggioli writes ("Re: [Xen-devel] [PATCH v2 10/10] xl: allow to set the ratelimit value online for Credit2"):
> I mean, AFAICT, there's xl calling a libxl function and printing an
> error if it fails. The libxl function also logs an error --true-- but
> this seems rather common to me, and apart from that, it looks
> reasonable as well, isn't it so?
It seems to me that we should have a coherent strategy for which code
is responsible for making what kind of log messages / error messages
under what circumstances.
One objective of this would be to try to arrange that when
approximately one thing is wrong, the user gets one (accurate and
informative) message about it, not half a dozen.
But the more important objective would be to arrange that errors do
not occur without _something_ providing an explanation.
We are quite a way away from having such a coherent strategy in
libxl.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 10/10] xl: allow to set the ratelimit value online for Credit2
2016-09-30 2:54 ` [PATCH v2 10/10] xl: " Dario Faggioli
2016-09-30 10:34 ` Ian Jackson
@ 2016-10-13 22:19 ` Jim Fehlig
2016-10-14 11:31 ` George Dunlap
1 sibling, 1 reply; 38+ messages in thread
From: Jim Fehlig @ 2016-10-13 22:19 UTC (permalink / raw)
To: Dario Faggioli, xen-devel; +Cc: Ian Jackson, Wei Liu, George Dunlap
On 09/29/2016 08:54 PM, Dario Faggioli wrote:
> Last part of the wiring necessary for allowing to
> change the value of the ratelimit_us parameter online,
> for Credit2 (like it is already for Credit1).
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Seeing this patch got me to thinking that it would be cool if libvirt supported
Credit2 :-). Do you have any plans/time to do that?
Regards,
Jim
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 10/10] xl: allow to set the ratelimit value online for Credit2
2016-10-13 22:19 ` Jim Fehlig
@ 2016-10-14 11:31 ` George Dunlap
0 siblings, 0 replies; 38+ messages in thread
From: George Dunlap @ 2016-10-14 11:31 UTC (permalink / raw)
To: Jim Fehlig, Dario Faggioli, xen-devel; +Cc: Ian Jackson, Wei Liu
On 13/10/16 23:19, Jim Fehlig wrote:
> On 09/29/2016 08:54 PM, Dario Faggioli wrote:
>> Last part of the wiring necessary for allowing to
>> change the value of the ratelimit_us parameter online,
>> for Credit2 (like it is already for Credit1).
>>
>> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
>> Reviewed-by: George Dunlap <george.dunlap@citrix.com>
>
> Seeing this patch got me to thinking that it would be cool if libvirt
> supported Credit2 :-). Do you have any plans/time to do that?
Hmm, well, we can put it on our list of "things it would be good to do"
-- not sure when it might actually manage to get attention
unfortunately. :-/
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2!
2016-09-30 2:53 [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! Dario Faggioli
` (9 preceding siblings ...)
2016-09-30 2:54 ` [PATCH v2 10/10] xl: " Dario Faggioli
@ 2016-09-30 13:51 ` George Dunlap
2016-09-30 14:06 ` Dario Faggioli
10 siblings, 1 reply; 38+ messages in thread
From: George Dunlap @ 2016-09-30 13:51 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Wei Liu, George Dunlap, Andrew Cooper, Anshul Makkar, Ian Jackson,
Meng Xu, Jan Beulich
On 30/09/16 03:53, Dario Faggioli wrote:
> Hey,
>
> This is v2 of my Credit1 and Credit2 improvements series. First posting is
> here:
>
> https://lists.xen.org/archives/html/xen-devel/2016-08/msg02183.html
>
> Now, couple of things:
> - some of the patches have been applied already out of v1;
> - I've reshuffled the remaining patches a bit, mostly upon reviewers'
> requests to do so;
> - I'm not including the 'soft affinity for Credit2 patches'.
>
> In fact, most of the soft affinity work still needs to be reviewed.
>
> OTOH, the patches that I've put together here, have been reviewed already, and
> I think I've addressed all the review comments, which means that --if people
> (i.e., mostly George!:-P) manage to have a quick look at them-- they can even
> go in for 4.8.
>
> And while there's really no point in rushing soft-affinity for Credit2 in at
> this stage, these patches brings some nice (and moderateely simple)
> improvements for both the schedulers, which I think should make it in the
> release.
>
> There's a branch available here:
> git://xenbits.xen.org/people/dariof/xen.git rel/sched/misc-credit1-credit2-plus-credit2-softaff-v2
> http://xenbits.xen.org/gitweb/?p=people/dariof/xen.git;a=shortlog;h=refs/heads/rel/sched/misc-credit1-credit2-plus-credit2-softaff-v2
> https://travis-ci.org/fdario/xen/builds/163898322
>
> Thanks and Regards,
> Dario
> ---
> Dario Faggioli (10):
[snip]
> xen: credit2: implement yield()
> xen: tracing: add trace records for schedule and rate-limiting.
I've pushed everything but these two; the first because I had questions
about the algorithm, the second because it didn't apply cleanly without
the first.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2!
2016-09-30 13:51 ` [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2! George Dunlap
@ 2016-09-30 14:06 ` Dario Faggioli
2016-09-30 14:10 ` George Dunlap
0 siblings, 1 reply; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 14:06 UTC (permalink / raw)
To: George Dunlap, xen-devel
Cc: Wei Liu, George Dunlap, Andrew Cooper, Anshul Makkar, Ian Jackson,
Meng Xu, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 893 bytes --]
On Fri, 2016-09-30 at 14:51 +0100, George Dunlap wrote:
> On 30/09/16 03:53, Dario Faggioli wrote:
> >
> > Dario Faggioli (10):
> [snip]
> >
> > xen: credit2: implement yield()
> > xen: tracing: add trace records for schedule and rate-
> > limiting.
>
> I've pushed everything but these two; the first because I had
> questions
> about the algorithm, the second because it didn't apply cleanly
> without
> the first.
>
Mm... looks to me that
3/10 xen: credit2: make tickling more deterministic
is also missing? Or should I re-`git pull' (I've just done that,
though...)?
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] 38+ messages in thread
* Re: [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2!
2016-09-30 14:06 ` Dario Faggioli
@ 2016-09-30 14:10 ` George Dunlap
2016-09-30 14:12 ` Dario Faggioli
0 siblings, 1 reply; 38+ messages in thread
From: George Dunlap @ 2016-09-30 14:10 UTC (permalink / raw)
To: Dario Faggioli, xen-devel
Cc: Wei Liu, George Dunlap, Andrew Cooper, Anshul Makkar, Ian Jackson,
Meng Xu, Jan Beulich
On 30/09/16 15:06, Dario Faggioli wrote:
> On Fri, 2016-09-30 at 14:51 +0100, George Dunlap wrote:
>> On 30/09/16 03:53, Dario Faggioli wrote:
>>>
>>> Dario Faggioli (10):
>> [snip]
>>>
>>> xen: credit2: implement yield()
>>> xen: tracing: add trace records for schedule and rate-
>>> limiting.
>>
>> I've pushed everything but these two; the first because I had
>> questions
>> about the algorithm, the second because it didn't apply cleanly
>> without
>> the first.
>>
> Mm... looks to me that
>
> 3/10 xen: credit2: make tickling more deterministic
>
> is also missing? Or should I re-`git pull' (I've just done that,
> though...)?
I see it there in my tree::
$ git log -n 8
47bd006 Fri Sep 30 04:54:28 2016 +0200 Dario Faggioli xl: allow to set
the ratelimit value online for Credit2
368db83 Fri Sep 30 04:54:21 2016 +0200 Dario Faggioli libxl: allow to
set the ratelimit value online for Credit2
9724606 Fri Sep 30 04:54:14 2016 +0200 Dario Faggioli libxl: fix coding
style of credit1 parameters related functions
4217c01 Fri Sep 30 04:54:07 2016 +0200 Dario Faggioli tools: tracing:
handle more scheduling related events.
5e4b419 Fri Sep 30 04:53:46 2016 +0200 Dario Faggioli xen: credit2: only
reset credit on reset condition
069cf39 Fri Sep 30 04:53:39 2016 +0200 Dario Faggioli xen: credit2: make
tickling more deterministic
9fdd2f3 Fri Sep 30 04:53:32 2016 +0200 Dario Faggioli xen: credit1:
don't rate limit context switches in case of yields
0053127 Fri Sep 30 04:53:25 2016 +0200 Dario Faggioli xen: credit1:
return the 'time remaining to the limit' as next timeslice.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v2 00/10] sched: Credit1 and Credit2 improvements... but *NO* soft-affinity for Credit2!
2016-09-30 14:10 ` George Dunlap
@ 2016-09-30 14:12 ` Dario Faggioli
0 siblings, 0 replies; 38+ messages in thread
From: Dario Faggioli @ 2016-09-30 14:12 UTC (permalink / raw)
To: George Dunlap, xen-devel
Cc: Wei Liu, George Dunlap, Andrew Cooper, Anshul Makkar, Ian Jackson,
Meng Xu, Jan Beulich
[-- Attachment #1.1: Type: text/plain, Size: 716 bytes --]
On Fri, 2016-09-30 at 15:10 +0100, George Dunlap wrote:
> On 30/09/16 15:06, Dario Faggioli wrote:
> > Mm... looks to me that
> >
> > 3/10 xen: credit2: make tickling more deterministic
> >
> > is also missing? Or should I re-`git pull' (I've just done that,
> > though...)?
>
> I see it there in my tree::
>
Yep, as said on IRC, it was indeed me that were on the wrong local
branch. Sorry for the noise. :-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] 38+ messages in thread