Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Date:   Fri, 20 Oct 2023 11:06:06 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     K Prateek Nayak <kprateek.nayak@amd.com>
Cc:     Anna-Maria Behnsen <anna-maria@linutronix.de>,
        linux-kernel@vger.kernel.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>,
        Sebastian Siewior <bigeasy@linutronix.de>,
        Giovanni Gherdovich <ggherdovich@suse.cz>,
        Lukasz Luba <lukasz.luba@arm.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        Srinivas Pandruvada <srinivas.pandruvada@intel.com>
Subject: Re: [PATCH v8 00/25] timer: Move from a push remote at enqueue to a
 pull at expiry model
Message-ID: <20231020090606.GA31411@noisy.programming.kicks-ass.net>
References: <20231004123454.15691-1-anna-maria@linutronix.de>
 <f7e9279b-a2dc-1980-4134-c6868b47b4ee@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f7e9279b-a2dc-1980-4134-c6868b47b4ee@amd.com>
Precedence: bulk

On Fri, Oct 06, 2023 at 10:35:43AM +0530, K Prateek Nayak wrote:
> Hello Anna-Maria,
> 
> On 10/4/2023 6:04 PM, Anna-Maria Behnsen wrote:
> > [..snip..]
> > 
> > Ping Pong Oberservation
> > ^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > During testing on a mostly idle machine a ping pong game could be observed:
> > a process_timeout timer is expired remotely on a non idle CPU. Then the CPU
> > where the schedule_timeout() was executed to enqueue the timer comes out of
> > idle and restarts the timer using schedule_timeout() and goes back to idle
> > again. This is due to the fair scheduler which tries to keep the task on
> > the CPU which it previously executed on.
> 
> Regarding above, are you referring to "wake_up_process(timeout->task)" in
> "process_timeout()" ends up waking the task on an idle CPU instead of the
> CPU where process_timeout() ran?
> 
> In which case, have you tried using the "WF_CURRENT_CPU" flag for the
> wakeup? (landed upstream in v6.6-rc1) It is only used by wait queues in
> kernel/sched/wait.c currently but perhaps we can have a
> "wake_up_process_on_current_cpu()" that process_timeout() can call.
> 
> Something along the lines of:
> 
> 	int wake_up_process_on_current_cpu(struct task_struct *p)
> 	{
> 		return try_to_wake_up(p, TASK_NORMAL, WF_CURRENT_CPU);
> 	}
> 	EXPORT_SYMBOL(wake_up_process_on_current_cpu);
> 
> Thoughts?

Yeah, we should definitely not export such a function. Also,
WF_CURRENT_CPU should be used sparingly.

So restarting the task on the previous CPU is done because of cache
affinity and is typically the right thing to do if you care about
performance.

The first question to ask is where this schedule_timeout() is coming
from. Is this some daft userspace that is polling on something that
eventually ends up in schedule_timeout() ?

Can we fix the userspace to not do silly things like that?

The alternative is adding heuristics and we all know where that ends :/