Re: [PATCH v7 19/21] timer: Implement the hierarchical pull model

[Frederic Weisbecker <frederic@kernel.org>]

Le Wed, May 24, 2023 at 09:06:27AM +0200, Anna-Maria Behnsen a écrit :
> +static bool tmigr_inactive_up(struct tmigr_group *group,
> +			      struct tmigr_group *child,
> +			      void *ptr)
> +{
> +	union tmigr_state curstate, newstate;
> +	struct tmigr_walk *data = ptr;
> +	bool walk_done;
> +	u8 childmask;
> +
> +	childmask = data->childmask;
> +	newstate = curstate = data->groupstate;
> +
> +retry:
> +	walk_done = true;
> +
> +	/* Reset active bit when child is no longer active */
> +	if (!data->childstate.active)
> +		newstate.active &= ~childmask;
> +
> +	if (newstate.migrator == childmask) {
> +		/*
> +		 * Find a new migrator for the group, because child group
> +		 * is idle!
> +		 */
> +		if (!data->childstate.active) {
> +			unsigned long new_migr_bit, active = newstate.active;
> +
> +			new_migr_bit = find_first_bit(&active, BIT_CNT);
> +
> +			if (new_migr_bit != BIT_CNT) {
> +				newstate.migrator = BIT(new_migr_bit);
> +			} else {
> +				newstate.migrator = TMIGR_NONE;
> +
> +				/* Changes need to be propagated */
> +				walk_done = false;
> +			}
> +		}
> +	}
> +
> +	newstate.seq++;
> +
> +	WARN_ON_ONCE((newstate.migrator != TMIGR_NONE) && !(newstate.active));
> +
> +	if (!atomic_try_cmpxchg(&group->migr_state, &curstate.state, newstate.state)) {
> +		newstate.state = curstate.state;
> +
> +		/*
> +		 * Something changed in child/parent group in the meantime,
> +		 * reread the state of child and parent; Update of
> +		 * data->childstate is required for event handling;
> +		 */
> +		if (child)
> +			data->childstate.state = atomic_read(&child->migr_state);
> +
> +		goto retry;
> +	}
> +
> +	data->groupstate = newstate;
> +	data->remote = false;
> +
> +	/* Event Handling */
> +	tmigr_update_events(group, child, data);
> +
> +	if (group->parent && (walk_done == false)) {
> +		data->childmask = group->childmask;
> +		data->childstate = newstate;
> +		data->groupstate.state = atomic_read(&group->parent->migr_state);
> +	}
> +
> +	/*
> +	 * data->nextexp was set by tmigr_update_events() and contains the
> +	 * expiry of first global event which needs to be handled
> +	 */
> +	if (data->nextexp != KTIME_MAX) {
> +		WARN_ON_ONCE(group->parent);
> +		/*
> +		 * Toplevel path - If this cpu is about going offline wake
> +		 * up some random other cpu so it will take over the
> +		 * migrator duty and program its timer properly. Ideally
> +		 * wake the cpu with the closest expiry time, but that's
> +		 * overkill to figure out.
> +		 */
> +		if (!(this_cpu_ptr(&tmigr_cpu)->online)) {
> +			unsigned int cpu = smp_processor_id();
> +
> +			cpu = cpumask_any_but(cpu_online_mask, cpu);
> +			smp_send_reschedule(cpu);

Is that enough?
Assume the following:

* CPU 0 is idle and CPU 1 is the migrator
* CPU 1 goes offline and send the above IPI to CPU 0.
* CPU 0 doesn't have any global timer in its own queue, it calls
  tmigr_cpu_deactivate() with KTIME_MAX. It's already ->idle, so it
  returns ->wakeup that may be KTIME_MAX (also the call to tmigr_new_timer()
  is ignored)
* CPU 0 doesn't handle the idle migrator duty

Or is there something else I'm missing?

I guess the effect should be mostly invisible due to the fact the hotplug
BP process has to be carried by another active CPU afterward, which will take
the migrator duty.

But still, it means you may have no migrator between CPUHP_AP_TMIGR_ONLINE and
CPUHP_AP_IDLE_DEAD.

Thanks.