| Message ID | 148977478448.22479.13625390869019347980.stgit@Palanthas.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/03/17 18:19, Dario Faggioli wrote: > Clarify and enforce (with ASSERTs) when the function > is called on the idle domain, and explain in comments > what it means and when it is ok to do so. > > While there, change the name of the function to a more > self-explanatory one, and do the same to VCPU2OP. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: Jan Beulich <jbeulich@suse.com> > --- > Changes from v1: > - new patch; > - renamed VCPU2OP, as suggested during v1's review of patch 1. > --- > xen/common/schedule.c | 56 ++++++++++++++++++++++++++++++++----------------- > 1 file changed, 37 insertions(+), 19 deletions(-) > > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > index d344b7c..fdb8ff4 100644 > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -77,8 +77,25 @@ static struct scheduler __read_mostly ops; > (( (opsptr)->fn != NULL ) ? (opsptr)->fn(opsptr, ##__VA_ARGS__ ) \ > : (typeof((opsptr)->fn(opsptr, ##__VA_ARGS__)))0 ) > > -#define DOM2OP(_d) (((_d)->cpupool == NULL) ? &ops : ((_d)->cpupool->sched)) > -static inline struct scheduler *VCPU2OP(const struct vcpu *v) > +static inline struct scheduler *dom_get_scheduler(const struct domain *d) Hmm -- I agree that VCPU2OP is probably not the right name, but I'm not a fan of the new name either; and I don't have an option I like better yet. With your permission I'll check in the first patch and come back to this. -George
On Mon, 2017-03-27 at 14:23 +0100, George Dunlap wrote: > On 17/03/17 18:19, Dario Faggioli wrote: > > --- a/xen/common/schedule.c > > +++ b/xen/common/schedule.c > > @@ -77,8 +77,25 @@ static struct scheduler __read_mostly ops; > > (( (opsptr)->fn != NULL ) ? (opsptr)->fn(opsptr, > > ##__VA_ARGS__ ) \ > > : (typeof((opsptr)->fn(opsptr, ##__VA_ARGS__)))0 ) > > > > -#define DOM2OP(_d) (((_d)->cpupool == NULL) ? &ops : ((_d)- > > >cpupool->sched)) > > -static inline struct scheduler *VCPU2OP(const struct vcpu *v) > > +static inline struct scheduler *dom_get_scheduler(const struct > > domain *d) > > Hmm -- I agree that VCPU2OP is probably not the right name, but I'm > not > a fan of the new name either; and I don't have an option I like > better yet. > > With your permission I'll check in the first patch and come back to > this. > Ok. The renaming was the least important part of what is done in this patch. But indeed I do think that, while here, we should take the chance the names as well (especially considering they won't be macro, so the "all capital letters" should go). But, sure, go ahead with the first patch... I'll try to think and propose a better name when/if I'll fine one. :-) Thanks, Dario
On Mon, 2017-03-27 at 14:23 +0100, George Dunlap wrote: > On 17/03/17 18:19, Dario Faggioli wrote: > > --- a/xen/common/schedule.c > > +++ b/xen/common/schedule.c > > @@ -77,8 +77,25 @@ static struct scheduler __read_mostly ops; > > (( (opsptr)->fn != NULL ) ? (opsptr)->fn(opsptr, > > ##__VA_ARGS__ ) \ > > : (typeof((opsptr)->fn(opsptr, ##__VA_ARGS__)))0 ) > > > > -#define DOM2OP(_d) (((_d)->cpupool == NULL) ? &ops : ((_d)- > > >cpupool->sched)) > > -static inline struct scheduler *VCPU2OP(const struct vcpu *v) > > +static inline struct scheduler *dom_get_scheduler(const struct > > domain *d) > > Hmm -- I agree that VCPU2OP is probably not the right name, but I'm > not > a fan of the new name either; and I don't have an option I like > better yet. > Maybe: domain_scheduler() vcpu_scheduler() or dom_scheduler() vcpu_scheduler() I.e., basically getting rid of the 'get' part, which may misleadingly hint at some kind of reference counting. Or, also trading 'scheduler' for 'ops': dom_ops() vcpu_ops() This is all I can come up with, my preference being {dom,vcpu}_scheduler(). Dario
On 06/04/17 12:59, Dario Faggioli wrote: > On Mon, 2017-03-27 at 14:23 +0100, George Dunlap wrote: >> On 17/03/17 18:19, Dario Faggioli wrote: >>> --- a/xen/common/schedule.c >>> +++ b/xen/common/schedule.c >>> @@ -77,8 +77,25 @@ static struct scheduler __read_mostly ops; >>> (( (opsptr)->fn != NULL ) ? (opsptr)->fn(opsptr, >>> ##__VA_ARGS__ ) \ >>> : (typeof((opsptr)->fn(opsptr, ##__VA_ARGS__)))0 ) >>> >>> -#define DOM2OP(_d) (((_d)->cpupool == NULL) ? &ops : ((_d)- >>>> cpupool->sched)) >>> -static inline struct scheduler *VCPU2OP(const struct vcpu *v) >>> +static inline struct scheduler *dom_get_scheduler(const struct >>> domain *d) >> >> Hmm -- I agree that VCPU2OP is probably not the right name, but I'm >> not >> a fan of the new name either; and I don't have an option I like >> better yet. >> > Maybe: > > domain_scheduler() > vcpu_scheduler() > > or > > dom_scheduler() > vcpu_scheduler() > > I.e., basically getting rid of the 'get' part, which may misleadingly > hint at some kind of reference counting. > > Or, also trading 'scheduler' for 'ops': > > dom_ops() > vcpu_ops() sched_ops_dom() sched_ops_vcpu() or sched_dom_ops() sched_vcpu_ops() Juergen
On Thu, 2017-04-06 at 13:06 +0200, Juergen Gross wrote: > On 06/04/17 12:59, Dario Faggioli wrote: > > Maybe: > > > > domain_scheduler() > > vcpu_scheduler() > > > > or > > > > dom_scheduler() > > vcpu_scheduler() > > > > Or, also trading 'scheduler' for 'ops': > > > > dom_ops() > > vcpu_ops() > > sched_ops_dom() > sched_ops_vcpu() > > or > > sched_dom_ops() > sched_vcpu_ops() > Yeah, these too. So, for now, I've resent using dom_scheduler() and vcpu_scheduler(). But I'm happy to change them to whatever and re-send again, as soon as George tells me which one is his preferred solution. :-D Thanks and Regards, Dario
diff --git a/xen/common/schedule.c b/xen/common/schedule.c index d344b7c..fdb8ff4 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -77,8 +77,25 @@ static struct scheduler __read_mostly ops; (( (opsptr)->fn != NULL ) ? (opsptr)->fn(opsptr, ##__VA_ARGS__ ) \ : (typeof((opsptr)->fn(opsptr, ##__VA_ARGS__)))0 ) -#define DOM2OP(_d) (((_d)->cpupool == NULL) ? &ops : ((_d)->cpupool->sched)) -static inline struct scheduler *VCPU2OP(const struct vcpu *v) +static inline struct scheduler *dom_get_scheduler(const struct domain *d) +{ + if ( likely(d->cpupool != NULL) ) + return d->cpupool->sched; + + /* + * If d->cpupool is NULL, this is the idle domain. This is special + * because the idle domain does not really bolong to any cpupool, and, + * hence, does not really have a scheduler. + * + * This is (should be!) only called like this for allocating the idle + * vCPUs for the first time, during boot, in which case what we want + * is the default scheduler that has been, choosen at boot. + */ + ASSERT(is_idle_domain(d)); + return &ops; +} + +static inline struct scheduler *vcpu_get_scheduler(const struct vcpu *v) { struct domain *d = v->domain; @@ -260,7 +277,8 @@ int sched_init_vcpu(struct vcpu *v, unsigned int processor) init_timer(&v->poll_timer, poll_timer_fn, v, v->processor); - v->sched_priv = SCHED_OP(DOM2OP(d), alloc_vdata, v, d->sched_priv); + v->sched_priv = SCHED_OP(dom_get_scheduler(d), alloc_vdata, v, + d->sched_priv); if ( v->sched_priv == NULL ) return 1; @@ -272,7 +290,7 @@ int sched_init_vcpu(struct vcpu *v, unsigned int processor) } else { - SCHED_OP(DOM2OP(d), insert_vcpu, v); + SCHED_OP(dom_get_scheduler(d), insert_vcpu, v); } return 0; @@ -326,7 +344,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c) domain_pause(d); - old_ops = DOM2OP(d); + old_ops = dom_get_scheduler(d); old_domdata = d->sched_priv; for_each_vcpu ( d, v ) @@ -389,8 +407,8 @@ void sched_destroy_vcpu(struct vcpu *v) kill_timer(&v->poll_timer); if ( test_and_clear_bool(v->is_urgent) ) atomic_dec(&per_cpu(schedule_data, v->processor).urgent_count); - SCHED_OP(VCPU2OP(v), remove_vcpu, v); - SCHED_OP(VCPU2OP(v), free_vdata, v->sched_priv); + SCHED_OP(vcpu_get_scheduler(v), remove_vcpu, v); + SCHED_OP(vcpu_get_scheduler(v), free_vdata, v->sched_priv); } int sched_init_domain(struct domain *d, int poolid) @@ -404,7 +422,7 @@ int sched_init_domain(struct domain *d, int poolid) SCHED_STAT_CRANK(dom_init); TRACE_1D(TRC_SCHED_DOM_ADD, d->domain_id); - return SCHED_OP(DOM2OP(d), init_domain, d); + return SCHED_OP(dom_get_scheduler(d), init_domain, d); } void sched_destroy_domain(struct domain *d) @@ -413,7 +431,7 @@ void sched_destroy_domain(struct domain *d) SCHED_STAT_CRANK(dom_destroy); TRACE_1D(TRC_SCHED_DOM_REM, d->domain_id); - SCHED_OP(DOM2OP(d), destroy_domain, d); + SCHED_OP(dom_get_scheduler(d), destroy_domain, d); cpupool_rm_domain(d); } @@ -432,7 +450,7 @@ void vcpu_sleep_nosync(struct vcpu *v) if ( v->runstate.state == RUNSTATE_runnable ) vcpu_runstate_change(v, RUNSTATE_offline, NOW()); - SCHED_OP(VCPU2OP(v), sleep, v); + SCHED_OP(vcpu_get_scheduler(v), sleep, v); } vcpu_schedule_unlock_irqrestore(lock, flags, v); @@ -461,7 +479,7 @@ void vcpu_wake(struct vcpu *v) { if ( v->runstate.state >= RUNSTATE_blocked ) vcpu_runstate_change(v, RUNSTATE_runnable, NOW()); - SCHED_OP(VCPU2OP(v), wake, v); + SCHED_OP(vcpu_get_scheduler(v), wake, v); } else if ( !(v->pause_flags & VPF_blocked) ) { @@ -516,8 +534,8 @@ static void vcpu_move_locked(struct vcpu *v, unsigned int new_cpu) * Actual CPU switch to new CPU. This is safe because the lock * pointer cant' change while the current lock is held. */ - if ( VCPU2OP(v)->migrate ) - SCHED_OP(VCPU2OP(v), migrate, v, new_cpu); + if ( vcpu_get_scheduler(v)->migrate ) + SCHED_OP(vcpu_get_scheduler(v), migrate, v, new_cpu); else v->processor = new_cpu; } @@ -583,7 +601,7 @@ static void vcpu_migrate(struct vcpu *v) break; /* Select a new CPU. */ - new_cpu = SCHED_OP(VCPU2OP(v), pick_cpu, v); + new_cpu = SCHED_OP(vcpu_get_scheduler(v), pick_cpu, v); if ( (new_lock == per_cpu(schedule_data, new_cpu).schedule_lock) && cpumask_test_cpu(new_cpu, v->domain->cpupool->cpu_valid) ) break; @@ -685,7 +703,7 @@ void restore_vcpu_affinity(struct domain *d) spin_unlock_irq(lock);; lock = vcpu_schedule_lock_irq(v); - v->processor = SCHED_OP(VCPU2OP(v), pick_cpu, v); + v->processor = SCHED_OP(vcpu_get_scheduler(v), pick_cpu, v); spin_unlock_irq(lock); } @@ -975,7 +993,7 @@ long vcpu_yield(void) struct vcpu * v=current; spinlock_t *lock = vcpu_schedule_lock_irq(v); - SCHED_OP(VCPU2OP(v), yield, v); + SCHED_OP(vcpu_get_scheduler(v), yield, v); vcpu_schedule_unlock_irq(lock, v); SCHED_STAT_CRANK(vcpu_yield); @@ -1288,7 +1306,7 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op) if ( ret ) return ret; - if ( op->sched_id != DOM2OP(d)->sched_id ) + if ( op->sched_id != dom_get_scheduler(d)->sched_id ) return -EINVAL; switch ( op->cmd ) @@ -1304,7 +1322,7 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op) /* NB: the pluggable scheduler code needs to take care * of locking by itself. */ - if ( (ret = SCHED_OP(DOM2OP(d), adjust, d, op)) == 0 ) + if ( (ret = SCHED_OP(dom_get_scheduler(d), adjust, d, op)) == 0 ) TRACE_1D(TRC_SCHED_ADJDOM, d->domain_id); return ret; @@ -1482,7 +1500,7 @@ void context_saved(struct vcpu *prev) /* Check for migration request /after/ clearing running flag. */ smp_mb(); - SCHED_OP(VCPU2OP(prev), context_saved, prev); + SCHED_OP(vcpu_get_scheduler(prev), context_saved, prev); if ( unlikely(prev->pause_flags & VPF_migrating) ) vcpu_migrate(prev);
Clarify and enforce (with ASSERTs) when the function is called on the idle domain, and explain in comments what it means and when it is ok to do so. While there, change the name of the function to a more self-explanatory one, and do the same to VCPU2OP. Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: George Dunlap <george.dunlap@citrix.com> Cc: Juergen Gross <jgross@suse.com> Cc: Jan Beulich <jbeulich@suse.com> --- Changes from v1: - new patch; - renamed VCPU2OP, as suggested during v1's review of patch 1. --- xen/common/schedule.c | 56 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 19 deletions(-)