Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1625446 > unrolled thread

[patch V2 00/10] timer: Move from a push remote at enqueue to a pull at expiry model

Back to article view | Back to linux.kernel

Contents

  [patch V2 00/10] timer: Move from a push remote at enqueue to a pull  at expiry model Thomas Gleixner <tglx@linutronix.de> - 2017-04-18 18:50 +0200
    [patch V2 06/10] timer: Restructure internal locking Thomas Gleixner <tglx@linutronix.de> - 2017-04-18 18:50 +0200
      Re: [patch V2 06/10] timer: Restructure internal locking Peter Zijlstra <peterz@infradead.org> - 2017-04-19 09:10 +0200
    [patch V2 10/10] timer: Always queue timers on the local CPU Thomas Gleixner <tglx@linutronix.de> - 2017-04-18 18:50 +0200
    [patch V2 07/10] tick/sched: Split out jiffies update helper function Thomas Gleixner <tglx@linutronix.de> - 2017-04-18 18:50 +0200
    [patch V2 08/10] timer: Implement the hierarchical pull model Thomas Gleixner <tglx@linutronix.de> - 2017-04-18 18:50 +0200
      Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 09:30 +0200
      Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 09:30 +0200
      Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 09:40 +0200
      Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 09:40 +0200
      Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 10:20 +0200
        Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 10:40 +0200
          Re: [patch V2 08/10] timer: Implement the hierarchical pull model Thomas Gleixner <tglx@linutronix.de> - 2017-04-19 11:10 +0200
        Re: [patch V2 08/10] timer: Implement the hierarchical pull model Thomas Gleixner <tglx@linutronix.de> - 2017-04-19 10:40 +0200
      Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 11:00 +0200
      Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 11:10 +0200
        Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 11:50 +0200
          Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 12:00 +0200
        Re: [patch V2 08/10] timer: Implement the hierarchical pull model Thomas Gleixner <tglx@linutronix.de> - 2017-04-19 11:50 +0200
          Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 12:00 +0200
      Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 11:30 +0200
      Re: [patch V2 08/10] timer: Implement the hierarchical pull model Peter Zijlstra <peterz@infradead.org> - 2017-04-19 12:30 +0200
    [patch V2 05/10] timer: Retrieve next expiry of pinned/non-pinned  timers seperately Thomas Gleixner <tglx@linutronix.de> - 2017-04-18 18:50 +0200
      Re: [patch V2 05/10] timer: Retrieve next expiry of  pinned/non-pinned timers seperately Peter Zijlstra <peterz@infradead.org> - 2017-04-19 09:10 +0200
        Re: [patch V2 05/10] timer: Retrieve next expiry of pinned/non-pinned  timers seperately Thomas Gleixner <tglx@linutronix.de> - 2017-04-19 12:00 +0200
    [patch V2 04/10] timer: Keep the pinned timers separate from the  others Thomas Gleixner <tglx@linutronix.de> - 2017-04-18 18:50 +0200
    Re: [patch V2 00/10] timer: Move from a push remote at enqueue to a  pull at expiry model "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-21 21:30 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1625970 — Re: [patch V2 08/10] timer: Implement the hierarchical pull model

On Tue, Apr 18, 2017 at 01:11:10PM +0200, Thomas Gleixner wrote:
> +#ifdef CONFIG_SMP
> +static u64
> +tick_tmigr_idle(struct tick_sched *ts, u64 next_global, u64 next_local)
> +{
> +	ts->tmigr_idle = 1;
> +
> +	/*
> +	 * If next_global is after next_local, event does not have to
> +	 * be queued in the timer migration hierarchy, but cpu needs
> +	 * to be marked as idle.
> +	 */
> +	if (next_global >= next_local)
> +		next_global = KTIME_MAX;
> +
> +	next_global = tmigr_cpu_idle(next_global);
> +
> +	return min_t(u64, next_local, next_global);
> +}
> +
> +static void tick_tmigr_stop_idle(struct tick_sched *ts)
> +{
> +	if (ts->tmigr_idle) {
> +		ts->tmigr_idle = 0;
> +		tmigr_cpu_activate();
> +	}
> +}
> +#else
> +static u64
> +tick_tmigr_idle(struct tick_sched *ts, u64 next_global, u64 next_local)
> +{
> +	return min_t(u64, next_global, next_local);
> +}
> +static inline void tick_tmigr_stop_idle(struct tick_sched *ts) { }
> +#endif /*CONFIG_SMP*/

Naming seems inconsistent with the rest of the file;

  s/tick_tmigr_idle/tick_tmigr_idle_enter/
  s/tick_tmigr_stop_idle/tick_tmigr_idle_exit/

Or something would fit better.

[toc] | [prev] | [next] | [standalone]

#1626000 — Re: [patch V2 08/10] timer: Implement the hierarchical pull model

On Tue, Apr 18, 2017 at 01:11:10PM +0200, Thomas Gleixner wrote:
> +static u64 tmigr_set_cpu_inactive(struct tmigr_group *group,
> +				  struct tmigr_group *child,
> +				  struct tmigr_event *evt,
> +				  unsigned int cpu)
> +{
> +	struct tmigr_group *parent;
> +	u64 nextevt = KTIME_MAX;
> +
> +	raw_spin_lock_nested(&group->lock, group->level);
> +
> +	DBG_BUG_ON(!group->active);
> +
> +	cpumask_clear_cpu(cpu, group->cpus);
> +	group->active--;
> +
> +	/*
> +	 * If @child is not NULL, then this is a recursive invocation to
> +	 * propagate the deactivation of @cpu. If @child has a new migrator
> +	 * set it active in @group.
> +	 */
> +	if (child && child->migrator != TMIGR_NONE) {
> +		cpumask_set_cpu(child->migrator, group->cpus);
> +		group->active++;

And I'm confused...

If we retain child->migrator as 'active', should we then not also re-set
our own bit for that child group?

> +	}
> +
> +	/* Add @evt to @group */
> +	tmigr_add_evt(group, evt);
> +
> +	/* If @cpu is not the active migrator, everything is up to date */
> +	if (group->migrator != cpu)
> +		goto done;

At this point we have already cleared @cpu's bit in our group->cpus. Is
that right?

> +	/* Update the migrator. */
> +	if (!group->active)
> +		group->migrator = TMIGR_NONE;
> +	else
> +		group->migrator = cpumask_first(group->cpus);

So here we could have changed ->migrator away from @cpu, no?

> +
> +	parent = group->parent;
> +	if (parent) {
> +		/*
> +		 * @cpu was the migrator in @group, so it is marked as
> +		 * active in its parent group(s) as well. Propagate the
> +		 * migrator change.
> +		 */

So how is that then still valid? Because this seems to hinge on the
assumption that @cpu is the migrator.

> +		evt = group->active ? NULL : &group->groupevt;
> +		nextevt = tmigr_set_cpu_inactive(parent, group, evt, cpu);



In general I'm a wee bit confused on how this works. Do we at all times
retain a migrator per group, or only one per group that has activity,
which then reduces to 1 per system when the whole system idles.

I'll stare at this a bit more, but I feel a comment explaining things
wouldn't go amiss.

[toc] | [prev] | [next] | [standalone]

#1625455 — [patch V2 05/10] timer: Retrieve next expiry of pinned/non-pinned timers seperately

To prepare for the conversion of the NOHZ timer placement to a pull at
expiry time model it's required to have seperate expiry times for the
pinned and the non-pinned (movable) timers.

No functional change

Signed-off-by: Richard Cochran <rcochran@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/time/tick-internal.h |    3 ++-
 kernel/time/tick-sched.c    |   10 ++++++----
 kernel/time/timer.c         |   41 +++++++++++++++++++++++++++++++++++------
 3 files changed, 43 insertions(+), 11 deletions(-)

--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -163,5 +163,6 @@ static inline void timers_update_migrati
 
 DECLARE_PER_CPU(struct hrtimer_cpu_base, hrtimer_bases);
 
-extern u64 get_next_timer_interrupt(unsigned long basej, u64 basem);
+extern u64 get_next_timer_interrupt(unsigned long basej, u64 basem,
+				    u64 *global_evt);
 void timer_clear_idle(void);
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -666,7 +666,7 @@ static ktime_t tick_nohz_stop_sched_tick
 					 ktime_t now, int cpu)
 {
 	struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
-	u64 basemono, next_tick, next_tmr, next_rcu, delta, expires;
+	u64 basemono, next_tick, next_local, next_global, next_rcu, delta, expires;
 	unsigned long seq, basejiff;
 	ktime_t	tick;
 
@@ -689,10 +689,12 @@ static ktime_t tick_nohz_stop_sched_tick
 		 * disabled this also looks at the next expiring
 		 * hrtimer.
 		 */
-		next_tmr = get_next_timer_interrupt(basejiff, basemono);
-		ts->next_timer = next_tmr;
+		next_local = get_next_timer_interrupt(basejiff, basemono,
+						      &next_global);
+		next_local = min(next_local, next_global);
+		ts->next_timer = next_local;
 		/* Take the next rcu event into account */
-		next_tick = next_rcu < next_tmr ? next_rcu : next_tmr;
+		next_tick = next_rcu < next_local ? next_rcu : next_local;
 	}
 
 	/*
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1472,23 +1472,27 @@ static u64 cmp_next_hrtimer_event(u64 ba
  * get_next_timer_interrupt - return the time (clock mono) of the next timer
  * @basej:	base time jiffies
  * @basem:	base time clock monotonic
+ * @global_evt:	Pointer to store the expiry time of the next global timer
  *
  * Returns the tick aligned clock monotonic time of the next pending
  * timer or KTIME_MAX if no timer is pending.
  */
-u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
+u64 get_next_timer_interrupt(unsigned long basej, u64 basem, u64 *global_evt)
 {
 	unsigned long nextevt, nextevt_local, nextevt_global;
 	bool local_empty, global_empty, local_first, is_idle;
 	struct timer_base *base_local, *base_global;
-	u64 expires = KTIME_MAX;
+	u64 local_evt = KTIME_MAX;
+
+	/* Preset global event */
+	*global_evt = KTIME_MAX;
 
 	/*
 	 * 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()))
-		return expires;
+		return local_evt;
 
 	base_local = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
 	base_global = this_cpu_ptr(&timer_bases[BASE_GLOBAL]);
@@ -1532,14 +1536,39 @@ u64 get_next_timer_interrupt(unsigned lo
 	spin_unlock(&base_local->lock);
 	spin_unlock(&base_global->lock);
 
-	if (!local_empty || !global_empty) {
+	/*
+	 * If the bases are not marked idle, i.e one of the events is at
+	 * max. one tick away, use the next event for calculating next
+	 * local expiry value. The next global event is left as KTIME_MAX,
+	 * so this CPU will not queue itself in the global expiry
+	 * mechanism.
+	 */
+	if (!is_idle) {
 		/* If we missed a tick already, force 0 delta */
 		if (time_before_eq(nextevt, basej))
 			nextevt = basej;
-		expires = basem + (nextevt - basej) * TICK_NSEC;
+		local_evt = basem + (nextevt - basej) * TICK_NSEC;
+		return cmp_next_hrtimer_event(basem, local_evt);
 	}
 
-	return cmp_next_hrtimer_event(basem, expires);
+	/*
+	 * If the bases are marked idle, i.e. the next event on both the
+	 * local and the global queue are farther away than a tick,
+	 * evaluate both bases. No need to check whether one of the bases
+	 * has an already expired timer as this is caught by the !is_idle
+	 * condition above.
+	 */
+	if (!local_empty)
+		local_evt = basem + (nextevt_local - basej) * TICK_NSEC;
+
+	/*
+	 * If the local queue expires first, there is no requirement for
+	 * queuing the CPU in the global expiry mechanism.
+	 */
+	if (!local_first && !global_empty)
+		*global_evt = basem + (nextevt_global - basej) * TICK_NSEC;
+
+	return cmp_next_hrtimer_event(basem, local_evt);
 }
 
 /**

[toc] | [prev] | [next] | [standalone]

#1625864 — Re: [patch V2 05/10] timer: Retrieve next expiry of pinned/non-pinned timers seperately

On Tue, Apr 18, 2017 at 01:11:07PM +0200, Thomas Gleixner wrote:
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1472,23 +1472,27 @@ static u64 cmp_next_hrtimer_event(u64 ba
>   * get_next_timer_interrupt - return the time (clock mono) of the next timer
>   * @basej:	base time jiffies
>   * @basem:	base time clock monotonic
> + * @global_evt:	Pointer to store the expiry time of the next global timer
>   *
>   * Returns the tick aligned clock monotonic time of the next pending
>   * timer or KTIME_MAX if no timer is pending.
>   */
> -u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
> +u64 get_next_timer_interrupt(unsigned long basej, u64 basem, u64 *global_evt)

Another tortured function signature. It seems entirely possible
@global_evt will be the next.


> +
> +	/*
> +	 * If the local queue expires first, there is no requirement for
> +	 * queuing the CPU in the global expiry mechanism.

The comment doesn't make sense... (maybe at this stage)

> +	 */
> +	if (!local_first && !global_empty)
> +		*global_evt = basem + (nextevt_global - basej) * TICK_NSEC;

I was initially thinking !local_first would have to imply !global_empty,
but after going back and reading the previous patches again, I found
this was not so. Still slightly surprising.

> +
> +	return cmp_next_hrtimer_event(basem, local_evt);
>  }
>  
>  /**
> 
> 

[toc] | [prev] | [next] | [standalone]

#1625976 — Re: [patch V2 05/10] timer: Retrieve next expiry of pinned/non-pinned timers seperately

On Wed, 19 Apr 2017, Peter Zijlstra wrote:
> On Tue, Apr 18, 2017 at 01:11:07PM +0200, Thomas Gleixner wrote:
> > +
> > +	/*
> > +	 * If the local queue expires first, there is no requirement for
> > +	 * queuing the CPU in the global expiry mechanism.
> 
> The comment doesn't make sense... (maybe at this stage)

Yeah, it's only useful once the real magic is in place.

> > +	 */
> > +	if (!local_first && !global_empty)
> > +		*global_evt = basem + (nextevt_global - basej) * TICK_NSEC;
> 
> I was initially thinking !local_first would have to imply !global_empty,
> but after going back and reading the previous patches again, I found
> this was not so. Still slightly surprising.

Indeed, that's confusing.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1625456 — [patch V2 04/10] timer: Keep the pinned timers separate from the others

Seperate the storage space for pinned timers.

This is preparatory work for changing the NOHZ timer placement from a push
at enqueue time to a pull at expiry time model.

No functional change.

Signed-off-by: Richard Cochran <rcochran@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/time/timer.c |   98 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 28 deletions(-)

--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -185,12 +185,14 @@ EXPORT_SYMBOL(jiffies_64);
 #define WHEEL_SIZE	(LVL_SIZE * LVL_DEPTH)
 
 #ifdef CONFIG_NO_HZ_COMMON
-# define NR_BASES	2
-# define BASE_STD	0
-# define BASE_DEF	1
+# define NR_BASES	3
+# define BASE_LOCAL	0
+# define BASE_GLOBAL	1
+# define BASE_DEF	2
 #else
 # define NR_BASES	1
-# define BASE_STD	0
+# define BASE_LOCAL	0
+# define BASE_GLOBAL	0
 # define BASE_DEF	0
 #endif
 
@@ -218,16 +220,18 @@ void timers_update_migration(bool update
 	unsigned int cpu;
 
 	/* Avoid the loop, if nothing to update */
-	if (this_cpu_read(timer_bases[BASE_STD].migration_enabled) == on)
+	if (this_cpu_read(timer_bases[BASE_GLOBAL].migration_enabled) == on)
 		return;
 
 	for_each_possible_cpu(cpu) {
-		per_cpu(timer_bases[BASE_STD].migration_enabled, cpu) = on;
+		per_cpu(timer_bases[BASE_LOCAL].migration_enabled, cpu) = on;
+		per_cpu(timer_bases[BASE_GLOBAL].migration_enabled, cpu) = on;
 		per_cpu(timer_bases[BASE_DEF].migration_enabled, cpu) = on;
 		per_cpu(hrtimer_bases.migration_enabled, cpu) = on;
 		if (!update_nohz)
 			continue;
-		per_cpu(timer_bases[BASE_STD].nohz_active, cpu) = true;
+		per_cpu(timer_bases[BASE_LOCAL].nohz_active, cpu) = true;
+		per_cpu(timer_bases[BASE_GLOBAL].nohz_active, cpu) = true;
 		per_cpu(timer_bases[BASE_DEF].nohz_active, cpu) = true;
 		per_cpu(hrtimer_bases.nohz_active, cpu) = true;
 	}
@@ -810,7 +814,10 @@ static int detach_if_pending(struct time
 
 static inline struct timer_base *get_timer_cpu_base(u32 tflags, u32 cpu)
 {
-	struct timer_base *base = per_cpu_ptr(&timer_bases[BASE_STD], cpu);
+	int index = tflags & TIMER_PINNED ? BASE_LOCAL : BASE_GLOBAL;
+	struct timer_base *base;
+
+	base = per_cpu_ptr(&timer_bases[index], cpu);
 
 	/*
 	 * If the timer is deferrable and nohz is active then we need to use
@@ -824,7 +831,10 @@ static inline struct timer_base *get_tim
 
 static inline struct timer_base *get_timer_this_cpu_base(u32 tflags)
 {
-	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
+	int index = tflags & TIMER_PINNED ? BASE_LOCAL : BASE_GLOBAL;
+	struct timer_base *base;
+
+	base = this_cpu_ptr(&timer_bases[index]);
 
 	/*
 	 * If the timer is deferrable and nohz is active then we need to use
@@ -1468,10 +1478,10 @@ static u64 cmp_next_hrtimer_event(u64 ba
  */
 u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
 {
-	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
+	unsigned long nextevt, nextevt_local, nextevt_global;
+	bool local_empty, global_empty, local_first, is_idle;
+	struct timer_base *base_local, *base_global;
 	u64 expires = KTIME_MAX;
-	unsigned long nextevt;
-	bool is_empty;
 
 	/*
 	 * Pretend that there is no timer pending if the cpu is offline.
@@ -1480,26 +1490,49 @@ u64 get_next_timer_interrupt(unsigned lo
 	if (cpu_is_offline(smp_processor_id()))
 		return expires;
 
-	spin_lock(&base->lock);
-	is_empty = __next_timer_interrupt(base);
-	nextevt = base->next_expiry;
+	base_local = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
+	base_global = this_cpu_ptr(&timer_bases[BASE_GLOBAL]);
+
+	spin_lock(&base_global->lock);
+	spin_lock_nested(&base_local->lock, SINGLE_DEPTH_NESTING);
+
+	local_empty = __next_timer_interrupt(base_local);
+	nextevt_local = base_local->next_expiry;
+
+	global_empty = __next_timer_interrupt(base_global);
+	nextevt_global = base_global->next_expiry;
+
 	/*
 	 * We have a fresh next event. Check whether we can forward the
 	 * base. We can only do that when @basej is past base->clk
 	 * otherwise we might rewind base->clk.
 	 */
-	if (time_after(basej, base->clk)) {
-		if (time_after(nextevt, basej))
-			base->clk = basej;
-		else if (time_after(nextevt, base->clk))
-			base->clk = nextevt;
+	if (time_after(basej, base_local->clk)) {
+		if (time_after(nextevt_local, basej))
+			base_local->clk = basej;
+		else if (time_after(nextevt_local, base_local->clk))
+			base_local->clk = nextevt_local;
+	}
+
+	if (time_after(basej, base_global->clk)) {
+		if (time_after(nextevt_global, basej))
+			base_global->clk = basej;
+		else if (time_after(nextevt_global, base_global->clk))
+			base_global->clk = nextevt_global;
 	}
 
 	/* Base is idle if the next event is more than a tick away. */
-	base->is_idle = time_after(nextevt, basej + 1);
-	spin_unlock(&base->lock);
+	local_first = time_before_eq(nextevt_local, nextevt_global);
+	nextevt = local_first ? nextevt_local : nextevt_global;
+	is_idle = time_after(nextevt, basej + 1);
+
+	/* We need to mark both bases in sync */
+	base_local->is_idle = base_global->is_idle = is_idle;
 
-	if (!is_empty) {
+	spin_unlock(&base_local->lock);
+	spin_unlock(&base_global->lock);
+
+	if (!local_empty || !global_empty) {
 		/* If we missed a tick already, force 0 delta */
 		if (time_before_eq(nextevt, basej))
 			nextevt = basej;
@@ -1516,7 +1549,7 @@ u64 get_next_timer_interrupt(unsigned lo
  */
 void timer_clear_idle(void)
 {
-	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
+	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
 
 	/*
 	 * We do this unlocked. The worst outcome is a remote enqueue sending
@@ -1525,6 +1558,9 @@ void timer_clear_idle(void)
 	 * the lock in the exit from idle path.
 	 */
 	base->is_idle = false;
+
+	base = this_cpu_ptr(&timer_bases[BASE_GLOBAL]);
+	base->is_idle = false;
 }
 
 static int collect_expired_timers(struct timer_base *base,
@@ -1614,11 +1650,17 @@ static inline void __run_timers(struct t
  */
 static __latent_entropy void run_timer_softirq(struct softirq_action *h)
 {
-	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
+	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
 
 	__run_timers(base);
-	if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && base->nohz_active)
-		__run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));
+	if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) {
+		base = this_cpu_ptr(&timer_bases[BASE_GLOBAL]);
+		__run_timers(base);
+
+		base = this_cpu_ptr(&timer_bases[BASE_DEF]);
+		if (base->nohz_active)
+			__run_timers(base);
+	}
 }
 
 /*
@@ -1626,7 +1668,7 @@ static __latent_entropy void run_timer_s
  */
 void run_local_timers(void)
 {
-	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
+	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
 
 	hrtimer_run_queues();
 	/* Raise the softirq only if required. */

[toc] | [prev] | [next] | [standalone]

#1628539 — Re: [patch V2 00/10] timer: Move from a push remote at enqueue to a pull at expiry model

On Tue, Apr 18, 2017 at 01:11:02PM +0200, Thomas Gleixner wrote:
> Placing timers at enqueue time on a target CPU based on dubious heuristics
> does not make any sense:
> 
>  1) Most timer wheel timers are canceled or rearmed before they expire.
> 
>  2) The heuristics to predict which CPU will be busy when the timer expires
>     are wrong by definition.
> 
> So we waste precious cycles to place timers at enqueue time.
> 
> The proper solution to this problem is to always queue the timers on the
> local CPU and allow the non pinned timers to be pulled onto a busy CPU at
> expiry time.
> 
> To achieve this the timer storage has been split into local pinned and
> global timers. Local pinned timers are always expired on the CPU on which
> they have been queued. Global timers can be expired on any CPU.
> 
> As long as a CPU is busy it expires both local and global timers. When a
> CPU goes idle it arms for the first expiring local timer. If the first
> expiring pinned (local) timer is before the first expiring movable timer,
> then no action is required because the CPU will wake up before the first
> movable timer expires. If the first expiring movable timer is before the
> first expiring pinned (local) timer, then this timer is queued into a idle
> timerqueue and eventually expired by some other active CPU.
> 
> To avoid global locking the timerqueues are implemented as a hierarchy. The
> lowest level of the hierarchy holds the CPUs. The CPUs are associated to
> groups of 8, which are seperated per node. If more than one CPU group
> exist, then a second level in the hierarchy collects the groups. Depending
> on the size of the system more than 2 levels are required. Each group has a
> "migrator" which checks the timerqueue during the tick for remote expirable
> timers.
> 
> If the last CPU in a group goes idle it reports the first expiring event in
> the group up to the next group(s) in the hierarchy. If the last CPU goes
> idle it arms its timer for the first system wide expiring timer to ensure
> that no timer event is missed.

OK, after several attempts this week, I formally state that I officially
suck at reviewing, though I must confess that I am quite partial to use
of hierarchies to avoid scalability bottlenecks.  I therefore applied
these patches and ran rcutorture on them.  Two of 19 scenarios resulted
in failures, namely TREE04 and TREE07.  I have posted their .config and
console.log files here:

http://www2.rdrop.com/users/paulmck/submission/TREE04.2017.04.21a.config
http://www2.rdrop.com/users/paulmck/submission/TREE04.2017.04.21a.console.log
http://www2.rdrop.com/users/paulmck/submission/TREE07.2017.04.21a.config
http://www2.rdrop.com/users/paulmck/submission/TREE07.2017.04.21a.console.log

The first splat from TREE04 is:

[    3.513964] WARNING: CPU: 1 PID: 755 at /home/paulmck/public_git/linux-rcu/kernel/time/timer_migration.c:387 tmigr_set_cpu_active+0xd7/0xf0
[    3.517320] Modules linked in:
[    3.518173] CPU: 1 PID: 755 Comm: rcu_torture_rea Not tainted 4.11.0-rc2+ #1
[    3.520134] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[    3.522692] Call Trace:
[    3.523354]  <IRQ>
[    3.523953]  dump_stack+0x4d/0x65
[    3.524846]  __warn+0xc6/0xe0
[    3.525680]  warn_slowpath_null+0x18/0x20
[    3.526787]  tmigr_set_cpu_active+0xd7/0xf0
[    3.527866]  tmigr_cpu_activate+0x36/0x40
[    3.528872]  tick_nohz_stop_sched_tick+0x248/0x330
[    3.530103]  tick_nohz_irq_exit+0x109/0x140
[    3.531214]  irq_exit+0x74/0xf0
[    3.532068]  smp_apic_timer_interrupt+0x38/0x50
[    3.533239]  apic_timer_interrupt+0x90/0xa0
[    3.534363] RIP: 0010:__local_bh_enable_ip+0x3e/0x80
[    3.535722] RSP: 0000:ffffc900014ffe60 EFLAGS: 00000286 ORIG_RAX: ffffffffffffff10
[    3.537640] RAX: 0000000000000000 RBX: ffffffff8319c0e0 RCX: 000000000000000a
[    3.539461] RDX: ffffffff81e46b40 RSI: 0000000000000200 RDI: ffffffff810b0630
[    3.541307] RBP: ffffc900014ffe60 R08: 0000000000000001 R09: 0000000000000101
[    3.543497] R10: 0000000000000000 R11: 0000000000000000 R12: fffffffffffffee5
[    3.545556] R13: 0000000000000000 R14: 00000000dfa1ef74 R15: fffffffffffffee6
[    3.547186]  </IRQ>
[    3.547689]  ? rcu_bh_torture_deferred_free+0x20/0x20
[    3.548837]  rcu_bh_torture_read_unlock+0x15/0x20
[    3.549861]  rcu_torture_reader+0xe4/0x2b0
[    3.550801]  ? rcu_torture_reader+0x2b0/0x2b0
[    3.551768]  kthread+0xfc/0x130
[    3.552465]  ? rcu_torture_fqs+0xe0/0xe0
[    3.553387]  ? kthread_create_on_node+0x40/0x40
[    3.554460]  ret_from_fork+0x29/0x40

Line 387 is this line in tmigr_set_cpu_active():

	if (WARN_ON(group->active == group->num_childs)) {

The splat from TREE07 looks quite similar, although it has three splats
rather than only two.

							Thanx, Paul

> The series is also available from git:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.timers
> 
> Changes vs. V1:
>   - Add missing inline stubs
>   - Bail out when running on UP
>   - Don't compile migration code for SMP=n
>   - Reorder trace point storage
> 
> Thanks,
> 
> 	tglx
> ---
>  /timer_migration.h              |  173 ++++++++++
>  b/kernel/time/timer_migration.c |  666 ++++++++++++++++++++++++++++++++++++++++
>  b/kernel/time/timer_migration.h |   83 ++++
>  include/linux/cpuhotplug.h      |    1 
>  kernel/time/Makefile            |    3 
>  kernel/time/tick-internal.h     |    4 
>  kernel/time/tick-sched.c        |  121 ++++++-
>  kernel/time/tick-sched.h        |    3 
>  kernel/time/timer.c             |  239 +++++++++-----
>  lib/timerqueue.c                |    8 
>  10 files changed, 1205 insertions(+), 96 deletions(-)
> 
> 
> 
> 

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web