Re: [PATCH v10 18/20] timers: Implement the hierarchical pull model

[Frederic Weisbecker]

Le Mon, Jan 15, 2024 at 03:37:41PM +0100, Anna-Maria Behnsen a écrit :
> +static void tmigr_connect_child_parent(struct tmigr_group *child,
> +				       struct tmigr_group *parent)
> +{
> +	union tmigr_state childstate;
> +
> +	raw_spin_lock_irq(&child->lock);
> +	raw_spin_lock_nested(&parent->lock, SINGLE_DEPTH_NESTING);
> +
> +	child->parent = parent;
> +	child->childmask = BIT(parent->num_children++);
> +
> +	raw_spin_unlock(&parent->lock);
> +	raw_spin_unlock_irq(&child->lock);
> +
> +	/*
> +	 * To prevent inconsistent states, active children need to be active in
> +	 * the new parent as well. Inactive children are already marked inactive
> +	 * in the parent group.
> +	 */
> +	childstate.state = atomic_read(&child->migr_state);
> +	if (childstate.migrator != TMIGR_NONE) {

Is it possible here to connect a running online child (not one that we just
created) to a new parent? If not, is it possible that a newly created child is
not TMIGR_NONE?


> +		struct tmigr_walk data;
> +
> +		data.childmask = child->childmask;
> +
> +		/*
> +		 * There is only one new level per time. When connecting the
> +		 * child and the parent and set the child active when the parent
> +		 * is inactive, the parent needs to be the uppermost
> +		 * level. Otherwise there went something wrong!
> +		 */
> +		WARN_ON(!tmigr_active_up(parent, child, &data) && parent->parent);
> +	}
> +}
[...]
> +static int tmigr_cpu_online(unsigned int cpu)
> +{
> +	struct tmigr_cpu *tmc = this_cpu_ptr(&tmigr_cpu);
> +	int ret;
> +
> +	/* First online attempt? Initialize CPU data */
> +	if (!tmc->tmgroup) {
> +		raw_spin_lock_init(&tmc->lock);
> +
> +		ret = tmigr_add_cpu(cpu);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (tmc->childmask == 0)
> +			return -EINVAL;
> +
> +		timerqueue_init(&tmc->cpuevt.nextevt);
> +		tmc->cpuevt.nextevt.expires = KTIME_MAX;
> +		tmc->cpuevt.ignore = true;
> +		tmc->cpuevt.cpu = cpu;
> +
> +		tmc->remote = false;
> +		WRITE_ONCE(tmc->wakeup, KTIME_MAX);
> +	}
> +	raw_spin_lock_irq(&tmc->lock);
> +	tmc->idle = timer_base_is_idle();
> +	if (!tmc->idle)
> +		__tmigr_cpu_activate(tmc);

Heh, I was about to say that it's impossible that timer_base_is_idle()
at this stage but actually if we run in nohz_full...

It happens so that nohz_full is deactivated until rcutree_online_cpu()
which calls tick_dep_clear() but it's a pure coincidence that might
disappear one day. So yes, let's keep it that way.

Thanks.

> +	tmc->online = true;
> +	raw_spin_unlock_irq(&tmc->lock);
> +	return 0;
> +}