lkml.org 
[lkml]   [2018]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/4] sched/topology: SD_ASYM_CPUCAPACITY flag detection
On Mon, Jul 23, 2018 at 02:25:34PM +0100, Qais Yousef wrote:
> Hi Morten
>
> On 20/07/18 14:32, Morten Rasmussen wrote:
> >The SD_ASYM_CPUCAPACITY sched_domain flag is supposed to mark the
> >sched_domain in the hierarchy where all cpu capacities are visible for
> >any cpu's point of view on asymmetric cpu capacity systems. The
> >scheduler can then take to take capacity asymmetry into account when
>
> Did you mean "s/take to take/try to take/"?

Yes.


[...]

> >+ /*
> >+ * Examine topology from all cpu's point of views to detect the lowest
> >+ * sched_domain_topology_level where a highest capacity cpu is visible
> >+ * to everyone.
> >+ */
> >+ for_each_cpu(i, cpu_map) {
> >+ unsigned long max_capacity = arch_scale_cpu_capacity(NULL, i);
> >+ int tl_id = 0;
> >+
> >+ for_each_sd_topology(tl) {
> >+ if (tl_id < asym_level)
> >+ goto next_level;
> >+
>
> I think if you increment and then continue here you might save the extra
> branch. I didn't look at any disassembly though to verify the generated
> code.
>
> I wonder if we can introduce for_each_sd_topology_from(tl, starting_level)
> so that you can start searching from a provided level - which will make this
> skipping logic unnecessary? So the code will look like
>
>             for_each_sd_topology_from(tl, asymc_level) {
>                 ...
>             }

Both options would work. Increment+contrinue instead of goto would be
slightly less readable I think since we would still have the increment
at the end of the loop, but easy to do. Introducing
for_each_sd_topology_from() improve things too, but I wonder if it is
worth it.

> >@@ -1647,18 +1707,27 @@ build_sched_domains(const struct cpumask *cpu_map, struct sched_domain_attr *att
> > struct s_data d;
> > struct rq *rq = NULL;
> > int i, ret = -ENOMEM;
> >+ struct sched_domain_topology_level *tl_asym;
> > alloc_state = __visit_domain_allocation_hell(&d, cpu_map);
> > if (alloc_state != sa_rootdomain)
> > goto error;
> >+ tl_asym = asym_cpu_capacity_level(cpu_map);
> >+
>
> Or maybe this is not a hot path and we don't care that much about optimizing
> the search since you call it unconditionally here even for systems that
> don't care?

It does increase the cost of things like hotplug slightly and
repartitioning of root_domains a slightly but I don't see how we can
avoid it if we want generic code to set this flag. If the costs are not
acceptable I think the only option is to make the detection architecture
specific.

In any case, AFAIK rebuilding the sched_domain hierarchy shouldn't be a
normal and common thing to do. If checking for the flag is not
acceptable on SMP-only architectures, I can move it under arch/arm[,64]
although it is not as clean.

Morten

\
 
 \ /
  Last update: 2018-07-23 17:28    [W:0.161 / U:8.936 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site