[RFC PATCH 0/2] Remove and replace SD_WAKE_AFFINE with SD_BALANCE_WAKE

[RFC PATCH 0/2] Remove and replace SD_WAKE_AFFINE with SD_BALANCE_WAKE

[Yuyang Du]

Inspired by Morten's recent patch "sched: Make SD_BALANCE_WAKE a topology flag"
(https://lkml.org/lkml/2016/5/23/173), I proposed to remove SD_WAKE_AFFINE and
then replace it with SD_BALANCE_WAKE.

Here is the patchset. There should be no functionality change, and I think the
semantics and names of the SD_BALANCE_* flags will be more "elegant".

Make this a RFC, as maybe some kernel tuning scripts will be broken if there is
any which disables SD_WAKE_AFFINE.

--

Yuyang Du (2):
  sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up
  sched: Remove SD_WAKE_AFFINE flag and replace it with SD_BALANCE_WAKE

 include/linux/sched.h   |    1 -
 kernel/sched/core.c     |   11 +++++------
 kernel/sched/deadline.c |    2 +-
 kernel/sched/fair.c     |    9 ++++-----
 kernel/sched/rt.c       |    2 +-
 5 files changed, 11 insertions(+), 14 deletions(-)

-- 
1.7.9.5

[RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Yuyang Du]

The SD_BALANCE_WAKE is irrelevant in the contexts of these two removals,
and in addition SD_BALANCE_WAKE is not and should not be set in any
sched_domain flags so far.

Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 kernel/sched/core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 7f2cae4..e00b8ea 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6229,10 +6229,10 @@ static void set_domain_attribute(struct sched_domain *sd,
 		request = attr->relax_domain_level;
 	if (request < sd->level) {
 		/* turn off idle balance on this domain */
-		sd->flags &= ~(SD_BALANCE_WAKE|SD_BALANCE_NEWIDLE);
+		sd->flags &= ~(SD_BALANCE_NEWIDLE);
 	} else {
 		/* turn on idle balance on this domain */
-		sd->flags |= (SD_BALANCE_WAKE|SD_BALANCE_NEWIDLE);
+		sd->flags |= (SD_BALANCE_NEWIDLE);
 	}
 }
 
-- 
1.7.9.5

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Peter Zijlstra]

On Tue, May 31, 2016 at 09:11:37AM +0800, Yuyang Du wrote:
> The SD_BALANCE_WAKE is irrelevant in the contexts of these two removals,
> and in addition SD_BALANCE_WAKE is not and should not be set in any
> sched_domain flags so far.

This Changelog doesn't make any sense...

> Signed-off-by: Yuyang Du <yuyang.du@intel.com>
> ---
>  kernel/sched/core.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 7f2cae4..e00b8ea 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6229,10 +6229,10 @@ static void set_domain_attribute(struct sched_domain *sd,
>  		request = attr->relax_domain_level;
>  	if (request < sd->level) {
>  		/* turn off idle balance on this domain */
> -		sd->flags &= ~(SD_BALANCE_WAKE|SD_BALANCE_NEWIDLE);
> +		sd->flags &= ~(SD_BALANCE_NEWIDLE);
>  	} else {
>  		/* turn on idle balance on this domain */
> -		sd->flags |= (SD_BALANCE_WAKE|SD_BALANCE_NEWIDLE);
> +		sd->flags |= (SD_BALANCE_NEWIDLE);
>  	}
>  }
>  
> -- 
> 1.7.9.5
> 

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Yuyang Du]

On Tue, May 31, 2016 at 11:21:46AM +0200, Peter Zijlstra wrote:
> On Tue, May 31, 2016 at 09:11:37AM +0800, Yuyang Du wrote:
> > The SD_BALANCE_WAKE is irrelevant in the contexts of these two removals,
> > and in addition SD_BALANCE_WAKE is not and should not be set in any
> > sched_domain flags so far.
> 
> This Changelog doesn't make any sense...

How? SD_BALANCE_WAKE is not in any sched_domain flags (sd->flags), even if
it is, it is not used anywhere, no?
 
> > Signed-off-by: Yuyang Du <yuyang.du@intel.com>
> > ---
> >  kernel/sched/core.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 7f2cae4..e00b8ea 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -6229,10 +6229,10 @@ static void set_domain_attribute(struct sched_domain *sd,
> >  		request = attr->relax_domain_level;
> >  	if (request < sd->level) {
> >  		/* turn off idle balance on this domain */
> > -		sd->flags &= ~(SD_BALANCE_WAKE|SD_BALANCE_NEWIDLE);
> > +		sd->flags &= ~(SD_BALANCE_NEWIDLE);
> >  	} else {
> >  		/* turn on idle balance on this domain */
> > -		sd->flags |= (SD_BALANCE_WAKE|SD_BALANCE_NEWIDLE);
> > +		sd->flags |= (SD_BALANCE_NEWIDLE);
> >  	}
> >  }
> >  
> > -- 
> > 1.7.9.5
> > 

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Peter Zijlstra]

On Tue, May 31, 2016 at 09:31:32AM +0800, Yuyang Du wrote:
> On Tue, May 31, 2016 at 11:21:46AM +0200, Peter Zijlstra wrote:
> > On Tue, May 31, 2016 at 09:11:37AM +0800, Yuyang Du wrote:
> > > The SD_BALANCE_WAKE is irrelevant in the contexts of these two removals,
> > > and in addition SD_BALANCE_WAKE is not and should not be set in any
> > > sched_domain flags so far.
> > 
> > This Changelog doesn't make any sense...
> 
> How? SD_BALANCE_WAKE is not in any sched_domain flags (sd->flags), even if
> it is, it is not used anywhere, no?

It is and it is. See select_task_fair_rq():

	if (tmp->flags & sd_flags)

Now, as long as WAKE_AFFINE is also set, its hard to actually get into
the find_idlest_cpu() balancing, but if you clear all that you will
still get there.

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Yuyang Du]

On Tue, May 31, 2016 at 12:41:21PM +0200, Peter Zijlstra wrote:
> On Tue, May 31, 2016 at 09:31:32AM +0800, Yuyang Du wrote:
> > On Tue, May 31, 2016 at 11:21:46AM +0200, Peter Zijlstra wrote:
> > > On Tue, May 31, 2016 at 09:11:37AM +0800, Yuyang Du wrote:
> > > > The SD_BALANCE_WAKE is irrelevant in the contexts of these two removals,
> > > > and in addition SD_BALANCE_WAKE is not and should not be set in any
> > > > sched_domain flags so far.
> > > 
> > > This Changelog doesn't make any sense...
> > 
> > How? SD_BALANCE_WAKE is not in any sched_domain flags (sd->flags), even if
> > it is, it is not used anywhere, no?
> 
> It is and it is. See select_task_fair_rq():
> 
> 	if (tmp->flags & sd_flags)
> 
> Now, as long as WAKE_AFFINE is also set, its hard to actually get into
> the find_idlest_cpu() balancing, but if you clear all that you will
> still get there.
 
Well, that is very true, and the next patch (2/2) just makes all this
what this is supposed to be: the SD_BALANCE_WAKE is a meaningful sched_domain
flag.

This particular patch is a pure cleanup, may I amend the changelog to:

According to the comment: "turn off/on idle balance on this domain",
the SD_BALANCE_WAKE has nothing to do with idle balance, so clean them up.

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Mike Galbraith]

On Tue, 2016-05-31 at 09:31 +0800, Yuyang Du wrote:
> On Tue, May 31, 2016 at 11:21:46AM +0200, Peter Zijlstra wrote:
> > On Tue, May 31, 2016 at 09:11:37AM +0800, Yuyang Du wrote:
> > > The SD_BALANCE_WAKE is irrelevant in the contexts of these two removals,
> > > and in addition SD_BALANCE_WAKE is not and should not be set in any
> > > sched_domain flags so far.
> > 
> > This Changelog doesn't make any sense...
> 
> How? SD_BALANCE_WAKE is not in any sched_domain flags (sd->flags), even if
> it is, it is not used anywhere, no?

If the user chooses to set SD_BALANCE_WAKE in sd->flags, it is in fact
used.  It's just not turned on by default due to full balance on every
wakeup being far too painful to do by default.

	-Mike

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Yuyang Du]

On Wed, Jun 01, 2016 at 07:07:13AM +0200, Mike Galbraith wrote:
> On Tue, 2016-05-31 at 09:31 +0800, Yuyang Du wrote:
> > On Tue, May 31, 2016 at 11:21:46AM +0200, Peter Zijlstra wrote:
> > > On Tue, May 31, 2016 at 09:11:37AM +0800, Yuyang Du wrote:
> > > > The SD_BALANCE_WAKE is irrelevant in the contexts of these two removals,
> > > > and in addition SD_BALANCE_WAKE is not and should not be set in any
> > > > sched_domain flags so far.
> > > 
> > > This Changelog doesn't make any sense...
> > 
> > How? SD_BALANCE_WAKE is not in any sched_domain flags (sd->flags), even if
> > it is, it is not used anywhere, no?
> 
> If the user chooses to set SD_BALANCE_WAKE in sd->flags, it is in fact
> used.  It's just not turned on by default due to full balance on every
> wakeup being far too painful to do by default.

Yup. Up to this point, we don't have any disagreement. And I don't think we
have any disagreement conceptually. What the next patch really does is:

(1) we don't remove SD_BALANCE_WAKE as an important sched_domain flag, on
    the contrary, we strengthen it.

(2) the semantic of SD_BALANCE_WAKE is currently represented by SD_WAKE_AFFINE,
    we actually remove this representation.

(3) regarding the semantic of SD_WAKE_AFFINE, it is really not about selecting
    waker CPU or about the fast path. Conceptually, it is just saying the waker
    CPU is a valid and important candidate if SD_BALANCE_WAKE, which is just so
    obvious, so I don't think it deserves to be a separate sched_domain flag.

(4) the outcome is, if SD_BALANCE_WAKE, we definitely will/should try waker CPU,
    and if !SD_BALANCE_WAKE, we don't try waker CPU. So nothing functional is
    changed.

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Vincent Guittot]

On 1 June 2016 at 02:01, Yuyang Du <yuyang.du@intel.com> wrote:
> On Wed, Jun 01, 2016 at 07:07:13AM +0200, Mike Galbraith wrote:
>> On Tue, 2016-05-31 at 09:31 +0800, Yuyang Du wrote:
>> > On Tue, May 31, 2016 at 11:21:46AM +0200, Peter Zijlstra wrote:
>> > > On Tue, May 31, 2016 at 09:11:37AM +0800, Yuyang Du wrote:
>> > > > The SD_BALANCE_WAKE is irrelevant in the contexts of these two removals,
>> > > > and in addition SD_BALANCE_WAKE is not and should not be set in any
>> > > > sched_domain flags so far.
>> > >
>> > > This Changelog doesn't make any sense...
>> >
>> > How? SD_BALANCE_WAKE is not in any sched_domain flags (sd->flags), even if
>> > it is, it is not used anywhere, no?
>>
>> If the user chooses to set SD_BALANCE_WAKE in sd->flags, it is in fact
>> used.  It's just not turned on by default due to full balance on every
>> wakeup being far too painful to do by default.
>
> Yup. Up to this point, we don't have any disagreement. And I don't think we
> have any disagreement conceptually. What the next patch really does is:
>
> (1) we don't remove SD_BALANCE_WAKE as an important sched_domain flag, on
>     the contrary, we strengthen it.
>
> (2) the semantic of SD_BALANCE_WAKE is currently represented by SD_WAKE_AFFINE,
>     we actually remove this representation.
>
> (3) regarding the semantic of SD_WAKE_AFFINE, it is really not about selecting
>     waker CPU or about the fast path. Conceptually, it is just saying the waker
>     CPU is a valid and important candidate if SD_BALANCE_WAKE, which is just so
>     obvious, so I don't think it deserves to be a separate sched_domain flag.
>
> (4) the outcome is, if SD_BALANCE_WAKE, we definitely will/should try waker CPU,
>     and if !SD_BALANCE_WAKE, we don't try waker CPU. So nothing functional is
>     changed.


AFAIU, there is 4 possible cases during wake up:
- we don't want any balance at wake so we don't have SD_BALANCE_WAKE
nor SD_WAKE_AFFINE in sched_domain->flags
- we only want wake affine balance check so we only have
SD_WAKE_AFFINE in sched_domain->flags
- we want wake_affine and full load balance at wake so we have both
SD_BALANCE_WAKE and SD_WAKE_AFFINE in sched_domain->flags
- we want  full load balance but want to skip wake affine fast path so
we only have SD_BALANCE_WAKE in sched_domain->flags

I'm not sure that we can still do only wake_affine or only full
load_balance with your changes whereas these sequences are valid ones

Vincent

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Yuyang Du]

On Wed, Jun 01, 2016 at 10:32:53AM +0200, Vincent Guittot wrote:
> > Yup. Up to this point, we don't have any disagreement. And I don't think we
> > have any disagreement conceptually. What the next patch really does is:
> >
> > (1) we don't remove SD_BALANCE_WAKE as an important sched_domain flag, on
> >     the contrary, we strengthen it.
> >
> > (2) the semantic of SD_BALANCE_WAKE is currently represented by SD_WAKE_AFFINE,
> >     we actually remove this representation.
> >
> > (3) regarding the semantic of SD_WAKE_AFFINE, it is really not about selecting
> >     waker CPU or about the fast path. Conceptually, it is just saying the waker
> >     CPU is a valid and important candidate if SD_BALANCE_WAKE, which is just so
> >     obvious, so I don't think it deserves to be a separate sched_domain flag.
> >
> > (4) the outcome is, if SD_BALANCE_WAKE, we definitely will/should try waker CPU,
> >     and if !SD_BALANCE_WAKE, we don't try waker CPU. So nothing functional is
> >     changed.
> 
> 
> AFAIU, there is 4 possible cases during wake up:
> - we don't want any balance at wake so we don't have SD_BALANCE_WAKE
> nor SD_WAKE_AFFINE in sched_domain->flags
> - we only want wake affine balance check so we only have
> SD_WAKE_AFFINE in sched_domain->flags
> - we want wake_affine and full load balance at wake so we have both
> SD_BALANCE_WAKE and SD_WAKE_AFFINE in sched_domain->flags
> - we want  full load balance but want to skip wake affine fast path so
> we only have SD_BALANCE_WAKE in sched_domain->flags
> 
> I'm not sure that we can still do only wake_affine or only full
> load_balance with your changes whereas these sequences are valid ones

So with the patch, we will have a little bit semantic change, SD_BALANCE_WAKE
implies SD_WAKE_AFFINE if allowed, and will favor "fast path" if possible. I don't
think we should do anything otherwise.

So I think this is a combined case better than either of the "only wake_affine"
or "only full" cases. Make sense?

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Vincent Guittot]

On 1 June 2016 at 03:03, Yuyang Du <yuyang.du@intel.com> wrote:
> On Wed, Jun 01, 2016 at 10:32:53AM +0200, Vincent Guittot wrote:
>> > Yup. Up to this point, we don't have any disagreement. And I don't think we
>> > have any disagreement conceptually. What the next patch really does is:
>> >
>> > (1) we don't remove SD_BALANCE_WAKE as an important sched_domain flag, on
>> >     the contrary, we strengthen it.
>> >
>> > (2) the semantic of SD_BALANCE_WAKE is currently represented by SD_WAKE_AFFINE,
>> >     we actually remove this representation.
>> >
>> > (3) regarding the semantic of SD_WAKE_AFFINE, it is really not about selecting
>> >     waker CPU or about the fast path. Conceptually, it is just saying the waker
>> >     CPU is a valid and important candidate if SD_BALANCE_WAKE, which is just so
>> >     obvious, so I don't think it deserves to be a separate sched_domain flag.
>> >
>> > (4) the outcome is, if SD_BALANCE_WAKE, we definitely will/should try waker CPU,
>> >     and if !SD_BALANCE_WAKE, we don't try waker CPU. So nothing functional is
>> >     changed.
>>
>>
>> AFAIU, there is 4 possible cases during wake up:
>> - we don't want any balance at wake so we don't have SD_BALANCE_WAKE
>> nor SD_WAKE_AFFINE in sched_domain->flags
>> - we only want wake affine balance check so we only have
>> SD_WAKE_AFFINE in sched_domain->flags
>> - we want wake_affine and full load balance at wake so we have both
>> SD_BALANCE_WAKE and SD_WAKE_AFFINE in sched_domain->flags
>> - we want  full load balance but want to skip wake affine fast path so
>> we only have SD_BALANCE_WAKE in sched_domain->flags
>>
>> I'm not sure that we can still do only wake_affine or only full
>> load_balance with your changes whereas these sequences are valid ones
>
> So with the patch, we will have a little bit semantic change, SD_BALANCE_WAKE
> implies SD_WAKE_AFFINE if allowed, and will favor "fast path" if possible. I don't
> think we should do anything otherwise.

Why should we not do anything else ?

The current default configuration is to only use the wake_affine path.
With your changes, the default configuration will try to use wake
affine and will fall back to long load balance sequence if wake affine
doesn't find a sched_domain

That's a major changes in the behavior

>
> So I think this is a combined case better than either of the "only wake_affine"
> or "only full" cases. Make sense?

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Yuyang Du]

On Wed, Jun 01, 2016 at 11:24:45AM +0200, Vincent Guittot wrote:
> >
> > So with the patch, we will have a little bit semantic change, SD_BALANCE_WAKE
> > implies SD_WAKE_AFFINE if allowed, and will favor "fast path" if possible. I don't
> > think we should do anything otherwise.
> 
> Why should we not do anything else ?
> 
> The current default configuration is to only use the wake_affine path.
> With your changes, the default configuration will try to use wake
> affine and will fall back to long load balance sequence if wake affine
> doesn't find a sched_domain
> 
> That's a major changes in the behavior
 
Well, I won't argue that this hasn't changed, but I'd argue that this change
isn't a bad change: (a) it restores the flags to their meanings and makes them
more "elegant", (b) we definitely need further work to improve
select_task_rq_fair(), there has already been a comment marked XXX, right? :)

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Vincent Guittot]

On 1 June 2016 at 21:35, Yuyang Du <yuyang.du@intel.com> wrote:
> On Wed, Jun 01, 2016 at 11:24:45AM +0200, Vincent Guittot wrote:
>> >
>> > So with the patch, we will have a little bit semantic change, SD_BALANCE_WAKE
>> > implies SD_WAKE_AFFINE if allowed, and will favor "fast path" if possible. I don't
>> > think we should do anything otherwise.
>>
>> Why should we not do anything else ?
>>
>> The current default configuration is to only use the wake_affine path.
>> With your changes, the default configuration will try to use wake
>> affine and will fall back to long load balance sequence if wake affine
>> doesn't find a sched_domain
>>
>> That's a major changes in the behavior
>
> Well, I won't argue that this hasn't changed, but I'd argue that this change
> isn't a bad change: (a) it restores the flags to their meanings and makes them

Have you any proof that this change is not a bad thing ? Moreover have
you got proof that it's a good thing ? Changing the meaning and the
behavior of flags, just because you find it elegant, doesn't seem to
be enough for me.

So if you just want to rename the flags please keep current behavior unchange

> more "elegant", (b) we definitely need further work to improve
> select_task_rq_fair(), there has already been a comment marked XXX, right? :)

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Yuyang Du]

On Thu, Jun 02, 2016 at 08:56:40AM +0200, Vincent Guittot wrote:
> > Well, I won't argue that this hasn't changed, but I'd argue that this change
> > isn't a bad change: (a) it restores the flags to their meanings and makes them
> 
> Have you any proof that this change is not a bad thing ? Moreover have
> you got proof that it's a good thing ? Changing the meaning and the
> behavior of flags, just because you find it elegant, doesn't seem to
> be enough for me.
> 
> So if you just want to rename the flags please keep current behavior unchange

Interestingly, I don't disagree with what you said, it is not just a renaming,
so I said the following:
 
"(b) we definitely need further work to improve select_task_rq_fair()"

That said, the changed behavior should be addressed, the waker CPU should be
a valid candidate for all SD_BALANCE_*, and whatnot...

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Mike Galbraith]

On Wed, 2016-06-01 at 08:01 +0800, Yuyang Du wrote:
> On Wed, Jun 01, 2016 at 07:07:13AM +0200, Mike Galbraith wrote:
> > On Tue, 2016-05-31 at 09:31 +0800, Yuyang Du wrote:
> > > On Tue, May 31, 2016 at 11:21:46AM +0200, Peter Zijlstra wrote:
> > > > On Tue, May 31, 2016 at 09:11:37AM +0800, Yuyang Du wrote:
> > > > > The SD_BALANCE_WAKE is irrelevant in the contexts of these two removals,
> > > > > and in addition SD_BALANCE_WAKE is not and should not be set in any
> > > > > sched_domain flags so far.
> > > > 
> > > > This Changelog doesn't make any sense...
> > > 
> > > How? SD_BALANCE_WAKE is not in any sched_domain flags (sd->flags), even if
> > > it is, it is not used anywhere, no?
> > 
> > If the user chooses to set SD_BALANCE_WAKE in sd->flags, it is in fact
> > used.  It's just not turned on by default due to full balance on every
> > wakeup being far too painful to do by default.
> 
> Yup. Up to this point, we don't have any disagreement. And I don't think we
> have any disagreement conceptually. What the next patch really does is:
> 
> (1) we don't remove SD_BALANCE_WAKE as an important sched_domain flag, on
>     the contrary, we strengthen it.
> 
> (2) the semantic of SD_BALANCE_WAKE is currently represented by SD_WAKE_AFFINE,
>     we actually remove this representation.

Nope, those two have different meanings.  We pass SD_BALANCE_WAKE to
identify a ttwu() wakeup, just as we pass SD_BALANCE_FORK to say we're
waking a child.  SD_WAKE_AFFINE means exactly what it says, but is only
applicable to ttwu() wakeups.

> (3) regarding the semantic of SD_WAKE_AFFINE, it is really not about selecting
>     waker CPU or about the fast path. Conceptually, it is just saying the waker
>     CPU is a valid and important candidate if SD_BALANCE_WAKE, which is just so
>     obvious, so I don't think it deserves to be a separate sched_domain flag.

SD_WAKE_AFFINE being a separate domain flag, the user can turn it
on/off... separately :)

> (4) the outcome is, if SD_BALANCE_WAKE, we definitely will/should try waker CPU,
>     and if !SD_BALANCE_WAKE, we don't try waker CPU. So nothing functional is
>     changed.

If wake_wide() says we do not want an affine wakeup, but you apply
SD_WAKE_AFFINE meaning to SD_BALANCE_WAKE and turn it on in ->flags,
we'll give the user a free sample of full balance cost, no?

	-Mike

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Yuyang Du]

On Wed, Jun 01, 2016 at 11:36:39AM +0200, Mike Galbraith wrote:
> > Yup. Up to this point, we don't have any disagreement. And I don't think we
> > have any disagreement conceptually. What the next patch really does is:
> > 
> > (1) we don't remove SD_BALANCE_WAKE as an important sched_domain flag, on
> >     the contrary, we strengthen it.
> > 
> > (2) the semantic of SD_BALANCE_WAKE is currently represented by SD_WAKE_AFFINE,
> >     we actually remove this representation.
> 
> Nope, those two have different meanings.  We pass SD_BALANCE_WAKE to
> identify a ttwu() wakeup, just as we pass SD_BALANCE_FORK to say we're
> waking a child.  SD_WAKE_AFFINE means exactly what it says, but is only
> applicable to ttwu() wakeups.
 
I don't disagree, but want to add that, SD_WAKE_AFFINE has no meaning that is so
special and so important for anyone to use the flag to tune anything. If you want
to do any SD_BALANCE_*, waker CPU is a valid candidate if allowed, that is it.

IIUC your XXX mark and your comment "Prefer wake_affine over balance flags", you
said the same thing: SD_WAKE_AFFINE should be part of SD_BALANCE_WAKE, and should
be part of all SD_BALANCE_* flags,

> > (3) regarding the semantic of SD_WAKE_AFFINE, it is really not about selecting
> >     waker CPU or about the fast path. Conceptually, it is just saying the waker
> >     CPU is a valid and important candidate if SD_BALANCE_WAKE, which is just so
> >     obvious, so I don't think it deserves to be a separate sched_domain flag.
> 
> SD_WAKE_AFFINE being a separate domain flag, the user can turn it
> on/off... separately :)
 
Sure, that is very true, :) But turning it off for what, that is a big question mark.
We don't want a flag unless the flag is for goodness, and not a flag with big question
mark.

> > (4) the outcome is, if SD_BALANCE_WAKE, we definitely will/should try waker CPU,
> >     and if !SD_BALANCE_WAKE, we don't try waker CPU. So nothing functional is
> >     changed.
> 
> If wake_wide() says we do not want an affine wakeup, but you apply
> SD_WAKE_AFFINE meaning to SD_BALANCE_WAKE and turn it on in ->flags,
> we'll give the user a free sample of full balance cost, no?
 
Yes, and otherwise we don't select anything? That is just bad engough whether worse
or not. So the whole fuss I made is really that this is a right thing to start with. :)

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Mike Galbraith]

On Thu, 2016-06-02 at 04:03 +0800, Yuyang Du wrote:
> On Wed, Jun 01, 2016 at 11:36:39AM +0200, Mike Galbraith wrote:
> > > Yup. Up to this point, we don't have any disagreement. And I don't think we
> > > have any disagreement conceptually. What the next patch really does is:
> > > 
> > > (1) we don't remove SD_BALANCE_WAKE as an important sched_domain flag, on
> > >     the contrary, we strengthen it.
> > > 
> > > (2) the semantic of SD_BALANCE_WAKE is currently represented by SD_WAKE_AFFINE,
> > >     we actually remove this representation.
> > 
> > Nope, those two have different meanings.  We pass SD_BALANCE_WAKE to
> > identify a ttwu() wakeup, just as we pass SD_BALANCE_FORK to say we're
> > waking a child.  SD_WAKE_AFFINE means exactly what it says, but is only
> > applicable to ttwu() wakeups.
>  
> I don't disagree, but want to add that, SD_WAKE_AFFINE has no meaning that is so
> special and so important for anyone to use the flag to tune anything. If you want
> to do any SD_BALANCE_*, waker CPU is a valid candidate if allowed, that is it.

That flag lets the user specifically tell us that he doesn't want us to
bounce his tasks around the box, cache misses be damned.  The user may
_know_ that say cross node migrations hurt his load more than help, and
not want us to do that, thus expresses himself by turning the flag off
at whatever level.  People do that.  You can force them to take other
measures, but why do that?

> IIUC your XXX mark and your comment "Prefer wake_affine over balance flags", you
> said the same thing: SD_WAKE_AFFINE should be part of SD_BALANCE_WAKE, and should
> be part of all SD_BALANCE_* flags,

Peter wrote that, but I don't read it the way you do.  I read as if the
user wants the benefits of affine wakeups, he surely doesn't want us to
send the wakee off to god know where on every wakeup _instead_ of
waking affine, he wants to balance iff he can't have an affine wakeup.

> > > (3) regarding the semantic of SD_WAKE_AFFINE, it is really not about selecting
> > >     waker CPU or about the fast path. Conceptually, it is just saying the waker
> > >     CPU is a valid and important candidate if SD_BALANCE_WAKE, which is just so
> > >     obvious, so I don't think it deserves to be a separate sched_domain flag.
> > 
> > SD_WAKE_AFFINE being a separate domain flag, the user can turn it
> > on/off... separately :)
>  
> Sure, that is very true, :) But turning it off for what, that is a big question mark.
> We don't want a flag unless the flag is for goodness, and not a flag with big question
> mark.

It has a good excuse to exist.

> > > (4) the outcome is, if SD_BALANCE_WAKE, we definitely will/should try waker CPU,
> > >     and if !SD_BALANCE_WAKE, we don't try waker CPU. So nothing functional is
> > >     changed.
> > 
> > If wake_wide() says we do not want an affine wakeup, but you apply
> > SD_WAKE_AFFINE meaning to SD_BALANCE_WAKE and turn it on in ->flags,
> > we'll give the user a free sample of full balance cost, no?
>  
> Yes, and otherwise we don't select anything? That is just bad engough whether worse
> or not. So the whole fuss I made is really that this is a right thing to start with. :)

Nope, leaving tasks where they were is not a bad thing.  Lots of stuff
likes the scheduler best when it leaves them the hell alone :)  That
works out well all around, balance cycles are spent in userspace
instead, scheduler produces wins by doing nothing, perfect.

	-Mike

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Yuyang Du]

On Thu, Jun 02, 2016 at 07:50:23AM +0200, Mike Galbraith wrote:
> > > Nope, those two have different meanings.  We pass SD_BALANCE_WAKE to
> > > identify a ttwu() wakeup, just as we pass SD_BALANCE_FORK to say we're
> > > waking a child.  SD_WAKE_AFFINE means exactly what it says, but is only
> > > applicable to ttwu() wakeups.
> >  
> > I don't disagree, but want to add that, SD_WAKE_AFFINE has no meaning that is so
> > special and so important for anyone to use the flag to tune anything. If you want
> > to do any SD_BALANCE_*, waker CPU is a valid candidate if allowed, that is it.
> 
> That flag lets the user specifically tell us that he doesn't want us to
> bounce his tasks around the box, cache misses be damned.  The user may
> _know_ that say cross node migrations hurt his load more than help, and
> not want us to do that, thus expresses himself by turning the flag off
> at whatever level.  People do that.  You can force them to take other
> measures, but why do that?
 
Agreed, and with this patch, just disable SD_BALANCE_WAKE.

> > IIUC your XXX mark and your comment "Prefer wake_affine over balance flags", you
> > said the same thing: SD_WAKE_AFFINE should be part of SD_BALANCE_WAKE, and should
> > be part of all SD_BALANCE_* flags,
> 
> Peter wrote that, but I don't read it the way you do.  I read as if the
> user wants the benefits of affine wakeups, he surely doesn't want us to
> send the wakee off to god know where on every wakeup _instead_ of
> waking affine, he wants to balance iff he can't have an affine wakeup.

That is another matter within SD_BALANCE_WAKE we may further define: how
much effort to scan or how frequent bouncing etc the user wants. This is now
defined by SD_WAKE_AFFINE flag, which I certainly don't think is good.
 
> > > If wake_wide() says we do not want an affine wakeup, but you apply
> > > SD_WAKE_AFFINE meaning to SD_BALANCE_WAKE and turn it on in ->flags,
> > > we'll give the user a free sample of full balance cost, no?
> >  
> > Yes, and otherwise we don't select anything? That is just bad engough whether worse
> > or not. So the whole fuss I made is really that this is a right thing to start with. :)
> 
> Nope, leaving tasks where they were is not a bad thing.  Lots of stuff
> likes the scheduler best when it leaves them the hell alone :)  That
> works out well all around, balance cycles are spent in userspace
> instead, scheduler produces wins by doing nothing, perfect.
> 
Again, agreed, and with this patch, just disable SD_BALANCE_WAKE. :)

Re: [RFC PATCH 1/2] sched: Clean up SD_BALANCE_WAKE flags in sched domain build-up

[Mike Galbraith]

On Thu, 2016-06-02 at 06:41 +0800, Yuyang Du wrote:

> Again, agreed, and with this patch, just disable SD_BALANCE_WAKE. :)

Yeah, we pretty much agree on everything.. other than whether you shoul
d be picking on poor defenseless little SD_WAKE_AFFINE or not :^)

	-Mike

[RFC PATCH 2/2] sched: Remove SD_WAKE_AFFINE flag and replace it with SD_BALANCE_WAKE

[Yuyang Du]

SD_BALANCE_{FORK|EXEC|WAKE} flags are for select_task_rq() to select a
CPU to run a new task or a waking task. SD_WAKE_AFFINE is a flag to
try selecting the waker CPU to run the waking task.

SD_BALANCE_WAKE is not a sched_domain flag, but SD_WAKE_AFFINE is.
Conceptually, SD_BALANCE_WAKE should be a sched_domain flag just like
the other two, so we first make SD_BALANCE_WAKE a sched_domain flag.

Moreover, the semantic of SD_WAKE_AFFINE is included in the semantic
of SD_BALANCE_WAKE. When in wakeup balancing, it is natual to try
the waker CPU if the waker CPU is allowed, in that sense, we don't
need a separate flag to specify it, not mentioning that SD_WAKE_AFFINE
is almost enabled in every sched_domains.

With the above combined, there is no need to have SD_WAKE_AFFINE, so
we remove and replace it with SD_BALANCE_WAKE. This can be accomplished
without any functionality change.

Signed-off-by: Yuyang Du <yuyang.du@intel.com>
---
 include/linux/sched.h   |    1 -
 kernel/sched/core.c     |    7 +++----
 kernel/sched/deadline.c |    2 +-
 kernel/sched/fair.c     |    9 ++++-----
 kernel/sched/rt.c       |    2 +-
 5 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index c39244d..fca0c39 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1014,7 +1014,6 @@ extern void wake_up_q(struct wake_q_head *head);
 #define SD_BALANCE_EXEC		0x0004	/* Balance on exec */
 #define SD_BALANCE_FORK		0x0008	/* Balance on fork, clone */
 #define SD_BALANCE_WAKE		0x0010  /* Balance on wakeup */
-#define SD_WAKE_AFFINE		0x0020	/* Wake task to waking CPU */
 #define SD_SHARE_CPUCAPACITY	0x0080	/* Domain members share cpu power */
 #define SD_SHARE_POWERDOMAIN	0x0100	/* Domain members share power domain */
 #define SD_SHARE_PKG_RESOURCES	0x0200	/* Domain members share cpu pkg resources */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e00b8ea..2206fe1 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5670,7 +5670,7 @@ static int sd_degenerate(struct sched_domain *sd)
 	}
 
 	/* Following flags don't use groups */
-	if (sd->flags & (SD_WAKE_AFFINE))
+	if (sd->flags & (SD_BALANCE_WAKE))
 		return 0;
 
 	return 1;
@@ -6355,8 +6355,7 @@ sd_init(struct sched_domain_topology_level *tl, int cpu)
 					| 1*SD_BALANCE_NEWIDLE
 					| 1*SD_BALANCE_EXEC
 					| 1*SD_BALANCE_FORK
-					| 0*SD_BALANCE_WAKE
-					| 1*SD_WAKE_AFFINE
+					| 1*SD_BALANCE_WAKE
 					| 0*SD_SHARE_CPUCAPACITY
 					| 0*SD_SHARE_PKG_RESOURCES
 					| 0*SD_SERIALIZE
@@ -6399,7 +6398,7 @@ sd_init(struct sched_domain_topology_level *tl, int cpu)
 		if (sched_domains_numa_distance[tl->numa_level] > RECLAIM_DISTANCE) {
 			sd->flags &= ~(SD_BALANCE_EXEC |
 				       SD_BALANCE_FORK |
-				       SD_WAKE_AFFINE);
+				       SD_BALANCE_WAKE);
 		}
 
 #endif
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index fcb7f02..037ab0f 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1323,7 +1323,7 @@ static int find_later_rq(struct task_struct *task)
 
 	rcu_read_lock();
 	for_each_domain(cpu, sd) {
-		if (sd->flags & SD_WAKE_AFFINE) {
+		if (sd->flags & SD_BALANCE_WAKE) {
 
 			/*
 			 * If possible, preempting this_cpu is
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 218f8e8..43f522a 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5287,8 +5287,7 @@ static int cpu_util(int cpu)
  * that have the 'sd_flag' flag set. In practice, this is SD_BALANCE_WAKE,
  * SD_BALANCE_FORK, or SD_BALANCE_EXEC.
  *
- * Balances load by selecting the idlest cpu in the idlest group, or under
- * certain conditions an idle sibling cpu if the domain has SD_WAKE_AFFINE set.
+ * Balances load by selecting the idlest cpu in the idlest group.
  *
  * Returns the target cpu number.
  *
@@ -5315,9 +5314,9 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f
 
 		/*
 		 * If both cpu and prev_cpu are part of this domain,
-		 * cpu is a valid SD_WAKE_AFFINE target.
+		 * cpu is a valid SD_BALANCE_WAKE target.
 		 */
-		if (want_affine && (tmp->flags & SD_WAKE_AFFINE) &&
+		if (want_affine && (tmp->flags & SD_BALANCE_WAKE) &&
 		    cpumask_test_cpu(prev_cpu, sched_domain_span(tmp))) {
 			affine_sd = tmp;
 			break;
@@ -5330,7 +5329,7 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f
 	}
 
 	if (affine_sd) {
-		sd = NULL; /* Prefer wake_affine over balance flags */
+		sd = NULL; /* Prefer SD_BALANCE_WAKE over other balance flags */
 		if (cpu != prev_cpu && wake_affine(affine_sd, p, sync))
 			new_cpu = cpu;
 	}
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index d5690b7..d1c8f41 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1655,7 +1655,7 @@ static int find_lowest_rq(struct task_struct *task)
 
 	rcu_read_lock();
 	for_each_domain(cpu, sd) {
-		if (sd->flags & SD_WAKE_AFFINE) {
+		if (sd->flags & SD_BALANCE_WAKE) {
 			int best_cpu;
 
 			/*
-- 
1.7.9.5

Re: [RFC PATCH 2/2] sched: Remove SD_WAKE_AFFINE flag and replace it with SD_BALANCE_WAKE

[Peter Zijlstra]

On Tue, May 31, 2016 at 09:11:38AM +0800, Yuyang Du wrote:
> SD_BALANCE_{FORK|EXEC|WAKE} flags are for select_task_rq() to select a
> CPU to run a new task or a waking task. SD_WAKE_AFFINE is a flag to
> try selecting the waker CPU to run the waking task.
> 
> SD_BALANCE_WAKE is not a sched_domain flag, but SD_WAKE_AFFINE is.

No, SD_BALANCE_{FORK,EXEC,WAKE} are very much sched_domain flags.

They indicate how wide we should look to place tasks on these events.

Re: [RFC PATCH 2/2] sched: Remove SD_WAKE_AFFINE flag and replace it with SD_BALANCE_WAKE

[Yuyang Du]

On Tue, May 31, 2016 at 11:23:14AM +0200, Peter Zijlstra wrote:
> On Tue, May 31, 2016 at 09:11:38AM +0800, Yuyang Du wrote:
> > SD_BALANCE_{FORK|EXEC|WAKE} flags are for select_task_rq() to select a
> > CPU to run a new task or a waking task. SD_WAKE_AFFINE is a flag to
> > try selecting the waker CPU to run the waking task.
> > 
> > SD_BALANCE_WAKE is not a sched_domain flag, but SD_WAKE_AFFINE is.
> 
> No, SD_BALANCE_{FORK,EXEC,WAKE} are very much sched_domain flags.
> 
> They indicate how wide we should look to place tasks on these events.

With this patch, they are, and in particular SD_BALANCE_WAKE is.

SD_WAKE_AFFINE is just a particular CPU candidate in SD_BALANCE_WAKE.