In dequeue_task_fair() we bail on dequeue when we encounter a parenting entity
with additional weight. However, we perform a double shares update on this
entity since we continue the shares update traversal from that point, despite
dequeue_entity() having already updated its queuing cfs_rq.
Avoid this by starting from the parent when we resume.
Signed-off-by: Paul Turner <[email protected]>
---
kernel/sched_fair.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Index: tip/kernel/sched_fair.c
===================================================================
--- tip.orig/kernel/sched_fair.c
+++ tip/kernel/sched_fair.c
@@ -1355,8 +1355,10 @@ static void dequeue_task_fair(struct rq
dequeue_entity(cfs_rq, se, flags);
/* Don't dequeue parent if it has other entities besides us */
- if (cfs_rq->load.weight)
+ if (cfs_rq->load.weight) {
+ se = parent_entity(se);
break;
+ }
flags |= DEQUEUE_SLEEP;
}
(2011/05/03 18:28), Paul Turner wrote:
> In dequeue_task_fair() we bail on dequeue when we encounter a parenting entity
> with additional weight. However, we perform a double shares update on this
> entity since we continue the shares update traversal from that point, despite
> dequeue_entity() having already updated its queuing cfs_rq.
>
> Avoid this by starting from the parent when we resume.
>
> Signed-off-by: Paul Turner <[email protected]>
> ---
> kernel/sched_fair.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Index: tip/kernel/sched_fair.c
> ===================================================================
> --- tip.orig/kernel/sched_fair.c
> +++ tip/kernel/sched_fair.c
> @@ -1355,8 +1355,10 @@ static void dequeue_task_fair(struct rq
> dequeue_entity(cfs_rq, se, flags);
>
> /* Don't dequeue parent if it has other entities besides us */
> - if (cfs_rq->load.weight)
> + if (cfs_rq->load.weight) {
> + se = parent_entity(se);
> break;
> + }
> flags |= DEQUEUE_SLEEP;
> }
>
Reviewed-by: Hidetoshi Seto <[email protected]>
This small fixlet can stand alone.
Peter, how about getting this into git tree first?
Thanks,
H.Seto
On Tue, 2011-05-10 at 16:14 +0900, Hidetoshi Seto wrote:
> (2011/05/03 18:28), Paul Turner wrote:
> > In dequeue_task_fair() we bail on dequeue when we encounter a parenting entity
> > with additional weight. However, we perform a double shares update on this
> > entity since we continue the shares update traversal from that point, despite
> > dequeue_entity() having already updated its queuing cfs_rq.
> >
> > Avoid this by starting from the parent when we resume.
> >
> > Signed-off-by: Paul Turner <[email protected]>
> > ---
> > kernel/sched_fair.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > Index: tip/kernel/sched_fair.c
> > ===================================================================
> > --- tip.orig/kernel/sched_fair.c
> > +++ tip/kernel/sched_fair.c
> > @@ -1355,8 +1355,10 @@ static void dequeue_task_fair(struct rq
> > dequeue_entity(cfs_rq, se, flags);
> >
> > /* Don't dequeue parent if it has other entities besides us */
> > - if (cfs_rq->load.weight)
> > + if (cfs_rq->load.weight) {
> > + se = parent_entity(se);
> > break;
> > + }
> > flags |= DEQUEUE_SLEEP;
> > }
> >
>
> Reviewed-by: Hidetoshi Seto <[email protected]>
>
> This small fixlet can stand alone.
> Peter, how about getting this into git tree first?
tip 2f36825b176f67e5c5228aa33d828bc39718811f contains the below.
/* Don't dequeue parent if it has other entities besides us */
- if (cfs_rq->load.weight)
+ if (cfs_rq->load.weight) {
+ /*
+ * Bias pick_next to pick a task from this cfs_rq, as
+ * p is sleeping when it is within its sched_slice.
+ */
+ if (task_sleep && parent_entity(se))
+ set_next_buddy(parent_entity(se));
break;
+ }
flags |= DEQUEUE_SLEEP;
}
(2011/05/10 17:32), Mike Galbraith wrote:
> On Tue, 2011-05-10 at 16:14 +0900, Hidetoshi Seto wrote:
>> This small fixlet can stand alone.
>> Peter, how about getting this into git tree first?
>
> tip 2f36825b176f67e5c5228aa33d828bc39718811f contains the below.
>
> /* Don't dequeue parent if it has other entities besides us */
> - if (cfs_rq->load.weight)
> + if (cfs_rq->load.weight) {
> + /*
> + * Bias pick_next to pick a task from this cfs_rq, as
> + * p is sleeping when it is within its sched_slice.
> + */
> + if (task_sleep && parent_entity(se))
> + set_next_buddy(parent_entity(se));
> break;
> + }
> flags |= DEQUEUE_SLEEP;
> }
Oh, thanks Mike!
It seems that this change in tip is better one.
Paul, don't you mind rebasing your patches onto tip/sched/core next time?
(...or is there better branch for rebase?)
Thanks,
H.Seto
On Wed, May 11, 2011 at 12:55 AM, Hidetoshi Seto
<[email protected]> wrote:
> (2011/05/10 17:32), Mike Galbraith wrote:
>> On Tue, 2011-05-10 at 16:14 +0900, Hidetoshi Seto wrote:
>>> This small fixlet can stand alone.
>>> Peter, how about getting this into git tree first?
>>
>> tip 2f36825b176f67e5c5228aa33d828bc39718811f contains the below.
>>
>> ? ? ? ? ? ? ? ? /* Don't dequeue parent if it has other entities besides us */
>> - ? ? ? ? ? ? ? if (cfs_rq->load.weight)
>> + ? ? ? ? ? ? ? if (cfs_rq->load.weight) {
>> + ? ? ? ? ? ? ? ? ? ? ? /*
>> + ? ? ? ? ? ? ? ? ? ? ? ?* Bias pick_next to pick a task from this cfs_rq, as
>> + ? ? ? ? ? ? ? ? ? ? ? ?* p is sleeping when it is within its sched_slice.
>> + ? ? ? ? ? ? ? ? ? ? ? ?*/
>> + ? ? ? ? ? ? ? ? ? ? ? if (task_sleep && parent_entity(se))
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_next_buddy(parent_entity(se));
>> ? ? ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? ? }
>> ? ? ? ? ? ? ? ? flags |= DEQUEUE_SLEEP;
>> ? ? ? ? }
>
> Oh, thanks Mike!
> It seems that this change in tip is better one.
>
> Paul, don't you mind rebasing your patches onto tip/sched/core next time?
> (...or is there better branch for rebase?)
>
I thought I had but apparently I missed this.
We still need to set se = parent_entity(se) to avoid the pointless
double update below.
Will definitely rebase.
Thanks!
>
> Thanks,
> H.Seto
>
>
On Wed, 2011-05-11 at 01:13 -0700, Paul Turner wrote:
> On Wed, May 11, 2011 at 12:55 AM, Hidetoshi Seto
> <[email protected]> wrote:
> > (2011/05/10 17:32), Mike Galbraith wrote:
> >> On Tue, 2011-05-10 at 16:14 +0900, Hidetoshi Seto wrote:
> >>> This small fixlet can stand alone.
> >>> Peter, how about getting this into git tree first?
> >>
> >> tip 2f36825b176f67e5c5228aa33d828bc39718811f contains the below.
> >>
> >> /* Don't dequeue parent if it has other entities besides us */
> >> - if (cfs_rq->load.weight)
> >> + if (cfs_rq->load.weight) {
> >> + /*
> >> + * Bias pick_next to pick a task from this cfs_rq, as
> >> + * p is sleeping when it is within its sched_slice.
> >> + */
> >> + if (task_sleep && parent_entity(se))
> >> + set_next_buddy(parent_entity(se));
> >> break;
> >> + }
> >> flags |= DEQUEUE_SLEEP;
> >> }
> >
> > Oh, thanks Mike!
> > It seems that this change in tip is better one.
> >
> > Paul, don't you mind rebasing your patches onto tip/sched/core next time?
> > (...or is there better branch for rebase?)
> >
>
> I thought I had but apparently I missed this.
>
> We still need to set se = parent_entity(se) to avoid the pointless
> double update below.
>
> Will definitely rebase.
Wish I could, wouldn't have 114 other patches just to get evaluation
tree up to speed :)
Index: linux-2.6.32/kernel/sched_fair.c
===================================================================
--- linux-2.6.32.orig/kernel/sched_fair.c
+++ linux-2.6.32/kernel/sched_fair.c
@@ -1308,12 +1308,15 @@ static void dequeue_task_fair(struct rq
/* Don't dequeue parent if it has other entities besides us */
if (cfs_rq->load.weight) {
+ /* Avoid double update below. */
+ se = parent_entity(se);
+
/*
* Bias pick_next to pick a task from this cfs_rq, as
* p is sleeping when it is within its sched_slice.
*/
- if (task_sleep && parent_entity(se))
- set_next_buddy(parent_entity(se));
+ if (task_sleep && se)
+ set_next_buddy(se);
break;
}
flags |= DEQUEUE_SLEEP;
(2011/05/11 17:45), Mike Galbraith wrote:
> On Wed, 2011-05-11 at 01:13 -0700, Paul Turner wrote:
>> On Wed, May 11, 2011 at 12:55 AM, Hidetoshi Seto
>> <[email protected]> wrote:
>>> (2011/05/10 17:32), Mike Galbraith wrote:
>>>> On Tue, 2011-05-10 at 16:14 +0900, Hidetoshi Seto wrote:
>>>>> This small fixlet can stand alone.
>>>>> Peter, how about getting this into git tree first?
>>>>
>>>> tip 2f36825b176f67e5c5228aa33d828bc39718811f contains the below.
>>>>
>>>> /* Don't dequeue parent if it has other entities besides us */
>>>> - if (cfs_rq->load.weight)
>>>> + if (cfs_rq->load.weight) {
>>>> + /*
>>>> + * Bias pick_next to pick a task from this cfs_rq, as
>>>> + * p is sleeping when it is within its sched_slice.
>>>> + */
>>>> + if (task_sleep && parent_entity(se))
>>>> + set_next_buddy(parent_entity(se));
>>>> break;
>>>> + }
>>>> flags |= DEQUEUE_SLEEP;
>>>> }
>>>
>>> Oh, thanks Mike!
>>> It seems that this change in tip is better one.
>>>
>>> Paul, don't you mind rebasing your patches onto tip/sched/core next time?
>>> (...or is there better branch for rebase?)
>>>
>>
>> I thought I had but apparently I missed this.
>>
>> We still need to set se = parent_entity(se) to avoid the pointless
>> double update below.
>>
>> Will definitely rebase.
>
> Wish I could, wouldn't have 114 other patches just to get evaluation
> tree up to speed :)
>
> Index: linux-2.6.32/kernel/sched_fair.c
> ===================================================================
> --- linux-2.6.32.orig/kernel/sched_fair.c
> +++ linux-2.6.32/kernel/sched_fair.c
> @@ -1308,12 +1308,15 @@ static void dequeue_task_fair(struct rq
>
> /* Don't dequeue parent if it has other entities besides us */
> if (cfs_rq->load.weight) {
> + /* Avoid double update below. */
> + se = parent_entity(se);
> +
> /*
> * Bias pick_next to pick a task from this cfs_rq, as
> * p is sleeping when it is within its sched_slice.
> */
> - if (task_sleep && parent_entity(se))
> - set_next_buddy(parent_entity(se));
> + if (task_sleep && se)
> + set_next_buddy(se);
> break;
> }
> flags |= DEQUEUE_SLEEP;
Nice!
It will be better to put this fixlet out from the cfs-bandwidth series
and post as a single patch.
Thanks,
H.Seto