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

Groups > linux.kernel > #1651040 > unrolled thread

Re: [PATCH 0/2] nohz: Deal with clock reprogram skipping issues v3

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/2] nohz: Deal with clock reprogram skipping issues v3 Frederic Weisbecker <fweisbec@gmail.com> - 2017-05-26 04:20 +0200
    Re: [PATCH 0/2] nohz: Deal with clock reprogram skipping issues v3 Ingo Molnar <mingo@kernel.org> - 2017-05-26 08:20 +0200
      Re: [PATCH 0/2] nohz: Deal with clock reprogram skipping issues v3 Frederic Weisbecker <fweisbec@gmail.com> - 2017-05-29 16:00 +0200
        Re: [PATCH 0/2] nohz: Deal with clock reprogram skipping issues v3 Ingo Molnar <mingo@kernel.org> - 2017-05-30 07:50 +0200
          Re: [PATCH 0/2] nohz: Deal with clock reprogram skipping issues v3 Frederic Weisbecker <fweisbec@gmail.com> - 2017-05-30 15:00 +0200

#1651040 — Re: [PATCH 0/2] nohz: Deal with clock reprogram skipping issues v3

On Wed, May 24, 2017 at 09:16:28AM +0200, Ingo Molnar wrote:
> So the interdiff between your two patches and the 3 commits already queued up is:
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index e3043873fcdc..30253ed0380b 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -150,12 +150,6 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
>  		touch_softlockup_watchdog_sched();
>  		if (is_idle_task(current))
>  			ts->idle_jiffies++;
> -		/*
> -		 * In case the current tick fired too early past its expected
> -		 * expiration, make sure we don't bypass the next clock reprogramming
> -		 * to the same deadline.
> -		 */
> -		ts->next_tick = 0;
>  	}
>  #endif
>  	update_process_times(user_mode(regs));
> @@ -1103,8 +1097,15 @@ static void tick_nohz_handler(struct clock_event_device *dev)
>  	tick_sched_handle(ts, regs);
>  
>  	/* No need to reprogram if we are running tickless  */
> -	if (unlikely(ts->tick_stopped))
> +	if (unlikely(ts->tick_stopped)) {
> +		/*
> +		 * In case the current tick fired too early past its expected
> +		 * expiration, make sure we don't bypass the next clock reprogramming
> +		 * to the same deadline.
> +		 */
> +		ts->next_tick = 0;
>  		return;
> +	}
>  
>  	hrtimer_forward(&ts->sched_timer, now, tick_period);
>  	tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
> @@ -1202,12 +1203,17 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
>  	 */
>  	if (regs)
>  		tick_sched_handle(ts, regs);
> -	else
> -		ts->next_tick = 0;
>  
>  	/* No need to reprogram if we are in idle or full dynticks mode */
> -	if (unlikely(ts->tick_stopped))
> +	if (unlikely(ts->tick_stopped)) {
> +		/*
> +		 * In case the current tick fired too early past its expected
> +		 * expiration, make sure we don't bypass the next clock reprogramming
> +		 * to the same deadline.
> +		 */
> +		ts->next_tick = 0;
>  		return HRTIMER_NORESTART;
> +	}
>  
>  	hrtimer_forward(timer, now, tick_period);
>  
> 
> ... so the two are not the same - I'd rather not rebase it, I'd like to keep what 
> is working, we had problems with these changes before ...
> 
> If you'd like the changes in this interdiff to be applied as well, please add a 
> changelog to it and post it as a fourth patch.
> 
> Thanks,
> 
> 	Ingo

So if you like, you can replace the top patch with the following. It's exactly
the same code, I've only added a comment and a changelog:

---
From 72956bf08c3b2e506a5ce5ec4faac9fd6b097307 Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <fweisbec@gmail.com>
Date: Mon, 15 May 2017 14:56:50 +0200
Subject: [PATCH] nohz: Reset next_tick cache even when the timer has no regs

The tick IRQ regs can be NULL if hrtimer_interrupt() is called from
non-interrupt contexts (ex: hotplug CPU down). For such very special
path we forget to clean the cached next tick deadline. If we are in
dynticks mode and the actual timer deadline is ahead of us, we might
perform a buggy bypass of the next clock reprogramming.

In fact since CPU down is the only user I'm aware of, this fix is likely
unnecessary as dying CPUs already clean their tick deadline cache. But
given how hard it is to debug such timer cache related issue, we should
never be short on paranoid measures.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/time/tick-sched.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 764d290..ed18ca5 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1200,8 +1200,17 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
 	 * Do not call, when we are not in irq context and have
 	 * no valid regs pointer
 	 */
-	if (regs)
+	if (regs) {
 		tick_sched_handle(ts, regs);
+	} else {
+		/*
+		 * IRQ regs are NULL if hrtimer_interrupt() is called from
+		 * non-interrupt contexts (ex: hotplug cpu down). Make sure to
+		 * clean the cached next tick deadline to avoid buggy bypass of
+		 * clock reprog.
+		 */
+		ts->next_tick = 0;
+	}
 
 	/* No need to reprogram if we are in idle or full dynticks mode */
 	if (unlikely(ts->tick_stopped))
-- 
2.7.4

[toc] | [next] | [standalone]

#1651097

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Wed, May 24, 2017 at 09:16:28AM +0200, Ingo Molnar wrote:
> > So the interdiff between your two patches and the 3 commits already queued up is:
> > 
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index e3043873fcdc..30253ed0380b 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -150,12 +150,6 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
> >  		touch_softlockup_watchdog_sched();
> >  		if (is_idle_task(current))
> >  			ts->idle_jiffies++;
> > -		/*
> > -		 * In case the current tick fired too early past its expected
> > -		 * expiration, make sure we don't bypass the next clock reprogramming
> > -		 * to the same deadline.
> > -		 */
> > -		ts->next_tick = 0;
> >  	}
> >  #endif
> >  	update_process_times(user_mode(regs));
> > @@ -1103,8 +1097,15 @@ static void tick_nohz_handler(struct clock_event_device *dev)
> >  	tick_sched_handle(ts, regs);
> >  
> >  	/* No need to reprogram if we are running tickless  */
> > -	if (unlikely(ts->tick_stopped))
> > +	if (unlikely(ts->tick_stopped)) {
> > +		/*
> > +		 * In case the current tick fired too early past its expected
> > +		 * expiration, make sure we don't bypass the next clock reprogramming
> > +		 * to the same deadline.
> > +		 */
> > +		ts->next_tick = 0;
> >  		return;
> > +	}
> >  
> >  	hrtimer_forward(&ts->sched_timer, now, tick_period);
> >  	tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
> > @@ -1202,12 +1203,17 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
> >  	 */
> >  	if (regs)
> >  		tick_sched_handle(ts, regs);
> > -	else
> > -		ts->next_tick = 0;
> >  
> >  	/* No need to reprogram if we are in idle or full dynticks mode */
> > -	if (unlikely(ts->tick_stopped))
> > +	if (unlikely(ts->tick_stopped)) {
> > +		/*
> > +		 * In case the current tick fired too early past its expected
> > +		 * expiration, make sure we don't bypass the next clock reprogramming
> > +		 * to the same deadline.
> > +		 */
> > +		ts->next_tick = 0;
> >  		return HRTIMER_NORESTART;
> > +	}
> >  
> >  	hrtimer_forward(timer, now, tick_period);
> >  
> > 
> > ... so the two are not the same - I'd rather not rebase it, I'd like to keep what 
> > is working, we had problems with these changes before ...
> > 
> > If you'd like the changes in this interdiff to be applied as well, please add a 
> > changelog to it and post it as a fourth patch.
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> So if you like, you can replace the top patch with the following. It's exactly
> the same code, I've only added a comment and a changelog:
> 
> ---
> From 72956bf08c3b2e506a5ce5ec4faac9fd6b097307 Mon Sep 17 00:00:00 2001
> From: Frederic Weisbecker <fweisbec@gmail.com>
> Date: Mon, 15 May 2017 14:56:50 +0200
> Subject: [PATCH] nohz: Reset next_tick cache even when the timer has no regs
> 
> The tick IRQ regs can be NULL if hrtimer_interrupt() is called from
> non-interrupt contexts (ex: hotplug CPU down). For such very special
> path we forget to clean the cached next tick deadline. If we are in
> dynticks mode and the actual timer deadline is ahead of us, we might
> perform a buggy bypass of the next clock reprogramming.
> 
> In fact since CPU down is the only user I'm aware of, this fix is likely
> unnecessary as dying CPUs already clean their tick deadline cache. But
> given how hard it is to debug such timer cache related issue, we should
> never be short on paranoid measures.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  kernel/time/tick-sched.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 764d290..ed18ca5 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -1200,8 +1200,17 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
>  	 * Do not call, when we are not in irq context and have
>  	 * no valid regs pointer
>  	 */
> -	if (regs)
> +	if (regs) {
>  		tick_sched_handle(ts, regs);
> +	} else {
> +		/*
> +		 * IRQ regs are NULL if hrtimer_interrupt() is called from
> +		 * non-interrupt contexts (ex: hotplug cpu down). Make sure to
> +		 * clean the cached next tick deadline to avoid buggy bypass of
> +		 * clock reprog.
> +		 */
> +		ts->next_tick = 0;
> +	}
>  
>  	/* No need to reprogram if we are in idle or full dynticks mode */
>  	if (unlikely(ts->tick_stopped))

Well, this does not answer my question: between latest tip:timers/nohz and the 
patches you posted there's a delta, so it's not just a pure rebase.

I can do a rebase to resolve the bisectability problem (which isn't very serious 
by the way, only a single commit wide window, right?), but only if 'git diff 
old_branch new_branch' comes up empty.

In every other case let's iterate the existing timers/nohz with additional 
patches, ok? I'd rather have a finegrained iteration with well-tested intermediate 
stages than break things again.

Thanks,

	Ingo

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

#1652543

On Fri, May 26, 2017 at 08:13:20AM +0200, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Wed, May 24, 2017 at 09:16:28AM +0200, Ingo Molnar wrote:
> > > So the interdiff between your two patches and the 3 commits already queued up is:
> > > 
> > > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > > index e3043873fcdc..30253ed0380b 100644
> > > --- a/kernel/time/tick-sched.c
> > > +++ b/kernel/time/tick-sched.c
> > > @@ -150,12 +150,6 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
> > >  		touch_softlockup_watchdog_sched();
> > >  		if (is_idle_task(current))
> > >  			ts->idle_jiffies++;
> > > -		/*
> > > -		 * In case the current tick fired too early past its expected
> > > -		 * expiration, make sure we don't bypass the next clock reprogramming
> > > -		 * to the same deadline.
> > > -		 */
> > > -		ts->next_tick = 0;
> > >  	}
> > >  #endif
> > >  	update_process_times(user_mode(regs));
> > > @@ -1103,8 +1097,15 @@ static void tick_nohz_handler(struct clock_event_device *dev)
> > >  	tick_sched_handle(ts, regs);
> > >  
> > >  	/* No need to reprogram if we are running tickless  */
> > > -	if (unlikely(ts->tick_stopped))
> > > +	if (unlikely(ts->tick_stopped)) {
> > > +		/*
> > > +		 * In case the current tick fired too early past its expected
> > > +		 * expiration, make sure we don't bypass the next clock reprogramming
> > > +		 * to the same deadline.
> > > +		 */
> > > +		ts->next_tick = 0;
> > >  		return;
> > > +	}
> > >  
> > >  	hrtimer_forward(&ts->sched_timer, now, tick_period);
> > >  	tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1);
> > > @@ -1202,12 +1203,17 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
> > >  	 */
> > >  	if (regs)
> > >  		tick_sched_handle(ts, regs);
> > > -	else
> > > -		ts->next_tick = 0;
> > >  
> > >  	/* No need to reprogram if we are in idle or full dynticks mode */
> > > -	if (unlikely(ts->tick_stopped))
> > > +	if (unlikely(ts->tick_stopped)) {
> > > +		/*
> > > +		 * In case the current tick fired too early past its expected
> > > +		 * expiration, make sure we don't bypass the next clock reprogramming
> > > +		 * to the same deadline.
> > > +		 */
> > > +		ts->next_tick = 0;
> > >  		return HRTIMER_NORESTART;
> > > +	}
> > >  
> > >  	hrtimer_forward(timer, now, tick_period);
> > >  
> > > 
> > > ... so the two are not the same - I'd rather not rebase it, I'd like to keep what 
> > > is working, we had problems with these changes before ...
> > > 
> > > If you'd like the changes in this interdiff to be applied as well, please add a 
> > > changelog to it and post it as a fourth patch.
> > > 
> > > Thanks,
> > > 
> > > 	Ingo
> > 
> > So if you like, you can replace the top patch with the following. It's exactly
> > the same code, I've only added a comment and a changelog:
> > 
> > ---
> > From 72956bf08c3b2e506a5ce5ec4faac9fd6b097307 Mon Sep 17 00:00:00 2001
> > From: Frederic Weisbecker <fweisbec@gmail.com>
> > Date: Mon, 15 May 2017 14:56:50 +0200
> > Subject: [PATCH] nohz: Reset next_tick cache even when the timer has no regs
> > 
> > The tick IRQ regs can be NULL if hrtimer_interrupt() is called from
> > non-interrupt contexts (ex: hotplug CPU down). For such very special
> > path we forget to clean the cached next tick deadline. If we are in
> > dynticks mode and the actual timer deadline is ahead of us, we might
> > perform a buggy bypass of the next clock reprogramming.
> > 
> > In fact since CPU down is the only user I'm aware of, this fix is likely
> > unnecessary as dying CPUs already clean their tick deadline cache. But
> > given how hard it is to debug such timer cache related issue, we should
> > never be short on paranoid measures.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > ---
> >  kernel/time/tick-sched.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index 764d290..ed18ca5 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -1200,8 +1200,17 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
> >  	 * Do not call, when we are not in irq context and have
> >  	 * no valid regs pointer
> >  	 */
> > -	if (regs)
> > +	if (regs) {
> >  		tick_sched_handle(ts, regs);
> > +	} else {
> > +		/*
> > +		 * IRQ regs are NULL if hrtimer_interrupt() is called from
> > +		 * non-interrupt contexts (ex: hotplug cpu down). Make sure to
> > +		 * clean the cached next tick deadline to avoid buggy bypass of
> > +		 * clock reprog.
> > +		 */
> > +		ts->next_tick = 0;
> > +	}
> >  
> >  	/* No need to reprogram if we are in idle or full dynticks mode */
> >  	if (unlikely(ts->tick_stopped))
> 
> Well, this does not answer my question: between latest tip:timers/nohz and the 
> patches you posted there's a delta, so it's not just a pure rebase.

Yeah but like I said, you can forget the series I posted because the diff is
mostly cosmetic and things are actually ok as they are in tip:timers/nohz

The only thing that bothers me is the fact that the HEAD of this branch doesn't have
a changelog or even just a comment.

> 
> I can do a rebase to resolve the bisectability problem (which isn't very serious 
> by the way, only a single commit wide window, right?), but only if 'git diff 
> old_branch new_branch' comes up empty.
> 
> In every other case let's iterate the existing timers/nohz with additional 
> patches, ok? I'd rather have a finegrained iteration with well-tested intermediate 
> stages than break things again.

Ok so either we simply fixup HEAD~ with HEAD or we provide a changelog to the very last
patch. Which way do you prefer?

Thanks.

> 
> Thanks,
> 
> 	Ingo

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

#1652818

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> > Well, this does not answer my question: between latest tip:timers/nohz and the 
> > patches you posted there's a delta, so it's not just a pure rebase.
> 
> Yeah but like I said, you can forget the series I posted because the diff is
> mostly cosmetic and things are actually ok as they are in tip:timers/nohz
> 
> The only thing that bothers me is the fact that the HEAD of this branch doesn't have
> a changelog or even just a comment.

We can still amend that - is this changelog what you had in mind:

  nohz: Reset next_tick cache even when the timer has no regs

  Handle tick interrupts whose regs are NULL, out of general paranoia. It happens 
  when hrtimer_interrupt() is called from non-interrupt contexts, such as hotplug
  CPU down events.

?

Or you can send me a longer version as well.

Thanks,

	Ingo

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

#1653213

On Tue, May 30, 2017 at 07:47:09AM +0200, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > > Well, this does not answer my question: between latest tip:timers/nohz and the 
> > > patches you posted there's a delta, so it's not just a pure rebase.
> > 
> > Yeah but like I said, you can forget the series I posted because the diff is
> > mostly cosmetic and things are actually ok as they are in tip:timers/nohz
> > 
> > The only thing that bothers me is the fact that the HEAD of this branch doesn't have
> > a changelog or even just a comment.
> 
> We can still amend that - is this changelog what you had in mind:
> 
>   nohz: Reset next_tick cache even when the timer has no regs
> 
>   Handle tick interrupts whose regs are NULL, out of general paranoia. It happens 
>   when hrtimer_interrupt() is called from non-interrupt contexts, such as hotplug
>   CPU down events.
> 
> ?

Yep that one is fine. Thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web