| Message ID | 20160212162926.25796.58870.stgit@Solace.station (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/02/16 16:29, Dario Faggioli wrote: > Moving a vCPU to a different pCPU means offlining it and > then waking it up, on the new pCPU. Credit1 grants BOOST > priority to vCPUs that wakes up, with the aim of improving > I/O latency. The net effect of this all is that vCPUs get > boosted when migrating, which shouldn't happen. > > For instance, this causes scheduling anomalies and, > potentially, performance problems, as reported here: > http://lists.xen.org/archives/html/xen-devel/2015-10/msg02851.html > > This patch fixes this by noting down (by means of a flag) > the fact that the vCPU is about to undergo a migration. > This way we can tell, later, during a wakeup, whether the > vCPU is migrating or unblocking, and decide whether or > not to apply the boosting. > > Note that it is important that atomic-safe bit operations > are used when manipulating vCPUs' flags. Take the chance > and add a comment about this. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Jan Beulich <JBeulich@suse.com> > --- > Changes from v2: > * test_and_clear() is necessary when accessing svc->flags; > * added a comment about such need at the top, where the flags > are defined. > > Changes from v1: > * rewritten, following suggestion got during review: there > are no wakeup flags any longer, and all is done in sched_credit.c > by setting a flag in csched_cpu_pick() and testing (and > cleating) it in csched_vcpu_wake(). > --- > xen/common/sched_credit.c | 30 ++++++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > index 5708701..597a784 100644 > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -63,9 +63,14 @@ > > /* > * Flags > + * > + * Note that svc->flags (where these flags live) is protected by an > + * inconsistent set of locks. Therefore atomic-safe bit operations must > + * be used for accessing it. > */ > #define CSCHED_FLAG_VCPU_PARKED 0x0 /* VCPU over capped credits */ > #define CSCHED_FLAG_VCPU_YIELD 0x1 /* VCPU yielding */ > +#define CSCHED_FLAG_VCPU_MIGRATING 0x2 /* VCPU may have moved to a new pcpu */ > > > /* > @@ -787,6 +792,16 @@ _csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc, bool_t commit) > static int > csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc) > { > + struct csched_vcpu *svc = CSCHED_VCPU(vc); > + > + /* > + * We have been called by vcpu_migrate() (in schedule.c), as part > + * of the process of seeing if vc can be migrated to another pcpu. > + * We make a note about this in svc->flags so that later, in > + * csched_vcpu_wake() (still called from vcpu_migrate()) we won't > + * get boosted, which we don't deserve as we are "only" migrating. > + */ > + set_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc->flags); > return _csched_cpu_pick(ops, vc, 1); > } > > @@ -1022,11 +1037,18 @@ csched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc) > * more CPU resource intensive VCPUs without impacting overall > * system fairness. > * > - * The one exception is for VCPUs of capped domains unpausing > - * after earning credits they had overspent. We don't boost > - * those. > + * There are two cases, when we don't want to boost: > + * - VCPUs that are waking up after a migration, rather than > + * after having block; > + * - VCPUs of capped domains unpausing after earning credits > + * they had overspent. > + * > + * Note that checking whether we are "only" migrating must be > + * done up front, as we do not want the clearing of the bit we > + * set in csched_cpu_pick() to be short-circuited away. > */ > - if ( svc->pri == CSCHED_PRI_TS_UNDER && > + if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc->flags) && > + svc->pri == CSCHED_PRI_TS_UNDER && > !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) ) Sorry to be late reviewing this. So we always want to clear the 'migrating' flag, regardless of whether we do anything with boosting. Would that logic be clearer if we cleared it as a separate step, storing the result in a local variable? E.g.: bool migrating; ... /* Always clear migrating flag if it's set */ migrating = test_and_clear_bit(...) if ( !migrating && ...) { } Then we wouldn't need the last paragraph in the comment. That said, this is v3, so if you'd rather just get this in as it is, then you can have my Acked-by as well. -George
On Wed, 2016-02-24 at 10:43 +0000, George Dunlap wrote: > On 12/02/16 16:29, Dario Faggioli wrote: > > > > @@ -1022,11 +1037,18 @@ csched_vcpu_wake(const struct scheduler > > *ops, struct vcpu *vc) > > * more CPU resource intensive VCPUs without impacting > > overall > > * system fairness. > > * > > - * The one exception is for VCPUs of capped domains unpausing > > - * after earning credits they had overspent. We don't boost > > - * those. > > + * There are two cases, when we don't want to boost: > > + * - VCPUs that are waking up after a migration, rather than > > + * after having block; > > + * - VCPUs of capped domains unpausing after earning credits > > + * they had overspent. > > + * > > + * Note that checking whether we are "only" migrating must be > > + * done up front, as we do not want the clearing of the bit we > > + * set in csched_cpu_pick() to be short-circuited away. > > */ > > - if ( svc->pri == CSCHED_PRI_TS_UNDER && > > + if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc- > > >flags) && > > + svc->pri == CSCHED_PRI_TS_UNDER && > > !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) ) > > Sorry to be late reviewing this. > No problem. Thanks for getting to it, actually, as I've got a few more patches stacked on top of these outstanding series. > So we always want to clear the 'migrating' flag, regardless of > whether > we do anything with boosting. Would that logic be clearer if we > cleared > it as a separate step, storing the result in a local variable? E.g.: > > bool migrating; > > ... > > /* Always clear migrating flag if it's set */ > migrating = test_and_clear_bit(...) > > if ( !migrating && ...) { > } > > Then we wouldn't need the last paragraph in the comment. > Yes, I think I like this better. > That said, this is v3, so if you'd rather just get this in as it is, > then you can have my Acked-by as well. > No, I'll resend... If I make (only) this change, can I resend directly with your Acked-by? Thanks and Regards, Dario
On 24/02/16 11:12, Dario Faggioli wrote: > On Wed, 2016-02-24 at 10:43 +0000, George Dunlap wrote: >> On 12/02/16 16:29, Dario Faggioli wrote: >>> >>> @@ -1022,11 +1037,18 @@ csched_vcpu_wake(const struct scheduler >>> *ops, struct vcpu *vc) >>> * more CPU resource intensive VCPUs without impacting >>> overall >>> * system fairness. >>> * >>> - * The one exception is for VCPUs of capped domains unpausing >>> - * after earning credits they had overspent. We don't boost >>> - * those. >>> + * There are two cases, when we don't want to boost: >>> + * - VCPUs that are waking up after a migration, rather than >>> + * after having block; >>> + * - VCPUs of capped domains unpausing after earning credits >>> + * they had overspent. >>> + * >>> + * Note that checking whether we are "only" migrating must be >>> + * done up front, as we do not want the clearing of the bit we >>> + * set in csched_cpu_pick() to be short-circuited away. >>> */ >>> - if ( svc->pri == CSCHED_PRI_TS_UNDER && >>> + if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc- >>>> flags) && >>> + svc->pri == CSCHED_PRI_TS_UNDER && >>> !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) ) >> >> Sorry to be late reviewing this. >> > No problem. Thanks for getting to it, actually, as I've got a few more > patches stacked on top of these outstanding series. > >> So we always want to clear the 'migrating' flag, regardless of >> whether >> we do anything with boosting. Would that logic be clearer if we >> cleared >> it as a separate step, storing the result in a local variable? E.g.: >> >> bool migrating; >> >> ... >> >> /* Always clear migrating flag if it's set */ >> migrating = test_and_clear_bit(...) >> >> if ( !migrating && ...) { >> } >> >> Then we wouldn't need the last paragraph in the comment. >> > Yes, I think I like this better. > >> That said, this is v3, so if you'd rather just get this in as it is, >> then you can have my Acked-by as well. >> > No, I'll resend... If I make (only) this change, can I resend directly > with your Acked-by? Well I won't know what it looks like until I see it, will I? :-) I prioritize recently-reviewed series, so if you resend I should be able to give an Acked-by the same day. -George
On Wed, 2016-02-24 at 11:14 +0000, George Dunlap wrote: > On 24/02/16 11:12, Dario Faggioli wrote: > > > > No, I'll resend... If I make (only) this change, can I resend > > directly > > with your Acked-by? > > Well I won't know what it looks like until I see it, will I? :-) > I prioritize recently-reviewed series, so if you resend I should be > able > to give an Acked-by the same day. > And I've been sidetracked by a few things, but v4 is out: http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03445.html <20160224174239.13376.58865.stgit@Solace.station> And given what time it is, feel free to Ack it tomorrow. :-) Thanks and Regards, Dario
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c index 5708701..597a784 100644 --- a/xen/common/sched_credit.c +++ b/xen/common/sched_credit.c @@ -63,9 +63,14 @@ /* * Flags + * + * Note that svc->flags (where these flags live) is protected by an + * inconsistent set of locks. Therefore atomic-safe bit operations must + * be used for accessing it. */ #define CSCHED_FLAG_VCPU_PARKED 0x0 /* VCPU over capped credits */ #define CSCHED_FLAG_VCPU_YIELD 0x1 /* VCPU yielding */ +#define CSCHED_FLAG_VCPU_MIGRATING 0x2 /* VCPU may have moved to a new pcpu */ /* @@ -787,6 +792,16 @@ _csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc, bool_t commit) static int csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc) { + struct csched_vcpu *svc = CSCHED_VCPU(vc); + + /* + * We have been called by vcpu_migrate() (in schedule.c), as part + * of the process of seeing if vc can be migrated to another pcpu. + * We make a note about this in svc->flags so that later, in + * csched_vcpu_wake() (still called from vcpu_migrate()) we won't + * get boosted, which we don't deserve as we are "only" migrating. + */ + set_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc->flags); return _csched_cpu_pick(ops, vc, 1); } @@ -1022,11 +1037,18 @@ csched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc) * more CPU resource intensive VCPUs without impacting overall * system fairness. * - * The one exception is for VCPUs of capped domains unpausing - * after earning credits they had overspent. We don't boost - * those. + * There are two cases, when we don't want to boost: + * - VCPUs that are waking up after a migration, rather than + * after having block; + * - VCPUs of capped domains unpausing after earning credits + * they had overspent. + * + * Note that checking whether we are "only" migrating must be + * done up front, as we do not want the clearing of the bit we + * set in csched_cpu_pick() to be short-circuited away. */ - if ( svc->pri == CSCHED_PRI_TS_UNDER && + if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc->flags) && + svc->pri == CSCHED_PRI_TS_UNDER && !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) ) { TRACE_2D(TRC_CSCHED_BOOST_START, vc->domain->domain_id, vc->vcpu_id);
Moving a vCPU to a different pCPU means offlining it and then waking it up, on the new pCPU. Credit1 grants BOOST priority to vCPUs that wakes up, with the aim of improving I/O latency. The net effect of this all is that vCPUs get boosted when migrating, which shouldn't happen. For instance, this causes scheduling anomalies and, potentially, performance problems, as reported here: http://lists.xen.org/archives/html/xen-devel/2015-10/msg02851.html This patch fixes this by noting down (by means of a flag) the fact that the vCPU is about to undergo a migration. This way we can tell, later, during a wakeup, whether the vCPU is migrating or unblocking, and decide whether or not to apply the boosting. Note that it is important that atomic-safe bit operations are used when manipulating vCPUs' flags. Take the chance and add a comment about this. Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Jan Beulich <JBeulich@suse.com> --- Changes from v2: * test_and_clear() is necessary when accessing svc->flags; * added a comment about such need at the top, where the flags are defined. Changes from v1: * rewritten, following suggestion got during review: there are no wakeup flags any longer, and all is done in sched_credit.c by setting a flag in csched_cpu_pick() and testing (and cleating) it in csched_vcpu_wake(). --- xen/common/sched_credit.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)