| Message ID | 20210520184705.10845-4-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tick/broadcast: Allow per-cpu timers to be used instead of broadcast | expand |
Hi! On 20.5.2021 21.47, Will Deacon wrote: > Some SoCs have two per-cpu timer implementations where the timer with > the higher rating stops in deep idle (i.e. suffers from > CLOCK_EVT_FEAT_C3STOP) but is otherwise preferable to the timer with the > lower rating. In such a design, we rely on a global broadcast timer and > IPIs to wake up from deep idle states. > > To avoid the reliance on a global broadcast timer and also to reduce the > overhead associated with the IPI wakeups, extend > tick_install_broadcast_device() to manage per-cpu wakeup timers > separately from the broadcast device. > > For now, these timers remain unused. > > Cc: Frederic Weisbecker <fweisbec@gmail.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Marc Zyngier <maz@kernel.org> > Cc: Lorenzo Colitti <lorenzo@google.com> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Stephen Boyd <sboyd@kernel.org> > Signed-off-by: Will Deacon <will@kernel.org> > --- > kernel/time/tick-broadcast.c | 57 +++++++++++++++++++++++++++++++++++- > kernel/time/tick-common.c | 2 +- > kernel/time/tick-internal.h | 4 +-- > 3 files changed, 59 insertions(+), 4 deletions(-) > > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > index f3f2f4ba4321..8bd8cd69c8c9 100644 > --- a/kernel/time/tick-broadcast.c > +++ b/kernel/time/tick-broadcast.c > @@ -33,6 +33,8 @@ static int tick_broadcast_forced; > static __cacheline_aligned_in_smp DEFINE_RAW_SPINLOCK(tick_broadcast_lock); > > #ifdef CONFIG_TICK_ONESHOT > +static DEFINE_PER_CPU(struct clock_event_device *, tick_oneshot_wakeup_device); > + > static void tick_broadcast_setup_oneshot(struct clock_event_device *bc); > static void tick_broadcast_clear_oneshot(int cpu); > static void tick_resume_broadcast_oneshot(struct clock_event_device *bc); > @@ -88,13 +90,59 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, > return !curdev || newdev->rating > curdev->rating; > } > > +#ifdef CONFIG_TICK_ONESHOT > +static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu) > +{ > + return per_cpu(tick_oneshot_wakeup_device, cpu); > +} > + > +static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev, > + int cpu) > +{ > + struct clock_event_device *curdev; > + > + if (!newdev) > + goto set_device; > + > + if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || > + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || > + !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) > + return false; > + > + if (!cpumask_equal(newdev->cpumask, cpumask_of(cpu))) > + return false; > + > + curdev = tick_get_oneshot_wakeup_device(cpu); > + if (curdev && newdev->rating <= curdev->rating) > + return false; > + > +set_device: > + per_cpu(tick_oneshot_wakeup_device, cpu) = newdev; > + return true; > +} > +#else > +static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu) > +{ > + return NULL; > +} > + > +static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev, > + int cpu) > +{ > + return false; > +} > +#endif > + > /* > * Conditionally install/replace broadcast device > */ > -void tick_install_broadcast_device(struct clock_event_device *dev) > +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) > { > struct clock_event_device *cur = tick_broadcast_device.evtdev; > > + if (tick_set_oneshot_wakeup_device(dev, cpu)) > + return; > + > if (!tick_check_broadcast_device(cur, dev)) > return; > Does this disable hpet registering as a global broadcast device on x86 ? I think it starts with cpumask = cpu0 so it qualifies for a percpu wakeup timer. > @@ -996,6 +1044,13 @@ void hotplug_cpu__broadcast_tick_pull(int deadcpu) > */ > static void tick_broadcast_oneshot_offline(unsigned int cpu) > { > + struct clock_event_device *dev = tick_get_oneshot_wakeup_device(cpu); > + > + if (dev) { > + clockevents_switch_state(dev, CLOCK_EVT_STATE_DETACHED); > + tick_set_oneshot_wakeup_device(NULL, cpu); > + } > + > /* > * Clear the broadcast masks for the dead cpu, but do not stop > * the broadcast device! > diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c > index e15bc0ef1912..d663249652ef 100644 > --- a/kernel/time/tick-common.c > +++ b/kernel/time/tick-common.c > @@ -373,7 +373,7 @@ void tick_check_new_device(struct clock_event_device *newdev) > /* > * Can the new device be used as a broadcast device ? > */ > - tick_install_broadcast_device(newdev); > + tick_install_broadcast_device(newdev, cpu); > } > > /** > diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h > index 7a981c9e87a4..30c89639e305 100644 > --- a/kernel/time/tick-internal.h > +++ b/kernel/time/tick-internal.h > @@ -61,7 +61,7 @@ extern ssize_t sysfs_get_uname(const char *buf, char *dst, size_t cnt); > /* Broadcasting support */ > # ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST > extern int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu); > -extern void tick_install_broadcast_device(struct clock_event_device *dev); > +extern void tick_install_broadcast_device(struct clock_event_device *dev, int cpu); > extern int tick_is_broadcast_device(struct clock_event_device *dev); > extern void tick_suspend_broadcast(void); > extern void tick_resume_broadcast(void); > @@ -72,7 +72,7 @@ extern int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq); > extern struct tick_device *tick_get_broadcast_device(void); > extern struct cpumask *tick_get_broadcast_mask(void); > # else /* !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST: */ > -static inline void tick_install_broadcast_device(struct clock_event_device *dev) { } > +static inline void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) { } > static inline int tick_is_broadcast_device(struct clock_event_device *dev) { return 0; } > static inline int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu) { return 0; } > static inline void tick_do_periodic_broadcast(struct clock_event_device *d) { }
On Fri, May 21, 2021 at 05:25:41AM +0300, Mika Penttilä wrote: > On 20.5.2021 21.47, Will Deacon wrote: > > /* > > * Conditionally install/replace broadcast device > > */ > > -void tick_install_broadcast_device(struct clock_event_device *dev) > > +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) > > { > > struct clock_event_device *cur = tick_broadcast_device.evtdev; > > + if (tick_set_oneshot_wakeup_device(dev, cpu)) > > + return; > > + > > if (!tick_check_broadcast_device(cur, dev)) > > return; > > Does this disable hpet registering as a global broadcast device on x86 ? I > think it starts with cpumask = cpu0 so it qualifies for a percpu wakeup > timer. Well spotted, I think you're probably right. I'll try to reproduce on my laptop to confirm, but I hadn't noticed the tricks played with the cpumask on x86. I'll probably need to rework things so that we install the broadcast timer first, but prefer global devices. Will
On Fri, May 21, 2021 at 12:25:03PM +0100, Will Deacon wrote: > On Fri, May 21, 2021 at 05:25:41AM +0300, Mika Penttilä wrote: > > On 20.5.2021 21.47, Will Deacon wrote: > > > /* > > > * Conditionally install/replace broadcast device > > > */ > > > -void tick_install_broadcast_device(struct clock_event_device *dev) > > > +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) > > > { > > > struct clock_event_device *cur = tick_broadcast_device.evtdev; > > > + if (tick_set_oneshot_wakeup_device(dev, cpu)) > > > + return; > > > + > > > if (!tick_check_broadcast_device(cur, dev)) > > > return; > > > > Does this disable hpet registering as a global broadcast device on x86 ? I > > think it starts with cpumask = cpu0 so it qualifies for a percpu wakeup > > timer. > > Well spotted, I think you're probably right. I'll try to reproduce on my > laptop to confirm, but I hadn't noticed the tricks played with the cpumask > on x86. > > I'll probably need to rework things so that we install the broadcast timer > first, but prefer global devices. ... and in doing that, I noticed my module refcounting is off as well so I'll definitely be spinning a v2. Will
On Fri, May 21 2021 at 12:25, Will Deacon wrote: > On Fri, May 21, 2021 at 05:25:41AM +0300, Mika Penttilä wrote: >> On 20.5.2021 21.47, Will Deacon wrote: >> > /* >> > * Conditionally install/replace broadcast device >> > */ >> > -void tick_install_broadcast_device(struct clock_event_device *dev) >> > +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) >> > { >> > struct clock_event_device *cur = tick_broadcast_device.evtdev; >> > + if (tick_set_oneshot_wakeup_device(dev, cpu)) >> > + return; >> > + >> > if (!tick_check_broadcast_device(cur, dev)) >> > return; >> >> Does this disable hpet registering as a global broadcast device on x86 ? I >> think it starts with cpumask = cpu0 so it qualifies for a percpu wakeup >> timer. > > Well spotted, I think you're probably right. I'll try to reproduce on my > laptop to confirm, but I hadn't noticed the tricks played with the cpumask > on x86. > > I'll probably need to rework things so that we install the broadcast timer > first, but prefer global devices. HPET has cpumask(0) but does not have CLOCK_EVT_FEAT_PERCPU set. The feature flag is a clear indicator for per cpu. Thanks, tglx
On Fri, May 21, 2021 at 03:49:39PM +0200, Thomas Gleixner wrote: > On Fri, May 21 2021 at 12:25, Will Deacon wrote: > > On Fri, May 21, 2021 at 05:25:41AM +0300, Mika Penttilä wrote: > >> On 20.5.2021 21.47, Will Deacon wrote: > >> > /* > >> > * Conditionally install/replace broadcast device > >> > */ > >> > -void tick_install_broadcast_device(struct clock_event_device *dev) > >> > +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) > >> > { > >> > struct clock_event_device *cur = tick_broadcast_device.evtdev; > >> > + if (tick_set_oneshot_wakeup_device(dev, cpu)) > >> > + return; > >> > + > >> > if (!tick_check_broadcast_device(cur, dev)) > >> > return; > >> > >> Does this disable hpet registering as a global broadcast device on x86 ? I > >> think it starts with cpumask = cpu0 so it qualifies for a percpu wakeup > >> timer. > > > > Well spotted, I think you're probably right. I'll try to reproduce on my > > laptop to confirm, but I hadn't noticed the tricks played with the cpumask > > on x86. > > > > I'll probably need to rework things so that we install the broadcast timer > > first, but prefer global devices. > > HPET has cpumask(0) but does not have CLOCK_EVT_FEAT_PERCPU set. The > feature flag is a clear indicator for per cpu. Thanks, that makes the logic much neater because that feature already causes the device to be rejected as the broadcast so there's no need to worry about considering a device for both broadcast and a local wakeup. I'll post a v2 on Monday once I've tested it on my laptop (which is throwing ext4 errors at the moment...) Will
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index f3f2f4ba4321..8bd8cd69c8c9 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -33,6 +33,8 @@ static int tick_broadcast_forced; static __cacheline_aligned_in_smp DEFINE_RAW_SPINLOCK(tick_broadcast_lock); #ifdef CONFIG_TICK_ONESHOT +static DEFINE_PER_CPU(struct clock_event_device *, tick_oneshot_wakeup_device); + static void tick_broadcast_setup_oneshot(struct clock_event_device *bc); static void tick_broadcast_clear_oneshot(int cpu); static void tick_resume_broadcast_oneshot(struct clock_event_device *bc); @@ -88,13 +90,59 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, return !curdev || newdev->rating > curdev->rating; } +#ifdef CONFIG_TICK_ONESHOT +static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu) +{ + return per_cpu(tick_oneshot_wakeup_device, cpu); +} + +static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev, + int cpu) +{ + struct clock_event_device *curdev; + + if (!newdev) + goto set_device; + + if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || + !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) + return false; + + if (!cpumask_equal(newdev->cpumask, cpumask_of(cpu))) + return false; + + curdev = tick_get_oneshot_wakeup_device(cpu); + if (curdev && newdev->rating <= curdev->rating) + return false; + +set_device: + per_cpu(tick_oneshot_wakeup_device, cpu) = newdev; + return true; +} +#else +static struct clock_event_device *tick_get_oneshot_wakeup_device(int cpu) +{ + return NULL; +} + +static bool tick_set_oneshot_wakeup_device(struct clock_event_device *newdev, + int cpu) +{ + return false; +} +#endif + /* * Conditionally install/replace broadcast device */ -void tick_install_broadcast_device(struct clock_event_device *dev) +void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) { struct clock_event_device *cur = tick_broadcast_device.evtdev; + if (tick_set_oneshot_wakeup_device(dev, cpu)) + return; + if (!tick_check_broadcast_device(cur, dev)) return; @@ -996,6 +1044,13 @@ void hotplug_cpu__broadcast_tick_pull(int deadcpu) */ static void tick_broadcast_oneshot_offline(unsigned int cpu) { + struct clock_event_device *dev = tick_get_oneshot_wakeup_device(cpu); + + if (dev) { + clockevents_switch_state(dev, CLOCK_EVT_STATE_DETACHED); + tick_set_oneshot_wakeup_device(NULL, cpu); + } + /* * Clear the broadcast masks for the dead cpu, but do not stop * the broadcast device! diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index e15bc0ef1912..d663249652ef 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -373,7 +373,7 @@ void tick_check_new_device(struct clock_event_device *newdev) /* * Can the new device be used as a broadcast device ? */ - tick_install_broadcast_device(newdev); + tick_install_broadcast_device(newdev, cpu); } /** diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h index 7a981c9e87a4..30c89639e305 100644 --- a/kernel/time/tick-internal.h +++ b/kernel/time/tick-internal.h @@ -61,7 +61,7 @@ extern ssize_t sysfs_get_uname(const char *buf, char *dst, size_t cnt); /* Broadcasting support */ # ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST extern int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu); -extern void tick_install_broadcast_device(struct clock_event_device *dev); +extern void tick_install_broadcast_device(struct clock_event_device *dev, int cpu); extern int tick_is_broadcast_device(struct clock_event_device *dev); extern void tick_suspend_broadcast(void); extern void tick_resume_broadcast(void); @@ -72,7 +72,7 @@ extern int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq); extern struct tick_device *tick_get_broadcast_device(void); extern struct cpumask *tick_get_broadcast_mask(void); # else /* !CONFIG_GENERIC_CLOCKEVENTS_BROADCAST: */ -static inline void tick_install_broadcast_device(struct clock_event_device *dev) { } +static inline void tick_install_broadcast_device(struct clock_event_device *dev, int cpu) { } static inline int tick_is_broadcast_device(struct clock_event_device *dev) { return 0; } static inline int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu) { return 0; } static inline void tick_do_periodic_broadcast(struct clock_event_device *d) { }
Some SoCs have two per-cpu timer implementations where the timer with the higher rating stops in deep idle (i.e. suffers from CLOCK_EVT_FEAT_C3STOP) but is otherwise preferable to the timer with the lower rating. In such a design, we rely on a global broadcast timer and IPIs to wake up from deep idle states. To avoid the reliance on a global broadcast timer and also to reduce the overhead associated with the IPI wakeups, extend tick_install_broadcast_device() to manage per-cpu wakeup timers separately from the broadcast device. For now, these timers remain unused. Cc: Frederic Weisbecker <fweisbec@gmail.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Marc Zyngier <maz@kernel.org> Cc: Lorenzo Colitti <lorenzo@google.com> Cc: John Stultz <john.stultz@linaro.org> Cc: Stephen Boyd <sboyd@kernel.org> Signed-off-by: Will Deacon <will@kernel.org> --- kernel/time/tick-broadcast.c | 57 +++++++++++++++++++++++++++++++++++- kernel/time/tick-common.c | 2 +- kernel/time/tick-internal.h | 4 +-- 3 files changed, 59 insertions(+), 4 deletions(-)