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

Frederic Weisbecker
2024-02-01T15:05:24Z

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;
> +}