LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [GIT PULL] timers fixes
@ 2021-07-15 10:42 Frederic Weisbecker
  2021-07-15 10:42 ` [PATCH 1/2] posix-cpu-timers: Fix rearm racing against process tick Frederic Weisbecker
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Frederic Weisbecker @ 2021-07-15 10:42 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar
  Cc: LKML, Frederic Weisbecker, Peter Zijlstra,
	Nicolas Saenz Julienne, Anna-Maria Behnsen

Thomas, Ingo,

Please pull the timers/urgent branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
	timers/urgent

HEAD: aebacb7f6ca1926918734faae14d1f0b6fae5cb7

Thanks,
	Frederic
---

Frederic Weisbecker (1):
      posix-cpu-timers: Fix rearm racing against process tick

Nicolas Saenz Julienne (1):
      timers: Fix get_next_timer_interrupt() with no timers pending


 kernel/time/posix-cpu-timers.c | 10 +++++-----
 kernel/time/timer.c            |  8 +++++---
 2 files changed, 10 insertions(+), 8 deletions(-)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] posix-cpu-timers: Fix rearm racing against process tick
  2021-07-15 10:42 [GIT PULL] timers fixes Frederic Weisbecker
@ 2021-07-15 10:42 ` Frederic Weisbecker
  2021-07-15 10:42 ` [PATCH 2/2] timers: Fix get_next_timer_interrupt() with no timers pending Frederic Weisbecker
  2021-07-20 10:55 ` [tip: timers/urgent] Merge branch 'timers/urgent' of git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks into timers/urgent tip-bot2 for Thomas Gleixner
  2 siblings, 0 replies; 4+ messages in thread
From: Frederic Weisbecker @ 2021-07-15 10:42 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar
  Cc: LKML, Frederic Weisbecker, Peter Zijlstra, Eric W . Biederman,
	Oleg Nesterov, stable, Nicolas Saenz Julienne

Since the process wide cputime counter is started locklessly from
posix_cpu_timer_rearm(), it can be concurrently stopped by operations
on other timers from the same thread group, such as in the following
unlucky scenario:

         CPU 0                                CPU 1
         -----                                -----
                                           timer_settime(TIMER B)
   posix_cpu_timer_rearm(TIMER A)
       cpu_clock_sample_group()
           (pct->timers_active already true)

                                           handle_posix_cpu_timers()
                                               check_process_timers()
                                                   stop_process_timers()
                                                       pct->timers_active = false
       arm_timer(TIMER A)

   tick -> run_posix_cpu_timers()
       // sees !pct->timers_active, ignore
       // our TIMER A

Fix this with simply locking process wide cputime counting start and
timer arm in the same block.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
Fixes: 60f2ceaa8111 ("posix-cpu-timers: Remove unnecessary locking around cpu_clock_sample_group")
Cc: stable@vger.kernel.org
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
---
 kernel/time/posix-cpu-timers.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 29a5e54e6e10..517be7fd175e 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -991,6 +991,11 @@ static void posix_cpu_timer_rearm(struct k_itimer *timer)
 	if (!p)
 		goto out;
 
+	/* Protect timer list r/w in arm_timer() */
+	sighand = lock_task_sighand(p, &flags);
+	if (unlikely(sighand == NULL))
+		goto out;
+
 	/*
 	 * Fetch the current sample and update the timer's expiry time.
 	 */
@@ -1001,11 +1006,6 @@ static void posix_cpu_timer_rearm(struct k_itimer *timer)
 
 	bump_cpu_timer(timer, now);
 
-	/* Protect timer list r/w in arm_timer() */
-	sighand = lock_task_sighand(p, &flags);
-	if (unlikely(sighand == NULL))
-		goto out;
-
 	/*
 	 * Now re-arm for the new expiry time.
 	 */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] timers: Fix get_next_timer_interrupt() with no timers pending
  2021-07-15 10:42 [GIT PULL] timers fixes Frederic Weisbecker
  2021-07-15 10:42 ` [PATCH 1/2] posix-cpu-timers: Fix rearm racing against process tick Frederic Weisbecker
@ 2021-07-15 10:42 ` Frederic Weisbecker
  2021-07-20 10:55 ` [tip: timers/urgent] Merge branch 'timers/urgent' of git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks into timers/urgent tip-bot2 for Thomas Gleixner
  2 siblings, 0 replies; 4+ messages in thread
From: Frederic Weisbecker @ 2021-07-15 10:42 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar
  Cc: LKML, Nicolas Saenz Julienne, Peter Zijlstra, Eric W . Biederman,
	Oleg Nesterov, Frederic Weisbecker, stable

From: Nicolas Saenz Julienne <nsaenzju@redhat.com>

31cd0e119d50 ("timers: Recalculate next timer interrupt only when
necessary") subtly altered get_next_timer_interrupt()'s behaviour. The
function no longer consistently returns KTIME_MAX with no timers
pending.

In order to decide if there are any timers pending we check whether the
next expiry will happen NEXT_TIMER_MAX_DELTA jiffies from now.
Unfortunately, the next expiry time and the timer base clock are no
longer updated in unison. The former changes upon certain timer
operations (enqueue, expire, detach), whereas the latter keeps track of
jiffies as they move forward. Ultimately breaking the logic above.

A simplified example:

- Upon entering get_next_timer_interrupt() with:

	jiffies = 1
	base->clk = 0;
	base->next_expiry = NEXT_TIMER_MAX_DELTA;

  'base->next_expiry == base->clk + NEXT_TIMER_MAX_DELTA', the function
  returns KTIME_MAX.

- 'base->clk' is updated to the jiffies value.

- The next time we enter get_next_timer_interrupt(), taking into account
  no timer operations happened:

	base->clk = 1;
	base->next_expiry = NEXT_TIMER_MAX_DELTA;

  'base->next_expiry != base->clk + NEXT_TIMER_MAX_DELTA', the function
  returns a valid expire time, which is incorrect.

This ultimately might unnecessarily rearm sched's timer on nohz_full
setups, and add latency to the system[1].

So, introduce 'base->timers_pending'[2], update it every time
'base->next_expiry' changes, and use it in get_next_timer_interrupt().

[1] See tick_nohz_stop_tick().
[2] A quick pahole check on x86_64 and arm64 shows it doesn't make
    'struct timer_base' any bigger.

Fixes: 31cd0e119d50 ("timers: Recalculate next timer interrupt only when necessary")
Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 kernel/time/timer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 3fadb58fc9d7..9eb11c2209e5 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -207,6 +207,7 @@ struct timer_base {
 	unsigned int		cpu;
 	bool			next_expiry_recalc;
 	bool			is_idle;
+	bool			timers_pending;
 	DECLARE_BITMAP(pending_map, WHEEL_SIZE);
 	struct hlist_head	vectors[WHEEL_SIZE];
 } ____cacheline_aligned;
@@ -595,6 +596,7 @@ static void enqueue_timer(struct timer_base *base, struct timer_list *timer,
 		 * can reevaluate the wheel:
 		 */
 		base->next_expiry = bucket_expiry;
+		base->timers_pending = true;
 		base->next_expiry_recalc = false;
 		trigger_dyntick_cpu(base, timer);
 	}
@@ -1582,6 +1584,7 @@ static unsigned long __next_timer_interrupt(struct timer_base *base)
 	}
 
 	base->next_expiry_recalc = false;
+	base->timers_pending = !(next == base->clk + NEXT_TIMER_MAX_DELTA);
 
 	return next;
 }
@@ -1633,7 +1636,6 @@ u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
 	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
 	u64 expires = KTIME_MAX;
 	unsigned long nextevt;
-	bool is_max_delta;
 
 	/*
 	 * Pretend that there is no timer pending if the cpu is offline.
@@ -1646,7 +1648,6 @@ u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
 	if (base->next_expiry_recalc)
 		base->next_expiry = __next_timer_interrupt(base);
 	nextevt = base->next_expiry;
-	is_max_delta = (nextevt == base->clk + NEXT_TIMER_MAX_DELTA);
 
 	/*
 	 * We have a fresh next event. Check whether we can forward the
@@ -1664,7 +1665,7 @@ u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
 		expires = basem;
 		base->is_idle = false;
 	} else {
-		if (!is_max_delta)
+		if (base->timers_pending)
 			expires = basem + (u64)(nextevt - basej) * TICK_NSEC;
 		/*
 		 * If we expect to sleep more than a tick, mark the base idle.
@@ -1947,6 +1948,7 @@ int timers_prepare_cpu(unsigned int cpu)
 		base = per_cpu_ptr(&timer_bases[b], cpu);
 		base->clk = jiffies;
 		base->next_expiry = base->clk + NEXT_TIMER_MAX_DELTA;
+		base->timers_pending = false;
 		base->is_idle = false;
 	}
 	return 0;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [tip: timers/urgent] Merge branch 'timers/urgent' of git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks into timers/urgent
  2021-07-15 10:42 [GIT PULL] timers fixes Frederic Weisbecker
  2021-07-15 10:42 ` [PATCH 1/2] posix-cpu-timers: Fix rearm racing against process tick Frederic Weisbecker
  2021-07-15 10:42 ` [PATCH 2/2] timers: Fix get_next_timer_interrupt() with no timers pending Frederic Weisbecker
@ 2021-07-20 10:55 ` tip-bot2 for Thomas Gleixner
  2 siblings, 0 replies; 4+ messages in thread
From: tip-bot2 for Thomas Gleixner @ 2021-07-20 10:55 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: x86, linux-kernel

The following commit has been merged into the timers/urgent branch of tip:

Commit-ID:     ff5a6a3550cef4a272fee19520a13699343b6a47
Gitweb:        https://git.kernel.org/tip/ff5a6a3550cef4a272fee19520a13699343b6a47
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 20 Jul 2021 12:51:23 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 20 Jul 2021 12:51:23 +02:00

Merge branch 'timers/urgent' of git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks into timers/urgent

Pull dyntick fixes from Frederic Weisbecker:

  - Fix a rearm race in the posix cpu timer code
  - Handle get_next_timer_interrupt() correctly when no timers are pending

Link: https://lore.kernel.org/r/20210715104218.81276-1-frederic@kernel.org
---

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-20 11:03 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-15 10:42 [GIT PULL] timers fixes Frederic Weisbecker
2021-07-15 10:42 ` [PATCH 1/2] posix-cpu-timers: Fix rearm racing against process tick Frederic Weisbecker
2021-07-15 10:42 ` [PATCH 2/2] timers: Fix get_next_timer_interrupt() with no timers pending Frederic Weisbecker
2021-07-20 10:55 ` [tip: timers/urgent] Merge branch 'timers/urgent' of git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks into timers/urgent tip-bot2 for Thomas Gleixner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).