Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1210135

Re: [PATCH v3 4/5] sched: sync a se with its cfs_rq when switching sched class to fair class

Show all headers | View raw

On Wed, Aug 19, 2015 at 07:12:41PM +0200, Peter Zijlstra wrote:
> On Wed, Aug 19, 2015 at 03:47:15PM +0900, byungchul.park@lge.com wrote:
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 1be042a..3419f6c 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -2711,6 +2711,17 @@ static inline void update_load_avg(struct sched_entity *se, int update_tg)
> >  
> >  static void attach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >  {
> > +	/*
> > +	 * in case of migration and cgroup-change, more care should be taken
> > +	 * because se's cfs_rq was changed, that means calling __update_load_avg
> > +	 * with new cfs_rq->avg.last_update_time is meaningless. so we skip the
> > +	 * update here. we have to update it with prev cfs_rq just before changing
> > +	 * se's cfs_rq, and get here soon.
> > +	 */
> > +	if (se->avg.last_update_time)
> > +		__update_load_avg(cfs_rq->avg.last_update_time, cpu_of(rq_of(cfs_rq)),
> > +				&se->avg, 0, 0, NULL);
> > +
> >  	se->avg.last_update_time = cfs_rq->avg.last_update_time;
> >  	cfs_rq->avg.load_avg += se->avg.load_avg;
> >  	cfs_rq->avg.load_sum += se->avg.load_sum;
> 
> you seem to have forgotten to remove the same logic from
> enqueue_entity_load_avg(), which will now call __update_load_avg()
> twice.

In case of enqueue_entity_load_avg(), that seems to be ok.

However, the problem is that he made it "entangled":

In enqueue_entity_load_avg():

	if (migrated)
		attach_entity_load_avg();

while in attach_entity_load_avg():

	if (!migrated)
		__update_load_avg();

so, if attach() is called from enqueue(), that if() is never true.

To Byungchul,

1) I suggest you not entangle the entire series by mixing problem
   sovling with code manipulating. That said, it is better you
   first solve the "move between task group" problem and the
   switch_to/from problem (if it is a problem, either way, comment
   with your explanation to how you deal with the lost record and why).
2) After that, make the code cleaner, without change to logic, especially
   avoid entangling the logic in order to do the code manipulation.
3) If you don't hate upper case letter, use it properly.

If it helps.

Thanks,
Yuyang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 0/5] sync a se with its cfs_rq when att(det)aching it byungchul.park@lge.com - 2015-08-19 08:50 +0200
  [PATCH v3 2/5] sched: make task_move_group_fair adjust cfs_rq's load in case of queued byungchul.park@lge.com - 2015-08-19 08:50 +0200
  [PATCH v3 5/5] sched: add two functions for att(det)aching a task to(from) a cfs_rq byungchul.park@lge.com - 2015-08-19 08:50 +0200
  [PATCH v3 1/5] sched: add two functions adjusting cfs_rq's load when att(det)aching a se byungchul.park@lge.com - 2015-08-19 08:50 +0200
  [PATCH v3 3/5] sched: sync a se with prev cfs_rq when changing cgroup byungchul.park@lge.com - 2015-08-19 08:50 +0200
  [PATCH v3 4/5] sched: sync a se with its cfs_rq when switching sched class to fair class byungchul.park@lge.com - 2015-08-19 08:50 +0200
    Re: [PATCH v3 4/5] sched: sync a se with its cfs_rq when switching  sched class to fair class Peter Zijlstra <peterz@infradead.org> - 2015-08-19 19:20 +0200
      Re: [PATCH v3 4/5] sched: sync a se with its cfs_rq when switching  sched class to fair class Yuyang Du <yuyang.du@intel.com> - 2015-08-20 06:10 +0200
        Re: [PATCH v3 4/5] sched: sync a se with its cfs_rq when switching  sched class to fair class Byungchul Park <byungchul.park@lge.com> - 2015-08-20 08:20 +0200
        Re: [PATCH v3 4/5] sched: sync a se with its cfs_rq when switching  sched class to fair class Peter Zijlstra <peterz@infradead.org> - 2015-08-20 23:20 +0200
          Re: [PATCH v3 4/5] sched: sync a se with its cfs_rq when switching  sched class to fair class Byungchul Park <byungchul.park@lge.com> - 2015-08-21 02:00 +0200
      Re: [PATCH v3 4/5] sched: sync a se with its cfs_rq when switching  sched class to fair class Byungchul Park <byungchul.park@lge.com> - 2015-08-20 07:30 +0200
  Re: [PATCH v3 0/5] sync a se with its cfs_rq when att(det)aching it Byungchul Park <byungchul.park@lge.com> - 2015-08-19 09:10 +0200
  Re: [PATCH v3 0/5] sync a se with its cfs_rq when att(det)aching it Peter Zijlstra <peterz@infradead.org> - 2015-08-20 03:20 +0200
    Re: [PATCH v3 0/5] sync a se with its cfs_rq when att(det)aching it Byungchul Park <byungchul.park@lge.com> - 2015-08-20 07:30 +0200
    Re: [PATCH v3 0/5] sync a se with its cfs_rq when att(det)aching it Byungchul Park <byungchul.park@lge.com> - 2015-08-20 10:40 +0200
      Re: [PATCH v3 0/5] sync a se with its cfs_rq when att(det)aching it Byungchul Park <byungchul.park@lge.com> - 2015-08-20 12:50 +0200
        Re: [PATCH v3 0/5] sync a se with its cfs_rq when att(det)aching it Peter Zijlstra <peterz@infradead.org> - 2015-08-20 23:20 +0200
          Re: [PATCH v3 0/5] sync a se with its cfs_rq when att(det)aching it Byungchul Park <byungchul.park@lge.com> - 2015-08-21 02:10 +0200

csiph-web