Received-SPF: pass (google.com: domain of linux-kernel+bounces-151164-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Sven Schnelle <svens@linux.ibm.com>
To: Tejun Heo <tj@kernel.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>, linux-kernel@vger.kernel.org,
        Heiko Carstens <hca@linux.ibm.com>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] workqueue: fix selection of wake_cpu in kick_pool()
In-Reply-To: <ZiB9rYBu-0qjsCbF@slm.duckdns.org> (Tejun Heo's message of "Wed,
	17 Apr 2024 15:55:57 -1000")
References: <20240415053550.538722-1-svens@linux.ibm.com>
	<Zh8EfxdVdiIj_27H@slm.duckdns.org> <yt9dpluogfw9.fsf@linux.ibm.com>
	<ZiB9rYBu-0qjsCbF@slm.duckdns.org>
Date: Fri, 19 Apr 2024 10:27:05 +0200
Message-ID: <yt9dzftp3gh2.fsf@linux.ibm.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Hi Tejun,

Tejun Heo <tj@kernel.org> writes:

> On Wed, Apr 17, 2024 at 05:36:38PM +0200, Sven Schnelle wrote:
>> > This generally seems like a good idea but isn't this still racy? The CPU may
>> > go down between setting p->wake_cpu and wake_up_process().
>> 
>> Don't know without reading the source, but how does this code normally
>> protect against that?
>
> Probably by wrapping determining the wake_cpu and the wake_up inside
> cpu_read_lock() section.

Do you mean rcu_read_lock()? cpus_read_lock() takes a mutex, and the
crash happens in softirq context - so cpus_read_lock() can't be the
correct lock.

If i read the code correctly, cpu hotplug uses stop_machine_cpuslocked()
- so rcu_read_lock() should be sufficient for non-atomic context.

Looking at the backtrace the crash is actually happening in
arch_vpu_is_preempted(). I don't know the semantics of that function,
whether it is ok to call it for offline CPUs, or whether the calling
code should make sure that the cpu is online (which would be my guess).

Following the backtrace from my initial mail, I can't find a place where
a check is done whether p->wake_cpu is actually online. Eventually
available_idle_cpu() is calling vcpu_is_preempted(). I wonder whether
available_idle_cpu() should do a cpu_online() check right at the
beginning?

Adding Peter to CC, he probably knows.

Thanks,
Sven