diff mbox series

[v8,05/25] timers: Clarify check in forward_timer_base()

Message ID 20231004123454.15691-6-anna-maria@linutronix.de
State New
Headers
Series timer: Move from a push remote at enqueue to a pull at expiry model |

Commit Message

Anna-Maria Behnsen Oct. 4, 2023, 12:34 p.m. UTC 
  The current check whether a forward of the timer base is required can be
simplified by using an already existing comparison function which is easier
to read. The related comment is outdated and was not updated when the check
changed in commit 36cd28a4cdd0 ("timers: Lower base clock forwarding
threshold").

Use time_before_eq() for the check and replace the comment by copying the
comment from the same check inside get_next_timer_interrupt().

No functional change.

Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
---
 kernel/time/timer.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Frederic Weisbecker Oct. 5, 2023, 10:17 a.m. UTC | #1 
On Wed, Oct 04, 2023 at 02:34:34PM +0200, Anna-Maria Behnsen wrote:
> The current check whether a forward of the timer base is required can be
> simplified by using an already existing comparison function which is easier
> to read. The related comment is outdated and was not updated when the check
> changed in commit 36cd28a4cdd0 ("timers: Lower base clock forwarding
> threshold").
> 
> Use time_before_eq() for the check and replace the comment by copying the
> comment from the same check inside get_next_timer_interrupt().
> 
> No functional change.
> 
> Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
> ---
>  kernel/time/timer.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 5e17244a9465..31aed8353db1 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -944,11 +944,10 @@ static inline void forward_timer_base(struct timer_base *base)
>  	unsigned long jnow = READ_ONCE(jiffies);
>  
>  	/*
> -	 * No need to forward if we are close enough below jiffies.
> -	 * Also while executing timers, base->clk is 1 offset ahead
> -	 * of jiffies to avoid endless requeuing to current jiffies.
> +	 * Check whether we can forward the base. We can only do that when
> +	 * @basej is past base->clk otherwise we might rewind base->clk.

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Also can we keep the precious information in the comment and move it to
the right place? Such as:

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 63a8ce7177dd..3e70ac818034 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -2015,6 +2015,10 @@ static inline void __run_timers(struct timer_base *base)
 		 */
 		WARN_ON_ONCE(!levels && !base->next_expiry_recalc
 			     && base->timers_pending);
+		/*
+		 * While executing timers, base->clk is set 1 offset ahead of
+		 * jiffies to avoid endless requeuing to current jiffies.
+		 */
 		base->clk++;
 		base->next_expiry = __next_timer_interrupt(base);
 

Thanks!

>  	 */
> -	if ((long)(jnow - base->clk) < 1)
> +	if (time_before_eq(jnow, base->clk))
>  		return;
>  
>  	/*
> -- 
> 2.39.2
>
Anna-Maria Behnsen Oct. 16, 2023, 8:11 a.m. UTC | #2 
On Thu, 5 Oct 2023, Frederic Weisbecker wrote:

> On Wed, Oct 04, 2023 at 02:34:34PM +0200, Anna-Maria Behnsen wrote:
> > The current check whether a forward of the timer base is required can be
> > simplified by using an already existing comparison function which is easier
> > to read. The related comment is outdated and was not updated when the check
> > changed in commit 36cd28a4cdd0 ("timers: Lower base clock forwarding
> > threshold").
> > 
> > Use time_before_eq() for the check and replace the comment by copying the
> > comment from the same check inside get_next_timer_interrupt().
> > 
> > No functional change.
> > 
> > Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
> > ---
> >  kernel/time/timer.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> > index 5e17244a9465..31aed8353db1 100644
> > --- a/kernel/time/timer.c
> > +++ b/kernel/time/timer.c
> > @@ -944,11 +944,10 @@ static inline void forward_timer_base(struct timer_base *base)
> >  	unsigned long jnow = READ_ONCE(jiffies);
> >  
> >  	/*
> > -	 * No need to forward if we are close enough below jiffies.
> > -	 * Also while executing timers, base->clk is 1 offset ahead
> > -	 * of jiffies to avoid endless requeuing to current jiffies.
> > +	 * Check whether we can forward the base. We can only do that when
> > +	 * @basej is past base->clk otherwise we might rewind base->clk.
> 
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> 
> Also can we keep the precious information in the comment and move it to
> the right place? Such as:
> 
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 63a8ce7177dd..3e70ac818034 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -2015,6 +2015,10 @@ static inline void __run_timers(struct timer_base *base)
>  		 */
>  		WARN_ON_ONCE(!levels && !base->next_expiry_recalc
>  			     && base->timers_pending);
> +		/*
> +		 * While executing timers, base->clk is set 1 offset ahead of
> +		 * jiffies to avoid endless requeuing to current jiffies.
> +		 */
>  		base->clk++;
>  		base->next_expiry = __next_timer_interrupt(base);
>  
> 
> Thanks!
> 

Good point! I will do this.

Thanks,

	Anna-Maria
diff mbox series

Patch

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 5e17244a9465..31aed8353db1 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -944,11 +944,10 @@  static inline void forward_timer_base(struct timer_base *base)
 	unsigned long jnow = READ_ONCE(jiffies);
 
 	/*
-	 * No need to forward if we are close enough below jiffies.
-	 * Also while executing timers, base->clk is 1 offset ahead
-	 * of jiffies to avoid endless requeuing to current jiffies.
+	 * 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 ((long)(jnow - base->clk) < 1)
+	if (time_before_eq(jnow, base->clk))
 		return;
 
 	/*