[PATCH v1] hrtimer: Simplify the logic of __hrtimer_get_next_event

[PATCH v1] hrtimer: Simplify the logic of __hrtimer_get_next_event

[Xavier Xia]

Currently, __hrtimer_get_next_event makes two separate calls to
__hrtimer_next_event_base for HRTIMER_ACTIVE_SOFT and HRTIMER_ACTIVE_HARD
respectively to obtain expires_next. However, __hrtimer_next_event_base is
capable of traversing all timer types simultaneously by simply controlling
the active mask. There is no need to distinguish the order of traversal
between soft and hard timers, as the sole purpose is to find the earliest
expiration time.
Therefore, the code can be simplified by reducing the two calls to a single
invocation of __hrtimer_next_event_base, making the code more
straightforward and easier to understand.

Signed-off-by: Xavier Xia <xavier_qy@163.com>
---
 kernel/time/hrtimer.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 517ee2590a29..7c23115d25b0 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -577,24 +577,15 @@ static ktime_t __hrtimer_next_event_base(struct hrtimer_cpu_base *cpu_base,
 static ktime_t
 __hrtimer_get_next_event(struct hrtimer_cpu_base *cpu_base, unsigned int active_mask)
 {
-	unsigned int active;
-	struct hrtimer *next_timer = NULL;
 	ktime_t expires_next = KTIME_MAX;
 
-	if (!cpu_base->softirq_activated && (active_mask & HRTIMER_ACTIVE_SOFT)) {
-		active = cpu_base->active_bases & HRTIMER_ACTIVE_SOFT;
-		cpu_base->softirq_next_timer = NULL;
-		expires_next = __hrtimer_next_event_base(cpu_base, NULL,
-							 active, KTIME_MAX);
+	if (cpu_base->softirq_activated)
+		active_mask &= ~HRTIMER_ACTIVE_SOFT;
 
-		next_timer = cpu_base->softirq_next_timer;
-	}
-
-	if (active_mask & HRTIMER_ACTIVE_HARD) {
-		active = cpu_base->active_bases & HRTIMER_ACTIVE_HARD;
-		cpu_base->next_timer = next_timer;
-		expires_next = __hrtimer_next_event_base(cpu_base, NULL, active,
-							 expires_next);
+	active_mask &= cpu_base->active_bases;
+	if (active_mask) {
+		expires_next = __hrtimer_next_event_base(cpu_base, NULL, active_mask,
+							 KTIME_MAX);
 	}
 
 	return expires_next;
-- 
2.34.1

Re: [PATCH v1] hrtimer: Simplify the logic of __hrtimer_get_next_event

[Thomas Gleixner]

On Fri, May 16 2025 at 15:01, Xavier Xia wrote:
> Currently, __hrtimer_get_next_event makes two separate calls to
> __hrtimer_next_event_base for HRTIMER_ACTIVE_SOFT and HRTIMER_ACTIVE_HARD
> respectively to obtain expires_next. However, __hrtimer_next_event_base is
> capable of traversing all timer types simultaneously by simply controlling
> the active mask. There is no need to distinguish the order of traversal
> between soft and hard timers, as the sole purpose is to find the earliest
> expiration time.
> Therefore, the code can be simplified by reducing the two calls to a single
> invocation of __hrtimer_next_event_base, making the code more
> straightforward and easier to understand.

... and broken

> -		cpu_base->softirq_next_timer = NULL;
> -		next_timer = cpu_base->softirq_next_timer;
> -		cpu_base->next_timer = next_timer;

because you removed the cpu_base::[softirq_]next_timer update logic.

Thanks,

        tglx

Re:Re: [PATCH v1] hrtimer: Simplify the logic of __hrtimer_get_next_event

[Xavier]

Hi Thomas,

At 2025-05-16 18:50:56, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>On Fri, May 16 2025 at 15:01, Xavier Xia wrote:
>> Currently, __hrtimer_get_next_event makes two separate calls to
>> __hrtimer_next_event_base for HRTIMER_ACTIVE_SOFT and HRTIMER_ACTIVE_HARD
>> respectively to obtain expires_next. However, __hrtimer_next_event_base is
>> capable of traversing all timer types simultaneously by simply controlling
>> the active mask. There is no need to distinguish the order of traversal
>> between soft and hard timers, as the sole purpose is to find the earliest
>> expiration time.
>> Therefore, the code can be simplified by reducing the two calls to a single
>> invocation of __hrtimer_next_event_base, making the code more
>> straightforward and easier to understand.
>
>... and broken
>
>> -		cpu_base->softirq_next_timer = NULL;
>> -		next_timer = cpu_base->softirq_next_timer;
>> -		cpu_base->next_timer = next_timer;
>
>because you removed the cpu_base::[softirq_]next_timer update logic.
>

Thanks for your reminder. You're right. We do need to update
cpu_base->next_timer and softirq_next_timer here. Do you think
it's feasible to directly place the updates of these two variables
inside __hrtimer_next_event_base, maybe just like this:

 static ktime_t __hrtimer_next_event_base(struct hrtimer_cpu_base *cpu_base,
 					 const struct hrtimer *exclude,
-					 unsigned int active,
+					 unsigned int active_mask,
 					 ktime_t expires_next)
 {
 	struct hrtimer_clock_base *base;
 	ktime_t expires;
+	unsigned int active;
+	bool include_hard;
+
+	active = cpu_base->active_bases & active_mask;
+	include_hard = !!(active_mask & HRTIMER_ACTIVE_HARD); 
+
+	cpu_base->next_timer = NULL;
+	cpu_base->softirq_next_timer = NULL;
 
 	for_each_active_base(base, cpu_base, active) {
 		struct timerqueue_node *next;
@@ -538,8 +546,11 @@ static ktime_t __hrtimer_next_event_base(struct hrtimer_cpu_base *cpu_base,
 			if (exclude)
 				continue;
 
-			if (timer->is_soft)
+			if (timer->is_soft) {
 				cpu_base->softirq_next_timer = timer;
+				if (include_hard)
+					cpu_base->next_timer = timer;
+			}
 			else
 				cpu_base->next_timer = timer;
 		}


I'm a bit confused. Why doesn't the hrtimer_next_event_without
function need to update cpu_base->next_timer like the
__hrtimer_get_next_event function does?

Do you know if there are other timer-related test cases besides
those in the tools/testing/selftests/timers path?

--
Thanks,
Xavier