Re: [PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
On Thu, Jul 05, 2018 at 04:03:11PM +0100, Quentin Perret wrote: > On Thursday 05 Jul 2018 at 15:13:49 (+0100), Morten Rasmussen wrote: > > 3. Detecting the flag in generic kernel/sched/* code means that all > > architectures will pay the for the overhead when building/rebuilding the > > sched_domain hierarchy, and all architectures that sets the cpu > > capacities to asymmetric will set the flag whether they like it or not. > > I'm not sure if this is a problem. > > That is true as well ... > > > > > In the end it is really about how much of this we want in generic code > > and how much we hide in arch/, and if we dare to touch the sched_domain > > build code ;-) > > Right so you can argue that the arch code is here to give you a > system-level information, and that if the scheduler wants to virtually > split that system, then it's its job to make sure that happens properly. > That is exactly what your patch does (IIUC), and I now think that this > is a very sensible middle-ground option. But this is debatable so I'm > interested to see what others think :-) I went ahead an hacked up some patches that sets the flag automatically as part of the sched_domain build process. I posted them so people can have a look: 1532093554-30504-1-git-send-email-morten.rasmus...@arm.com With those patches this patch has to be reverted/dropped. Morten
Re: [PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
On Thu, Jul 05, 2018 at 04:03:11PM +0100, Quentin Perret wrote: > On Thursday 05 Jul 2018 at 15:13:49 (+0100), Morten Rasmussen wrote: > > 3. Detecting the flag in generic kernel/sched/* code means that all > > architectures will pay the for the overhead when building/rebuilding the > > sched_domain hierarchy, and all architectures that sets the cpu > > capacities to asymmetric will set the flag whether they like it or not. > > I'm not sure if this is a problem. > > That is true as well ... > > > > > In the end it is really about how much of this we want in generic code > > and how much we hide in arch/, and if we dare to touch the sched_domain > > build code ;-) > > Right so you can argue that the arch code is here to give you a > system-level information, and that if the scheduler wants to virtually > split that system, then it's its job to make sure that happens properly. > That is exactly what your patch does (IIUC), and I now think that this > is a very sensible middle-ground option. But this is debatable so I'm > interested to see what others think :-) I went ahead an hacked up some patches that sets the flag automatically as part of the sched_domain build process. I posted them so people can have a look: 1532093554-30504-1-git-send-email-morten.rasmus...@arm.com With those patches this patch has to be reverted/dropped. Morten
Re: [PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
On Thursday 05 Jul 2018 at 15:13:49 (+0100), Morten Rasmussen wrote:
> On Thu, Jul 05, 2018 at 02:31:43PM +0100, Quentin Perret wrote:
> > If SD_ASYM_CPUCAPACITY means that some CPUs have different
> > arch_scale_cpu_capacity() values, we could also automatically _set_
> > the flag in sd_init() no ? Why should we let the arch set it and just
> > correct it later ?
> >
> > I understand the moment at which we know the capacities of CPUs varies
> > from arch to arch, but the arch code could just call
> > rebuild_sched_domain when the capacities of CPUs change and let the
> > scheduler detect things automatically. I mean, even if the arch code
> > sets the flag in its topology level table, it will have to rebuild
> > the sched domains anyway ...
> >
> > What do you think ?
>
> We could as well set the flag here so the architecture doesn't have to
> do it. It is a bit more complicated though for few reasons:
>
> 1. Detecting when to disable the flag is a lot simpler than checking
> which level is should be set on. You basically have to work you way up
> from the lowest topology level until you get to a level spanning all the
> capacities available in the system to figure out where the flag should
> be set. I don't think this fits easily with how we build the
> sched_domain hierarchy. It can of course be done.
Ah, that is something I missed. I see in 1f6e6c7cb9bc ("sched/core:
Introduce SD_ASYM_CPUCAPACITY sched_domain topology flag") that this
flag should be set only at the _lowest_ level at which there is
asymmetry. I had the wrong impression that the flag was supposed to be
set at _all_ level where there is some asymmetry. And actually having
'some asymmetry' isn't enough, we want to see the full range of CPU
capacities. Hmmm, that is indeed more complex than I thought ... :/
>
> 2. As you say, we still need the arch code (or cpufreq?) to rebuild the
> whole thing once we know that the capacities have been determined. That
> currently implies implementing arch_update_cpu_topology() which is
> arch-specific. So we would need some arch code to make rebuild happen at
> the right point in time. If the rebuild should be triggering the rebuild
> we need another way to force a full rebuild. This can also be done.
Yeah, with just this patch the arch code will have to:
1. update the arch_scale_cpu_capacity() of the CPUs;
2. detect the asymmetry to set the flag in the topology table;
3. rebuild the sched domains to let the scheduler know about the new
topology information.
By doing what I suggest we would just save 2 from the arch side and do
it in the scheduler. So actually, I really start to wonder if it's worth
it given the added complexity ...
> 3. Detecting the flag in generic kernel/sched/* code means that all
> architectures will pay the for the overhead when building/rebuilding the
> sched_domain hierarchy, and all architectures that sets the cpu
> capacities to asymmetric will set the flag whether they like it or not.
> I'm not sure if this is a problem.
That is true as well ...
>
> In the end it is really about how much of this we want in generic code
> and how much we hide in arch/, and if we dare to touch the sched_domain
> build code ;-)
Right so you can argue that the arch code is here to give you a
system-level information, and that if the scheduler wants to virtually
split that system, then it's its job to make sure that happens properly.
That is exactly what your patch does (IIUC), and I now think that this
is a very sensible middle-ground option. But this is debatable so I'm
interested to see what others think :-)
Thanks,
Quentin
Re: [PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
On Thursday 05 Jul 2018 at 15:13:49 (+0100), Morten Rasmussen wrote:
> On Thu, Jul 05, 2018 at 02:31:43PM +0100, Quentin Perret wrote:
> > If SD_ASYM_CPUCAPACITY means that some CPUs have different
> > arch_scale_cpu_capacity() values, we could also automatically _set_
> > the flag in sd_init() no ? Why should we let the arch set it and just
> > correct it later ?
> >
> > I understand the moment at which we know the capacities of CPUs varies
> > from arch to arch, but the arch code could just call
> > rebuild_sched_domain when the capacities of CPUs change and let the
> > scheduler detect things automatically. I mean, even if the arch code
> > sets the flag in its topology level table, it will have to rebuild
> > the sched domains anyway ...
> >
> > What do you think ?
>
> We could as well set the flag here so the architecture doesn't have to
> do it. It is a bit more complicated though for few reasons:
>
> 1. Detecting when to disable the flag is a lot simpler than checking
> which level is should be set on. You basically have to work you way up
> from the lowest topology level until you get to a level spanning all the
> capacities available in the system to figure out where the flag should
> be set. I don't think this fits easily with how we build the
> sched_domain hierarchy. It can of course be done.
Ah, that is something I missed. I see in 1f6e6c7cb9bc ("sched/core:
Introduce SD_ASYM_CPUCAPACITY sched_domain topology flag") that this
flag should be set only at the _lowest_ level at which there is
asymmetry. I had the wrong impression that the flag was supposed to be
set at _all_ level where there is some asymmetry. And actually having
'some asymmetry' isn't enough, we want to see the full range of CPU
capacities. Hmmm, that is indeed more complex than I thought ... :/
>
> 2. As you say, we still need the arch code (or cpufreq?) to rebuild the
> whole thing once we know that the capacities have been determined. That
> currently implies implementing arch_update_cpu_topology() which is
> arch-specific. So we would need some arch code to make rebuild happen at
> the right point in time. If the rebuild should be triggering the rebuild
> we need another way to force a full rebuild. This can also be done.
Yeah, with just this patch the arch code will have to:
1. update the arch_scale_cpu_capacity() of the CPUs;
2. detect the asymmetry to set the flag in the topology table;
3. rebuild the sched domains to let the scheduler know about the new
topology information.
By doing what I suggest we would just save 2 from the arch side and do
it in the scheduler. So actually, I really start to wonder if it's worth
it given the added complexity ...
> 3. Detecting the flag in generic kernel/sched/* code means that all
> architectures will pay the for the overhead when building/rebuilding the
> sched_domain hierarchy, and all architectures that sets the cpu
> capacities to asymmetric will set the flag whether they like it or not.
> I'm not sure if this is a problem.
That is true as well ...
>
> In the end it is really about how much of this we want in generic code
> and how much we hide in arch/, and if we dare to touch the sched_domain
> build code ;-)
Right so you can argue that the arch code is here to give you a
system-level information, and that if the scheduler wants to virtually
split that system, then it's its job to make sure that happens properly.
That is exactly what your patch does (IIUC), and I now think that this
is a very sensible middle-ground option. But this is debatable so I'm
interested to see what others think :-)
Thanks,
Quentin
Re: [PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
On Thu, Jul 05, 2018 at 02:31:43PM +0100, Quentin Perret wrote:
> Hi Morten,
>
> On Wednesday 04 Jul 2018 at 11:17:49 (+0100), Morten Rasmussen wrote:
> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> > index 71330e0e41db..29c186961345 100644
> > --- a/kernel/sched/topology.c
> > +++ b/kernel/sched/topology.c
> > @@ -1160,6 +1160,26 @@ sd_init(struct sched_domain_topology_level *tl,
> > sd_id = cpumask_first(sched_domain_span(sd));
> >
> > /*
> > +* Check if cpu_map eclipses cpu capacity asymmetry.
> > +*/
> > +
> > + if (sd->flags & SD_ASYM_CPUCAPACITY) {
> > + int i;
> > + bool disable = true;
> > + long capacity = arch_scale_cpu_capacity(NULL, sd_id);
> > +
> > + for_each_cpu(i, sched_domain_span(sd)) {
> > + if (capacity != arch_scale_cpu_capacity(NULL, i)) {
> > + disable = false;
> > + break;
> > + }
> > + }
> > +
> > + if (disable)
> > + sd->flags &= ~SD_ASYM_CPUCAPACITY;
> > + }
> > +
> > + /*
> > * Convert topological properties into behaviour.
> > */
>
> If SD_ASYM_CPUCAPACITY means that some CPUs have different
> arch_scale_cpu_capacity() values, we could also automatically _set_
> the flag in sd_init() no ? Why should we let the arch set it and just
> correct it later ?
>
> I understand the moment at which we know the capacities of CPUs varies
> from arch to arch, but the arch code could just call
> rebuild_sched_domain when the capacities of CPUs change and let the
> scheduler detect things automatically. I mean, even if the arch code
> sets the flag in its topology level table, it will have to rebuild
> the sched domains anyway ...
>
> What do you think ?
We could as well set the flag here so the architecture doesn't have to
do it. It is a bit more complicated though for few reasons:
1. Detecting when to disable the flag is a lot simpler than checking
which level is should be set on. You basically have to work you way up
from the lowest topology level until you get to a level spanning all the
capacities available in the system to figure out where the flag should
be set. I don't think this fits easily with how we build the
sched_domain hierarchy. It can of course be done.
2. As you say, we still need the arch code (or cpufreq?) to rebuild the
whole thing once we know that the capacities have been determined. That
currently implies implementing arch_update_cpu_topology() which is
arch-specific. So we would need some arch code to make rebuild happen at
the right point in time. If the rebuild should be triggering the rebuild
we need another way to force a full rebuild. This can also be done.
3. Detecting the flag in generic kernel/sched/* code means that all
architectures will pay the for the overhead when building/rebuilding the
sched_domain hierarchy, and all architectures that sets the cpu
capacities to asymmetric will set the flag whether they like it or not.
I'm not sure if this is a problem.
In the end it is really about how much of this we want in generic code
and how much we hide in arch/, and if we dare to touch the sched_domain
build code ;-)
Morten
Re: [PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
On Thu, Jul 05, 2018 at 02:31:43PM +0100, Quentin Perret wrote:
> Hi Morten,
>
> On Wednesday 04 Jul 2018 at 11:17:49 (+0100), Morten Rasmussen wrote:
> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> > index 71330e0e41db..29c186961345 100644
> > --- a/kernel/sched/topology.c
> > +++ b/kernel/sched/topology.c
> > @@ -1160,6 +1160,26 @@ sd_init(struct sched_domain_topology_level *tl,
> > sd_id = cpumask_first(sched_domain_span(sd));
> >
> > /*
> > +* Check if cpu_map eclipses cpu capacity asymmetry.
> > +*/
> > +
> > + if (sd->flags & SD_ASYM_CPUCAPACITY) {
> > + int i;
> > + bool disable = true;
> > + long capacity = arch_scale_cpu_capacity(NULL, sd_id);
> > +
> > + for_each_cpu(i, sched_domain_span(sd)) {
> > + if (capacity != arch_scale_cpu_capacity(NULL, i)) {
> > + disable = false;
> > + break;
> > + }
> > + }
> > +
> > + if (disable)
> > + sd->flags &= ~SD_ASYM_CPUCAPACITY;
> > + }
> > +
> > + /*
> > * Convert topological properties into behaviour.
> > */
>
> If SD_ASYM_CPUCAPACITY means that some CPUs have different
> arch_scale_cpu_capacity() values, we could also automatically _set_
> the flag in sd_init() no ? Why should we let the arch set it and just
> correct it later ?
>
> I understand the moment at which we know the capacities of CPUs varies
> from arch to arch, but the arch code could just call
> rebuild_sched_domain when the capacities of CPUs change and let the
> scheduler detect things automatically. I mean, even if the arch code
> sets the flag in its topology level table, it will have to rebuild
> the sched domains anyway ...
>
> What do you think ?
We could as well set the flag here so the architecture doesn't have to
do it. It is a bit more complicated though for few reasons:
1. Detecting when to disable the flag is a lot simpler than checking
which level is should be set on. You basically have to work you way up
from the lowest topology level until you get to a level spanning all the
capacities available in the system to figure out where the flag should
be set. I don't think this fits easily with how we build the
sched_domain hierarchy. It can of course be done.
2. As you say, we still need the arch code (or cpufreq?) to rebuild the
whole thing once we know that the capacities have been determined. That
currently implies implementing arch_update_cpu_topology() which is
arch-specific. So we would need some arch code to make rebuild happen at
the right point in time. If the rebuild should be triggering the rebuild
we need another way to force a full rebuild. This can also be done.
3. Detecting the flag in generic kernel/sched/* code means that all
architectures will pay the for the overhead when building/rebuilding the
sched_domain hierarchy, and all architectures that sets the cpu
capacities to asymmetric will set the flag whether they like it or not.
I'm not sure if this is a problem.
In the end it is really about how much of this we want in generic code
and how much we hide in arch/, and if we dare to touch the sched_domain
build code ;-)
Morten
Re: [PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
Hi Morten,
On Wednesday 04 Jul 2018 at 11:17:49 (+0100), Morten Rasmussen wrote:
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 71330e0e41db..29c186961345 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1160,6 +1160,26 @@ sd_init(struct sched_domain_topology_level *tl,
> sd_id = cpumask_first(sched_domain_span(sd));
>
> /*
> + * Check if cpu_map eclipses cpu capacity asymmetry.
> + */
> +
> + if (sd->flags & SD_ASYM_CPUCAPACITY) {
> + int i;
> + bool disable = true;
> + long capacity = arch_scale_cpu_capacity(NULL, sd_id);
> +
> + for_each_cpu(i, sched_domain_span(sd)) {
> + if (capacity != arch_scale_cpu_capacity(NULL, i)) {
> + disable = false;
> + break;
> + }
> + }
> +
> + if (disable)
> + sd->flags &= ~SD_ASYM_CPUCAPACITY;
> + }
> +
> + /*
>* Convert topological properties into behaviour.
>*/
If SD_ASYM_CPUCAPACITY means that some CPUs have different
arch_scale_cpu_capacity() values, we could also automatically _set_
the flag in sd_init() no ? Why should we let the arch set it and just
correct it later ?
I understand the moment at which we know the capacities of CPUs varies
from arch to arch, but the arch code could just call
rebuild_sched_domain when the capacities of CPUs change and let the
scheduler detect things automatically. I mean, even if the arch code
sets the flag in its topology level table, it will have to rebuild
the sched domains anyway ...
What do you think ?
Thanks,
Quentin
Re: [PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
Hi Morten,
On Wednesday 04 Jul 2018 at 11:17:49 (+0100), Morten Rasmussen wrote:
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index 71330e0e41db..29c186961345 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -1160,6 +1160,26 @@ sd_init(struct sched_domain_topology_level *tl,
> sd_id = cpumask_first(sched_domain_span(sd));
>
> /*
> + * Check if cpu_map eclipses cpu capacity asymmetry.
> + */
> +
> + if (sd->flags & SD_ASYM_CPUCAPACITY) {
> + int i;
> + bool disable = true;
> + long capacity = arch_scale_cpu_capacity(NULL, sd_id);
> +
> + for_each_cpu(i, sched_domain_span(sd)) {
> + if (capacity != arch_scale_cpu_capacity(NULL, i)) {
> + disable = false;
> + break;
> + }
> + }
> +
> + if (disable)
> + sd->flags &= ~SD_ASYM_CPUCAPACITY;
> + }
> +
> + /*
>* Convert topological properties into behaviour.
>*/
If SD_ASYM_CPUCAPACITY means that some CPUs have different
arch_scale_cpu_capacity() values, we could also automatically _set_
the flag in sd_init() no ? Why should we let the arch set it and just
correct it later ?
I understand the moment at which we know the capacities of CPUs varies
from arch to arch, but the arch code could just call
rebuild_sched_domain when the capacities of CPUs change and let the
scheduler detect things automatically. I mean, even if the arch code
sets the flag in its topology level table, it will have to rebuild
the sched domains anyway ...
What do you think ?
Thanks,
Quentin
[PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
When hotplugging cpus out or creating exclusive cpusets (disabling
sched_load_balance) systems which were asymmetric at boot might become
symmetric. In this case leaving the flag set might lead to suboptimal
scheduling decisions.
The arch-code proving the flag doesn't have visibility of the cpuset
configuration so it must either be told by passing a cpumask or the
generic topology code has to verify if the flag should still be set
when taking the actual sched_domain_span() into account. This patch
implements the latter approach.
We need to detect capacity based on calling arch_scale_cpu_capacity()
directly as rq->cpu_capacity_orig hasn't been set yet early in the boot
process.
cc: Ingo Molnar
cc: Peter Zijlstra
Signed-off-by: Morten Rasmussen
---
kernel/sched/topology.c | 20
1 file changed, 20 insertions(+)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 71330e0e41db..29c186961345 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1160,6 +1160,26 @@ sd_init(struct sched_domain_topology_level *tl,
sd_id = cpumask_first(sched_domain_span(sd));
/*
+* Check if cpu_map eclipses cpu capacity asymmetry.
+*/
+
+ if (sd->flags & SD_ASYM_CPUCAPACITY) {
+ int i;
+ bool disable = true;
+ long capacity = arch_scale_cpu_capacity(NULL, sd_id);
+
+ for_each_cpu(i, sched_domain_span(sd)) {
+ if (capacity != arch_scale_cpu_capacity(NULL, i)) {
+ disable = false;
+ break;
+ }
+ }
+
+ if (disable)
+ sd->flags &= ~SD_ASYM_CPUCAPACITY;
+ }
+
+ /*
* Convert topological properties into behaviour.
*/
--
2.7.4
[PATCHv4 11/12] sched/core: Disable SD_ASYM_CPUCAPACITY for root_domains without asymmetry
When hotplugging cpus out or creating exclusive cpusets (disabling
sched_load_balance) systems which were asymmetric at boot might become
symmetric. In this case leaving the flag set might lead to suboptimal
scheduling decisions.
The arch-code proving the flag doesn't have visibility of the cpuset
configuration so it must either be told by passing a cpumask or the
generic topology code has to verify if the flag should still be set
when taking the actual sched_domain_span() into account. This patch
implements the latter approach.
We need to detect capacity based on calling arch_scale_cpu_capacity()
directly as rq->cpu_capacity_orig hasn't been set yet early in the boot
process.
cc: Ingo Molnar
cc: Peter Zijlstra
Signed-off-by: Morten Rasmussen
---
kernel/sched/topology.c | 20
1 file changed, 20 insertions(+)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 71330e0e41db..29c186961345 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -1160,6 +1160,26 @@ sd_init(struct sched_domain_topology_level *tl,
sd_id = cpumask_first(sched_domain_span(sd));
/*
+* Check if cpu_map eclipses cpu capacity asymmetry.
+*/
+
+ if (sd->flags & SD_ASYM_CPUCAPACITY) {
+ int i;
+ bool disable = true;
+ long capacity = arch_scale_cpu_capacity(NULL, sd_id);
+
+ for_each_cpu(i, sched_domain_span(sd)) {
+ if (capacity != arch_scale_cpu_capacity(NULL, i)) {
+ disable = false;
+ break;
+ }
+ }
+
+ if (disable)
+ sd->flags &= ~SD_ASYM_CPUCAPACITY;
+ }
+
+ /*
* Convert topological properties into behaviour.
*/
--
2.7.4
