[PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Lin Ming]

Currently, entity_tick calls check_preempt_tick if WAKEUP_PREEMPT feature is
disabled. That's wrong. It should do that if the feature is enabled.

And actually the check is duplicate because check_preempt_tick will do
that. So just remove it from entity_tick.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 kernel/sched_fair.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index bc8ee99..af2fe9d 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1233,7 +1233,7 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
 		return;
 #endif
 
-	if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
+	if (cfs_rq->nr_running > 1)
 		check_preempt_tick(cfs_rq, curr);
 }
 
-- 
1.7.2.3

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Yong Zhang]

On Thu, Jul 28, 2011 at 05:43:23PM +0800, Lin Ming wrote:
> Currently, entity_tick calls check_preempt_tick if WAKEUP_PREEMPT feature is
> disabled. That's wrong. It should do that if the feature is enabled.

Why is it wrong?
check_preempt_wakeup() is used for wakeup.

> 
> And actually the check is duplicate because check_preempt_tick will do
> that. So just remove it from entity_tick.

It's not exactly duplicated. entity_tick() will resched_task(*p)
if p's slice is over. So if there is an following wakeup(say X),
then there is an opportunity for X to schedule quickly.

Thanks,
Yong

> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  kernel/sched_fair.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
> index bc8ee99..af2fe9d 100644
> --- a/kernel/sched_fair.c
> +++ b/kernel/sched_fair.c
> @@ -1233,7 +1233,7 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
>  		return;
>  #endif
>  
> -	if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
> +	if (cfs_rq->nr_running > 1)
>  		check_preempt_tick(cfs_rq, curr);
>  }
>  
> -- 
> 1.7.2.3
> 
> 
> 
> --
> 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/

-- 
Only stand for myself

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Yong Zhang]

On Fri, Jul 29, 2011 at 2:21 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> On Thu, Jul 28, 2011 at 05:43:23PM +0800, Lin Ming wrote:
>> Currently, entity_tick calls check_preempt_tick if WAKEUP_PREEMPT feature is
>> disabled. That's wrong. It should do that if the feature is enabled.
>
> Why is it wrong?
> check_preempt_wakeup() is used for wakeup.
>
>>
>> And actually the check is duplicate because check_preempt_tick will do
>> that. So just remove it from entity_tick.
>
> It's not exactly duplicated. entity_tick() will resched_task(*p)

s/entity_tick/check_preempt_tick

> if p's slice is over. So if there is an following wakeup(say X),
> then there is an opportunity for X to schedule quickly.
>
> Thanks,
> Yong
>
>>
>> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
>> ---
>>  kernel/sched_fair.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
>> index bc8ee99..af2fe9d 100644
>> --- a/kernel/sched_fair.c
>> +++ b/kernel/sched_fair.c
>> @@ -1233,7 +1233,7 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
>>               return;
>>  #endif
>>
>> -     if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
>> +     if (cfs_rq->nr_running > 1)
>>               check_preempt_tick(cfs_rq, curr);
>>  }
>>
>> --
>> 1.7.2.3
>>
>>
>>
>> --
>> 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/
>
> --
> Only stand for myself
>



-- 
Only stand for myself

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Lin Ming]

On Fri, 2011-07-29 at 14:21 +0800, Yong Zhang wrote:
> On Thu, Jul 28, 2011 at 05:43:23PM +0800, Lin Ming wrote:
> > Currently, entity_tick calls check_preempt_tick if WAKEUP_PREEMPT feature is
> > disabled. That's wrong. It should do that if the feature is enabled.
> 
> Why is it wrong?
> check_preempt_wakeup() is used for wakeup.

I guess you mean "check_preempt_tick" here, yes?

in entity_tick(...):
        if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
                check_preempt_tick(cfs_rq, curr);

Note that, above "if" statement says "if WAKEUP_PREEMPT feature is
*disabled* then calls check_preempt_tick".

Shouldn't it be "if WAKEUP_PREEMPT feature is *enabled* then ...."?

> 
> > 
> > And actually the check is duplicate because check_preempt_tick will do
> > that. So just remove it from entity_tick.
> 
> It's not exactly duplicated. entity_tick() will resched_task(*p)
> if p's slice is over. So if there is an following wakeup(say X),
> then there is an opportunity for X to schedule quickly.

Understood this.

But what I mean is both "entity_tick" and "check_preempt_tick" check
WAKEUP_PREEMPT feature. That's duplicated.

Only need to check it in "check_preempt_tick".

Thanks,
Lin Ming

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Yong Zhang]

On Fri, Jul 29, 2011 at 02:49:40PM +0800, Lin Ming wrote:
> On Fri, 2011-07-29 at 14:21 +0800, Yong Zhang wrote:
> > On Thu, Jul 28, 2011 at 05:43:23PM +0800, Lin Ming wrote:
> > > Currently, entity_tick calls check_preempt_tick if WAKEUP_PREEMPT feature is
> > > disabled. That's wrong. It should do that if the feature is enabled.
> > 
> > Why is it wrong?
> > check_preempt_wakeup() is used for wakeup.
> 
> I guess you mean "check_preempt_tick" here, yes?

check_preempt_wakeup() excactly.
try_to_wake_up()
  check_preempt_curr()
    sched_fair->check_preempt_wakeup()  <========== [1]

> 
> in entity_tick(...):
>         if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
>                 check_preempt_tick(cfs_rq, curr);
> 
> Note that, above "if" statement says "if WAKEUP_PREEMPT feature is
> *disabled* then calls check_preempt_tick".

Yeah, if !sched_feat(WAKEUP_PREEMPT) [1] will just return;
thus new waked task will wait until the next tick to schedule.

> 
> Shouldn't it be "if WAKEUP_PREEMPT feature is *enabled* then ...."?

So no IMHO.

> 
> > 
> > > 
> > > And actually the check is duplicate because check_preempt_tick will do
> > > that. So just remove it from entity_tick.
> > 
> > It's not exactly duplicated. entity_tick() will resched_task(*p)
> > if p's slice is over. So if there is an following wakeup(say X),
> > then there is an opportunity for X to schedule quickly.
> 
> Understood this.
> 
> But what I mean is both "entity_tick" and "check_preempt_tick" check
> WAKEUP_PREEMPT feature. That's duplicated.
> 
> Only need to check it in "check_preempt_tick".

I think we need that check(!sched_feat(WAKEUP_PREEMPT)) in entity_tick()
to give new waked task better opportunity.

Thanks,
Yong

-- 
Only stand for myself

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Lin Ming]

On Fri, 2011-07-29 at 15:03 +0800, Yong Zhang wrote:
> On Fri, Jul 29, 2011 at 02:49:40PM +0800, Lin Ming wrote:
> > On Fri, 2011-07-29 at 14:21 +0800, Yong Zhang wrote:
> > > On Thu, Jul 28, 2011 at 05:43:23PM +0800, Lin Ming wrote:
> > > > Currently, entity_tick calls check_preempt_tick if WAKEUP_PREEMPT feature is
> > > > disabled. That's wrong. It should do that if the feature is enabled.
> > > 
> > > Why is it wrong?
> > > check_preempt_wakeup() is used for wakeup.
> > 
> > I guess you mean "check_preempt_tick" here, yes?
> 
> check_preempt_wakeup() excactly.
> try_to_wake_up()
>   check_preempt_curr()
>     sched_fair->check_preempt_wakeup()  <========== [1]
> 
> > 
> > in entity_tick(...):
> >         if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
> >                 check_preempt_tick(cfs_rq, curr);
> > 
> > Note that, above "if" statement says "if WAKEUP_PREEMPT feature is
> > *disabled* then calls check_preempt_tick".
> 
> Yeah, if !sched_feat(WAKEUP_PREEMPT) [1] will just return;
> thus new waked task will wait until the next tick to schedule.
> 
> > 
> > Shouldn't it be "if WAKEUP_PREEMPT feature is *enabled* then ...."?
> 
> So no IMHO.
> 
> > 
> > > 
> > > > 
> > > > And actually the check is duplicate because check_preempt_tick will do
> > > > that. So just remove it from entity_tick.
> > > 
> > > It's not exactly duplicated. entity_tick() will resched_task(*p)
> > > if p's slice is over. So if there is an following wakeup(say X),
> > > then there is an opportunity for X to schedule quickly.
> > 
> > Understood this.
> > 
> > But what I mean is both "entity_tick" and "check_preempt_tick" check
> > WAKEUP_PREEMPT feature. That's duplicated.
> > 
> > Only need to check it in "check_preempt_tick".
> 
> I think we need that check(!sched_feat(WAKEUP_PREEMPT)) in entity_tick()
> to give new waked task better opportunity.

Thanks for your explanation.

>From another point of view, below !sched_feat(WAKEUP_PREEMPT) still
looks like duplicated.

        if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
                check_preempt_tick(cfs_rq, curr);

if "!sched_feat(WAKEUP_PREEMPT)" is run,
that implies cfs_rq->nr_running == 1.

Why do we need to call check_preempt_tick when there is only 1 task
runnable?

Thanks,
Lin Ming

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Yong Zhang]

On Fri, Jul 29, 2011 at 03:36:15PM +0800, Lin Ming wrote:
> >From another point of view, below !sched_feat(WAKEUP_PREEMPT) still
> looks like duplicated.
> 
>         if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
>                 check_preempt_tick(cfs_rq, curr);
> 
> if "!sched_feat(WAKEUP_PREEMPT)" is run,
> that implies cfs_rq->nr_running == 1.

That's true.

> 
> Why do we need to call check_preempt_tick when there is only 1 task
> runnable?

Just set_tsk_need_resched(p) if p's slice is over, thus:

	(n tick)	--->		(n+1 tick)
set_tsk_need_resched(p);
		another task Q is awaked

If we don't have !sched_feat(WAKEUP_PREEMPT), Q maybe will wait
for tick coming to get scheduled. If we have
!sched_feat(WAKEUP_PREEMPT), Q will get scheduled when some event
happen, like IRQ.

Thanks,
Yong

-- 
Only stand for myself

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Lin Ming]

On Fri, 2011-07-29 at 15:46 +0800, Yong Zhang wrote:
> On Fri, Jul 29, 2011 at 03:36:15PM +0800, Lin Ming wrote:
> > >From another point of view, below !sched_feat(WAKEUP_PREEMPT) still
> > looks like duplicated.
> > 
> >         if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
> >                 check_preempt_tick(cfs_rq, curr);
> > 
> > if "!sched_feat(WAKEUP_PREEMPT)" is run,
> > that implies cfs_rq->nr_running == 1.
> 
> That's true.
> 
> > 
> > Why do we need to call check_preempt_tick when there is only 1 task
> > runnable?
> 
> Just set_tsk_need_resched(p) if p's slice is over, thus:
> 
> 	(n tick)	--->		(n+1 tick)
> set_tsk_need_resched(p);
> 		another task Q is awaked
> 
> If we don't have !sched_feat(WAKEUP_PREEMPT), Q maybe will wait
> for tick coming to get scheduled. If we have
> !sched_feat(WAKEUP_PREEMPT), Q will get scheduled when some event
> happen, like IRQ.

Wow. That's really clear explanation.
Got it now.

Thanks!

> 
> Thanks,
> Yong
> 

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Peter Zijlstra]

On Fri, 2011-07-29 at 15:46 +0800, Yong Zhang wrote:
> On Fri, Jul 29, 2011 at 03:36:15PM +0800, Lin Ming wrote:
> > >From another point of view, below !sched_feat(WAKEUP_PREEMPT) still
> > looks like duplicated.
> > 
> >         if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
> >                 check_preempt_tick(cfs_rq, curr);
> > 
> > if "!sched_feat(WAKEUP_PREEMPT)" is run,
> > that implies cfs_rq->nr_running == 1.
> 
> That's true.
> 
> > 
> > Why do we need to call check_preempt_tick when there is only 1 task
> > runnable?
> 
> Just set_tsk_need_resched(p) if p's slice is over, thus:
> 
> 	(n tick)	--->		(n+1 tick)
> set_tsk_need_resched(p);
> 		another task Q is awaked
> 
> If we don't have !sched_feat(WAKEUP_PREEMPT), Q maybe will wait
> for tick coming to get scheduled. If we have
> !sched_feat(WAKEUP_PREEMPT), Q will get scheduled when some event
> happen, like IRQ.

Nah, if there is 1 runnable task it will always run, preemption simply
doesn't matter. There's nothing to preempt it with.

I've queued Lin's patch as I don't see the point of this thing either,
normally WAKEUP_PREEMPT is enabled so it says || 0 which is kinda
useless :-)

And I'm starting to think we should just kill all of WAKEUP_PREEMPT I
don't think we ever want to disable it anyway.. 

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Yong Zhang]

On Fri, Jul 29, 2011 at 09:56:58AM +0200, Peter Zijlstra wrote:
> On Fri, 2011-07-29 at 15:46 +0800, Yong Zhang wrote:
> > On Fri, Jul 29, 2011 at 03:36:15PM +0800, Lin Ming wrote:
> > > >From another point of view, below !sched_feat(WAKEUP_PREEMPT) still
> > > looks like duplicated.
> > > 
> > >         if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
> > >                 check_preempt_tick(cfs_rq, curr);
> > > 
> > > if "!sched_feat(WAKEUP_PREEMPT)" is run,
> > > that implies cfs_rq->nr_running == 1.
> > 
> > That's true.
> > 
> > > 
> > > Why do we need to call check_preempt_tick when there is only 1 task
> > > runnable?
> > 
> > Just set_tsk_need_resched(p) if p's slice is over, thus:
> > 
> > 	(n tick)	--->		(n+1 tick)
> > set_tsk_need_resched(p);
> > 		another task Q is awaked
> > 
> > If we don't have !sched_feat(WAKEUP_PREEMPT), Q maybe will wait
> > for tick coming to get scheduled. If we have
> > !sched_feat(WAKEUP_PREEMPT), Q will get scheduled when some event
> > happen, like IRQ.
> 
> Nah, if there is 1 runnable task it will always run, preemption simply
> doesn't matter. There's nothing to preempt it with.

Hmmm, so the newly waked task could be scheduled a little later.
That means schedule tick judge everything.

Thanks,
Yong

> 
> I've queued Lin's patch as I don't see the point of this thing either,
> normally WAKEUP_PREEMPT is enabled so it says || 0 which is kinda
> useless :-)
> 
> And I'm starting to think we should just kill all of WAKEUP_PREEMPT I
> don't think we ever want to disable it anyway.. 
> --
> 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/

-- 
Only stand for myself

[RFC PATCH] sched: Kill WAKEUP_PREEMPT

[Yong Zhang]

On Fri, Jul 29, 2011 at 09:56:58AM +0200, Peter Zijlstra wrote:
> I've queued Lin's patch as I don't see the point of this thing either,
> normally WAKEUP_PREEMPT is enabled so it says || 0 which is kinda
> useless :-)
> 
> And I'm starting to think we should just kill all of WAKEUP_PREEMPT I
> don't think we ever want to disable it anyway.. 

Someting like this?

---
From: Yong Zhang <yong.zhang0@gmail.com>
Subject: [RFC PATCH] sched: Kill WAKEUP_PREEMPT

Per Peter Zijlstra:
> And I'm starting to think we should just kill all of WAKEUP_PREEMPT I
> don't think we ever want to disable it anyway..

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
---
 kernel/sched_fair.c     |    8 +-------
 kernel/sched_features.h |    5 -----
 2 files changed, 1 insertions(+), 12 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index 46b7855..3c58042 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1114,9 +1114,6 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
 	 * narrow margin doesn't have to wait for a full slice.
 	 * This also mitigates buddy induced latencies under load.
 	 */
-	if (!sched_feat(WAKEUP_PREEMPT))
-		return;
-
 	if (delta_exec < sysctl_sched_min_granularity)
 		return;
 
@@ -1252,7 +1249,7 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
 		return;
 #endif
 
-	if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
+	if (cfs_rq->nr_running > 1)
 		check_preempt_tick(cfs_rq, curr);
 }
 
@@ -1918,9 +1915,6 @@ static void check_preempt_wakeup(struct rq *rq, struct task_struct *p, int wake_
 		return;
 
 
-	if (!sched_feat(WAKEUP_PREEMPT))
-		return;
-
 	update_curr(cfs_rq);
 	find_matching_se(&se, &pse);
 	BUG_ON(!pse);
diff --git a/kernel/sched_features.h b/kernel/sched_features.h
index 1e7066d..878a5f9 100644
--- a/kernel/sched_features.h
+++ b/kernel/sched_features.h
@@ -12,11 +12,6 @@ SCHED_FEAT(GENTLE_FAIR_SLEEPERS, 1)
 SCHED_FEAT(START_DEBIT, 1)
 
 /*
- * Should wakeups try to preempt running tasks.
- */
-SCHED_FEAT(WAKEUP_PREEMPT, 1)
-
-/*
  * Based on load and program behaviour, see if it makes sense to place
  * a newly woken task on the same cpu as the task that woke it --
  * improve cache locality. Typically used with SYNC wakeups as
-- 
1.7.4.1


-- 
Only stand for myself

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Peter Zijlstra]

On Fri, 2011-07-29 at 16:18 +0800, Yong Zhang wrote:
> > Nah, if there is 1 runnable task it will always run, preemption simply
> > doesn't matter. There's nothing to preempt it with.
> 
> Hmmm, so the newly waked task could be scheduled a little later.
> That means schedule tick judge everything. 

Oh, are you referring to the case where a task gets woken on an idle
remote cpu?

Re: [RFC PATCH] sched: Kill WAKEUP_PREEMPT

[Lin Ming]

On Fri, 2011-07-29 at 16:20 +0800, Yong Zhang wrote:
> On Fri, Jul 29, 2011 at 09:56:58AM +0200, Peter Zijlstra wrote:
> > I've queued Lin's patch as I don't see the point of this thing either,
> > normally WAKEUP_PREEMPT is enabled so it says || 0 which is kinda
> > useless :-)
> > 
> > And I'm starting to think we should just kill all of WAKEUP_PREEMPT I
> > don't think we ever want to disable it anyway.. 
> 
> Someting like this?
> 
> ---
> From: Yong Zhang <yong.zhang0@gmail.com>
> Subject: [RFC PATCH] sched: Kill WAKEUP_PREEMPT
> 
> Per Peter Zijlstra:
> > And I'm starting to think we should just kill all of WAKEUP_PREEMPT I
> > don't think we ever want to disable it anyway..
> 
> Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> ---
>  kernel/sched_fair.c     |    8 +-------
>  kernel/sched_features.h |    5 -----
>  2 files changed, 1 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
> index 46b7855..3c58042 100644
> --- a/kernel/sched_fair.c
> +++ b/kernel/sched_fair.c
> @@ -1114,9 +1114,6 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
>  	 * narrow margin doesn't have to wait for a full slice.
>  	 * This also mitigates buddy induced latencies under load.
>  	 */
> -	if (!sched_feat(WAKEUP_PREEMPT))
> -		return;
> -

-       /*
-        * Ensure that a task that missed wakeup preemption by a
-        * narrow margin doesn't have to wait for a full slice.
-        * This also mitigates buddy induced latencies under load.
-        */
-       if (!sched_feat(WAKEUP_PREEMPT))
-               return;
-

Then remove the comments also.

Re: [RFC PATCH] sched: Kill WAKEUP_PREEMPT

[Lin Ming]

On Fri, 2011-07-29 at 16:30 +0800, Lin Ming wrote:
> On Fri, 2011-07-29 at 16:20 +0800, Yong Zhang wrote:
> > On Fri, Jul 29, 2011 at 09:56:58AM +0200, Peter Zijlstra wrote:
> > > I've queued Lin's patch as I don't see the point of this thing either,
> > > normally WAKEUP_PREEMPT is enabled so it says || 0 which is kinda
> > > useless :-)
> > > 
> > > And I'm starting to think we should just kill all of WAKEUP_PREEMPT I
> > > don't think we ever want to disable it anyway.. 
> > 
> > Someting like this?
> > 
> > ---
> > From: Yong Zhang <yong.zhang0@gmail.com>
> > Subject: [RFC PATCH] sched: Kill WAKEUP_PREEMPT
> > 
> > Per Peter Zijlstra:
> > > And I'm starting to think we should just kill all of WAKEUP_PREEMPT I
> > > don't think we ever want to disable it anyway..
> > 
> > Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
> > ---
> >  kernel/sched_fair.c     |    8 +-------
> >  kernel/sched_features.h |    5 -----
> >  2 files changed, 1 insertions(+), 12 deletions(-)
> > 
> > diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
> > index 46b7855..3c58042 100644
> > --- a/kernel/sched_fair.c
> > +++ b/kernel/sched_fair.c
> > @@ -1114,9 +1114,6 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> >  	 * narrow margin doesn't have to wait for a full slice.
> >  	 * This also mitigates buddy induced latencies under load.
> >  	 */
> > -	if (!sched_feat(WAKEUP_PREEMPT))
> > -		return;
> > -
> 
> -       /*
> -        * Ensure that a task that missed wakeup preemption by a
> -        * narrow margin doesn't have to wait for a full slice.
> -        * This also mitigates buddy induced latencies under load.
> -        */
> -       if (!sched_feat(WAKEUP_PREEMPT))
> -               return;
> -
> 
> Then remove the comments also.

Sorry, don't remove that comments.

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Lin Ming]

On Fri, 2011-07-29 at 16:20 +0800, Peter Zijlstra wrote:
> On Fri, 2011-07-29 at 16:18 +0800, Yong Zhang wrote:
> > > Nah, if there is 1 runnable task it will always run, preemption simply
> > > doesn't matter. There's nothing to preempt it with.
> > 
> > Hmmm, so the newly waked task could be scheduled a little later.
> > That means schedule tick judge everything. 
> 
> Oh, are you referring to the case where a task gets woken on an idle
> remote cpu?

Yong means the case below.
At (n tick) there is only 1 task, then another task is woken before
(n+1) tick.

On Fri, 2011-07-29 at 15:46 +0800, Yong Zhang wrote:
> 
> Just set_tsk_need_resched(p) if p's slice is over, thus:
> 
> 	(n tick)	--->		(n+1 tick)
> set_tsk_need_resched(p);
> 		another task Q is awaked
> 
> If we don't have !sched_feat(WAKEUP_PREEMPT), Q maybe will wait
> for tick coming to get scheduled. If we have
> !sched_feat(WAKEUP_PREEMPT), Q will get scheduled when some event
> happen, like IRQ.

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Yong Zhang]

On Fri, Jul 29, 2011 at 10:20:50AM +0200, Peter Zijlstra wrote:
> On Fri, 2011-07-29 at 16:18 +0800, Yong Zhang wrote:
> > > Nah, if there is 1 runnable task it will always run, preemption simply
> > > doesn't matter. There's nothing to preempt it with.
> > 
> > Hmmm, so the newly waked task could be scheduled a little later.
> > That means schedule tick judge everything. 
> 
> Oh, are you referring to the case where a task gets woken on an idle
> remote cpu?

Not really.

Let's take UP for example, we have cpu-hug task A and threadirq B.

	n tick		--->		n+1 tick
set_tsk_need_resched(A);
		B comes in and
		wake up thread-B;

So for system on which we disable WAKEUP_PREEMPT,
if we don't have that check, thread-B will wait until n+1 tick comes
to get to run.
But if we have that check, thread-B will get to run after IRQ-B returns.

Thanks,
Yong

-- 
Only stand for myself

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Mike Galbraith]

On Fri, 2011-07-29 at 09:56 +0200, Peter Zijlstra wrote:

> And I'm starting to think we should just kill all of WAKEUP_PREEMPT I
> don't think we ever want to disable it anyway..

Agreed.  You have to be really into pain to flip that switch.

	-Mike

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Peter Zijlstra]

On Fri, 2011-07-29 at 16:46 +0800, Yong Zhang wrote:
> Let's take UP for example, we have cpu-hug task A and threadirq B.
> 
>         n tick          --->            n+1 tick
> set_tsk_need_resched(A);
>                 B comes in and
>                 wake up thread-B;
> 
> So for system on which we disable WAKEUP_PREEMPT,
> if we don't have that check, thread-B will wait until n+1 tick comes
> to get to run.
> But if we have that check, thread-B will get to run after IRQ-B returns.

But that's exactly what wakeup preemption is about, waking tasks don't
get to preempt running tasks. So no doing that preemption is exactly
right for !WAKEUP_PREEMPT.

Anyway, I've queued the removal patch since that removes all
confusion ;-)

Re: [PATCH] sched: Remove WAKEUP_PREEMPT feature check in entity_tick

[Yong Zhang]

On Fri, Jul 29, 2011 at 01:45:33PM +0200, Peter Zijlstra wrote:
> On Fri, 2011-07-29 at 16:46 +0800, Yong Zhang wrote:
> > Let's take UP for example, we have cpu-hug task A and threadirq B.
> > 
> >         n tick          --->            n+1 tick
> > set_tsk_need_resched(A);
> >                 B comes in and
> >                 wake up thread-B;
> > 
> > So for system on which we disable WAKEUP_PREEMPT,
> > if we don't have that check, thread-B will wait until n+1 tick comes
> > to get to run.
> > But if we have that check, thread-B will get to run after IRQ-B returns.
> 
> But that's exactly what wakeup preemption is about, waking tasks don't
> get to preempt running tasks. So no doing that preemption is exactly
> right for !WAKEUP_PREEMPT.
> 
> Anyway, I've queued the removal patch since that removes all
> confusion ;-)

Yup, that sounds good. Thanks Peter.

-Yong

-- 
Only stand for myself

[tip:sched/core] sched: Kill WAKEUP_PREEMPT

[tip-bot for Yong Zhang]

Commit-ID:  2c2efaed9bc973e3aeab1385c618017b56c8f6d7
Gitweb:     http://git.kernel.org/tip/2c2efaed9bc973e3aeab1385c618017b56c8f6d7
Author:     Yong Zhang <yong.zhang0@gmail.com>
AuthorDate: Fri, 29 Jul 2011 16:20:33 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 14 Aug 2011 12:00:41 +0200

sched: Kill WAKEUP_PREEMPT

Remove the WAKEUP_PREEMPT feature, disabling it doesn't make any sense
and its outlived its use by a long long while.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
Acked-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20110729082033.GB12106@zhy
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/sched_fair.c     |    9 +--------
 kernel/sched_features.h |    5 -----
 2 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index bc8ee99..241fc86 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1095,9 +1095,6 @@ check_preempt_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr)
 	 * narrow margin doesn't have to wait for a full slice.
 	 * This also mitigates buddy induced latencies under load.
 	 */
-	if (!sched_feat(WAKEUP_PREEMPT))
-		return;
-
 	if (delta_exec < sysctl_sched_min_granularity)
 		return;
 
@@ -1233,7 +1230,7 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
 		return;
 #endif
 
-	if (cfs_rq->nr_running > 1 || !sched_feat(WAKEUP_PREEMPT))
+	if (cfs_rq->nr_running > 1)
 		check_preempt_tick(cfs_rq, curr);
 }
 
@@ -1899,10 +1896,6 @@ static void check_preempt_wakeup(struct rq *rq, struct task_struct *p, int wake_
 	if (unlikely(p->policy != SCHED_NORMAL))
 		return;
 
-
-	if (!sched_feat(WAKEUP_PREEMPT))
-		return;
-
 	find_matching_se(&se, &pse);
 	update_curr(cfs_rq_of(se));
 	BUG_ON(!pse);
diff --git a/kernel/sched_features.h b/kernel/sched_features.h
index 2e74677..efa0a7b 100644
--- a/kernel/sched_features.h
+++ b/kernel/sched_features.h
@@ -12,11 +12,6 @@ SCHED_FEAT(GENTLE_FAIR_SLEEPERS, 1)
 SCHED_FEAT(START_DEBIT, 1)
 
 /*
- * Should wakeups try to preempt running tasks.
- */
-SCHED_FEAT(WAKEUP_PREEMPT, 1)
-
-/*
  * Based on load and program behaviour, see if it makes sense to place
  * a newly woken task on the same cpu as the task that woke it --
  * improve cache locality. Typically used with SYNC wakeups as