Messages in this thread | | | Subject | Re: [RFC 3/3] sched/topology: Different sched groups must not have the same balance cpu | From | Rik van Riel <> | Date | Thu, 13 Apr 2017 11:27:49 -0400 |
| |
On Thu, 2017-04-13 at 10:56 -0300, Lauro Ramos Venancio wrote: > Currently, the group balance cpu is the groups's first CPU. But with > overlapping groups, two different groups can have the same first CPU. > > This patch uses the group mask to mark all the CPUs that have a > particular group as its main sched group. The group balance cpu is > the > first group CPU that is also in the mask. >
This is not your fault, but this code is really hard to understand.
Your comments tell me what the code does, but not really why.
> +++ b/kernel/sched/topology.c > @@ -477,27 +477,31 @@ enum s_alloc { > }; > > /* > - * Build an iteration mask that can exclude certain CPUs from the > upwards > - * domain traversal. > + * An overlap sched group may not be present in all CPUs that > compose the > + * group. So build the mask, marking all the group CPUs where it is > present. > * > * Asymmetric node setups can result in situations where the domain > tree is of > * unequal depth, make sure to skip domains that already cover the > entire > * range. > - * > - * In that case build_sched_domains() will have terminated the > iteration early > - * and our sibling sd spans will be empty. Domains should always > include the > - * CPU they're built on, so check that. > */
Why are we doing this?
Could the comment explain why things need to be this way?
> - for_each_cpu(i, span) { > + for_each_cpu(i, sg_span) { > sibling = *per_cpu_ptr(sdd->sd, i); > - if (!cpumask_test_cpu(i, > sched_domain_span(sibling))) > + > + /* > + * Asymmetric node setups: skip domains that are > already > + * done. > + */ > + if (!sibling->groups) > + continue; > +
What does this mean?
I would really like it if this code was a little better documented. This is not something I can put on you for most of the code, but I can at least ask it for the code you are adding :)
The same goes for the rest of this patch.
| |