diff mbox series

[v9,23/32] timers: Retrieve next expiry of pinned/non-pinned timers separately

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

Commit Message

Anna-Maria Behnsen Dec. 1, 2023, 9:26 a.m. UTC 
  For the conversion of the NOHZ timer placement to a pull at expiry time
model it's required to have separate expiry times for the pinned and the
non-pinned (movable) timers. Therefore struct timer_events is introduced.

No functional change

Originally-by: Richard Cochran (linutronix GmbH) <richardcochran@gmail.com>
Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
---
v9: Update was required (change of preceding patches)
---
 kernel/time/timer.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

Comments

Sebastian Andrzej Siewior Dec. 6, 2023, 9:47 a.m. UTC | #1 
On 2023-12-01 10:26:45 [+0100], Anna-Maria Behnsen wrote:
> For the conversion of the NOHZ timer placement to a pull at expiry time
> model it's required to have separate expiry times for the pinned and the
> non-pinned (movable) timers. Therefore struct timer_events is introduced.
> 
> No functional change
> 
> Originally-by: Richard Cochran (linutronix GmbH) <richardcochran@gmail.com>
> Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
…
> index 366ea26ce3ba..0d53d853ae22 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
…
> @@ -2022,13 +2028,31 @@ static inline u64 __get_next_timer_interrupt(unsigned long basej, u64 basem,
>  
>  	nextevt = local_first ? nextevt_local : nextevt_global;
>  
> -	if (base_local->timers_pending || base_global->timers_pending) {
> +	/*
> +	 * If the @nextevt is at max. one tick away, use @nextevt and store
> +	 * it in the local expiry value. The next global event is irrelevant in
> +	 * this case and can be left as KTIME_MAX.
> +	 */
> +	if (time_before_eq(nextevt, basej + 1)) {
>  		/* If we missed a tick already, force 0 delta */
>  		if (time_before(nextevt, basej))
>  			nextevt = basej;
> -		expires = basem + (u64)(nextevt - basej) * TICK_NSEC;
> +		tevt.local = basem + (u64)(nextevt - basej) * TICK_NSEC;
> +		goto unlock;

You claim "No functional change" in the patch description. However if
you take the shortcut here you don't update `idle' if set and you don't
__forward_timer_base(). The `idle` parameter doesn't matter because it
was false and will remain false as per current logic.

But what about the forward of the timer base? It is probably not real
problem since the next add/mod timer call will forward it.

Sebastian
Anna-Maria Behnsen Dec. 7, 2023, 10:12 a.m. UTC | #2 
Sebastian Siewior <bigeasy@linutronix.de> writes:

> On 2023-12-01 10:26:45 [+0100], Anna-Maria Behnsen wrote:
>> For the conversion of the NOHZ timer placement to a pull at expiry time
>> model it's required to have separate expiry times for the pinned and the
>> non-pinned (movable) timers. Therefore struct timer_events is introduced.
>> 
>> No functional change
>> 
>> Originally-by: Richard Cochran (linutronix GmbH) <richardcochran@gmail.com>
>> Signed-off-by: Anna-Maria Behnsen <anna-maria@linutronix.de>
>> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> …
>> index 366ea26ce3ba..0d53d853ae22 100644
>> --- a/kernel/time/timer.c
>> +++ b/kernel/time/timer.c
> …
>> @@ -2022,13 +2028,31 @@ static inline u64 __get_next_timer_interrupt(unsigned long basej, u64 basem,
>>  
>>  	nextevt = local_first ? nextevt_local : nextevt_global;
>>  
>> -	if (base_local->timers_pending || base_global->timers_pending) {
>> +	/*
>> +	 * If the @nextevt is at max. one tick away, use @nextevt and store
>> +	 * it in the local expiry value. The next global event is irrelevant in
>> +	 * this case and can be left as KTIME_MAX.
>> +	 */
>> +	if (time_before_eq(nextevt, basej + 1)) {
>>  		/* If we missed a tick already, force 0 delta */
>>  		if (time_before(nextevt, basej))
>>  			nextevt = basej;
>> -		expires = basem + (u64)(nextevt - basej) * TICK_NSEC;
>> +		tevt.local = basem + (u64)(nextevt - basej) * TICK_NSEC;
>> +		goto unlock;
>
> You claim "No functional change" in the patch description. However if
> you take the shortcut here you don't update `idle' if set and you don't
> __forward_timer_base(). The `idle` parameter doesn't matter because it
> was false and will remain false as per current logic.
>
> But what about the forward of the timer base? It is probably not real
> problem since the next add/mod timer call will forward it.

You are right. It is not a problem as the timer base will be forwarded
by add/mod timer and also when timers needs to expire. (It is reworked
by the next patch...)

But it is not consistent and happend within one of the last rework
iterations. I'll change the goto label into 'forward' and place it
before the forward calls.

Thanks,

	Anna-Maria
diff mbox series

Patch

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 366ea26ce3ba..0d53d853ae22 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -221,6 +221,11 @@  struct timer_base {
 
 static DEFINE_PER_CPU(struct timer_base, timer_bases[NR_BASES]);
 
+struct timer_events {
+	u64	local;
+	u64	global;
+};
+
 #ifdef CONFIG_NO_HZ_COMMON
 
 static DEFINE_STATIC_KEY_FALSE(timers_nohz_active);
@@ -1983,10 +1988,11 @@  static unsigned long next_timer_interrupt(struct timer_base *base,
 static inline u64 __get_next_timer_interrupt(unsigned long basej, u64 basem,
 					     bool *idle)
 {
+	struct timer_events tevt = { .local = KTIME_MAX, .global = KTIME_MAX };
 	unsigned long nextevt, nextevt_local, nextevt_global;
 	struct timer_base *base_local, *base_global;
-	u64 expires = KTIME_MAX;
 	bool local_first;
+	u64 expires;
 
 	/*
 	 * Pretend that there is no timer pending if the cpu is offline.
@@ -1995,7 +2001,7 @@  static inline u64 __get_next_timer_interrupt(unsigned long basej, u64 basem,
 	if (cpu_is_offline(smp_processor_id())) {
 		if (idle)
 			*idle = true;
-		return expires;
+		return tevt.local;
 	}
 
 	base_local = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
@@ -2022,13 +2028,31 @@  static inline u64 __get_next_timer_interrupt(unsigned long basej, u64 basem,
 
 	nextevt = local_first ? nextevt_local : nextevt_global;
 
-	if (base_local->timers_pending || base_global->timers_pending) {
+	/*
+	 * If the @nextevt is at max. one tick away, use @nextevt and store
+	 * it in the local expiry value. The next global event is irrelevant in
+	 * this case and can be left as KTIME_MAX.
+	 */
+	if (time_before_eq(nextevt, basej + 1)) {
 		/* If we missed a tick already, force 0 delta */
 		if (time_before(nextevt, basej))
 			nextevt = basej;
-		expires = basem + (u64)(nextevt - basej) * TICK_NSEC;
+		tevt.local = basem + (u64)(nextevt - basej) * TICK_NSEC;
+		goto unlock;
 	}
 
+	/*
+	 * Update tevt.* values:
+	 *
+	 * If the local queue expires first, then the global event can be
+	 * ignored. If the global queue is empty, nothing to do either.
+	 */
+	if (!local_first && base_global->timers_pending)
+		tevt.global = basem + (u64)(nextevt_global - basej) * TICK_NSEC;
+
+	if (base_local->timers_pending)
+		tevt.local = basem + (u64)(nextevt_local - basej) * TICK_NSEC;
+
 	/*
 	 * We have a fresh next event. Check whether we can forward the
 	 * base.
@@ -2058,9 +2082,12 @@  static inline u64 __get_next_timer_interrupt(unsigned long basej, u64 basem,
 		trace_timer_base_idle(base_local->is_idle, base_local->cpu);
 	}
 
+unlock:
 	raw_spin_unlock(&base_global->lock);
 	raw_spin_unlock(&base_local->lock);
 
+	expires = min_t(u64, tevt.local, tevt.global);
+
 	return cmp_next_hrtimer_event(basem, expires);
 }