Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Tue, 5 Dec 2023 22:11:06 +0100
From:   Sebastian Siewior <bigeasy@linutronix.de>
To:     Anna-Maria Behnsen <anna-maria@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>,
        Richard Cochran <richardcochran@gmail.com>
Subject: Re: [PATCH v9 22/32] timers: Keep the pinned timers separate from
 the others
Message-ID: <20231205211106.ykKsi921@linutronix.de>
References: <20231201092654.34614-1-anna-maria@linutronix.de>
 <20231201092654.34614-23-anna-maria@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20231201092654.34614-23-anna-maria@linutronix.de>
Precedence: bulk

On 2023-12-01 10:26:44 [+0100], Anna-Maria Behnsen wrote:
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1985,10 +1998,31 @@ static inline u64 __get_next_timer_interrupt(unsigned long basej, u64 basem,
>  		return expires;
>  	}
>  
> -	raw_spin_lock(&base->lock);
> -	nextevt = next_timer_interrupt(base, basej);
> +	base_local = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
> +	base_global = this_cpu_ptr(&timer_bases[BASE_GLOBAL]);
> +
> +	raw_spin_lock(&base_local->lock);
> +	raw_spin_lock_nested(&base_global->lock, SINGLE_DEPTH_NESTING);
> +
> +	nextevt_local = next_timer_interrupt(base_local, basej);
> +	nextevt_global = next_timer_interrupt(base_global, basej);
>  
> -	if (base->timers_pending) {
> +	/*
> +	 * Check whether the local event is expiring before or at the same
> +	 * time as the global event.
> +	 *
> +	 * Note, that nextevt_global and nextevt_local might be based on
> +	 * different base->clk values. So it's not guaranteed that
> +	 * comparing with empty bases results in a correct local_first.

This ends like an unsolved mystery case. Could you add why one should
not worry about an incorrect local_first?

But seriously, how far apart can they get and what difference does it
make?  At timer enqueue time clk equals jiffies. At this point one clk
base could be at jiffies and the other might be a few jiffies before
that.
The next event (as in next_expiry) should be valid for both compare
wise. Both must be larger than jiffies. The delta between jiffies and
next event has to be less than NEXT_TIMER_MAX_DELTA for each base.

> +	 */
> +	if (base_local->timers_pending && base_global->timers_pending)
> +		local_first = time_before_eq(nextevt_local, nextevt_global);
> +	else
> +		local_first = base_local->timers_pending;
> +
> +	nextevt = local_first ? nextevt_local : nextevt_global;
> +
> +	if (base_local->timers_pending || base_global->timers_pending) {
>  		/* If we missed a tick already, force 0 delta */
>  		if (time_before(nextevt, basej))
>  			nextevt = basej;

So if nextevt_local missed a tick and nextevt_global is
NEXT_TIMER_MAX_DELTA-1 (so we get the largest difference possible
between those two) then the time_before_eq() should still come out
right. We could still miss more than one tick.

This looks good. I just don't understand the (above) comment.

Sebastian