Messages in this thread | | | Date | Mon, 15 May 2023 12:19:36 +0200 | From | Sebastian Siewior <> | Subject | Re: [PATCH v6 19/21] timer: Implement the hierarchical pull model |
| |
On 2023-05-10 12:32:53 [+0200], Frederic Weisbecker wrote: > Le Wed, May 10, 2023 at 09:28:15AM +0200, Anna-Maria Behnsen a écrit : > > +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; > > + > > + tmc = per_cpu_ptr(&tmigr_cpu, cpu); > > + > > + raw_spin_lock_irq(&tmc->lock); > > + /* > > + * 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_irq(&tmc->lock); > > + return next; > > + } > > + > > + tmc->remote = 1; > > + > > + /* Drop the lock to allow the remote CPU to exit idle */ > > + raw_spin_unlock_irq(&tmc->lock); > > + > > + if (cpu != smp_processor_id()) > > + timer_expire_remote(cpu); > > + > > + /* > > + * Pretend that there is no timer pending if the cpu is offline. > > + * Possible pending timers will be migrated later to an active cpu. > > + */ > > + if (cpu_is_offline(smp_processor_id())) { > > + raw_spin_lock_irq(&tmc->lock); > > + tevt.local = tevt.global = KTIME_MAX; > > + } else { > > + /* > > + * Lock ordering needs to be preserved - timer_base locks > > + * before tmigr related locks. During fetching the next > > + * timer interrupt, also tmc->lock needs to be > > + * held. Otherwise there is a possible race window against > > + * the CPU itself when it comes out of idle, updates the > > + * first timer and goes back to idle. > > + */ > > + timer_lock_remote_bases(cpu); > > So the return value is ignored here. > > In the case of !PREEMPT_RT, I suppose it's impossible for the target > CPU to be offline. You checked above tmc->online and in-between the > call to timer_lock_remote_bases(), the path is BH-disabled, this prevents > stop_machine from running and from setting the CPU as offline.
I think you refer to the last one invoked from takedown_cpu(). This does not matter, see below.
What bothers me is that _current_ CPU is check for cpu_is_offline() and not the variable 'cpu'. Before the check timer_expire_remote() is invoked on 'cpu' and not on current.
> However in PREEMPT_RT, ksoftirqd (or timersd) is preemptible, so it seems > that it could happen in theory. And that could create a locking imbalance.
The ksoftirqd thread is part of smpboot_park_threads(). They have to stop running and clean up before the machinery continues bringing down the CPU (that is before takedown_cpu()). On the way down we have: - tmigr_cpu_offline() followed by - smpboot_park_threads().
So ksoftirqd (preempted or not) finishes before. This is for the _target_ CPU.
After the "tmc->online" check the lock is dropped and this is invoked from run_timer_softirq(). That means that _this_ CPU could get preempted (by an IRQ for instance) at this point, and once the CPU gets back here, the remote CPU (as specified in `cpu') can already be offline by the time timer_lock_remote_bases() is invoked.
So RT or not, this is racy.
> My suggestion would be to unconditionally lock the bases, you already checked if > !tmc->online before. The remote CPU may have gone down since then because the > tmc lock has been relaxed but it should be rare enough that you don't care > about optimizing with a lockless check. So you can just lock the bases, > lock the tmc and check again if tmc->online. If not then you can just ignore > the tmigr_new_timer_up call and propagation.
Regardless the previous point, this still looks odd as you pointed out. The return code is ignored and the two functions perform lock + unlock depending on it.
> Thanks.
Sebastian
| |