From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D60C026E703 for ; Wed, 19 Nov 2025 22:02:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763589725; cv=none; b=KGdHdb+Aouw4pCkLsUrbwLYPP0qvdH2ykOBz9K4pyXSDaHqMfxtoU2wH3LKqsNxbk+Lh+0MYmgUJzXva7vzcezjtrB5OXq31seQ39y7CWLbGvriC4qdvghFfDs1+ZCDLrG0Koxz22Xlhx2KEOGAld4h34ltesSiXuxpYJ/jdmX8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1763589725; c=relaxed/simple; bh=l9AmfAzKufSi8YZ3WbtQM45L7430EBZb/rQBp984Gqc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=aAY/Zakalh2P5VCSlhhc53zVzRWxZZq3VL1LUORzZ/TT597o2HheJG0hgw9KcDe6qPsred8DRbiOpBrZiwgo1cRy/sDwEsGtF41Kq2+6e+CcW+UscfNB/mRxfrtelUdsbHTz7mNNSi0sOI1znfFSbA7mF5RBD8+Z9W8zq1dr5G0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=j/PpOD5p; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="j/PpOD5p" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0CDDCC116D0; Wed, 19 Nov 2025 22:02:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1763589725; bh=l9AmfAzKufSi8YZ3WbtQM45L7430EBZb/rQBp984Gqc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=j/PpOD5pxubo22SRpWJfI0tOEvYpM/7eXrrls1SEe1ZP/QTiLsYRbeIY4NPBg5EtL 9cXf2AXY+tt4hMdPzPYG/RAC+xAN2OMyMO9DhWHiga8FNB6jMaasZGm9UeK5GcbyYK i/L5DN+G/I4F8hbzLgLyHoyS2CU0RXjcOLsKLUGGOpLgzrnx5ZxQ5AyqdfeQDr1V7e DURRcbkroRfTQ+y00g/ZTrsPagjjZ6AJDkpcf2z46lSUTjB7xdjeecUmjGorJuNVEN T/6yfNnpGQxERfKF48EfAxox+SqQ4kCeV3uYcNNpRyJU+QNHpizEAjpCPUczcs/J5W AHcziYf5M1ylQ== Date: Wed, 19 Nov 2025 23:02:02 +0100 From: Frederic Weisbecker To: Thomas Gleixner Cc: Gabriele Monaco , linux-kernel@vger.kernel.org, Anna-Maria Behnsen , Waiman Long , "John B. Wyatt IV" , "John B. Wyatt IV" Subject: Re: [PATCH v15 7/7] timers: Exclude isolated cpus from timer migration Message-ID: References: <20251113083324.33490-1-gmonaco@redhat.com> <20251113083324.33490-8-gmonaco@redhat.com> <87pl9eklvc.ffs@tglx> <87jyzllwhd.ffs@tglx> <87fra9lnsw.ffs@tglx> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87fra9lnsw.ffs@tglx> Le Wed, Nov 19, 2025 at 10:23:11PM +0100, Thomas Gleixner a écrit : > On Wed, Nov 19 2025 at 21:13, Frederic Weisbecker wrote: > > Le Wed, Nov 19, 2025 at 07:15:42PM +0100, Thomas Gleixner a écrit : > >> On Wed, Nov 19 2025 at 18:14, Frederic Weisbecker wrote: > >> > Le Wed, Nov 19, 2025 at 05:50:15PM +0100, Thomas Gleixner a écrit : > >> >> But thinking more about it. What's the actual point of moving this 'clear' > >> >> out instead of just moving it further down? > >> >> > >> >> It does not matter at all whether the isol/unisol muck clears an already > >> >> cleared bit or not. But it would keep the function name comprehensible > >> >> and avoid all this online/offline wrapper nonsense. > >> > > >> > That was my suggestion. > >> > > >> > It's because tmigr_clear_cpu_available() and tmigr_set_cpu_available() > >> > can now all be called concurrently through the workqueues and race and > >> > mess up the cpumask if they all try to clear/set at the same time... > >> > >> Huch? > >> > >> cpumask_set_cpu() uses set_bit() and cpumask_clear_cpu() uses > >> clear_bit(). Both are atomic and nothing gets messed up. > > > > Urgh, right... > > > >> > >> The only undefined case would be if you end up setting/clearing the same > >> bit, which would require that the unisol and isol maps overlap. But that > >> would be a bug on it's own, no? > > > > Ok. > > > > But then the "unisolate" works must be flushed before this line: > > > > + /* Set up the mask earlier to avoid races with the migrator CPU */ > > + cpumask_andnot(tmigr_available_cpumask, tmigr_available_cpumask, cpumask_isol); > > > > Because that is non-atomic and can race with the cpumask_set_cpu() from the > > works, right? > > Correct, but I still have to understand why this has to happen > upfront. As I said before this comment is useless: > > > + /* Set up the mask earlier to avoid races with the migrator CPU */ > > which decodes to: > > Set up the mask earlier than something unspecified to avoid > unspecified races with the migrator CPU. > > Seriously? > > But I finally understood what this is about after staring at it with > less disgust again for five times in a row: > > tmigr_clear_cpu_available() requires a stable cpumask to find a > migrator. > > So what this is about is to avoid touching the cpumask in those worker > functions so that tmigr_isolated_exclude_cpumask() can fiddle with them > asynchronously. > > Right? > > That's voodoo programming which nobody - even those involved - will > understand anymore three months down the road. > > The idea that updating the masks upfront will provide stable state is > flawed to begin with. Let's assume the following scenario: > > 1) The isol/unisol mask is just flipped around > > 2) The newly available CPUs are marked in the mask > > 3) The work is scheduled on those CPUs > > 4) The newly unavailable CPUs are cleared in the mask > > 5) The work is scheduled on those CPUs > > 6) The newly available CPU workers are delayed so that the newly > unavailable workers get there first. They find a stable CPU mask, > but none of these now "available" CPUs are actually brought into > active state yet. > > That's just a perfect recipe for some completely undecodable bug to > happen anytime soon even it it supposed to "work" by chance. Urgh, indeed I completely missed that... > > The way how this was designed in the first place is that changing the > "availability" is fully serialized by the CPU hotplug machinery. Which > means zero surprises. > > Now you create a side channel, which lacks these serialization > guarantees for absolutely no reason. Changing this isolation muck at > run-time is not a hotpath operation and trying to optimize it for no > good reason is just stupid. > > The most trivial solution is to schedule the works one by one walking > through the newly available and then through the unavailable maps. Right. > > If you want to be a bit smarter, then you can just use a global mutex, > which is taken inside the set/clear_available() functions which > serializes the related functionality and bulk schedule/flush the unisol > (make available) first and then proceed to the isol (make unavailable). > > Then nothing has to change vs. the set/clear operations and everything > just works. > > That mutex does not do any harm in the CPU hotplug case and the > serialization vs. the workers is not going to be the end of the world. > > I'm willing to bet that no real-world use-case will ever notice the > existance of this mutex. The microbenchmark which shows off the "I'm so > smart" metric is completely irrelevant especially when the result is > fragile, incomprehensible and therefore unmaintainable. > > Keep it correct and simple is still the most important engineering > principle. Premature optimization is a guaranteed path to failure. > > If there is a compelling use case which justifies the resulting > complexity, then it can be built on top. I'm not holding my breath. See > above... Perhaps the only thing that worries me is if an isolated partition is inverted. Say 0-3 is non isolated and 4-7 is isolated. And then cpuset is overwritten so that the reverse is applied: 0-3 is isolated and 4-7 is not isolated. If all isol works reach before unisol works, then tmigr_clear_cpu_available() -> cpumask_any(tmigr_available_mask) won't find any CPU left on the last call. Now last time I tried to invert an isolated cpumask in cpuset, I got -EINVAL so something must be preventing from that. Just in case let's have a WARN_ON_ONCE(cpumask_any(tmigr_available_mask) >= nr_cpu_ids). But other than this detail, your solution looks good! Thanks. -- Frederic Weisbecker SUSE Labs