* [patch 01/11] hrtimer: Add a mechanism to catch runaway timers
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-09-24 8:16 ` Dmitry Vyukov
2021-09-23 16:04 ` [patch 02/11] mac80211-hwsim: Fix late beacon hrtimer handling Thomas Gleixner
` (9 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML; +Cc: Peter Zijlstra, Dmitry Vyukov
A recent report from syzbot unearthed a problem with self rearming timers
which fire late and try to catch up to now with a short period. That causes
the timer to be rearmed in the past until it eventually catches up with
now. If that rearming happens from the timer callback the hard or soft
interrupt expiry loop can run for a long time with either interrupts or
bottom halves disabled which causes RCU stalls and other lockups.
There is no safety net to stop or at least identify such runaway timers.
Detection is trivial. Cache the pointer to the last expired timer. The next
invocation from the same loop compares the pointer with the next expiring
hrtimer pointer and if they match 10 times in a row (in the same hard or
soft interrupt expiry instance) then it's reasonable to declare it as a
runaway.
In that case emit a warning and skip the callback invocation which stops
the misbehaving timer right there.
It's obviously incomplete, but it's definitely better than nothing and would
have caught the reported issue in mac80211_hwsim.
Suggested-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
kernel/time/hrtimer.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1714,6 +1714,13 @@ static void __run_hrtimer(struct hrtimer
base->running = NULL;
}
+static void hrtimer_del_runaway(struct hrtimer_clock_base *base,
+ struct hrtimer *timer)
+{
+ __remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
+ pr_warn("Runaway hrtimer %p %ps stopped\n", timer, timer->function);
+}
+
static void __hrtimer_run_queues(struct hrtimer_cpu_base *cpu_base, ktime_t now,
unsigned long flags, unsigned int active_mask)
{
@@ -1722,6 +1729,8 @@ static void __hrtimer_run_queues(struct
for_each_active_base(base, cpu_base, active) {
struct timerqueue_node *node;
+ struct hrtimer *last = NULL;
+ unsigned int cnt = 0;
ktime_t basenow;
basenow = ktime_add(now, base->offset);
@@ -1732,6 +1741,22 @@ static void __hrtimer_run_queues(struct
timer = container_of(node, struct hrtimer, node);
/*
+ * Catch timers which rearm themself with a expiry
+ * time in the past over and over which makes this
+ * loop run forever.
+ */
+ if (IS_ENABLED(CONFIG_DEBUG_OBJECTS_TIMERS)) {
+ if (unlikely(last == timer)) {
+ if (++cnt == 10) {
+ hrtimer_del_runaway(base, timer);
+ continue;
+ }
+ }
+ last = timer;
+ cnt = 0;
+ }
+
+ /*
* The immediate goal for using the softexpires is
* minimizing wakeups, not running timers at the
* earliest interrupt after their soft expiration.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch 01/11] hrtimer: Add a mechanism to catch runaway timers
2021-09-23 16:04 ` [patch 01/11] hrtimer: Add a mechanism to catch runaway timers Thomas Gleixner
@ 2021-09-24 8:16 ` Dmitry Vyukov
0 siblings, 0 replies; 17+ messages in thread
From: Dmitry Vyukov @ 2021-09-24 8:16 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: LKML, Peter Zijlstra
On Thu, 23 Sept 2021 at 18:04, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> A recent report from syzbot unearthed a problem with self rearming timers
> which fire late and try to catch up to now with a short period. That causes
> the timer to be rearmed in the past until it eventually catches up with
> now. If that rearming happens from the timer callback the hard or soft
> interrupt expiry loop can run for a long time with either interrupts or
> bottom halves disabled which causes RCU stalls and other lockups.
>
> There is no safety net to stop or at least identify such runaway timers.
>
> Detection is trivial. Cache the pointer to the last expired timer. The next
> invocation from the same loop compares the pointer with the next expiring
> hrtimer pointer and if they match 10 times in a row (in the same hard or
> soft interrupt expiry instance) then it's reasonable to declare it as a
> runaway.
>
> In that case emit a warning and skip the callback invocation which stops
> the misbehaving timer right there.
>
> It's obviously incomplete, but it's definitely better than nothing and would
> have caught the reported issue in mac80211_hwsim.
>
> Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> kernel/time/hrtimer.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1714,6 +1714,13 @@ static void __run_hrtimer(struct hrtimer
> base->running = NULL;
> }
>
> +static void hrtimer_del_runaway(struct hrtimer_clock_base *base,
> + struct hrtimer *timer)
> +{
> + __remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
> + pr_warn("Runaway hrtimer %p %ps stopped\n", timer, timer->function);
Thanks for implementing this, Thomas.
Please use some standard kernel bug reporting facility here, e.g. WARN
I think will be appropriate. The ad-hoc format won't be recognized by
any testing system.
Otherwise looks good to me.
> +}
> +
> static void __hrtimer_run_queues(struct hrtimer_cpu_base *cpu_base, ktime_t now,
> unsigned long flags, unsigned int active_mask)
> {
> @@ -1722,6 +1729,8 @@ static void __hrtimer_run_queues(struct
>
> for_each_active_base(base, cpu_base, active) {
> struct timerqueue_node *node;
> + struct hrtimer *last = NULL;
> + unsigned int cnt = 0;
> ktime_t basenow;
>
> basenow = ktime_add(now, base->offset);
> @@ -1732,6 +1741,22 @@ static void __hrtimer_run_queues(struct
> timer = container_of(node, struct hrtimer, node);
>
> /*
> + * Catch timers which rearm themself with a expiry
> + * time in the past over and over which makes this
> + * loop run forever.
> + */
> + if (IS_ENABLED(CONFIG_DEBUG_OBJECTS_TIMERS)) {
> + if (unlikely(last == timer)) {
> + if (++cnt == 10) {
> + hrtimer_del_runaway(base, timer);
> + continue;
> + }
> + }
> + last = timer;
> + cnt = 0;
> + }
> +
> + /*
> * The immediate goal for using the softexpires is
> * minimizing wakeups, not running timers at the
> * earliest interrupt after their soft expiration.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [patch 02/11] mac80211-hwsim: Fix late beacon hrtimer handling
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
2021-09-23 16:04 ` [patch 01/11] hrtimer: Add a mechanism to catch runaway timers Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-09-23 16:04 ` [patch 03/11] net: iosm: Use hrtimer_forward_now() Thomas Gleixner
` (8 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML; +Cc: Peter Zijlstra, Dmitry Vyukov, Johannes Berg
From: Johannes Berg <johannes.berg@intel.com>
Thomas explained in https://lore.kernel.org/r/87mtoeb4hb.ffs@tglx
that our handling of the hrtimer here is wrong: If the timer fires
late (e.g. due to vCPU scheduling, as reported by Dmitry/syzbot)
then it tries to actually rearm the timer at the next deadline,
which might be in the past already:
1 2 3 N N+1
| | | ... | |
^ intended to fire here (1)
^ next deadline here (2)
^ actually fired here
The next time it fires, it's later, but will still try to schedule
for the next deadline (now 3), etc. until it catches up with N,
but that might take a long time, causing stalls etc.
Now, all of this is simulation, so we just have to fix it, but
note that the behaviour is wrong even per spec, since there's no
value then in sending all those beacons unaligned - they should be
aligned to the TBTT (1, 2, 3, ... in the picture), and if we're a
bit (or a lot) late, then just resume at that point.
Therefore, change the code to use hrtimer_forward_now() which will
ensure that the next firing of the timer would be at N+1 (in the
picture), i.e. the next interval point after the current time.
Fixes: 01e59e467ecf ("mac80211_hwsim: hrtimer beacon")
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Reported-by: syzbot+0e964fad69a9c462bc1e@syzkaller.appspotmail.com
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
Note: This patch has already been applied in the wireless tree. It's just
carried here for completeness sake so that the confinement of
hrtimer_forward() in the last patch works.
v2: add fixes tag - it's kind of old and the patch won't apply,
but even the original hrtimer code here had this problem
---
drivers/net/wireless/mac80211_hwsim.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index ffa894f7312a..0adae76eb8df 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1867,8 +1867,8 @@ mac80211_hwsim_beacon(struct hrtimer *timer)
bcn_int -= data->bcn_delta;
data->bcn_delta = 0;
}
- hrtimer_forward(&data->beacon_timer, hrtimer_get_expires(timer),
- ns_to_ktime(bcn_int * NSEC_PER_USEC));
+ hrtimer_forward_now(&data->beacon_timer,
+ ns_to_ktime(bcn_int * NSEC_PER_USEC));
return HRTIMER_RESTART;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [patch 03/11] net: iosm: Use hrtimer_forward_now()
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
2021-09-23 16:04 ` [patch 01/11] hrtimer: Add a mechanism to catch runaway timers Thomas Gleixner
2021-09-23 16:04 ` [patch 02/11] mac80211-hwsim: Fix late beacon hrtimer handling Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-09-23 16:04 ` [patch 04/11] ALSA: pcsp: Make hrtimer forwarding more robust Thomas Gleixner
` (7 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML
Cc: Peter Zijlstra, Loic Poulain, netdev, Sergey Ryazanov,
Jakub Kicinski, M Chetan Kumar, Johannes Berg, David S. Miller,
Intel Corporation
hrtimer_forward_now() is providing the same functionality. Preparation for
making hrtimer_forward() timer core code only.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Loic Poulain <loic.poulain@linaro.org>
Cc: netdev@vger.kernel.org
Cc: Sergey Ryazanov <ryazanov.s.a@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: M Chetan Kumar <m.chetan.kumar@intel.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Intel Corporation <linuxwwan@intel.com>
---
drivers/net/wwan/iosm/iosm_ipc_imem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/drivers/net/wwan/iosm/iosm_ipc_imem.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c
@@ -482,8 +482,8 @@ static enum hrtimer_restart ipc_imem_sta
container_of(hr_timer, struct iosm_imem, startup_timer);
if (ktime_to_ns(ipc_imem->hrtimer_period)) {
- hrtimer_forward(&ipc_imem->startup_timer, ktime_get(),
- ipc_imem->hrtimer_period);
+ hrtimer_forward_now(&ipc_imem->startup_timer,
+ ipc_imem->hrtimer_period);
result = HRTIMER_RESTART;
}
^ permalink raw reply [flat|nested] 17+ messages in thread* [patch 04/11] ALSA: pcsp: Make hrtimer forwarding more robust
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
` (2 preceding siblings ...)
2021-09-23 16:04 ` [patch 03/11] net: iosm: Use hrtimer_forward_now() Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-09-28 8:58 ` Takashi Iwai
2021-09-23 16:04 ` [patch 05/11] can: bcm: Use hrtimer_forward_now() Thomas Gleixner
` (6 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML; +Cc: Peter Zijlstra, alsa-devel, Takashi Iwai, Jaroslav Kysela
The hrtimer callback pcsp_do_timer() prepares rearming of the timer with
hrtimer_forward(). hrtimer_forward() is intended to provide a mechanism to
forward the expiry time of the hrtimer by a multiple of the period argument
so that the expiry time greater than the time provided in the 'now'
argument.
pcsp_do_timer() invokes hrtimer_forward() with the current timer expiry
time as 'now' argument. That's providing a periodic timer expiry, but is
not really robust when the timer callback is delayed so that the resulting
new expiry time is already in the past which causes the callback to be
invoked immediately again. If the timer is delayed then the back to back
invocation is not really making it better than skipping the missed
periods. Sound is distorted in any case.
Use hrtimer_forward_now() which ensures that the next expiry is in the
future. This prevents hogging the CPU in the timer expiry code and allows
later on to remove hrtimer_forward() from the public interfaces.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: alsa-devel@alsa-project.org
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Jaroslav Kysela <perex@perex.cz>
---
sound/drivers/pcsp/pcsp_lib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/sound/drivers/pcsp/pcsp_lib.c
+++ b/sound/drivers/pcsp/pcsp_lib.c
@@ -143,7 +143,7 @@ enum hrtimer_restart pcsp_do_timer(struc
if (pointer_update)
pcsp_pointer_update(chip);
- hrtimer_forward(handle, hrtimer_get_expires(handle), ns_to_ktime(ns));
+ hrtimer_forward_now(handle, ns_to_ktime(ns));
return HRTIMER_RESTART;
}
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch 04/11] ALSA: pcsp: Make hrtimer forwarding more robust
2021-09-23 16:04 ` [patch 04/11] ALSA: pcsp: Make hrtimer forwarding more robust Thomas Gleixner
@ 2021-09-28 8:58 ` Takashi Iwai
0 siblings, 0 replies; 17+ messages in thread
From: Takashi Iwai @ 2021-09-28 8:58 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, Peter Zijlstra, alsa-devel, Takashi Iwai, Jaroslav Kysela
On Thu, 23 Sep 2021 18:04:25 +0200,
Thomas Gleixner wrote:
>
> The hrtimer callback pcsp_do_timer() prepares rearming of the timer with
> hrtimer_forward(). hrtimer_forward() is intended to provide a mechanism to
> forward the expiry time of the hrtimer by a multiple of the period argument
> so that the expiry time greater than the time provided in the 'now'
> argument.
>
> pcsp_do_timer() invokes hrtimer_forward() with the current timer expiry
> time as 'now' argument. That's providing a periodic timer expiry, but is
> not really robust when the timer callback is delayed so that the resulting
> new expiry time is already in the past which causes the callback to be
> invoked immediately again. If the timer is delayed then the back to back
> invocation is not really making it better than skipping the missed
> periods. Sound is distorted in any case.
>
> Use hrtimer_forward_now() which ensures that the next expiry is in the
> future. This prevents hogging the CPU in the timer expiry code and allows
> later on to remove hrtimer_forward() from the public interfaces.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: alsa-devel@alsa-project.org
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Jaroslav Kysela <perex@perex.cz>
Thanks, applied now to sound git tree.
Takashi
^ permalink raw reply [flat|nested] 17+ messages in thread
* [patch 05/11] can: bcm: Use hrtimer_forward_now()
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
` (3 preceding siblings ...)
2021-09-23 16:04 ` [patch 04/11] ALSA: pcsp: Make hrtimer forwarding more robust Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-10-13 13:59 ` Marc Kleine-Budde
2021-09-23 16:04 ` [patch 06/11] power: reset: ltc2952: " Thomas Gleixner
` (5 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML
Cc: Peter Zijlstra, Oliver Hartkopp, linux-can, Marc Kleine-Budde,
netdev, Jakub Kicinski, David S. Miller
hrtimer_forward_now() provides the same functionality as the open coded
hrimer_forward() invocation. Prepares for removal of hrtimer_forward() from
the public interfaces.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: linux-can@vger.kernel.org
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: netdev@vger.kernel.org
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
---
net/can/bcm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -625,7 +625,7 @@ static enum hrtimer_restart bcm_rx_thr_h
struct bcm_op *op = container_of(hrtimer, struct bcm_op, thrtimer);
if (bcm_rx_thr_flush(op)) {
- hrtimer_forward(hrtimer, ktime_get(), op->kt_ival2);
+ hrtimer_forward_now(hrtimer, op->kt_ival2);
return HRTIMER_RESTART;
} else {
/* rearm throttle handling */
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch 05/11] can: bcm: Use hrtimer_forward_now()
2021-09-23 16:04 ` [patch 05/11] can: bcm: Use hrtimer_forward_now() Thomas Gleixner
@ 2021-10-13 13:59 ` Marc Kleine-Budde
0 siblings, 0 replies; 17+ messages in thread
From: Marc Kleine-Budde @ 2021-10-13 13:59 UTC (permalink / raw)
To: Thomas Gleixner
Cc: LKML, Peter Zijlstra, Oliver Hartkopp, linux-can, netdev,
Jakub Kicinski, David S. Miller
[-- Attachment #1: Type: text/plain, Size: 863 bytes --]
On 23.09.2021 18:04:27, Thomas Gleixner wrote:
> hrtimer_forward_now() provides the same functionality as the open coded
> hrimer_forward() invocation. Prepares for removal of hrtimer_forward() from
> the public interfaces.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: linux-can@vger.kernel.org
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: netdev@vger.kernel.org
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
Tnx, applied to linux-can-next/testing.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [patch 06/11] power: reset: ltc2952: Use hrtimer_forward_now()
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
` (4 preceding siblings ...)
2021-09-23 16:04 ` [patch 05/11] can: bcm: Use hrtimer_forward_now() Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-09-27 12:34 ` Sebastian Reichel
2021-09-23 16:04 ` [patch 07/11] drm/i915/pmu: " Thomas Gleixner
` (4 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML; +Cc: Peter Zijlstra, Sebastian Reichel, linux-pm
hrtimer_forward_now() provides the same functionality as the open coded
hrtimer_forward() invocation. Prepares for removal of hrtimer_forward()
from the public interfaces.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Reichel <sre@kernel.org>
Cc: linux-pm@vger.kernel.org
---
drivers/power/reset/ltc2952-poweroff.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
--- a/drivers/power/reset/ltc2952-poweroff.c
+++ b/drivers/power/reset/ltc2952-poweroff.c
@@ -94,7 +94,6 @@ static struct ltc2952_poweroff *ltc2952_
*/
static enum hrtimer_restart ltc2952_poweroff_timer_wde(struct hrtimer *timer)
{
- ktime_t now;
int state;
struct ltc2952_poweroff *data = to_ltc2952(timer, timer_wde);
@@ -104,8 +103,7 @@ static enum hrtimer_restart ltc2952_powe
state = gpiod_get_value(data->gpio_watchdog);
gpiod_set_value(data->gpio_watchdog, !state);
- now = hrtimer_cb_get_time(timer);
- hrtimer_forward(timer, now, data->wde_interval);
+ hrtimer_forward_now(timer, data->wde_interval);
return HRTIMER_RESTART;
}
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [patch 06/11] power: reset: ltc2952: Use hrtimer_forward_now()
2021-09-23 16:04 ` [patch 06/11] power: reset: ltc2952: " Thomas Gleixner
@ 2021-09-27 12:34 ` Sebastian Reichel
0 siblings, 0 replies; 17+ messages in thread
From: Sebastian Reichel @ 2021-09-27 12:34 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: LKML, Peter Zijlstra, linux-pm
[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]
Hi,
On Thu, Sep 23, 2021 at 06:04:28PM +0200, Thomas Gleixner wrote:
> hrtimer_forward_now() provides the same functionality as the open coded
> hrtimer_forward() invocation. Prepares for removal of hrtimer_forward()
> from the public interfaces.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: linux-pm@vger.kernel.org
> ---
Thanks, queued to power-supply's for-next branch.
-- Sebastian
> drivers/power/reset/ltc2952-poweroff.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> --- a/drivers/power/reset/ltc2952-poweroff.c
> +++ b/drivers/power/reset/ltc2952-poweroff.c
> @@ -94,7 +94,6 @@ static struct ltc2952_poweroff *ltc2952_
> */
> static enum hrtimer_restart ltc2952_poweroff_timer_wde(struct hrtimer *timer)
> {
> - ktime_t now;
> int state;
> struct ltc2952_poweroff *data = to_ltc2952(timer, timer_wde);
>
> @@ -104,8 +103,7 @@ static enum hrtimer_restart ltc2952_powe
> state = gpiod_get_value(data->gpio_watchdog);
> gpiod_set_value(data->gpio_watchdog, !state);
>
> - now = hrtimer_cb_get_time(timer);
> - hrtimer_forward(timer, now, data->wde_interval);
> + hrtimer_forward_now(timer, data->wde_interval);
>
> return HRTIMER_RESTART;
> }
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [patch 07/11] drm/i915/pmu: Use hrtimer_forward_now()
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
` (5 preceding siblings ...)
2021-09-23 16:04 ` [patch 06/11] power: reset: ltc2952: " Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-09-24 9:03 ` [Intel-gfx] " Tvrtko Ursulin
2021-09-23 16:04 ` [patch 08/11] signal: Move itimer rearming into itimer code Thomas Gleixner
` (3 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML
Cc: Peter Zijlstra, David Airlie, intel-gfx, Joonas Lahtinen,
Jani Nikula, dri-devel, Daniel Vetter, Rodrigo Vivi
hrtimer_forward() is about to be removed from the public
interfaces. Replace it with hrtimer_forward_now() which provides the same
functionality.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: David Airlie <airlied@linux.ie>
Cc: intel-gfx@lists.freedesktop.org
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
drivers/gpu/drm/i915/i915_pmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -439,7 +439,7 @@ static enum hrtimer_restart i915_sample(
engines_sample(gt, period_ns);
frequency_sample(gt, period_ns);
- hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
+ hrtimer_forward_now(hrtimer, ns_to_ktime(PERIOD));
return HRTIMER_RESTART;
}
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Intel-gfx] [patch 07/11] drm/i915/pmu: Use hrtimer_forward_now()
2021-09-23 16:04 ` [patch 07/11] drm/i915/pmu: " Thomas Gleixner
@ 2021-09-24 9:03 ` Tvrtko Ursulin
0 siblings, 0 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2021-09-24 9:03 UTC (permalink / raw)
To: Thomas Gleixner, LKML
Cc: Peter Zijlstra, David Airlie, intel-gfx, Joonas Lahtinen,
Jani Nikula, dri-devel, Daniel Vetter, Rodrigo Vivi
On 23/09/2021 17:04, Thomas Gleixner wrote:
> hrtimer_forward() is about to be removed from the public
> interfaces. Replace it with hrtimer_forward_now() which provides the same
> functionality.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -439,7 +439,7 @@ static enum hrtimer_restart i915_sample(
> engines_sample(gt, period_ns);
> frequency_sample(gt, period_ns);
>
> - hrtimer_forward(hrtimer, now, ns_to_ktime(PERIOD));
> + hrtimer_forward_now(hrtimer, ns_to_ktime(PERIOD));
>
> return HRTIMER_RESTART;
> }
>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 17+ messages in thread
* [patch 08/11] signal: Move itimer rearming into itimer code
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
` (6 preceding siblings ...)
2021-09-23 16:04 ` [patch 07/11] drm/i915/pmu: " Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-09-23 16:04 ` [patch 09/11] posix-timers: Fixup stale commnt and reduce ifdeffery Thomas Gleixner
` (2 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML; +Cc: Peter Zijlstra, Eric W. Biederman
Move the itimer rearming functionality into itimer.c which cleans up the
ifdeffery in the signal code.
While at it replace the open coded hrtimer_forward() invocation with
hrtimer_forward_now() which is equivalent.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
---
include/linux/posix-timers.h | 3 +++
kernel/signal.c | 14 ++------------
kernel/time/itimer.c | 13 +++++++++++++
3 files changed, 18 insertions(+), 12 deletions(-)
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -174,6 +174,8 @@ static inline void posix_cputimers_rt_wa
.posix_cputimers = { \
.bases = INIT_CPU_TIMERBASES(s.posix_cputimers.bases), \
},
+
+void itimer_restart(void);
#else
struct posix_cputimers { };
struct cpu_timer { };
@@ -181,6 +183,7 @@ struct cpu_timer { };
static inline void posix_cputimers_init(struct posix_cputimers *pct) { }
static inline void posix_cputimers_group_init(struct posix_cputimers *pct,
u64 cpu_limit) { }
+static inline void itimer_restart(void) { }
#endif
#ifdef CONFIG_POSIX_CPU_TIMERS_TASK_WORK
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -651,7 +651,6 @@ int dequeue_signal(struct task_struct *t
if (!signr) {
signr = __dequeue_signal(&tsk->signal->shared_pending,
mask, info, &resched_timer);
-#ifdef CONFIG_POSIX_TIMERS
/*
* itimer signal ?
*
@@ -665,17 +664,8 @@ int dequeue_signal(struct task_struct *t
* reducing the timer noise on heavy loaded !highres
* systems too.
*/
- if (unlikely(signr == SIGALRM)) {
- struct hrtimer *tmr = &tsk->signal->real_timer;
-
- if (!hrtimer_is_queued(tmr) &&
- tsk->signal->it_real_incr != 0) {
- hrtimer_forward(tmr, tmr->base->get_time(),
- tsk->signal->it_real_incr);
- hrtimer_restart(tmr);
- }
- }
-#endif
+ if (unlikely(signr == SIGALRM))
+ itimer_restart();
}
recalc_sigpending();
--- a/kernel/time/itimer.c
+++ b/kernel/time/itimer.c
@@ -150,6 +150,19 @@ COMPAT_SYSCALL_DEFINE2(getitimer, int, w
}
#endif
+void itimer_restart(void)
+{
+ struct task_struct *tsk = current;
+ struct hrtimer *tmr = &tsk->signal->real_timer;
+
+ lockdep_assert_task_sighand_held(current);
+
+ if (!hrtimer_is_queued(tmr) && tsk->signal->it_real_incr != 0) {
+ hrtimer_forward_now(tmr, tsk->signal->it_real_incr);
+ hrtimer_restart(tmr);
+ }
+}
+
/*
* The timer is automagically restarted, when interval != 0
*/
^ permalink raw reply [flat|nested] 17+ messages in thread* [patch 09/11] posix-timers: Fixup stale commnt and reduce ifdeffery
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
` (7 preceding siblings ...)
2021-09-23 16:04 ` [patch 08/11] signal: Move itimer rearming into itimer code Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-09-23 16:04 ` [patch 10/11] posix-timers: Use hrtimer_forward_now() Thomas Gleixner
2021-09-23 16:04 ` [patch 11/11] hrtimer: Make hrtimer_forward() private to core timer code Thomas Gleixner
10 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML; +Cc: Peter Zijlstra
When chasing all invocations of hrtimer_forward() the comment in
posix_timer_fn() turned out to be stale and misleading.
When it was written there was an idea floating around to actually let
periodic posix timers which fail signal delivery due to SIG_IGN not rearm
the underlying hrtimer and restart the timer when SIG_IGN is lifted. In
principle a good idea, but all implementation attempts ended in creating
more problems than they solved. But the comment was left around and
suggests for 14 years now that this is going to be fixed soonish.
Rewrite it to reflect reality and replace the ifdeffery close to it with
IS_ENABLED().
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
kernel/time/posix-timers.c | 38 +++++++++++++++-----------------------
1 file changed, 15 insertions(+), 23 deletions(-)
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -387,35 +387,27 @@ static enum hrtimer_restart posix_timer_
ktime_t now = hrtimer_cb_get_time(timer);
/*
- * FIXME: What we really want, is to stop this
- * timer completely and restart it in case the
- * SIG_IGN is removed. This is a non trivial
- * change which involves sighand locking
- * (sigh !), which we don't want to do late in
- * the release cycle.
- *
- * For now we just let timers with an interval
- * less than a jiffie expire every jiffie to
- * avoid softirq starvation in case of SIG_IGN
- * and a very small interval, which would put
- * the timer right back on the softirq pending
- * list. By moving now ahead of time we trick
- * hrtimer_forward() to expire the timer
- * later, while we still maintain the overrun
- * accuracy, but have some inconsistency in
- * the timer_gettime() case. This is at least
- * better than a starved softirq. A more
- * complex fix which solves also another related
- * inconsistency is already in the pipeline.
+ * What we really want, is to stop this timer
+ * completely and restart it in case the SIG_IGN is
+ * removed. That's not trivial. For now we just let
+ * timers with an interval less than a jiffie
+ * expire every jiffie to avoid softirq starvation
+ * in case of SIG_IGN and a very small interval,
+ * which would put the timer right back on the
+ * softirq pending list. By moving now ahead of
+ * time we trick hrtimer_forward() to expire the
+ * timer later, while we still maintain the overrun
+ * accuracy, but have some inconsistency in the
+ * timer_gettime() case. This is at least better
+ * than a starved softirq.
*/
-#ifdef CONFIG_HIGH_RES_TIMERS
- {
+ if (IS_ENABLED(CONFIG_HIGH_RES_TIMERS)) {
ktime_t kj = NSEC_PER_SEC / HZ;
if (timr->it_interval < kj)
now = ktime_add(now, kj);
}
-#endif
+
timr->it_overrun += hrtimer_forward(timer, now,
timr->it_interval);
ret = HRTIMER_RESTART;
^ permalink raw reply [flat|nested] 17+ messages in thread* [patch 10/11] posix-timers: Use hrtimer_forward_now()
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
` (8 preceding siblings ...)
2021-09-23 16:04 ` [patch 09/11] posix-timers: Fixup stale commnt and reduce ifdeffery Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
2021-09-23 16:04 ` [patch 11/11] hrtimer: Make hrtimer_forward() private to core timer code Thomas Gleixner
10 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML; +Cc: Peter Zijlstra
Replace the open coded invocation of hrtimer_forward() with
hrtimer_forward_now() which does exactly the same.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
kernel/time/posix-timers.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -294,8 +294,7 @@ static void common_hrtimer_rearm(struct
{
struct hrtimer *timer = &timr->it.real.timer;
- timr->it_overrun += hrtimer_forward(timer, timer->base->get_time(),
- timr->it_interval);
+ timr->it_overrun += hrtimer_forward_now(timer, timr->it_interval);
hrtimer_restart(timer);
}
^ permalink raw reply [flat|nested] 17+ messages in thread* [patch 11/11] hrtimer: Make hrtimer_forward() private to core timer code
2021-09-23 16:04 [patch 00/11] hrtimers: Cleanup hrtimer_forward() [ab]use Thomas Gleixner
` (9 preceding siblings ...)
2021-09-23 16:04 ` [patch 10/11] posix-timers: Use hrtimer_forward_now() Thomas Gleixner
@ 2021-09-23 16:04 ` Thomas Gleixner
10 siblings, 0 replies; 17+ messages in thread
From: Thomas Gleixner @ 2021-09-23 16:04 UTC (permalink / raw)
To: LKML; +Cc: Peter Zijlstra
hrtimer_forward() invites to be used in the wrong way and there is no real
requirement to have it outside of the timers core code.
All users outside of the timer core code have been converted to
hrtimer_forward_now() which provides all what the non-core code usage
needs.
Export hrtimer_forward_now() instead and move the prototype to a timer core
header.
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
include/linux/hrtimer.h | 26 +-------------------------
kernel/time/hrtimer.c | 23 ++++++++++++++++++++++-
kernel/time/posix-timers.c | 1 +
kernel/time/tick-internal.h | 1 +
4 files changed, 25 insertions(+), 26 deletions(-)
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -480,31 +480,7 @@ static inline int hrtimer_callback_runni
return timer->base->running == timer;
}
-/* Forward a hrtimer so it expires after now: */
-extern u64
-hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval);
-
-/**
- * hrtimer_forward_now - forward the timer expiry so it expires after now
- * @timer: hrtimer to forward
- * @interval: the interval to forward
- *
- * Forward the timer expiry so it will expire after the current time
- * of the hrtimer clock base. Returns the number of overruns.
- *
- * Can be safely called from the callback function of @timer. If
- * called from other contexts @timer must neither be enqueued nor
- * running the callback and the caller needs to take care of
- * serialization.
- *
- * Note: This only updates the timer expiry value and does not requeue
- * the timer.
- */
-static inline u64 hrtimer_forward_now(struct hrtimer *timer,
- ktime_t interval)
-{
- return hrtimer_forward(timer, timer->base->get_time(), interval);
-}
+extern u64 hrtimer_forward_now(struct hrtimer *timer, ktime_t interval);
/* Precise sleep: */
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1067,7 +1067,28 @@ u64 hrtimer_forward(struct hrtimer *time
return orun;
}
-EXPORT_SYMBOL_GPL(hrtimer_forward);
+
+/**
+ * hrtimer_forward_now - forward the timer expiry so it expires after now
+ * @timer: hrtimer to forward
+ * @interval: the interval to forward
+ *
+ * Forward the timer expiry so it will expire after the current time
+ * of the hrtimer clock base. Returns the number of overruns.
+ *
+ * Can be safely called from the callback function of @timer. If
+ * called from other contexts @timer must neither be enqueued nor
+ * running the callback and the caller needs to take care of
+ * serialization.
+ *
+ * Note: This only updates the timer expiry value and does not requeue
+ * the timer.
+ */
+extern u64 hrtimer_forward_now(struct hrtimer *timer, ktime_t interval)
+{
+ return hrtimer_forward(timer, timer->base->get_time(), interval);
+}
+EXPORT_SYMBOL_GPL(hrtimer_forward_now);
/*
* enqueue_hrtimer - internal function to (re)start a timer
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -33,6 +33,7 @@
#include <linux/time_namespace.h>
#include "timekeeping.h"
+#include "tick-internal.h"
#include "posix-timers.h"
/*
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -177,6 +177,7 @@ void clock_was_set(unsigned int bases);
void clock_was_set_delayed(void);
void hrtimers_resume_local(void);
+u64 hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval);
/* Since jiffies uses a simple TICK_NSEC multiplier
* conversion, the .shift value could be zero. However
^ permalink raw reply [flat|nested] 17+ messages in thread