[PATCH] sched: fix first task of a task group is attached twice

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] sched: fix first task of a task group is attached twice

Vincent Guittot
The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
that the 1st sched_entity that will be attached, will keep its
last_update_time set to 0 and will attached once again during the
enqueue.
Initialize cfs_rq->avg.last_update_time to current rq's clock

Signed-off-by: Vincent Guittot <[hidden email]>
---
 kernel/sched/fair.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 218f8e8..a4e2c10 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8586,6 +8586,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
  se->depth = parent->depth + 1;
  }
 
+ /*
+ * Set last_update_time to something different from 0 to make
+ * sure the 1st sched_entity will not be attached twice: once
+ * when attaching the task to the group and one more time when
+ * enqueueing the task.
+ */
+ tg->cfs_rq[cpu]->avg.last_update_time = rq_clock_task(rq_of(cfs_rq));
+
  se->my_q = cfs_rq;
  /* guarantee group entities always have weight */
  update_load_set(&se->load, NICE_0_LOAD);
--
1.9.1

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH v2] sched: fix first task of a task group is attached twice

Vincent Guittot
The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
that the 1st sched_entity that will be attached, will keep its
last_update_time set to 0 and will attached once again during the
enqueue.
Initialize cfs_rq->avg.last_update_time to 1 instead.

Signed-off-by: Vincent Guittot <[hidden email]>
---

v2:
- rq_clock_task(rq_of(cfs_rq)) can't be used because lock is not held

 kernel/sched/fair.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 218f8e8..3724656 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8586,6 +8586,14 @@ void init_tg_cfs_entry(struct task_group *tg, struct cfs_rq *cfs_rq,
  se->depth = parent->depth + 1;
  }
 
+ /*
+ * Set last_update_time to something different from 0 to make
+ * sure the 1st sched_entity will not be attached twice: once
+ * when attaching the task to the group and one more time when
+ * enqueueing the task.
+ */
+ tg->cfs_rq[cpu]->avg.last_update_time = 1;
+
  se->my_q = cfs_rq;
  /* guarantee group entities always have weight */
  update_load_set(&se->load, NICE_0_LOAD);
--
1.9.1

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2] sched: fix first task of a task group is attached twice

Yuyang Du
On Wed, May 25, 2016 at 05:01:11PM +0200, Vincent Guittot wrote:
> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
> that the 1st sched_entity that will be attached, will keep its
> last_update_time set to 0 and will attached once again during the
> enqueue.
> Initialize cfs_rq->avg.last_update_time to 1 instead.
 
Actually, the first time (attach_task_cfs_rq()) is called even way
before init_entity_runnable_average(), no?
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2] sched: fix first task of a task group is attached twice

Vincent Guittot
On 26 May 2016 at 00:38, Yuyang Du <[hidden email]> wrote:
> On Wed, May 25, 2016 at 05:01:11PM +0200, Vincent Guittot wrote:
>> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
>> that the 1st sched_entity that will be attached, will keep its
>> last_update_time set to 0 and will attached once again during the
>> enqueue.
>> Initialize cfs_rq->avg.last_update_time to 1 instead.
>
> Actually, the first time (attach_task_cfs_rq()) is called even way
> before init_entity_runnable_average(), no?

maybe there is an issue during the fork of a task too
This patch is about the init of the sched_avg of a task group. The
problem happens when the 1st task is moved the group but it's not
about the fork of a task
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2] sched: fix first task of a task group is attached twice

Yuyang Du
On Thu, May 26, 2016 at 10:26:54AM +0200, Vincent Guittot wrote:

> On 26 May 2016 at 00:38, Yuyang Du <[hidden email]> wrote:
> > On Wed, May 25, 2016 at 05:01:11PM +0200, Vincent Guittot wrote:
> >> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
> >> that the 1st sched_entity that will be attached, will keep its
> >> last_update_time set to 0 and will attached once again during the
> >> enqueue.
> >> Initialize cfs_rq->avg.last_update_time to 1 instead.
> >
> > Actually, the first time (attach_task_cfs_rq()) is called even way
> > before init_entity_runnable_average(), no?
>
> maybe there is an issue during the fork of a task too
> This patch is about the init of the sched_avg of a task group. The
> problem happens when the 1st task is moved the group but it's not
> about the fork of a task

Right, but the root cause is not addressed, which is when forked, the task
should not be touched on sched avgs before init_entity_runnable_average()
in wake_up_new_task(). You think?
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH v2] sched: fix first task of a task group is attached twice

Vincent Guittot
On 26 May 2016 at 02:40, Yuyang Du <[hidden email]> wrote:

> On Thu, May 26, 2016 at 10:26:54AM +0200, Vincent Guittot wrote:
>> On 26 May 2016 at 00:38, Yuyang Du <[hidden email]> wrote:
>> > On Wed, May 25, 2016 at 05:01:11PM +0200, Vincent Guittot wrote:
>> >> The cfs_rq->avg.last_update_time is initialize to 0 with the main effect
>> >> that the 1st sched_entity that will be attached, will keep its
>> >> last_update_time set to 0 and will attached once again during the
>> >> enqueue.
>> >> Initialize cfs_rq->avg.last_update_time to 1 instead.
>> >
>> > Actually, the first time (attach_task_cfs_rq()) is called even way
>> > before init_entity_runnable_average(), no?
>>
>> maybe there is an issue during the fork of a task too
>> This patch is about the init of the sched_avg of a task group. The
>> problem happens when the 1st task is moved the group but it's not
>> about the fork of a task
>
> Right, but the root cause is not addressed, which is when forked, the task
> should not be touched on sched avgs before init_entity_runnable_average()
> in wake_up_new_task(). You think?

so yes, this patch doesn't not address the fact that sched avgs should
not be touch before init_entity_runnable_average(). This patch is only
about inti of sched_avg of task group
Loading...