Re: [PATCH 1/4] sched/topology: SD_ASYM_CPUCAPACITY flag detection

[Morten Rasmussen]

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