* [PATCH 0/2] xen: sched: rtds refactor code
@ 2016-05-15 23:54 Tianyang Chen
2016-05-15 23:54 ` [PATCH 1/2] " Tianyang Chen
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Tianyang Chen @ 2016-05-15 23:54 UTC (permalink / raw)
To: xen-devel; +Cc: dario.faggioli, Tianyang Chen, george.dunlap, mengxu
The first part of this patch series aims at fixing coding syle issue
for control structures. Because locks are grabbed in schedule.c before
hooks are called, underscores in front of function names are removed.
The second part replaces atomic bit-ops with non-atomic ones since locks
are grabbed in schedule.c.
Discussions:
http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01528.html
http://www.gossamer-threads.com/lists/xen/devel/431251?do=post_view_threaded#431251
Tianyang Chen (2):
xen: sched: rtds refactor code
xen: sched: rtds: use non-atomic bit-ops
xen/common/sched_rt.c | 122 ++++++++++++++++++++++++++-----------------------
1 file changed, 64 insertions(+), 58 deletions(-)
--
1.7.9.5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] xen: sched: rtds refactor code 2016-05-15 23:54 [PATCH 0/2] xen: sched: rtds refactor code Tianyang Chen @ 2016-05-15 23:54 ` Tianyang Chen 2016-05-17 15:06 ` Meng Xu 2016-06-22 15:51 ` George Dunlap 2016-05-15 23:54 ` [PATCH 2/2] xen: sched: rtds: use non-atomic bit-ops Tianyang Chen 2016-05-17 15:05 ` [PATCH 0/2] xen: sched: rtds refactor code Meng Xu 2 siblings, 2 replies; 11+ messages in thread From: Tianyang Chen @ 2016-05-15 23:54 UTC (permalink / raw) To: xen-devel; +Cc: dario.faggioli, Tianyang Chen, george.dunlap, mengxu No functional change: -Various coding style fix -Added comments for UPDATE_LIMIT_SHIFT. Signed-off-by: Tianyang Chen <tiche@seas.upenn.edu> --- xen/common/sched_rt.c | 106 ++++++++++++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 50 deletions(-) diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c index 7f8f411..1584d53 100644 --- a/xen/common/sched_rt.c +++ b/xen/common/sched_rt.c @@ -80,7 +80,7 @@ * in schedule.c * * The functions involes RunQ and needs to grab locks are: - * vcpu_insert, vcpu_remove, context_saved, __runq_insert + * vcpu_insert, vcpu_remove, context_saved, runq_insert */ @@ -107,6 +107,12 @@ */ #define RTDS_MIN_BUDGET (MICROSECS(10)) +/* + * UPDATE_LIMIT_SHIT: a constant used in rt_update_deadline(). When finding + * the next deadline, performing addition could be faster if the difference + * between cur_deadline and now is small. If the difference is bigger than + * 1024 * period, use multiplication. + */ #define UPDATE_LIMIT_SHIFT 10 /* @@ -158,25 +164,25 @@ static void repl_timer_handler(void *data); /* - * Systme-wide private data, include global RunQueue/DepletedQ + * System-wide private data, include global RunQueue/DepletedQ * Global lock is referenced by schedule_data.schedule_lock from all * physical cpus. It can be grabbed via vcpu_schedule_lock_irq() */ struct rt_private { - spinlock_t lock; /* the global coarse grand lock */ - struct list_head sdom; /* list of availalbe domains, used for dump */ - struct list_head runq; /* ordered list of runnable vcpus */ - struct list_head depletedq; /* unordered list of depleted vcpus */ - struct list_head replq; /* ordered list of vcpus that need replenishment */ - cpumask_t tickled; /* cpus been tickled */ - struct timer *repl_timer; /* replenishment timer */ + spinlock_t lock; /* the global coarse grand lock */ + struct list_head sdom; /* list of availalbe domains, used for dump */ + struct list_head runq; /* ordered list of runnable vcpus */ + struct list_head depletedq; /* unordered list of depleted vcpus */ + struct list_head replq; /* ordered list of vcpus that need replenishment */ + cpumask_t tickled; /* cpus been tickled */ + struct timer *repl_timer; /* replenishment timer */ }; /* * Virtual CPU */ struct rt_vcpu { - struct list_head q_elem; /* on the runq/depletedq list */ + struct list_head q_elem; /* on the runq/depletedq list */ struct list_head replq_elem; /* on the replenishment events list */ /* Up-pointers */ @@ -188,19 +194,19 @@ struct rt_vcpu { s_time_t budget; /* VCPU current infomation in nanosecond */ - s_time_t cur_budget; /* current budget */ - s_time_t last_start; /* last start time */ - s_time_t cur_deadline; /* current deadline for EDF */ + s_time_t cur_budget; /* current budget */ + s_time_t last_start; /* last start time */ + s_time_t cur_deadline; /* current deadline for EDF */ - unsigned flags; /* mark __RTDS_scheduled, etc.. */ + unsigned flags; /* mark __RTDS_scheduled, etc.. */ }; /* * Domain */ struct rt_dom { - struct list_head sdom_elem; /* link list on rt_priv */ - struct domain *dom; /* pointer to upper domain */ + struct list_head sdom_elem; /* link list on rt_priv */ + struct domain *dom; /* pointer to upper domain */ }; /* @@ -241,13 +247,13 @@ static inline struct list_head *rt_replq(const struct scheduler *ops) * and the replenishment events queue. */ static int -__vcpu_on_q(const struct rt_vcpu *svc) +vcpu_on_q(const struct rt_vcpu *svc) { return !list_empty(&svc->q_elem); } static struct rt_vcpu * -__q_elem(struct list_head *elem) +q_elem(struct list_head *elem) { return list_entry(elem, struct rt_vcpu, q_elem); } @@ -303,7 +309,7 @@ rt_dump_vcpu(const struct scheduler *ops, const struct rt_vcpu *svc) svc->cur_budget, svc->cur_deadline, svc->last_start, - __vcpu_on_q(svc), + vcpu_on_q(svc), vcpu_runnable(svc->vcpu), svc->flags, keyhandler_scratch); @@ -339,28 +345,28 @@ rt_dump(const struct scheduler *ops) replq = rt_replq(ops); printk("Global RunQueue info:\n"); - list_for_each( iter, runq ) + list_for_each ( iter, runq ) { - svc = __q_elem(iter); + svc = q_elem(iter); rt_dump_vcpu(ops, svc); } printk("Global DepletedQueue info:\n"); - list_for_each( iter, depletedq ) + list_for_each ( iter, depletedq ) { - svc = __q_elem(iter); + svc = q_elem(iter); rt_dump_vcpu(ops, svc); } printk("Global Replenishment Events info:\n"); - list_for_each( iter, replq ) + list_for_each ( iter, replq ) { svc = replq_elem(iter); rt_dump_vcpu(ops, svc); } printk("Domain info:\n"); - list_for_each( iter, &prv->sdom ) + list_for_each ( iter, &prv->sdom ) { struct vcpu *v; @@ -380,7 +386,7 @@ rt_dump(const struct scheduler *ops) /* * update deadline and budget when now >= cur_deadline - * it need to be updated to the deadline of the current period + * it needs to be updated to the deadline of the current period */ static void rt_update_deadline(s_time_t now, struct rt_vcpu *svc) @@ -463,14 +469,14 @@ deadline_queue_insert(struct rt_vcpu * (*qelem)(struct list_head *), return !pos; } #define deadline_runq_insert(...) \ - deadline_queue_insert(&__q_elem, ##__VA_ARGS__) + deadline_queue_insert(&q_elem, ##__VA_ARGS__) #define deadline_replq_insert(...) \ deadline_queue_insert(&replq_elem, ##__VA_ARGS__) static inline void -__q_remove(struct rt_vcpu *svc) +q_remove(struct rt_vcpu *svc) { - ASSERT( __vcpu_on_q(svc) ); + ASSERT( vcpu_on_q(svc) ); list_del_init(&svc->q_elem); } @@ -506,13 +512,13 @@ replq_remove(const struct scheduler *ops, struct rt_vcpu *svc) * Insert svc without budget in DepletedQ unsorted; */ static void -__runq_insert(const struct scheduler *ops, struct rt_vcpu *svc) +runq_insert(const struct scheduler *ops, struct rt_vcpu *svc) { struct rt_private *prv = rt_priv(ops); struct list_head *runq = rt_runq(ops); ASSERT( spin_is_locked(&prv->lock) ); - ASSERT( !__vcpu_on_q(svc) ); + ASSERT( !vcpu_on_q(svc) ); ASSERT( vcpu_on_replq(svc) ); /* add svc to runq if svc still has budget */ @@ -840,12 +846,12 @@ rt_vcpu_insert(const struct scheduler *ops, struct vcpu *vc) if ( now >= svc->cur_deadline ) rt_update_deadline(now, svc); - if ( !__vcpu_on_q(svc) && vcpu_runnable(vc) ) + if ( !vcpu_on_q(svc) && vcpu_runnable(vc) ) { replq_insert(ops, svc); if ( !vc->is_running ) - __runq_insert(ops, svc); + runq_insert(ops, svc); } vcpu_schedule_unlock_irq(lock, vc); @@ -867,8 +873,8 @@ rt_vcpu_remove(const struct scheduler *ops, struct vcpu *vc) BUG_ON( sdom == NULL ); lock = vcpu_schedule_lock_irq(vc); - if ( __vcpu_on_q(svc) ) - __q_remove(svc); + if ( vcpu_on_q(svc) ) + q_remove(svc); if ( vcpu_on_replq(svc) ) replq_remove(ops,svc); @@ -955,7 +961,7 @@ burn_budget(const struct scheduler *ops, struct rt_vcpu *svc, s_time_t now) * lock is grabbed before calling this function */ static struct rt_vcpu * -__runq_pick(const struct scheduler *ops, const cpumask_t *mask) +runq_pick(const struct scheduler *ops, const cpumask_t *mask) { struct list_head *runq = rt_runq(ops); struct list_head *iter; @@ -964,9 +970,9 @@ __runq_pick(const struct scheduler *ops, const cpumask_t *mask) cpumask_t cpu_common; cpumask_t *online; - list_for_each(iter, runq) + list_for_each ( iter, runq ) { - iter_svc = __q_elem(iter); + iter_svc = q_elem(iter); /* mask cpu_hard_affinity & cpupool & mask */ online = cpupool_domain_cpumask(iter_svc->vcpu->domain); @@ -1028,7 +1034,7 @@ rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched } else { - snext = __runq_pick(ops, cpumask_of(cpu)); + snext = runq_pick(ops, cpumask_of(cpu)); if ( snext == NULL ) snext = rt_vcpu(idle_vcpu[cpu]); @@ -1052,7 +1058,7 @@ rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched { if ( snext != scurr ) { - __q_remove(snext); + q_remove(snext); set_bit(__RTDS_scheduled, &snext->flags); } if ( snext->vcpu->processor != cpu ) @@ -1081,9 +1087,9 @@ rt_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc) if ( curr_on_cpu(vc->processor) == vc ) cpu_raise_softirq(vc->processor, SCHEDULE_SOFTIRQ); - else if ( __vcpu_on_q(svc) ) + else if ( vcpu_on_q(svc) ) { - __q_remove(svc); + q_remove(svc); replq_remove(ops, svc); } else if ( svc->flags & RTDS_delayed_runq_add ) @@ -1201,7 +1207,7 @@ rt_vcpu_wake(const struct scheduler *ops, struct vcpu *vc) } /* on RunQ/DepletedQ, just update info is ok */ - if ( unlikely(__vcpu_on_q(svc)) ) + if ( unlikely(vcpu_on_q(svc)) ) { SCHED_STAT_CRANK(vcpu_wake_onrunq); return; @@ -1245,7 +1251,7 @@ rt_vcpu_wake(const struct scheduler *ops, struct vcpu *vc) /* Replenishment event got cancelled when we blocked. Add it back. */ replq_insert(ops, svc); /* insert svc to runq/depletedq because svc is not in queue now */ - __runq_insert(ops, svc); + runq_insert(ops, svc); runq_tickle(ops, svc); } @@ -1268,7 +1274,7 @@ rt_context_saved(const struct scheduler *ops, struct vcpu *vc) if ( test_and_clear_bit(__RTDS_delayed_runq_add, &svc->flags) && likely(vcpu_runnable(vc)) ) { - __runq_insert(ops, svc); + runq_insert(ops, svc); runq_tickle(ops, svc); } else @@ -1414,10 +1420,10 @@ static void repl_timer_handler(void *data){ rt_update_deadline(now, svc); list_add(&svc->replq_elem, &tmp_replq); - if ( __vcpu_on_q(svc) ) + if ( vcpu_on_q(svc) ) { - __q_remove(svc); - __runq_insert(ops, svc); + q_remove(svc); + runq_insert(ops, svc); } } @@ -1435,12 +1441,12 @@ static void repl_timer_handler(void *data){ if ( curr_on_cpu(svc->vcpu->processor) == svc->vcpu && !list_empty(runq) ) { - struct rt_vcpu *next_on_runq = __q_elem(runq->next); + struct rt_vcpu *next_on_runq = q_elem(runq->next); if ( svc->cur_deadline > next_on_runq->cur_deadline ) runq_tickle(ops, next_on_runq); } - else if ( __vcpu_on_q(svc) && + else if ( vcpu_on_q(svc) && test_and_clear_bit(__RTDS_depleted, &svc->flags) ) runq_tickle(ops, svc); -- 1.7.9.5 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xen: sched: rtds refactor code 2016-05-15 23:54 ` [PATCH 1/2] " Tianyang Chen @ 2016-05-17 15:06 ` Meng Xu 2016-06-22 15:51 ` George Dunlap 1 sibling, 0 replies; 11+ messages in thread From: Meng Xu @ 2016-05-17 15:06 UTC (permalink / raw) To: Tianyang Chen; +Cc: xen-devel, Dario Faggioli, George Dunlap On Sun, May 15, 2016 at 7:54 PM, Tianyang Chen <tiche@seas.upenn.edu> wrote: > No functional change: > -Various coding style fix > -Added comments for UPDATE_LIMIT_SHIFT. > > Signed-off-by: Tianyang Chen <tiche@seas.upenn.edu> Reviewed-by: Meng Xu <mengxu@cis.upenn.edu> ----------- 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 http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xen: sched: rtds refactor code 2016-05-15 23:54 ` [PATCH 1/2] " Tianyang Chen 2016-05-17 15:06 ` Meng Xu @ 2016-06-22 15:51 ` George Dunlap 2016-06-22 16:16 ` Meng Xu 1 sibling, 1 reply; 11+ messages in thread From: George Dunlap @ 2016-06-22 15:51 UTC (permalink / raw) To: Tianyang Chen; +Cc: xen-devel, Dario Faggioli, Meng Xu On Mon, May 16, 2016 at 12:54 AM, Tianyang Chen <tiche@seas.upenn.edu> wrote: > No functional change: > -Various coding style fix > -Added comments for UPDATE_LIMIT_SHIFT. > > Signed-off-by: Tianyang Chen <tiche@seas.upenn.edu> Hey Tianyang, The changes here for the most part look good (with a few comments -- see below), but the title and changelog could use some work. For one, you're not actually doing any refactoring -- I'd call this patch a "clean-up" patch. Secondly, you should go through and enumerate the different clean-ups you do. For instance, you mention why you remove the __ at the head of functions in your cover letter, but you don't mention it here. > --- > xen/common/sched_rt.c | 106 ++++++++++++++++++++++++++----------------------- > 1 file changed, 56 insertions(+), 50 deletions(-) > > diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c > index 7f8f411..1584d53 100644 > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c > @@ -80,7 +80,7 @@ > * in schedule.c > * > * The functions involes RunQ and needs to grab locks are: > - * vcpu_insert, vcpu_remove, context_saved, __runq_insert > + * vcpu_insert, vcpu_remove, context_saved, runq_insert > */ > > > @@ -107,6 +107,12 @@ > */ > #define RTDS_MIN_BUDGET (MICROSECS(10)) > > +/* > + * UPDATE_LIMIT_SHIT: a constant used in rt_update_deadline(). When finding Missing an 'F'. :-) > + * the next deadline, performing addition could be faster if the difference > + * between cur_deadline and now is small. If the difference is bigger than > + * 1024 * period, use multiplication. > + */ > #define UPDATE_LIMIT_SHIFT 10 > > /* > @@ -158,25 +164,25 @@ > static void repl_timer_handler(void *data); > > /* > - * Systme-wide private data, include global RunQueue/DepletedQ > + * System-wide private data, include global RunQueue/DepletedQ > * Global lock is referenced by schedule_data.schedule_lock from all > * physical cpus. It can be grabbed via vcpu_schedule_lock_irq() > */ > struct rt_private { > - spinlock_t lock; /* the global coarse grand lock */ > - struct list_head sdom; /* list of availalbe domains, used for dump */ > - struct list_head runq; /* ordered list of runnable vcpus */ > - struct list_head depletedq; /* unordered list of depleted vcpus */ > - struct list_head replq; /* ordered list of vcpus that need replenishment */ > - cpumask_t tickled; /* cpus been tickled */ > - struct timer *repl_timer; /* replenishment timer */ > + spinlock_t lock; /* the global coarse grand lock */ * course-grained Also, I'm not sure what the point of indenting all these comments out an extra space is. I don't object, of course, if Meng doesn't object, but at very least it could use a one-line explanation in the changelog. Otherwise, looks good, thanks. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xen: sched: rtds refactor code 2016-06-22 15:51 ` George Dunlap @ 2016-06-22 16:16 ` Meng Xu 2016-06-23 10:42 ` George Dunlap 0 siblings, 1 reply; 11+ messages in thread From: Meng Xu @ 2016-06-22 16:16 UTC (permalink / raw) To: George Dunlap; +Cc: xen-devel, Dario Faggioli, Tianyang Chen On Wed, Jun 22, 2016 at 11:51 AM, George Dunlap <george.dunlap@citrix.com> wrote: > On Mon, May 16, 2016 at 12:54 AM, Tianyang Chen <tiche@seas.upenn.edu> wrote: >> No functional change: >> -Various coding style fix >> -Added comments for UPDATE_LIMIT_SHIFT. >> >> Signed-off-by: Tianyang Chen <tiche@seas.upenn.edu> > > Hey Tianyang, > > The changes here for the most part look good (with a few comments -- > see below), but the title and changelog could use some work. > > For one, you're not actually doing any refactoring -- I'd call this > patch a "clean-up" patch. > > Secondly, you should go through and enumerate the different clean-ups > you do. For instance, you mention why you remove the __ at the head > of functions in your cover letter, but you don't mention it here. > >> --- >> xen/common/sched_rt.c | 106 ++++++++++++++++++++++++++----------------------- >> 1 file changed, 56 insertions(+), 50 deletions(-) >> >> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c >> index 7f8f411..1584d53 100644 >> --- a/xen/common/sched_rt.c >> +++ b/xen/common/sched_rt.c >> @@ -80,7 +80,7 @@ >> * in schedule.c >> * >> * The functions involes RunQ and needs to grab locks are: >> - * vcpu_insert, vcpu_remove, context_saved, __runq_insert >> + * vcpu_insert, vcpu_remove, context_saved, runq_insert >> */ >> >> >> @@ -107,6 +107,12 @@ >> */ >> #define RTDS_MIN_BUDGET (MICROSECS(10)) >> >> +/* >> + * UPDATE_LIMIT_SHIT: a constant used in rt_update_deadline(). When finding > > Missing an 'F'. :-) Ah, my bad.. I should have caught these typos. :-( > >> + * the next deadline, performing addition could be faster if the difference >> + * between cur_deadline and now is small. If the difference is bigger than >> + * 1024 * period, use multiplication. >> + */ >> #define UPDATE_LIMIT_SHIFT 10 >> >> /* >> @@ -158,25 +164,25 @@ >> static void repl_timer_handler(void *data); >> >> /* >> - * Systme-wide private data, include global RunQueue/DepletedQ >> + * System-wide private data, include global RunQueue/DepletedQ >> * Global lock is referenced by schedule_data.schedule_lock from all >> * physical cpus. It can be grabbed via vcpu_schedule_lock_irq() >> */ >> struct rt_private { >> - spinlock_t lock; /* the global coarse grand lock */ >> - struct list_head sdom; /* list of availalbe domains, used for dump */ >> - struct list_head runq; /* ordered list of runnable vcpus */ >> - struct list_head depletedq; /* unordered list of depleted vcpus */ >> - struct list_head replq; /* ordered list of vcpus that need replenishment */ >> - cpumask_t tickled; /* cpus been tickled */ >> - struct timer *repl_timer; /* replenishment timer */ >> + spinlock_t lock; /* the global coarse grand lock */ > > * course-grained > > Also, I'm not sure what the point of indenting all these comments out > an extra space is. I don't object, of course, if Meng doesn't object, > but at very least it could use a one-line explanation in the > changelog. I think he is trying to align those comments to make them start from the same column. I was confused at the reason at the very beginning. Then I pulled his repo and checked this change. 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 http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xen: sched: rtds refactor code 2016-06-22 16:16 ` Meng Xu @ 2016-06-23 10:42 ` George Dunlap 2016-06-24 7:45 ` Dario Faggioli 0 siblings, 1 reply; 11+ messages in thread From: George Dunlap @ 2016-06-23 10:42 UTC (permalink / raw) To: Meng Xu; +Cc: xen-devel, Dario Faggioli, Tianyang Chen On 22/06/16 17:16, Meng Xu wrote: > On Wed, Jun 22, 2016 at 11:51 AM, George Dunlap > <george.dunlap@citrix.com> wrote: >> On Mon, May 16, 2016 at 12:54 AM, Tianyang Chen <tiche@seas.upenn.edu> wrote: >>> No functional change: >>> -Various coding style fix >>> -Added comments for UPDATE_LIMIT_SHIFT. >>> >>> Signed-off-by: Tianyang Chen <tiche@seas.upenn.edu> >> >> Hey Tianyang, >> >> The changes here for the most part look good (with a few comments -- >> see below), but the title and changelog could use some work. >> >> For one, you're not actually doing any refactoring -- I'd call this >> patch a "clean-up" patch. >> >> Secondly, you should go through and enumerate the different clean-ups >> you do. For instance, you mention why you remove the __ at the head >> of functions in your cover letter, but you don't mention it here. >> >>> --- >>> xen/common/sched_rt.c | 106 ++++++++++++++++++++++++++----------------------- >>> 1 file changed, 56 insertions(+), 50 deletions(-) >>> >>> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c >>> index 7f8f411..1584d53 100644 >>> --- a/xen/common/sched_rt.c >>> +++ b/xen/common/sched_rt.c >>> @@ -80,7 +80,7 @@ >>> * in schedule.c >>> * >>> * The functions involes RunQ and needs to grab locks are: >>> - * vcpu_insert, vcpu_remove, context_saved, __runq_insert >>> + * vcpu_insert, vcpu_remove, context_saved, runq_insert >>> */ >>> >>> >>> @@ -107,6 +107,12 @@ >>> */ >>> #define RTDS_MIN_BUDGET (MICROSECS(10)) >>> >>> +/* >>> + * UPDATE_LIMIT_SHIT: a constant used in rt_update_deadline(). When finding >> >> Missing an 'F'. :-) > > Ah, my bad.. I should have caught these typos. :-( > >> >>> + * the next deadline, performing addition could be faster if the difference >>> + * between cur_deadline and now is small. If the difference is bigger than >>> + * 1024 * period, use multiplication. >>> + */ >>> #define UPDATE_LIMIT_SHIFT 10 >>> >>> /* >>> @@ -158,25 +164,25 @@ >>> static void repl_timer_handler(void *data); >>> >>> /* >>> - * Systme-wide private data, include global RunQueue/DepletedQ >>> + * System-wide private data, include global RunQueue/DepletedQ >>> * Global lock is referenced by schedule_data.schedule_lock from all >>> * physical cpus. It can be grabbed via vcpu_schedule_lock_irq() >>> */ >>> struct rt_private { >>> - spinlock_t lock; /* the global coarse grand lock */ >>> - struct list_head sdom; /* list of availalbe domains, used for dump */ >>> - struct list_head runq; /* ordered list of runnable vcpus */ >>> - struct list_head depletedq; /* unordered list of depleted vcpus */ >>> - struct list_head replq; /* ordered list of vcpus that need replenishment */ >>> - cpumask_t tickled; /* cpus been tickled */ >>> - struct timer *repl_timer; /* replenishment timer */ >>> + spinlock_t lock; /* the global coarse grand lock */ >> >> * course-grained >> >> Also, I'm not sure what the point of indenting all these comments out >> an extra space is. I don't object, of course, if Meng doesn't object, >> but at very least it could use a one-line explanation in the >> changelog. > > I think he is trying to align those comments to make them start from > the same column. I was confused at the reason at the very beginning. > Then I pulled his repo and checked this change. Right -- well neither you as a reviewer nor anyone in the future looking back at this changeset should have to try to guess what the purpose was; if he did want to align them, that's perfectly fine, it just needs a brief mention in the changelog. :-) -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xen: sched: rtds refactor code 2016-06-23 10:42 ` George Dunlap @ 2016-06-24 7:45 ` Dario Faggioli 2016-06-24 11:36 ` Meng Xu 0 siblings, 1 reply; 11+ messages in thread From: Dario Faggioli @ 2016-06-24 7:45 UTC (permalink / raw) To: George Dunlap, Meng Xu; +Cc: xen-devel, Tianyang Chen [-- Attachment #1.1: Type: text/plain, Size: 1349 bytes --] On Thu, 2016-06-23 at 11:42 +0100, George Dunlap wrote: > On 22/06/16 17:16, Meng Xu wrote: > > > > I think he is trying to align those comments to make them start > > from > > the same column. I was confused at the reason at the very > > beginning. > > Then I pulled his repo and checked this change. > Right -- well neither you as a reviewer nor anyone in the future > looking > back at this changeset should have to try to guess what the purpose > was; > if he did want to align them, that's perfectly fine, it just needs a > brief mention in the changelog. :-) > Indeed. BTW, I don't recall if we discussed this alignment previously,neither, in case we did, what my position was back then :-P In any case, I (now) think that having these comments aligned on a per-struct base is just fine, and there really is no need to have _all_ of them aligned, across all structs. I don't have a super strong opinion on this, and I'd be fine with it, if Meng is. I just think it's not worth the effort (of patching, reviewing, checking in, etc.) 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: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xen: sched: rtds refactor code 2016-06-24 7:45 ` Dario Faggioli @ 2016-06-24 11:36 ` Meng Xu 0 siblings, 0 replies; 11+ messages in thread From: Meng Xu @ 2016-06-24 11:36 UTC (permalink / raw) To: Dario Faggioli; +Cc: xen-devel, Tianyang Chen, George Dunlap On Fri, Jun 24, 2016 at 3:45 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Thu, 2016-06-23 at 11:42 +0100, George Dunlap wrote: >> On 22/06/16 17:16, Meng Xu wrote: >> > >> > I think he is trying to align those comments to make them start >> > from >> > the same column. I was confused at the reason at the very >> > beginning. >> > Then I pulled his repo and checked this change. >> Right -- well neither you as a reviewer nor anyone in the future >> looking >> back at this changeset should have to try to guess what the purpose >> was; >> if he did want to align them, that's perfectly fine, it just needs a >> brief mention in the changelog. :-) >> > Indeed. BTW, I don't recall if we discussed this alignment > previously,neither, in case we did, what my position was back then :-P > > In any case, I (now) think that having these comments aligned on a > per-struct base is just fine, and there really is no need to have _all_ > of them aligned, across all structs. Agree.. > > I don't have a super strong opinion on this, and I'd be fine with it, > if Meng is. I just think it's not worth the effort (of patching, > reviewing, checking in, etc.) Hmm, I don't have a strong opinion on this either. I agree with George's comments on the commit message. I think it should make this code change easier to understand in the future. (Well, the code change is not hard to understand. So the improvement of the commit message is not that critical.) Now it's a matter of perfection or not. If we want a "better" commit, I don't mind reviewing a new version. I don't think this patch has been committed yet. So the extra work is in our side, if we want to resend the patch. The work is as follows: 1) Replace "refactor" with "clean-up" for the patch title. 2) Go through and enumerate the different clean-ups this patch does. Explain the reason for each type of the clean-ups. For instance, why remove the __ at the head of functions (as described in the cover letter) OK. We need an action. How about: Tianyang send another version for the work listed above. I will review it again? 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 http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] xen: sched: rtds: use non-atomic bit-ops 2016-05-15 23:54 [PATCH 0/2] xen: sched: rtds refactor code Tianyang Chen 2016-05-15 23:54 ` [PATCH 1/2] " Tianyang Chen @ 2016-05-15 23:54 ` Tianyang Chen 2016-05-17 15:08 ` Meng Xu 2016-05-17 15:05 ` [PATCH 0/2] xen: sched: rtds refactor code Meng Xu 2 siblings, 1 reply; 11+ messages in thread From: Tianyang Chen @ 2016-05-15 23:54 UTC (permalink / raw) To: xen-devel; +Cc: dario.faggioli, Tianyang Chen, george.dunlap, mengxu Vcpu flags are checked and cleared atomically. Performance can be improved with corresponding non-atomic versions since schedule.c already has spin_locks in place. Signed-off-by: Tianyang Chen <tiche@seas.upenn.edu> --- xen/common/sched_rt.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c index 1584d53..1a18f6d 100644 --- a/xen/common/sched_rt.c +++ b/xen/common/sched_rt.c @@ -936,7 +936,7 @@ burn_budget(const struct scheduler *ops, struct rt_vcpu *svc, s_time_t now) if ( svc->cur_budget <= 0 ) { svc->cur_budget = 0; - set_bit(__RTDS_depleted, &svc->flags); + __set_bit(__RTDS_depleted, &svc->flags); } /* TRACE */ @@ -1050,7 +1050,7 @@ rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched if ( snext != scurr && !is_idle_vcpu(current) && vcpu_runnable(current) ) - set_bit(__RTDS_delayed_runq_add, &scurr->flags); + __set_bit(__RTDS_delayed_runq_add, &scurr->flags); snext->last_start = now; ret.time = -1; /* if an idle vcpu is picked */ @@ -1059,7 +1059,7 @@ rt_schedule(const struct scheduler *ops, s_time_t now, bool_t tasklet_work_sched if ( snext != scurr ) { q_remove(snext); - set_bit(__RTDS_scheduled, &snext->flags); + __set_bit(__RTDS_scheduled, &snext->flags); } if ( snext->vcpu->processor != cpu ) { @@ -1093,7 +1093,7 @@ rt_vcpu_sleep(const struct scheduler *ops, struct vcpu *vc) replq_remove(ops, svc); } else if ( svc->flags & RTDS_delayed_runq_add ) - clear_bit(__RTDS_delayed_runq_add, &svc->flags); + __clear_bit(__RTDS_delayed_runq_add, &svc->flags); } /* @@ -1235,7 +1235,7 @@ rt_vcpu_wake(const struct scheduler *ops, struct vcpu *vc) */ if ( unlikely(svc->flags & RTDS_scheduled) ) { - set_bit(__RTDS_delayed_runq_add, &svc->flags); + __set_bit(__RTDS_delayed_runq_add, &svc->flags); /* * The vcpu is waking up already, and we didn't even had the time to * remove its next replenishment event from the replenishment queue @@ -1266,12 +1266,12 @@ rt_context_saved(const struct scheduler *ops, struct vcpu *vc) struct rt_vcpu *svc = rt_vcpu(vc); spinlock_t *lock = vcpu_schedule_lock_irq(vc); - clear_bit(__RTDS_scheduled, &svc->flags); + __clear_bit(__RTDS_scheduled, &svc->flags); /* not insert idle vcpu to runq */ if ( is_idle_vcpu(vc) ) goto out; - if ( test_and_clear_bit(__RTDS_delayed_runq_add, &svc->flags) && + if ( __test_and_clear_bit(__RTDS_delayed_runq_add, &svc->flags) && likely(vcpu_runnable(vc)) ) { runq_insert(ops, svc); @@ -1447,7 +1447,7 @@ static void repl_timer_handler(void *data){ runq_tickle(ops, next_on_runq); } else if ( vcpu_on_q(svc) && - test_and_clear_bit(__RTDS_depleted, &svc->flags) ) + __test_and_clear_bit(__RTDS_depleted, &svc->flags) ) runq_tickle(ops, svc); list_del(&svc->replq_elem); -- 1.7.9.5 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xen: sched: rtds: use non-atomic bit-ops 2016-05-15 23:54 ` [PATCH 2/2] xen: sched: rtds: use non-atomic bit-ops Tianyang Chen @ 2016-05-17 15:08 ` Meng Xu 0 siblings, 0 replies; 11+ messages in thread From: Meng Xu @ 2016-05-17 15:08 UTC (permalink / raw) To: Tianyang Chen; +Cc: xen-devel, Dario Faggioli, George Dunlap On Sun, May 15, 2016 at 7:54 PM, Tianyang Chen <tiche@seas.upenn.edu> wrote: > Vcpu flags are checked and cleared atomically. Performance can be > improved with corresponding non-atomic versions since schedule.c > already has spin_locks in place. > > Signed-off-by: Tianyang Chen <tiche@seas.upenn.edu> 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 http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] xen: sched: rtds refactor code 2016-05-15 23:54 [PATCH 0/2] xen: sched: rtds refactor code Tianyang Chen 2016-05-15 23:54 ` [PATCH 1/2] " Tianyang Chen 2016-05-15 23:54 ` [PATCH 2/2] xen: sched: rtds: use non-atomic bit-ops Tianyang Chen @ 2016-05-17 15:05 ` Meng Xu 2 siblings, 0 replies; 11+ messages in thread From: Meng Xu @ 2016-05-17 15:05 UTC (permalink / raw) To: Tianyang Chen; +Cc: xen-devel, Dario Faggioli, George Dunlap On Sun, May 15, 2016 at 7:54 PM, Tianyang Chen <tiche@seas.upenn.edu> wrote: > The first part of this patch series aims at fixing coding syle issue > for control structures. Because locks are grabbed in schedule.c before > hooks are called, underscores in front of function names are removed. > > The second part replaces atomic bit-ops with non-atomic ones since locks > are grabbed in schedule.c. > > Discussions: > http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01528.html > http://www.gossamer-threads.com/lists/xen/devel/431251?do=post_view_threaded#431251 > > Tianyang Chen (2): > xen: sched: rtds refactor code > xen: sched: rtds: use non-atomic bit-ops > > xen/common/sched_rt.c | 122 ++++++++++++++++++++++++++----------------------- > 1 file changed, 64 insertions(+), 58 deletions(-) > Tianyang, Thanks for the patch! One comment for the future: please add the version number into the title so that we can easily tell it is a new patch. :-) Best, 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 http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-06-24 11:36 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-15 23:54 [PATCH 0/2] xen: sched: rtds refactor code Tianyang Chen 2016-05-15 23:54 ` [PATCH 1/2] " Tianyang Chen 2016-05-17 15:06 ` Meng Xu 2016-06-22 15:51 ` George Dunlap 2016-06-22 16:16 ` Meng Xu 2016-06-23 10:42 ` George Dunlap 2016-06-24 7:45 ` Dario Faggioli 2016-06-24 11:36 ` Meng Xu 2016-05-15 23:54 ` [PATCH 2/2] xen: sched: rtds: use non-atomic bit-ops Tianyang Chen 2016-05-17 15:08 ` Meng Xu 2016-05-17 15:05 ` [PATCH 0/2] xen: sched: rtds refactor code Meng Xu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).