On Tue, Apr 18, 2017 at 01:11:10PM +0200, Thomas Gleixner wrote:
> +static u64 tmigr_set_cpu_inactive(struct tmigr_group *group,
> + struct tmigr_group *child,
> + struct tmigr_event *evt,
> + unsigned int cpu)
> +{
> + struct tmigr_group *parent;
> + u64 nextevt = KTIME_MAX;
> +
> + raw_spin_lock_nested(&group->lock, group->level);
> +
> + DBG_BUG_ON(!group->active);
> +
> + cpumask_clear_cpu(cpu, group->cpus);
> + group->active--;
> +
> + /*
> + * If @child is not NULL, then this is a recursive invocation to
> + * propagate the deactivation of @cpu. If @child has a new migrator
> + * set it active in @group.
> + */
> + if (child && child->migrator != TMIGR_NONE) {
> + cpumask_set_cpu(child->migrator, group->cpus);
> + group->active++;
And I'm confused...
If we retain child->migrator as 'active', should we then not also re-set
our own bit for that child group?
> + }
> +
> + /* Add @evt to @group */
> + tmigr_add_evt(group, evt);
> +
> + /* If @cpu is not the active migrator, everything is up to date */
> + if (group->migrator != cpu)
> + goto done;
At this point we have already cleared @cpu's bit in our group->cpus. Is
that right?
> + /* Update the migrator. */
> + if (!group->active)
> + group->migrator = TMIGR_NONE;
> + else
> + group->migrator = cpumask_first(group->cpus);
So here we could have changed ->migrator away from @cpu, no?
> +
> + parent = group->parent;
> + if (parent) {
> + /*
> + * @cpu was the migrator in @group, so it is marked as
> + * active in its parent group(s) as well. Propagate the
> + * migrator change.
> + */
So how is that then still valid? Because this seems to hinge on the
assumption that @cpu is the migrator.
> + evt = group->active ? NULL : &group->groupevt;
> + nextevt = tmigr_set_cpu_inactive(parent, group, evt, cpu);
In general I'm a wee bit confused on how this works. Do we at all times
retain a migrator per group, or only one per group that has activity,
which then reduces to 1 per system when the whole system idles.
I'll stare at this a bit more, but I feel a comment explaining things
wouldn't go amiss.
