Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Fri, Apr 06, 2018 at 02:56:36PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> Subject: [PATCH] nohz: Avoid duplication of code related to got_idle_tick
> 
> Move the code setting ts->got_idle_tick into tick_sched_do_timer() to
> avoid code duplication.
> 
> No intentional changes in functionality.
> 
> Suggested-by: Frederic Weisbecker 
> Signed-off-by: Rafael J. Wysocki 

Reviewed-by: Frederic Weisbecker 

Thanks!

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Fri, Apr 06, 2018 at 02:56:36PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> Subject: [PATCH] nohz: Avoid duplication of code related to got_idle_tick
> 
> Move the code setting ts->got_idle_tick into tick_sched_do_timer() to
> avoid code duplication.
> 
> No intentional changes in functionality.
> 
> Suggested-by: Frederic Weisbecker 
> Signed-off-by: Rafael J. Wysocki 

Reviewed-by: Frederic Weisbecker 

Thanks!

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Fri, Apr 06, 2018 at 10:11:04AM +0200, Rafael J. Wysocki wrote:
> On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> > On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki 
> > > Index: linux-pm/kernel/time/tick-sched.c
> > > ===
> > > --- linux-pm.orig/kernel/time/tick-sched.c
> > > +++ linux-pm/kernel/time/tick-sched.c
> > > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> > >  }
> > >  
> > >  /**
> > > + * tick_nohz_idle_got_tick - Check whether or not the tick handler has 
> > > run
> > > + */
> > > +bool tick_nohz_idle_got_tick(void)
> > > +{
> > > + struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> > > +
> > > + if (ts->inidle > 1) {
> > > + ts->inidle = 1;
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> > > +
> > > +/**
> > >   * tick_nohz_get_sleep_length - return the length of the current sleep
> > >   *
> > >   * Called from power state control code with interrupts disabled
> > > @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
> > >   struct pt_regs *regs = get_irq_regs();
> > >   ktime_t now = ktime_get();
> > >  
> > > + if (ts->inidle)
> > > + ts->inidle = 2;
> > > +
> > 
> > You can move that to tick_sched_do_timer() to avoid code duplication.
> > 
> > Also these constants are very opaque. And even with proper symbols it 
> > wouldn't look
> > right to extend ts->inidle that way.
> > 
> > Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> > fields
> > after the below patch:
> > 
> > --
> > From c7b2ca5a4c512517ddfeb9f922d5999f82542ced Mon Sep 17 00:00:00 2001
> > From: Frederic Weisbecker 
> > Date: Fri, 6 Apr 2018 04:32:37 +0200
> > Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> > 
> > This optimize the space and leave plenty of room for further flags.
> > 
> > Signed-off-by: Frederic Weisbecker 
> > ---
> >  kernel/time/tick-sched.h | 10 ++
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> > index 954b43d..38f24dc 100644
> > --- a/kernel/time/tick-sched.h
> > +++ b/kernel/time/tick-sched.h
> > @@ -45,14 +45,17 @@ struct tick_sched {
> > struct hrtimer  sched_timer;
> > unsigned long   check_clocks;
> > enum tick_nohz_mode nohz_mode;
> > +
> > +   unsigned intinidle  : 1;
> > +   unsigned inttick_stopped: 1;
> 
> This particular change breaks build, because tick_stopped is
> accessed via __this_cpu_read() in tick_nohz_tick_stopped().

Oops...

> 
> > +   unsigned intidle_active : 1;
> > +   unsigned intdo_timer_last   : 1;
> > +
> > ktime_t last_tick;
> > ktime_t next_tick;
> > -   int inidle;
> > -   int tick_stopped;
> > unsigned long   idle_jiffies;
> > unsigned long   idle_calls;
> > unsigned long   idle_sleeps;
> > -   int idle_active;
> > ktime_t idle_entrytime;
> > ktime_t idle_waketime;
> > ktime_t idle_exittime;
> > @@ -62,7 +65,6 @@ struct tick_sched {
> > unsigned long   last_jiffies;
> > u64 next_timer;
> > ktime_t idle_expires;
> > -   int do_timer_last;
> > atomic_ttick_dep_mask;
> >  };
> >  
> > 
> 
> So what about this?  And moving the duplicated piece of got_idle_tick
> manipulation on top of it?
> 
> ---
> From: Frederic Weisbecker 
> Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> 
> Optimize the space and leave plenty of room for further flags.
> 
> Signed-off-by: Frederic Weisbecker 
> [ rjw: Do not use __this_cpu_read() to access tick_stopped and add
>got_idle_tick to avoid overloading inidle ]
> Signed-off-by: Rafael J. Wysocki 

Yeah looks good, thanks!

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Fri, Apr 06, 2018 at 10:11:04AM +0200, Rafael J. Wysocki wrote:
> On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> > On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki 
> > > Index: linux-pm/kernel/time/tick-sched.c
> > > ===
> > > --- linux-pm.orig/kernel/time/tick-sched.c
> > > +++ linux-pm/kernel/time/tick-sched.c
> > > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> > >  }
> > >  
> > >  /**
> > > + * tick_nohz_idle_got_tick - Check whether or not the tick handler has 
> > > run
> > > + */
> > > +bool tick_nohz_idle_got_tick(void)
> > > +{
> > > + struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> > > +
> > > + if (ts->inidle > 1) {
> > > + ts->inidle = 1;
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> > > +
> > > +/**
> > >   * tick_nohz_get_sleep_length - return the length of the current sleep
> > >   *
> > >   * Called from power state control code with interrupts disabled
> > > @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
> > >   struct pt_regs *regs = get_irq_regs();
> > >   ktime_t now = ktime_get();
> > >  
> > > + if (ts->inidle)
> > > + ts->inidle = 2;
> > > +
> > 
> > You can move that to tick_sched_do_timer() to avoid code duplication.
> > 
> > Also these constants are very opaque. And even with proper symbols it 
> > wouldn't look
> > right to extend ts->inidle that way.
> > 
> > Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> > fields
> > after the below patch:
> > 
> > --
> > From c7b2ca5a4c512517ddfeb9f922d5999f82542ced Mon Sep 17 00:00:00 2001
> > From: Frederic Weisbecker 
> > Date: Fri, 6 Apr 2018 04:32:37 +0200
> > Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> > 
> > This optimize the space and leave plenty of room for further flags.
> > 
> > Signed-off-by: Frederic Weisbecker 
> > ---
> >  kernel/time/tick-sched.h | 10 ++
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> > index 954b43d..38f24dc 100644
> > --- a/kernel/time/tick-sched.h
> > +++ b/kernel/time/tick-sched.h
> > @@ -45,14 +45,17 @@ struct tick_sched {
> > struct hrtimer  sched_timer;
> > unsigned long   check_clocks;
> > enum tick_nohz_mode nohz_mode;
> > +
> > +   unsigned intinidle  : 1;
> > +   unsigned inttick_stopped: 1;
> 
> This particular change breaks build, because tick_stopped is
> accessed via __this_cpu_read() in tick_nohz_tick_stopped().

Oops...

> 
> > +   unsigned intidle_active : 1;
> > +   unsigned intdo_timer_last   : 1;
> > +
> > ktime_t last_tick;
> > ktime_t next_tick;
> > -   int inidle;
> > -   int tick_stopped;
> > unsigned long   idle_jiffies;
> > unsigned long   idle_calls;
> > unsigned long   idle_sleeps;
> > -   int idle_active;
> > ktime_t idle_entrytime;
> > ktime_t idle_waketime;
> > ktime_t idle_exittime;
> > @@ -62,7 +65,6 @@ struct tick_sched {
> > unsigned long   last_jiffies;
> > u64 next_timer;
> > ktime_t idle_expires;
> > -   int do_timer_last;
> > atomic_ttick_dep_mask;
> >  };
> >  
> > 
> 
> So what about this?  And moving the duplicated piece of got_idle_tick
> manipulation on top of it?
> 
> ---
> From: Frederic Weisbecker 
> Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> 
> Optimize the space and leave plenty of room for further flags.
> 
> Signed-off-by: Frederic Weisbecker 
> [ rjw: Do not use __this_cpu_read() to access tick_stopped and add
>got_idle_tick to avoid overloading inidle ]
> Signed-off-by: Rafael J. Wysocki 

Yeah looks good, thanks!

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Fri, Apr 06, 2018 at 09:58:37AM +0200, Peter Zijlstra wrote:
> On Fri, Apr 06, 2018 at 04:44:14AM +0200, Frederic Weisbecker wrote:
> > You can move that to tick_sched_do_timer() to avoid code duplication.
> 
> I expect the reason I didn't was that it didn't have @ts, but that's
> easily fixable.
> 
> > Also these constants are very opaque. And even with proper symbols it 
> > wouldn't look
> > right to extend ts->inidle that way.
> > 
> > Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> > fields
> > after the below patch:
> 
> > @@ -45,14 +45,17 @@ struct tick_sched {
> > struct hrtimer  sched_timer;
> > unsigned long   check_clocks;
> > enum tick_nohz_mode nohz_mode;
> > +
> > +   unsigned intinidle  : 1;
> > +   unsigned inttick_stopped: 1;
> > +   unsigned intidle_active : 1;
> > +   unsigned intdo_timer_last   : 1;
> 
> That would generate worse code, but yes, the C might be prettier.

Well, not too bad I think. MOV become OR/AND, the rest is just
TESTs...

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Fri, Apr 06, 2018 at 09:58:37AM +0200, Peter Zijlstra wrote:
> On Fri, Apr 06, 2018 at 04:44:14AM +0200, Frederic Weisbecker wrote:
> > You can move that to tick_sched_do_timer() to avoid code duplication.
> 
> I expect the reason I didn't was that it didn't have @ts, but that's
> easily fixable.
> 
> > Also these constants are very opaque. And even with proper symbols it 
> > wouldn't look
> > right to extend ts->inidle that way.
> > 
> > Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> > fields
> > after the below patch:
> 
> > @@ -45,14 +45,17 @@ struct tick_sched {
> > struct hrtimer  sched_timer;
> > unsigned long   check_clocks;
> > enum tick_nohz_mode nohz_mode;
> > +
> > +   unsigned intinidle  : 1;
> > +   unsigned inttick_stopped: 1;
> > +   unsigned intidle_active : 1;
> > +   unsigned intdo_timer_last   : 1;
> 
> That would generate worse code, but yes, the C might be prettier.

Well, not too bad I think. MOV become OR/AND, the rest is just
TESTs...

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Fri, Apr 06, 2018 at 09:24:42AM +0200, Rafael J. Wysocki wrote:
> On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> > On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki 
> > > Index: linux-pm/kernel/time/tick-sched.c
> > > ===
> > > --- linux-pm.orig/kernel/time/tick-sched.c
> > > +++ linux-pm/kernel/time/tick-sched.c
> > > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> > >  }
> > >  
> > >  /**
> > > + * tick_nohz_idle_got_tick - Check whether or not the tick handler has 
> > > run
> > > + */
> > > +bool tick_nohz_idle_got_tick(void)
> > > +{
> > > + struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> > > +
> > > + if (ts->inidle > 1) {
> > > + ts->inidle = 1;
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> > > +
> > > +/**
> > >   * tick_nohz_get_sleep_length - return the length of the current sleep
> > >   *
> > >   * Called from power state control code with interrupts disabled
> > > @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
> > >   struct pt_regs *regs = get_irq_regs();
> > >   ktime_t now = ktime_get();
> > >  
> > > + if (ts->inidle)
> > > + ts->inidle = 2;
> > > +
> > 
> > You can move that to tick_sched_do_timer() to avoid code duplication.
> 
> Right.
> 
> > Also these constants are very opaque. And even with proper symbols it 
> > wouldn't look
> > right to extend ts->inidle that way.
> 
> Well, this was a Peter's idea. :-)
> 
> > Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> > fields
> > after the below patch:
> 
> OK, but at this point I'd prefer to make such changes on top of the existing
> set, because that's got quite some testing coverage already and honestly this
> is cosmetics in my view (albeit important).

Sure!

Thanks.

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Fri, Apr 06, 2018 at 09:24:42AM +0200, Rafael J. Wysocki wrote:
> On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> > On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki 
> > > Index: linux-pm/kernel/time/tick-sched.c
> > > ===
> > > --- linux-pm.orig/kernel/time/tick-sched.c
> > > +++ linux-pm/kernel/time/tick-sched.c
> > > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> > >  }
> > >  
> > >  /**
> > > + * tick_nohz_idle_got_tick - Check whether or not the tick handler has 
> > > run
> > > + */
> > > +bool tick_nohz_idle_got_tick(void)
> > > +{
> > > + struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> > > +
> > > + if (ts->inidle > 1) {
> > > + ts->inidle = 1;
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> > > +
> > > +/**
> > >   * tick_nohz_get_sleep_length - return the length of the current sleep
> > >   *
> > >   * Called from power state control code with interrupts disabled
> > > @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
> > >   struct pt_regs *regs = get_irq_regs();
> > >   ktime_t now = ktime_get();
> > >  
> > > + if (ts->inidle)
> > > + ts->inidle = 2;
> > > +
> > 
> > You can move that to tick_sched_do_timer() to avoid code duplication.
> 
> Right.
> 
> > Also these constants are very opaque. And even with proper symbols it 
> > wouldn't look
> > right to extend ts->inidle that way.
> 
> Well, this was a Peter's idea. :-)
> 
> > Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> > fields
> > after the below patch:
> 
> OK, but at this point I'd prefer to make such changes on top of the existing
> set, because that's got quite some testing coverage already and honestly this
> is cosmetics in my view (albeit important).

Sure!

Thanks.

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Rafael J. Wysocki]

On Friday, April 6, 2018 10:11:04 AM CEST Rafael J. Wysocki wrote:
> On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> > On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki 
> > > Index: linux-pm/kernel/time/tick-sched.c
> > > ===
> > > --- linux-pm.orig/kernel/time/tick-sched.c
> > > +++ linux-pm/kernel/time/tick-sched.c
> > > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> > >  }

[cut]

> So what about this?  And moving the duplicated piece of got_idle_tick
> manipulation on top of it?

And appended is a patch to get rid of the code duplication (on top of the one
below), for completeness.

> ---
> From: Frederic Weisbecker 
> Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> 
> Optimize the space and leave plenty of room for further flags.
> 
> Signed-off-by: Frederic Weisbecker 
> [ rjw: Do not use __this_cpu_read() to access tick_stopped and add
>got_idle_tick to avoid overloading inidle ]
> Signed-off-by: Rafael J. Wysocki 
> ---
>  kernel/time/tick-sched.c |   12 +++-
>  kernel/time/tick-sched.h |   12 
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/kernel/time/tick-sched.h
> ===
> --- linux-pm.orig/kernel/time/tick-sched.h
> +++ linux-pm/kernel/time/tick-sched.h
> @@ -41,19 +41,24 @@ enum tick_nohz_mode {
>   * @timer_expires:   Anticipated timer expiration time (in case sched tick 
> is stopped)
>   * @timer_expires_base:  Base time clock monotonic for @timer_expires
>   * @do_timer_lst:CPU was the last one doing do_timer before going idle
> + * @got_idle_tick:   Tick timer function has run with @inidle set
>   */
>  struct tick_sched {
>   struct hrtimer  sched_timer;
>   unsigned long   check_clocks;
>   enum tick_nohz_mode nohz_mode;
> +
> + unsigned intinidle  : 1;
> + unsigned inttick_stopped: 1;
> + unsigned intidle_active : 1;
> + unsigned intdo_timer_last   : 1;
> + unsigned intgot_idle_tick   : 1;
> +
>   ktime_t last_tick;
>   ktime_t next_tick;
> - int inidle;
> - int tick_stopped;
>   unsigned long   idle_jiffies;
>   unsigned long   idle_calls;
>   unsigned long   idle_sleeps;
> - int idle_active;
>   ktime_t idle_entrytime;
>   ktime_t idle_waketime;
>   ktime_t idle_exittime;
> @@ -64,7 +69,6 @@ struct tick_sched {
>   u64 timer_expires_base;
>   u64 next_timer;
>   ktime_t idle_expires;
> - int do_timer_last;
>   atomic_ttick_dep_mask;
>  };
>  
> Index: linux-pm/kernel/time/tick-sched.c
> ===
> --- linux-pm.orig/kernel/time/tick-sched.c
> +++ linux-pm/kernel/time/tick-sched.c
> @@ -465,7 +465,9 @@ __setup("nohz=", setup_tick_nohz);
>  
>  bool tick_nohz_tick_stopped(void)
>  {
> - return __this_cpu_read(tick_cpu_sched.tick_stopped);
> + struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> +
> + return ts->tick_stopped;
>  }
>  
>  bool tick_nohz_tick_stopped_cpu(int cpu)
> @@ -1014,8 +1016,8 @@ bool tick_nohz_idle_got_tick(void)
>  {
>   struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
>  
> - if (ts->inidle > 1) {
> - ts->inidle = 1;
> + if (ts->got_idle_tick) {
> + ts->got_idle_tick = 0;
>   return true;
>   }
>   return false;
> @@ -1161,7 +1163,7 @@ static void tick_nohz_handler(struct clo
>   ktime_t now = ktime_get();
>  
>   if (ts->inidle)
> - ts->inidle = 2;
> + ts->got_idle_tick = 1;
>  
>   dev->next_event = KTIME_MAX;
>  
> @@ -1261,7 +1263,7 @@ static enum hrtimer_restart tick_sched_t
>   ktime_t now = ktime_get();
>  
>   if (ts->inidle)
> - ts->inidle = 2;
> + ts->got_idle_tick = 1;
>  
>   tick_sched_do_timer(now);
>  

---
From: Rafael J. Wysocki 
Subject: [PATCH] nohz: Avoid duplication of code related to got_idle_tick

Move the code setting ts->got_idle_tick into tick_sched_do_timer() to
avoid code duplication.

No intentional changes in functionality.

Suggested-by: Frederic Weisbecker 
Signed-off-by: 

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Rafael J. Wysocki]

On Friday, April 6, 2018 10:11:04 AM CEST Rafael J. Wysocki wrote:
> On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> > On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki 
> > > Index: linux-pm/kernel/time/tick-sched.c
> > > ===
> > > --- linux-pm.orig/kernel/time/tick-sched.c
> > > +++ linux-pm/kernel/time/tick-sched.c
> > > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> > >  }

[cut]

> So what about this?  And moving the duplicated piece of got_idle_tick
> manipulation on top of it?

And appended is a patch to get rid of the code duplication (on top of the one
below), for completeness.

> ---
> From: Frederic Weisbecker 
> Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> 
> Optimize the space and leave plenty of room for further flags.
> 
> Signed-off-by: Frederic Weisbecker 
> [ rjw: Do not use __this_cpu_read() to access tick_stopped and add
>got_idle_tick to avoid overloading inidle ]
> Signed-off-by: Rafael J. Wysocki 
> ---
>  kernel/time/tick-sched.c |   12 +++-
>  kernel/time/tick-sched.h |   12 
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/kernel/time/tick-sched.h
> ===
> --- linux-pm.orig/kernel/time/tick-sched.h
> +++ linux-pm/kernel/time/tick-sched.h
> @@ -41,19 +41,24 @@ enum tick_nohz_mode {
>   * @timer_expires:   Anticipated timer expiration time (in case sched tick 
> is stopped)
>   * @timer_expires_base:  Base time clock monotonic for @timer_expires
>   * @do_timer_lst:CPU was the last one doing do_timer before going idle
> + * @got_idle_tick:   Tick timer function has run with @inidle set
>   */
>  struct tick_sched {
>   struct hrtimer  sched_timer;
>   unsigned long   check_clocks;
>   enum tick_nohz_mode nohz_mode;
> +
> + unsigned intinidle  : 1;
> + unsigned inttick_stopped: 1;
> + unsigned intidle_active : 1;
> + unsigned intdo_timer_last   : 1;
> + unsigned intgot_idle_tick   : 1;
> +
>   ktime_t last_tick;
>   ktime_t next_tick;
> - int inidle;
> - int tick_stopped;
>   unsigned long   idle_jiffies;
>   unsigned long   idle_calls;
>   unsigned long   idle_sleeps;
> - int idle_active;
>   ktime_t idle_entrytime;
>   ktime_t idle_waketime;
>   ktime_t idle_exittime;
> @@ -64,7 +69,6 @@ struct tick_sched {
>   u64 timer_expires_base;
>   u64 next_timer;
>   ktime_t idle_expires;
> - int do_timer_last;
>   atomic_ttick_dep_mask;
>  };
>  
> Index: linux-pm/kernel/time/tick-sched.c
> ===
> --- linux-pm.orig/kernel/time/tick-sched.c
> +++ linux-pm/kernel/time/tick-sched.c
> @@ -465,7 +465,9 @@ __setup("nohz=", setup_tick_nohz);
>  
>  bool tick_nohz_tick_stopped(void)
>  {
> - return __this_cpu_read(tick_cpu_sched.tick_stopped);
> + struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> +
> + return ts->tick_stopped;
>  }
>  
>  bool tick_nohz_tick_stopped_cpu(int cpu)
> @@ -1014,8 +1016,8 @@ bool tick_nohz_idle_got_tick(void)
>  {
>   struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
>  
> - if (ts->inidle > 1) {
> - ts->inidle = 1;
> + if (ts->got_idle_tick) {
> + ts->got_idle_tick = 0;
>   return true;
>   }
>   return false;
> @@ -1161,7 +1163,7 @@ static void tick_nohz_handler(struct clo
>   ktime_t now = ktime_get();
>  
>   if (ts->inidle)
> - ts->inidle = 2;
> + ts->got_idle_tick = 1;
>  
>   dev->next_event = KTIME_MAX;
>  
> @@ -1261,7 +1263,7 @@ static enum hrtimer_restart tick_sched_t
>   ktime_t now = ktime_get();
>  
>   if (ts->inidle)
> - ts->inidle = 2;
> + ts->got_idle_tick = 1;
>  
>   tick_sched_do_timer(now);
>  

---
From: Rafael J. Wysocki 
Subject: [PATCH] nohz: Avoid duplication of code related to got_idle_tick

Move the code setting ts->got_idle_tick into tick_sched_do_timer() to
avoid code duplication.

No intentional changes in functionality.

Suggested-by: Frederic Weisbecker 
Signed-off-by: Rafael J. Wysocki 
---
 kernel/time/tick-sched.c |   16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

Index: 

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Rafael J. Wysocki]

On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> > Index: linux-pm/kernel/time/tick-sched.c
> > ===
> > --- linux-pm.orig/kernel/time/tick-sched.c
> > +++ linux-pm/kernel/time/tick-sched.c
> > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> >  }
> >  
> >  /**
> > + * tick_nohz_idle_got_tick - Check whether or not the tick handler has run
> > + */
> > +bool tick_nohz_idle_got_tick(void)
> > +{
> > +   struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> > +
> > +   if (ts->inidle > 1) {
> > +   ts->inidle = 1;
> > +   return true;
> > +   }
> > +   return false;
> > +}
> > +
> > +/**
> >   * tick_nohz_get_sleep_length - return the length of the current sleep
> >   *
> >   * Called from power state control code with interrupts disabled
> > @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
> > struct pt_regs *regs = get_irq_regs();
> > ktime_t now = ktime_get();
> >  
> > +   if (ts->inidle)
> > +   ts->inidle = 2;
> > +
> 
> You can move that to tick_sched_do_timer() to avoid code duplication.
> 
> Also these constants are very opaque. And even with proper symbols it 
> wouldn't look
> right to extend ts->inidle that way.
> 
> Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> fields
> after the below patch:
> 
> --
> From c7b2ca5a4c512517ddfeb9f922d5999f82542ced Mon Sep 17 00:00:00 2001
> From: Frederic Weisbecker 
> Date: Fri, 6 Apr 2018 04:32:37 +0200
> Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> 
> This optimize the space and leave plenty of room for further flags.
> 
> Signed-off-by: Frederic Weisbecker 
> ---
>  kernel/time/tick-sched.h | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> index 954b43d..38f24dc 100644
> --- a/kernel/time/tick-sched.h
> +++ b/kernel/time/tick-sched.h
> @@ -45,14 +45,17 @@ struct tick_sched {
>   struct hrtimer  sched_timer;
>   unsigned long   check_clocks;
>   enum tick_nohz_mode nohz_mode;
> +
> + unsigned intinidle  : 1;
> + unsigned inttick_stopped: 1;

This particular change breaks build, because tick_stopped is
accessed via __this_cpu_read() in tick_nohz_tick_stopped().

> + unsigned intidle_active : 1;
> + unsigned intdo_timer_last   : 1;
> +
>   ktime_t last_tick;
>   ktime_t next_tick;
> - int inidle;
> - int tick_stopped;
>   unsigned long   idle_jiffies;
>   unsigned long   idle_calls;
>   unsigned long   idle_sleeps;
> - int idle_active;
>   ktime_t idle_entrytime;
>   ktime_t idle_waketime;
>   ktime_t idle_exittime;
> @@ -62,7 +65,6 @@ struct tick_sched {
>   unsigned long   last_jiffies;
>   u64 next_timer;
>   ktime_t idle_expires;
> - int do_timer_last;
>   atomic_ttick_dep_mask;
>  };
>  
> 

So what about this?  And moving the duplicated piece of got_idle_tick
manipulation on top of it?

---
From: Frederic Weisbecker 
Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field

Optimize the space and leave plenty of room for further flags.

Signed-off-by: Frederic Weisbecker 
[ rjw: Do not use __this_cpu_read() to access tick_stopped and add
   got_idle_tick to avoid overloading inidle ]
Signed-off-by: Rafael J. Wysocki 
---
 kernel/time/tick-sched.c |   12 +++-
 kernel/time/tick-sched.h |   12 
 2 files changed, 15 insertions(+), 9 deletions(-)

Index: linux-pm/kernel/time/tick-sched.h
===
--- linux-pm.orig/kernel/time/tick-sched.h
+++ linux-pm/kernel/time/tick-sched.h
@@ -41,19 +41,24 @@ enum tick_nohz_mode {
  * @timer_expires: Anticipated timer expiration time (in case sched tick 
is stopped)
  * @timer_expires_base:Base time clock monotonic for @timer_expires
  * @do_timer_lst:  CPU was the last one doing do_timer before going idle
+ * @got_idle_tick: Tick timer function has run with @inidle set
  */
 struct tick_sched {
struct hrtimer  sched_timer;
unsigned long   

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Rafael J. Wysocki]

On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> > Index: linux-pm/kernel/time/tick-sched.c
> > ===
> > --- linux-pm.orig/kernel/time/tick-sched.c
> > +++ linux-pm/kernel/time/tick-sched.c
> > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> >  }
> >  
> >  /**
> > + * tick_nohz_idle_got_tick - Check whether or not the tick handler has run
> > + */
> > +bool tick_nohz_idle_got_tick(void)
> > +{
> > +   struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> > +
> > +   if (ts->inidle > 1) {
> > +   ts->inidle = 1;
> > +   return true;
> > +   }
> > +   return false;
> > +}
> > +
> > +/**
> >   * tick_nohz_get_sleep_length - return the length of the current sleep
> >   *
> >   * Called from power state control code with interrupts disabled
> > @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
> > struct pt_regs *regs = get_irq_regs();
> > ktime_t now = ktime_get();
> >  
> > +   if (ts->inidle)
> > +   ts->inidle = 2;
> > +
> 
> You can move that to tick_sched_do_timer() to avoid code duplication.
> 
> Also these constants are very opaque. And even with proper symbols it 
> wouldn't look
> right to extend ts->inidle that way.
> 
> Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> fields
> after the below patch:
> 
> --
> From c7b2ca5a4c512517ddfeb9f922d5999f82542ced Mon Sep 17 00:00:00 2001
> From: Frederic Weisbecker 
> Date: Fri, 6 Apr 2018 04:32:37 +0200
> Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> 
> This optimize the space and leave plenty of room for further flags.
> 
> Signed-off-by: Frederic Weisbecker 
> ---
>  kernel/time/tick-sched.h | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> index 954b43d..38f24dc 100644
> --- a/kernel/time/tick-sched.h
> +++ b/kernel/time/tick-sched.h
> @@ -45,14 +45,17 @@ struct tick_sched {
>   struct hrtimer  sched_timer;
>   unsigned long   check_clocks;
>   enum tick_nohz_mode nohz_mode;
> +
> + unsigned intinidle  : 1;
> + unsigned inttick_stopped: 1;

This particular change breaks build, because tick_stopped is
accessed via __this_cpu_read() in tick_nohz_tick_stopped().

> + unsigned intidle_active : 1;
> + unsigned intdo_timer_last   : 1;
> +
>   ktime_t last_tick;
>   ktime_t next_tick;
> - int inidle;
> - int tick_stopped;
>   unsigned long   idle_jiffies;
>   unsigned long   idle_calls;
>   unsigned long   idle_sleeps;
> - int idle_active;
>   ktime_t idle_entrytime;
>   ktime_t idle_waketime;
>   ktime_t idle_exittime;
> @@ -62,7 +65,6 @@ struct tick_sched {
>   unsigned long   last_jiffies;
>   u64 next_timer;
>   ktime_t idle_expires;
> - int do_timer_last;
>   atomic_ttick_dep_mask;
>  };
>  
> 

So what about this?  And moving the duplicated piece of got_idle_tick
manipulation on top of it?

---
From: Frederic Weisbecker 
Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field

Optimize the space and leave plenty of room for further flags.

Signed-off-by: Frederic Weisbecker 
[ rjw: Do not use __this_cpu_read() to access tick_stopped and add
   got_idle_tick to avoid overloading inidle ]
Signed-off-by: Rafael J. Wysocki 
---
 kernel/time/tick-sched.c |   12 +++-
 kernel/time/tick-sched.h |   12 
 2 files changed, 15 insertions(+), 9 deletions(-)

Index: linux-pm/kernel/time/tick-sched.h
===
--- linux-pm.orig/kernel/time/tick-sched.h
+++ linux-pm/kernel/time/tick-sched.h
@@ -41,19 +41,24 @@ enum tick_nohz_mode {
  * @timer_expires: Anticipated timer expiration time (in case sched tick 
is stopped)
  * @timer_expires_base:Base time clock monotonic for @timer_expires
  * @do_timer_lst:  CPU was the last one doing do_timer before going idle
+ * @got_idle_tick: Tick timer function has run with @inidle set
  */
 struct tick_sched {
struct hrtimer  sched_timer;
unsigned long   check_clocks;
enum tick_nohz_mode nohz_mode;
+
+   unsigned intinidle  : 1;
+   unsigned int  

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Peter Zijlstra]

On Fri, Apr 06, 2018 at 04:44:14AM +0200, Frederic Weisbecker wrote:
> You can move that to tick_sched_do_timer() to avoid code duplication.

I expect the reason I didn't was that it didn't have @ts, but that's
easily fixable.

> Also these constants are very opaque. And even with proper symbols it 
> wouldn't look
> right to extend ts->inidle that way.
> 
> Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> fields
> after the below patch:

> @@ -45,14 +45,17 @@ struct tick_sched {
>   struct hrtimer  sched_timer;
>   unsigned long   check_clocks;
>   enum tick_nohz_mode nohz_mode;
> +
> + unsigned intinidle  : 1;
> + unsigned inttick_stopped: 1;
> + unsigned intidle_active : 1;
> + unsigned intdo_timer_last   : 1;

That would generate worse code, but yes, the C might be prettier.

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Peter Zijlstra]

On Fri, Apr 06, 2018 at 04:44:14AM +0200, Frederic Weisbecker wrote:
> You can move that to tick_sched_do_timer() to avoid code duplication.

I expect the reason I didn't was that it didn't have @ts, but that's
easily fixable.

> Also these constants are very opaque. And even with proper symbols it 
> wouldn't look
> right to extend ts->inidle that way.
> 
> Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> fields
> after the below patch:

> @@ -45,14 +45,17 @@ struct tick_sched {
>   struct hrtimer  sched_timer;
>   unsigned long   check_clocks;
>   enum tick_nohz_mode nohz_mode;
> +
> + unsigned intinidle  : 1;
> + unsigned inttick_stopped: 1;
> + unsigned intidle_active : 1;
> + unsigned intdo_timer_last   : 1;

That would generate worse code, but yes, the C might be prettier.

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Rafael J. Wysocki]

On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> > Index: linux-pm/kernel/time/tick-sched.c
> > ===
> > --- linux-pm.orig/kernel/time/tick-sched.c
> > +++ linux-pm/kernel/time/tick-sched.c
> > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> >  }
> >  
> >  /**
> > + * tick_nohz_idle_got_tick - Check whether or not the tick handler has run
> > + */
> > +bool tick_nohz_idle_got_tick(void)
> > +{
> > +   struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> > +
> > +   if (ts->inidle > 1) {
> > +   ts->inidle = 1;
> > +   return true;
> > +   }
> > +   return false;
> > +}
> > +
> > +/**
> >   * tick_nohz_get_sleep_length - return the length of the current sleep
> >   *
> >   * Called from power state control code with interrupts disabled
> > @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
> > struct pt_regs *regs = get_irq_regs();
> > ktime_t now = ktime_get();
> >  
> > +   if (ts->inidle)
> > +   ts->inidle = 2;
> > +
> 
> You can move that to tick_sched_do_timer() to avoid code duplication.

Right.

> Also these constants are very opaque. And even with proper symbols it 
> wouldn't look
> right to extend ts->inidle that way.

Well, this was a Peter's idea. :-)

> Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> fields
> after the below patch:

OK, but at this point I'd prefer to make such changes on top of the existing
set, because that's got quite some testing coverage already and honestly this
is cosmetics in my view (albeit important).

> --
> From c7b2ca5a4c512517ddfeb9f922d5999f82542ced Mon Sep 17 00:00:00 2001
> From: Frederic Weisbecker 
> Date: Fri, 6 Apr 2018 04:32:37 +0200
> Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> 
> This optimize the space and leave plenty of room for further flags.
> 
> Signed-off-by: Frederic Weisbecker 
> ---
>  kernel/time/tick-sched.h | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> index 954b43d..38f24dc 100644
> --- a/kernel/time/tick-sched.h
> +++ b/kernel/time/tick-sched.h
> @@ -45,14 +45,17 @@ struct tick_sched {
>   struct hrtimer  sched_timer;
>   unsigned long   check_clocks;
>   enum tick_nohz_mode nohz_mode;
> +
> + unsigned intinidle  : 1;
> + unsigned inttick_stopped: 1;
> + unsigned intidle_active : 1;
> + unsigned intdo_timer_last   : 1;
> +
>   ktime_t last_tick;
>   ktime_t next_tick;
> - int inidle;
> - int tick_stopped;
>   unsigned long   idle_jiffies;
>   unsigned long   idle_calls;
>   unsigned long   idle_sleeps;
> - int idle_active;
>   ktime_t idle_entrytime;
>   ktime_t idle_waketime;
>   ktime_t idle_exittime;
> @@ -62,7 +65,6 @@ struct tick_sched {
>   unsigned long   last_jiffies;
>   u64 next_timer;
>   ktime_t idle_expires;
> - int do_timer_last;
>   atomic_ttick_dep_mask;
>  };
>  
> 

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Rafael J. Wysocki]

On Friday, April 6, 2018 4:44:14 AM CEST Frederic Weisbecker wrote:
> On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> > Index: linux-pm/kernel/time/tick-sched.c
> > ===
> > --- linux-pm.orig/kernel/time/tick-sched.c
> > +++ linux-pm/kernel/time/tick-sched.c
> > @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
> >  }
> >  
> >  /**
> > + * tick_nohz_idle_got_tick - Check whether or not the tick handler has run
> > + */
> > +bool tick_nohz_idle_got_tick(void)
> > +{
> > +   struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> > +
> > +   if (ts->inidle > 1) {
> > +   ts->inidle = 1;
> > +   return true;
> > +   }
> > +   return false;
> > +}
> > +
> > +/**
> >   * tick_nohz_get_sleep_length - return the length of the current sleep
> >   *
> >   * Called from power state control code with interrupts disabled
> > @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
> > struct pt_regs *regs = get_irq_regs();
> > ktime_t now = ktime_get();
> >  
> > +   if (ts->inidle)
> > +   ts->inidle = 2;
> > +
> 
> You can move that to tick_sched_do_timer() to avoid code duplication.

Right.

> Also these constants are very opaque. And even with proper symbols it 
> wouldn't look
> right to extend ts->inidle that way.

Well, this was a Peter's idea. :-)

> Perhaps you should add a field such as ts->got_idle_tick under the boolean 
> fields
> after the below patch:

OK, but at this point I'd prefer to make such changes on top of the existing
set, because that's got quite some testing coverage already and honestly this
is cosmetics in my view (albeit important).

> --
> From c7b2ca5a4c512517ddfeb9f922d5999f82542ced Mon Sep 17 00:00:00 2001
> From: Frederic Weisbecker 
> Date: Fri, 6 Apr 2018 04:32:37 +0200
> Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field
> 
> This optimize the space and leave plenty of room for further flags.
> 
> Signed-off-by: Frederic Weisbecker 
> ---
>  kernel/time/tick-sched.h | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
> index 954b43d..38f24dc 100644
> --- a/kernel/time/tick-sched.h
> +++ b/kernel/time/tick-sched.h
> @@ -45,14 +45,17 @@ struct tick_sched {
>   struct hrtimer  sched_timer;
>   unsigned long   check_clocks;
>   enum tick_nohz_mode nohz_mode;
> +
> + unsigned intinidle  : 1;
> + unsigned inttick_stopped: 1;
> + unsigned intidle_active : 1;
> + unsigned intdo_timer_last   : 1;
> +
>   ktime_t last_tick;
>   ktime_t next_tick;
> - int inidle;
> - int tick_stopped;
>   unsigned long   idle_jiffies;
>   unsigned long   idle_calls;
>   unsigned long   idle_sleeps;
> - int idle_active;
>   ktime_t idle_entrytime;
>   ktime_t idle_waketime;
>   ktime_t idle_exittime;
> @@ -62,7 +65,6 @@ struct tick_sched {
>   unsigned long   last_jiffies;
>   u64 next_timer;
>   ktime_t idle_expires;
> - int do_timer_last;
>   atomic_ttick_dep_mask;
>  };
>  
> 

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> Index: linux-pm/kernel/time/tick-sched.c
> ===
> --- linux-pm.orig/kernel/time/tick-sched.c
> +++ linux-pm/kernel/time/tick-sched.c
> @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
>  }
>  
>  /**
> + * tick_nohz_idle_got_tick - Check whether or not the tick handler has run
> + */
> +bool tick_nohz_idle_got_tick(void)
> +{
> + struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> +
> + if (ts->inidle > 1) {
> + ts->inidle = 1;
> + return true;
> + }
> + return false;
> +}
> +
> +/**
>   * tick_nohz_get_sleep_length - return the length of the current sleep
>   *
>   * Called from power state control code with interrupts disabled
> @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
>   struct pt_regs *regs = get_irq_regs();
>   ktime_t now = ktime_get();
>  
> + if (ts->inidle)
> + ts->inidle = 2;
> +

You can move that to tick_sched_do_timer() to avoid code duplication.

Also these constants are very opaque. And even with proper symbols it wouldn't 
look
right to extend ts->inidle that way.

Perhaps you should add a field such as ts->got_idle_tick under the boolean 
fields
after the below patch:

--
>From c7b2ca5a4c512517ddfeb9f922d5999f82542ced Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker 
Date: Fri, 6 Apr 2018 04:32:37 +0200
Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field

This optimize the space and leave plenty of room for further flags.

Signed-off-by: Frederic Weisbecker 
---
 kernel/time/tick-sched.h | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index 954b43d..38f24dc 100644
--- a/kernel/time/tick-sched.h
+++ b/kernel/time/tick-sched.h
@@ -45,14 +45,17 @@ struct tick_sched {
struct hrtimer  sched_timer;
unsigned long   check_clocks;
enum tick_nohz_mode nohz_mode;
+
+   unsigned intinidle  : 1;
+   unsigned inttick_stopped: 1;
+   unsigned intidle_active : 1;
+   unsigned intdo_timer_last   : 1;
+
ktime_t last_tick;
ktime_t next_tick;
-   int inidle;
-   int tick_stopped;
unsigned long   idle_jiffies;
unsigned long   idle_calls;
unsigned long   idle_sleeps;
-   int idle_active;
ktime_t idle_entrytime;
ktime_t idle_waketime;
ktime_t idle_exittime;
@@ -62,7 +65,6 @@ struct tick_sched {
unsigned long   last_jiffies;
u64 next_timer;
ktime_t idle_expires;
-   int do_timer_last;
atomic_ttick_dep_mask;
 };
 
-- 
2.7.4

Re: [PATCH v9 05/10] cpuidle: Return nohz hint from cpuidle_select()

[Frederic Weisbecker]

On Wed, Apr 04, 2018 at 10:39:50AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> Index: linux-pm/kernel/time/tick-sched.c
> ===
> --- linux-pm.orig/kernel/time/tick-sched.c
> +++ linux-pm/kernel/time/tick-sched.c
> @@ -991,6 +991,20 @@ void tick_nohz_irq_exit(void)
>  }
>  
>  /**
> + * tick_nohz_idle_got_tick - Check whether or not the tick handler has run
> + */
> +bool tick_nohz_idle_got_tick(void)
> +{
> + struct tick_sched *ts = this_cpu_ptr(_cpu_sched);
> +
> + if (ts->inidle > 1) {
> + ts->inidle = 1;
> + return true;
> + }
> + return false;
> +}
> +
> +/**
>   * tick_nohz_get_sleep_length - return the length of the current sleep
>   *
>   * Called from power state control code with interrupts disabled
> @@ -1101,6 +1115,9 @@ static void tick_nohz_handler(struct clo
>   struct pt_regs *regs = get_irq_regs();
>   ktime_t now = ktime_get();
>  
> + if (ts->inidle)
> + ts->inidle = 2;
> +

You can move that to tick_sched_do_timer() to avoid code duplication.

Also these constants are very opaque. And even with proper symbols it wouldn't 
look
right to extend ts->inidle that way.

Perhaps you should add a field such as ts->got_idle_tick under the boolean 
fields
after the below patch:

--
>From c7b2ca5a4c512517ddfeb9f922d5999f82542ced Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker 
Date: Fri, 6 Apr 2018 04:32:37 +0200
Subject: [PATCH] nohz: Gather tick_sched booleans under a common flag field

This optimize the space and leave plenty of room for further flags.

Signed-off-by: Frederic Weisbecker 
---
 kernel/time/tick-sched.h | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/time/tick-sched.h b/kernel/time/tick-sched.h
index 954b43d..38f24dc 100644
--- a/kernel/time/tick-sched.h
+++ b/kernel/time/tick-sched.h
@@ -45,14 +45,17 @@ struct tick_sched {
struct hrtimer  sched_timer;
unsigned long   check_clocks;
enum tick_nohz_mode nohz_mode;
+
+   unsigned intinidle  : 1;
+   unsigned inttick_stopped: 1;
+   unsigned intidle_active : 1;
+   unsigned intdo_timer_last   : 1;
+
ktime_t last_tick;
ktime_t next_tick;
-   int inidle;
-   int tick_stopped;
unsigned long   idle_jiffies;
unsigned long   idle_calls;
unsigned long   idle_sleeps;
-   int idle_active;
ktime_t idle_entrytime;
ktime_t idle_waketime;
ktime_t idle_exittime;
@@ -62,7 +65,6 @@ struct tick_sched {
unsigned long   last_jiffies;
u64 next_timer;
ktime_t idle_expires;
-   int do_timer_last;
atomic_ttick_dep_mask;
 };
 
-- 
2.7.4