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

[Xavier <xavier_qy@163.com>]

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