Messages in this thread | | | Date | Fri, 9 Apr 2021 13:51:46 -0300 | From | Marcelo Tosatti <> | Subject | Re: [PATCH] hrtimer: avoid retrigger_next_event IPI |
| |
+CC Anna-Maria.
On Fri, Apr 09, 2021 at 04:15:13PM +0200, Thomas Gleixner wrote: > On Wed, Apr 07 2021 at 10:53, Marcelo Tosatti wrote: > > Setting the realtime clock triggers an IPI to all CPUs to reprogram > > hrtimers. > > > > However, only base, boottime and tai clocks have their offsets updated > > base clock?
Heh...
> And why boottime? Boottime is not affected by a clock > realtime set. It's clock REALTIME and TAI, nothing else.
OK!
> > +#define CLOCK_SET_BASES ((1U << HRTIMER_BASE_REALTIME)| \ > > + (1U << HRTIMER_BASE_REALTIME_SOFT)| \ > > + (1U << HRTIMER_BASE_BOOTTIME)| \ > > + (1U << HRTIMER_BASE_BOOTTIME_SOFT)| \ > > + (1U << HRTIMER_BASE_TAI)| \ > > + (1U << HRTIMER_BASE_TAI_SOFT)) > > + > > +static bool need_reprogram_timer(struct hrtimer_cpu_base *cpu_base) > > +{ > > + unsigned int active = 0; > > + > > + if (!cpu_base->softirq_activated) > > + active = cpu_base->active_bases & HRTIMER_ACTIVE_SOFT;
Again, if (cpu_base->softirq_activated), need to IPI (will resend).
> > + active = active | (cpu_base->active_bases & HRTIMER_ACTIVE_HARD); > > + > > + if ((active & CLOCK_SET_BASES) == 0) > > + return false; > > + > > + return true; > > +} > > Errm.
What?
> > + /* Avoid interrupting nohz_full CPUs if possible */ > > + preempt_disable(); > > + for_each_online_cpu(cpu) { > > + if (tick_nohz_full_cpu(cpu)) { > > + unsigned long flags; > > + struct hrtimer_cpu_base *cpu_base = &per_cpu(hrtimer_bases, cpu); > > + > > + raw_spin_lock_irqsave(&cpu_base->lock, flags); > > + if (need_reprogram_timer(cpu_base)) > > + cpumask_set_cpu(cpu, mask); > > + else > > + hrtimer_update_base(cpu_base); > > + raw_spin_unlock_irqrestore(&cpu_base->lock, flags); > > + } > > + } > > How is that supposed to be correct? > > CPU0 CPU1 > > clock_was_set() hrtimer_start(CLOCK_REALTIME) > > if (!active_mask[CPU1] & XXX) > continue; > active_mask |= REALTIME; > > ---> fail because that newly started timer is on the old offset.
CPU0 CPU1
clock_was_set() Case-1: CPU-1 grabs base->lock before CPU-0: CPU-0 sees active_mask[CPU1] and IPIs.
base = lock_hrtimer_base(timer, &flags); if (__hrtimer_start_range_ns(timer, tim, ... hrtimer_reprogram(timer, true);
unlock_hrtimer_base(timer, &flags);
raw_spin_lock_irqsave(&cpu_base->lock, flags); if (need_reprogram_timer(cpu_base)) cpumask_set_cpu(cpu, mask); else hrtimer_update_base(cpu_base); raw_spin_unlock_irqrestore(&cpu_base->lock, flags);
Case-2: CPU-1 grabs base->lock after CPU-0: CPU-0 will have updated the offsets remotely.
base = lock_hrtimer_base(timer, &flags); if (__hrtimer_start_range_ns(timer, tim, ... hrtimer_reprogram(timer, true);
unlock_hrtimer_base(timer, &flags);
No?
| |