Re: [PATCH v5 16/18] timer: Implement the hierarchical pull model

[Anna-Maria Behnsen]

On Tue, 21 Mar 2023, Frederic Weisbecker wrote:

> On Wed, Mar 01, 2023 at 03:17:42PM +0100, Anna-Maria Behnsen wrote:
> > +static u64 tmigr_handle_remote_cpu(unsigned int cpu, u64 now,
> > +				   unsigned long jif)
> > +{
> > +	struct timer_events tevt;
> > +	struct tmigr_walk data;
> > +	struct tmigr_cpu *tmc;
> > +	u64 next = KTIME_MAX;
> > +	unsigned long flags;
> > +
> > +	tmc = per_cpu_ptr(&tmigr_cpu, cpu);
> > +
> > +	raw_spin_lock_irqsave(&tmc->lock, flags);
> > +	/*
> > +	 * Remote CPU is offline or no longer idle or other cpu handles cpu
> > +	 * timers already or next event was already expired - return!
> > +	 */
> > +	if (!tmc->online || tmc->remote || tmc->cpuevt.ignore ||
> > +	    now < tmc->cpuevt.nextevt.expires) {
> > +		raw_spin_unlock_irqrestore(&tmc->lock, flags);
> > +		return next;
> > +	}
> > +
> > +	tmc->remote = 1;
> > +
> > +	/* Drop the lock to allow the remote CPU to exit idle */
> > +	raw_spin_unlock_irqrestore(&tmc->lock, flags);
> > +
> > +	if (cpu != smp_processor_id())
> > +		timer_expire_remote(cpu);
> > +
> > +	/* next event of cpu */
> > +	fetch_next_timer_interrupt_remote(jif, now, &tevt, cpu);
> 
> If the target CPU gets an idle interrupt right after the above call and enqueues
> a new timer (which becomes the new earliest), tmigr_cpu_deactivate() ->
> tmigr_new_timer() is going to ignore it due to tmc->remote = 1, right?

It's worse. The newly enqueued timer is updated in the timer migration
hierarchy when CPU goes back idle and afterwards it will be overwritten by
the group walk propagating the old first timer in
tmigr_handle_remote_cpu()...

I will change the code after remote timer expiry:

1. take the remote timer bases locks
2. take the tmc->lock
3. get the next timer interrupt remote
4. drop the remote timer bases locks
5. propagate new timer changes
6. drop the tmc->lock

Thanks,

	Anna-Maria