Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
From:   Anna-Maria Behnsen <anna-maria@linutronix.de>
To:     Sebastian Siewior <bigeasy@linutronix.de>
Cc:     linux-kernel@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        John Stultz <jstultz@google.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Eric Dumazet <edumazet@google.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
        Arjan van de Ven <arjan@infradead.org>,
        "Paul E . McKenney" <paulmck@kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        Rik van Riel <riel@surriel.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Giovanni Gherdovich <ggherdovich@suse.cz>,
        Lukasz Luba <lukasz.luba@arm.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        Srinivas Pandruvada <srinivas.pandruvada@intel.com>,
        K Prateek Nayak <kprateek.nayak@amd.com>
Subject: Re: [PATCH v9 12/32] timers: Fix nextevt calculation when no timers
 are pending
In-Reply-To: <20231204160350.OTCnqCJf@linutronix.de>
References: <20231201092654.34614-1-anna-maria@linutronix.de>
 <20231201092654.34614-13-anna-maria@linutronix.de>
 <20231204160350.OTCnqCJf@linutronix.de>
Date:   Tue, 05 Dec 2023 12:53:03 +0100
Message-ID: <87zfyodfxc.fsf@somnus>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Sebastian Siewior <bigeasy@linutronix.de> writes:

> On 2023-12-01 10:26:34 [+0100], Anna-Maria Behnsen wrote:
>> When no timer is queued into an empty timer base, the next_expiry will not
>> be updated. It was originally calculated as
>> 
>>   base->clk + NEXT_TIMER_MAX_DELTA
>> 
>> When the timer base stays empty long enough (> NEXT_TIMER_MAX_DELTA), the
>> next_expiry value of the empty base suggests that there is a timer pending
>> soon. This might be more a kind of a theoretical problem, but the fix
>> doesn't hurt.
>
> So __run_timers() sets base::next_expiry to base->clk +
> NEXT_TIMER_MAX_DELTA and then we have no more timers enqueued.
>
> But wouldn't base->timers_pending remain false? Therefore it would use
> "expires = KTIME_MAX" as return value (well cmp_next_hrtimer_event())?

Jupp.

> Based on the code as of #11, it would only set timer_base::is_idle
> wrongly false if it wraps around. Other than that, I don't see an issue.
> What do I miss?

And it will raise an unnecessary softirq when it wraps around as you
also mentioned on the next paragraph.

> If you update it regardless here then it would make a difference to
> run_local_timers() assuming we have still hrtimer which expire and this
> next_expiry check might raise a softirq since it does not consider the
> timers_pending value.

The only difference with this change would be that the softirq will not
be raised when it wraps around.

>> Use only base->next_expiry value as nextevt when timers are
>> pending. Otherwise nextevt will be jiffies + NEXT_TIMER_MAX_DELTA. As all
>> information is in place, update base->next_expiry value of the empty timer
>> base as well.
>
> or consider timers_pending in run_local_timers()? An additional read vs
> write?

This would also be a possibility to add the check in run_local_timers()
with timers_pending. And we also have to make the is_idle marking in
get_next_timer_interrupt() dependant on base::timers_pending bit. But
this also means, we cannot rely on next_expiry when no timer is pending.

Frederic, what do you think?

Thanks,

	Anna-Maria