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

Groups > linux.kernel > #1627598 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] nohz: Deal with clock reprogram skipping issues Frederic Weisbecker <fweisbec@gmail.com> - 2017-04-20 17:40 +0200
    [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming Frederic Weisbecker <fweisbec@gmail.com> - 2017-04-20 17:40 +0200
      Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing  reprogramming Rik van Riel <riel@redhat.com> - 2017-04-20 18:10 +0200
      Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing  reprogramming Thomas Gleixner <tglx@linutronix.de> - 2017-04-20 20:00 +0200
        Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing  reprogramming Frederic Weisbecker <fweisbec@gmail.com> - 2017-04-20 20:30 +0200
          Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing  reprogramming Thomas Gleixner <tglx@linutronix.de> - 2017-04-20 21:50 +0200
            Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing  reprogramming Frederic Weisbecker <fweisbec@gmail.com> - 2017-04-21 01:30 +0200

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

Now that we've understood the issue that Pavel and Tim have reported,
here's the new version of 24b91e360ef521a2808771633d76ebc68bd5604b that
Linus reverted. I took extra care on CPU hotplug as well. Hopefully this
time the fix doesn't introduce a new bug. The second patch should warn
when such an issue arise.

Thanks.

Frederic Weisbecker (2):
  nohz: Fix again collision between tick and other hrtimers
  tick: Make sure tick timer is active when bypassing reprogramming

 kernel/time/tick-sched.c | 28 ++++++++++++++++++++++++++--
 kernel/time/tick-sched.h |  2 ++
 2 files changed, 28 insertions(+), 2 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1627603 — [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming

So far we have run into too much troubles with the optimization path
that skips reprogramming the clock on IRQ exit when the expiration
deadline hasn't changed. If by accident the cached deadline happens to
be out of sync with the hardware deadline, the buggy result and its
cause are hard to investigate. So lets detect and warn about the issue
early.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Tim Wright <tim@binbash.co.uk>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: James Hartsock <hartsjc@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
---
 kernel/time/tick-sched.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 502b320..eb1366e 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -783,8 +783,10 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
 	tick = expires;
 
 	/* Skip reprogram of event if its not changed */
-	if (ts->tick_stopped && (expires == ts->next_tick))
+	if (ts->tick_stopped && (expires == ts->next_tick)) {
+		WARN_ON_ONCE(dev->next_event > ts->next_tick);
 		goto out;
+	}
 
 	/*
 	 * nohz_stop_sched_tick can be called several times before
-- 
2.7.4

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

#1627613 — Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming

On Thu, 2017-04-20 at 17:30 +0200, Frederic Weisbecker wrote:
> So far we have run into too much troubles with the optimization path
> that skips reprogramming the clock on IRQ exit when the expiration
> deadline hasn't changed. If by accident the cached deadline happens
> to
> be out of sync with the hardware deadline, the buggy result and its
> cause are hard to investigate. So lets detect and warn about the
> issue
> early.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Tim Wright <tim@binbash.co.uk>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: James Hartsock <hartsjc@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>

Acked-by: Rik van Riel <riel@redhat.com>

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

#1627719 — Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming

On Thu, 20 Apr 2017, Frederic Weisbecker wrote:

> So far we have run into too much troubles with the optimization path
> that skips reprogramming the clock on IRQ exit when the expiration
> deadline hasn't changed. If by accident the cached deadline happens to
> be out of sync with the hardware deadline, the buggy result and its
> cause are hard to investigate. So lets detect and warn about the issue
> early.
> 
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Tim Wright <tim@binbash.co.uk>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: James Hartsock <hartsjc@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> ---
>  kernel/time/tick-sched.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 502b320..eb1366e 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -783,8 +783,10 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
>  	tick = expires;
>  
>  	/* Skip reprogram of event if its not changed */
> -	if (ts->tick_stopped && (expires == ts->next_tick))
> +	if (ts->tick_stopped && (expires == ts->next_tick)) {
> +		WARN_ON_ONCE(dev->next_event > ts->next_tick);

What about handling it proper ? dev->next_event might be KTIME_MAX,
i.e. no more event for the next 500+ years.

Thanks,

	tglx

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

#1627733 — Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming

On Thu, Apr 20, 2017 at 07:56:22PM +0200, Thomas Gleixner wrote:
> On Thu, 20 Apr 2017, Frederic Weisbecker wrote:
> 
> > So far we have run into too much troubles with the optimization path
> > that skips reprogramming the clock on IRQ exit when the expiration
> > deadline hasn't changed. If by accident the cached deadline happens to
> > be out of sync with the hardware deadline, the buggy result and its
> > cause are hard to investigate. So lets detect and warn about the issue
> > early.
> > 
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Tim Wright <tim@binbash.co.uk>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: James Hartsock <hartsjc@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Rik van Riel <riel@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > ---
> >  kernel/time/tick-sched.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index 502b320..eb1366e 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -783,8 +783,10 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
> >  	tick = expires;
> >  
> >  	/* Skip reprogram of event if its not changed */
> > -	if (ts->tick_stopped && (expires == ts->next_tick))
> > +	if (ts->tick_stopped && (expires == ts->next_tick)) {
> > +		WARN_ON_ONCE(dev->next_event > ts->next_tick);
> 
> What about handling it proper ? dev->next_event might be KTIME_MAX,
> i.e. no more event for the next 500+ years.

I thought I handled this case, what I'm I missing?

> Thanks,
> 
> 	tglx

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

#1627763 — Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming

On Thu, 20 Apr 2017, Frederic Weisbecker wrote:
> On Thu, Apr 20, 2017 at 07:56:22PM +0200, Thomas Gleixner wrote:
> > >  	/* Skip reprogram of event if its not changed */
> > > -	if (ts->tick_stopped && (expires == ts->next_tick))
> > > +	if (ts->tick_stopped && (expires == ts->next_tick)) {
> > > +		WARN_ON_ONCE(dev->next_event > ts->next_tick);
> > 
> > What about handling it proper ? dev->next_event might be KTIME_MAX,
> > i.e. no more event for the next 500+ years.
> 
> I thought I handled this case, what I'm I missing?

	if (ts->tick_stopped && (expires == ts->next_tick)) {
		WARN_ON_ONCE(dev->next_event > ts->next_tick);
		goto out;
	}

IOW, the WARN_ON yells in dmesg, but despite seing the wreckage it just
leaves it and goes out doing nothing.

Why can't you just do

	if (ts->tick_stopped && (expires == ts->next_tick)) {
	   	if (dev->next_event > ts->next_tick)) {
			WARN_ONCE();
			do_something_sensible();
		}		
		goto out;
	}

Hmm?

	tglx

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

#1627865 — Re: [PATCH 2/2] tick: Make sure tick timer is active when bypassing reprogramming

On Thu, Apr 20, 2017 at 09:40:12PM +0200, Thomas Gleixner wrote:
> On Thu, 20 Apr 2017, Frederic Weisbecker wrote:
> > On Thu, Apr 20, 2017 at 07:56:22PM +0200, Thomas Gleixner wrote:
> > > >  	/* Skip reprogram of event if its not changed */
> > > > -	if (ts->tick_stopped && (expires == ts->next_tick))
> > > > +	if (ts->tick_stopped && (expires == ts->next_tick)) {
> > > > +		WARN_ON_ONCE(dev->next_event > ts->next_tick);
> > > 
> > > What about handling it proper ? dev->next_event might be KTIME_MAX,
> > > i.e. no more event for the next 500+ years.
> > 
> > I thought I handled this case, what I'm I missing?
> 
> 	if (ts->tick_stopped && (expires == ts->next_tick)) {
> 		WARN_ON_ONCE(dev->next_event > ts->next_tick);
> 		goto out;
> 	}
> 
> IOW, the WARN_ON yells in dmesg, but despite seing the wreckage it just
> leaves it and goes out doing nothing.
> 
> Why can't you just do
> 
> 	if (ts->tick_stopped && (expires == ts->next_tick)) {
> 	   	if (dev->next_event > ts->next_tick)) {
> 			WARN_ONCE();
> 			do_something_sensible();
> 		}		
> 		goto out;
> 	}
> 
> Hmm?

Ah ok, right!

So something like this:

    if (ts->tick_stopped && (expires == ts->next_tick)) {
        if (likely(dev->next_event <= ts->next_tick))
            goto out;
        WARN_ON_ONCE(1);
    }

So that we fall down to clock reprogramming if the sanity check fails.

I'm resending the patches.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web