Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1210135 > unrolled thread
| Started by | Yuyang Du <yuyang.du@intel.com> |
|---|---|
| First post | 2015-08-20 06:10 +0200 |
| Last post | 2015-08-21 02:00 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
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
| From | Yuyang Du <yuyang.du@intel.com> |
|---|---|
| Date | 2015-08-20 06:10 +0200 |
| Subject | Re: [PATCH v3 4/5] sched: sync a se with its cfs_rq when switching sched class to fair class |
| Message-ID | <pZpkS-1Os-13@gated-at.bofh.it> |
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/
[toc] | [next] | [standalone]
| From | Byungchul Park <byungchul.park@lge.com> |
|---|---|
| Date | 2015-08-20 08:20 +0200 |
| Message-ID | <pZrmF-4VL-3@gated-at.bofh.it> |
| In reply to | #1210135 |
On Thu, Aug 20, 2015 at 04:11:06AM +0800, Yuyang Du wrote:
> 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.
in case of migration, we should not call __update_load_avg() when attaching,
because se->avg.last_update_time is not meaningless on changed cfs_rq. that
is why attach_entity_load_avg() conditionally calls __update_load_avg().
enqueue() calls attach() only when migrating, so the if() should be never
true. i think it is normal. is it a problem?
>
> To Byungchul,
>
> 1) I suggest you not entangle the entire series by mixing problem
i think i didn't mix any problems. i solved each problem indivisually.
> 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
patch 2, 3 solve the former, and patch 4 solves the latter.
> with your explanation to how you deal with the lost record and why).
how i deal with the lost record, is the "decaying" as you can see easily.
i think it is the most natural way to deal with it.
> 2) After that, make the code cleaner, without change to logic, especially
> avoid entangling the logic in order to do the code manipulation.
i think i should modify patches if i entangled code, and could you tell me
where i did it?
> 3) If you don't hate upper case letter, use it properly.
i will try it.
thanks,
byungchul
>
> 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/
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Peter Zijlstra <peterz@infradead.org> |
|---|---|
| Date | 2015-08-20 23:20 +0200 |
| Message-ID | <pZFpE-8nP-19@gated-at.bofh.it> |
| In reply to | #1210135 |
On Thu, Aug 20, 2015 at 04:11:06AM +0800, Yuyang Du wrote:
> 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.
Right, I noticed the same yesterday when I took a second look at that
stuff. It was a little confusing indeed.
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Byungchul Park <byungchul.park@lge.com> |
|---|---|
| Date | 2015-08-21 02:00 +0200 |
| Message-ID | <pZHUt-3qg-13@gated-at.bofh.it> |
| In reply to | #1210739 |
On Thu, Aug 20, 2015 at 11:13:23PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 20, 2015 at 04:11:06AM +0800, Yuyang Du wrote:
> > 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.
>
> Right, I noticed the same yesterday when I took a second look at that
> stuff. It was a little confusing indeed.
don't couple functions to each other between functions. i believe it's
acceptable if you look at each function itself.
in other words, attach_entity_load_avg() must call __update_load_avg()
with the condition, whoever calls attach_entity_load_avg(). and
enqueue_entity_load_avg(), too.
> --
> 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/
--
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/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web