Message ID | 20231114164416.208285-1-ubizjak@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:6358:a59:b0:164:83eb:24d7 with SMTP id 25csp2052103rwb; Tue, 14 Nov 2023 08:44:49 -0800 (PST) X-Google-Smtp-Source: AGHT+IEODMXJBJPyByHyT69GMLHtQRT9Um0blvjoAfv6/KlUU+oiqTS9igFEx/l9i+MQl39refCC X-Received: by 2002:a05:6a00:27a9:b0:6b2:7a88:7128 with SMTP id bd41-20020a056a0027a900b006b27a887128mr9294706pfb.22.1699980289060; Tue, 14 Nov 2023 08:44:49 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1699980289; cv=none; d=google.com; s=arc-20160816; b=A/8AHFyeMzAncFVwhUOqZCtXbuDhG6alGCXG0XWLCYe7wAgEbRyFChf67cxfO8dYJQ LRdtwZ6wJFBq1c3INbod7T7s+/toPXocghClxp98LSToJ7mSrTv4li14gtV/H8/HrDFQ ut8xhs1mVeSP6gBq4Y3vBA354hJi3lmlRGpsZK3xpU9aIQ/6qZWIEBS7QE0JhB9/I0Wd pLAkAtKCh6i3CqOCqqBzh88WXe81dlZ0i43e2sEMh6MTaz3lVoQASfa8NphkkMQmzn5z T/A97y+IdMfu7HiVjE6Q6y8L44k64j/B4wNBXUCuNjF+mgFn2DMsDGPWQ63rV/CKoViy nUOg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=wpdezhDPEqhtB7SZt0xZhm1topcNCcCV8z1dKB+mAIY=; fh=Sc1NU2In/E+GAgZqot566PXenAIT69OTFMqs+GLix9s=; b=Ik0n9rxHlUgM3iMolhgFS00Tlwi8KJx6dgORNXU8B1mUQFERbsLn6hC4MdtlkT5+6z WPcbHDKtAHki7rXMkwg8P1KZV2Yop+WldGYqaxAZo0Io1H1n0ta4gMHxm9mY1wui3Fea p305KC3IKsRuUw1Uu0efGAMyjaUOTA2hefknSFv25VcvhK4zakRX5n0BfvlXMKuOeIrv Uh8TMd5p7H5xBgQdXxiOBsHW0vJSBcN4vWdfvJM/B0Ah7l1+TBWRuqKXBVR+Hat+9Z0T 37RVHGkQjwLIR7yFjZb8q7BNXcm0pVAQezIGj8OR+CgCUBamv0NtPRMv0An5ZoZdMPhb uIfQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=CBxcliuf; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id r20-20020a62e414000000b006be0278445esi7810875pfh.138.2023.11.14.08.44.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Nov 2023 08:44:49 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=CBxcliuf; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 3F1D280C2574; Tue, 14 Nov 2023 08:44:46 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233741AbjKNQod (ORCPT <rfc822;lhua1029@gmail.com> + 29 others); Tue, 14 Nov 2023 11:44:33 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33236 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230461AbjKNQoc (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 14 Nov 2023 11:44:32 -0500 Received: from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com [IPv6:2a00:1450:4864:20::42e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1064D11D for <linux-kernel@vger.kernel.org>; Tue, 14 Nov 2023 08:44:29 -0800 (PST) Received: by mail-wr1-x42e.google.com with SMTP id ffacd0b85a97d-32f87b1c725so4042192f8f.3 for <linux-kernel@vger.kernel.org>; Tue, 14 Nov 2023 08:44:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1699980267; x=1700585067; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=wpdezhDPEqhtB7SZt0xZhm1topcNCcCV8z1dKB+mAIY=; b=CBxcliuf53QIl1HxHjIRurINlLx4cKoNZ2axNXkRWgzvldLaT3592BQF1xAmO3NpYJ 96WTTEWDlhdUuNNMPfTMckwrEtZgExZjM+87FVPd8eLDQIIAbzovPZYhf2HJSsa2QX4K vOfcWiCI2vANxzOUhCVD1y+YpRxfhiuHUUMI0NyVYn1QG0SP66OZWBo8s1e2QGxlHe3o CYjy323E0OLRgGeNuuOFbbnAdEBWmE4xu04Vi/25mRGJsLWqevo3airkKPqWeLKpIb7I hf9/sadX6XjNWC41ma2Ou8cqWoVoNUhwnqQ3Gr6+Rbuz3EHbuJBTYYOytMW3U+b8MX5o QB7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699980267; x=1700585067; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=wpdezhDPEqhtB7SZt0xZhm1topcNCcCV8z1dKB+mAIY=; b=Wq3kxaE9zZ+CmqZlPN0jeofBsjERlSS1y3w45QoQAmtgSazYQ5YM2Uqn6GpFrsoGJD 0EjLGGlcjcaYmiumVQVj8scO2A20+R9AfjlhGHDqfg1bmBaJwQJHj4OxKg4xElXv0bWa A6z6NxVACTcWie4DEHv5k3ebufJObEe9QWQasVHh7QiMUuUxQKPi7GPq4l94QLBKbHj3 dZhoNzRPdlaZ1r21C/4uhGBSIXYJB9LvapJbSCQ/5frd9NMEgEr8Rb6rQixtlcEWH9LR ytJ3z5gpOzDYVUoc0exJIvaN4aidw1fG3oN38JVsLxouyECerPM0LK1lGXMpzWXTRcT9 NeCw== X-Gm-Message-State: AOJu0YwwMBPZTIIoM/P5/aI+SL+ArZH7E/tx/ATZbEXcohYHGMi0lgBz uB9RaeiRerAK8Y4WSCSOQnE= X-Received: by 2002:a05:6000:549:b0:331:4e4e:cb63 with SMTP id b9-20020a056000054900b003314e4ecb63mr3779446wrf.28.1699980267179; Tue, 14 Nov 2023 08:44:27 -0800 (PST) Received: from localhost.localdomain ([46.248.82.114]) by smtp.gmail.com with ESMTPSA id d1-20020a5d6441000000b0032f7f4d008dsm8270755wrw.20.2023.11.14.08.44.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Nov 2023 08:44:26 -0800 (PST) From: Uros Bizjak <ubizjak@gmail.com> To: x86@kernel.org, linux-kernel@vger.kernel.org Cc: Uros Bizjak <ubizjak@gmail.com>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, "H. Peter Anvin" <hpa@zytor.com>, "Peter Zijlstra (Intel)" <peterz@infradead.org> Subject: [PATCH] x86/smp: Use atomic_try_cmpxchg() to micro-optimize native_stop_other_cpus() Date: Tue, 14 Nov 2023 17:43:46 +0100 Message-ID: <20231114164416.208285-1-ubizjak@gmail.com> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Tue, 14 Nov 2023 08:44:46 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1782558532065766351 X-GMAIL-MSGID: 1782558532065766351 |
Series |
x86/smp: Use atomic_try_cmpxchg() to micro-optimize native_stop_other_cpus()
|
|
Commit Message
Uros Bizjak
Nov. 14, 2023, 4:43 p.m. UTC
Use atomic_try_cmpxchg() instead of atomic_cmpxchg(*ptr, old, new) == old
in native_stop_other_cpus(). On x86 the CMPXCHG instruction returns success
in the ZF flag, so this change saves a compare after CMPXCHG. Together
with a small code reorder, the generated asm code improves from:
74: 8b 05 00 00 00 00 mov 0x0(%rip),%eax
7a: 41 54 push %r12
7c: 55 push %rbp
7d: 65 8b 2d 00 00 00 00 mov %gs:0x0(%rip),%ebp
84: 53 push %rbx
85: 85 c0 test %eax,%eax
87: 75 71 jne fa <native_stop_other_cpus+0x8a>
89: b8 ff ff ff ff mov $0xffffffff,%eax
8e: f0 0f b1 2d 00 00 00 lock cmpxchg %ebp,0x0(%rip)
95: 00
96: 83 f8 ff cmp $0xffffffff,%eax
99: 75 5f jne fa <native_stop_other_cpus+0x8a>
to:
74: 8b 05 00 00 00 00 mov 0x0(%rip),%eax
7a: 85 c0 test %eax,%eax
7c: 0f 85 84 00 00 00 jne 106 <native_stop_other_cpus+0x96>
82: 41 54 push %r12
84: b8 ff ff ff ff mov $0xffffffff,%eax
89: 55 push %rbp
8a: 53 push %rbx
8b: 65 8b 1d 00 00 00 00 mov %gs:0x0(%rip),%ebx
92: f0 0f b1 1d 00 00 00 lock cmpxchg %ebx,0x0(%rip)
99: 00
9a: 75 5e jne fa <native_stop_other_cpus+0x8a>
Please note early exit and lack of CMP after CMPXCHG.
No functional change intended.
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
arch/x86/kernel/smp.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
Comments
On 11/14/23 08:43, Uros Bizjak wrote: > Use atomic_try_cmpxchg() instead of atomic_cmpxchg(*ptr, old, new) == old > in native_stop_other_cpus(). On x86 the CMPXCHG instruction returns success > in the ZF flag, so this change saves a compare after CMPXCHG. Together > with a small code reorder, the generated asm code improves from: > > 74: 8b 05 00 00 00 00 mov 0x0(%rip),%eax > 7a: 41 54 push %r12 > 7c: 55 push %rbp > 7d: 65 8b 2d 00 00 00 00 mov %gs:0x0(%rip),%ebp > 84: 53 push %rbx > 85: 85 c0 test %eax,%eax > 87: 75 71 jne fa <native_stop_other_cpus+0x8a> > 89: b8 ff ff ff ff mov $0xffffffff,%eax > 8e: f0 0f b1 2d 00 00 00 lock cmpxchg %ebp,0x0(%rip) > 95: 00 > 96: 83 f8 ff cmp $0xffffffff,%eax > 99: 75 5f jne fa <native_stop_other_cpus+0x8a> > > to: > > 74: 8b 05 00 00 00 00 mov 0x0(%rip),%eax > 7a: 85 c0 test %eax,%eax > 7c: 0f 85 84 00 00 00 jne 106 <native_stop_other_cpus+0x96> > 82: 41 54 push %r12 > 84: b8 ff ff ff ff mov $0xffffffff,%eax > 89: 55 push %rbp > 8a: 53 push %rbx > 8b: 65 8b 1d 00 00 00 00 mov %gs:0x0(%rip),%ebx > 92: f0 0f b1 1d 00 00 00 lock cmpxchg %ebx,0x0(%rip) > 99: 00 > 9a: 75 5e jne fa <native_stop_other_cpus+0x8a> > > Please note early exit and lack of CMP after CMPXCHG. Uros, I really do appreciate that you are trying to optimize these paths. But the thing we have to balance is the _need_ for optimization with the chance that this will break something. This is about as much of a slow path as we have in the kernel. It's only used at shutdown, right? That means this is one of the places in the kernel that least needs optimization. It can only possibly get run once per boot. So, the benefit is that it might make this code a few cycles faster. In practice, it might not even be measurably faster. On the other hand, this is relatively untested and also makes the C code more complicated. Is there some other side benefit that I'm missing here? Applying this patch doesn't seem to have a great risk/reward ratio.
On Fri, Nov 17, 2023 at 5:18 PM Dave Hansen <dave.hansen@intel.com> wrote: > > On 11/14/23 08:43, Uros Bizjak wrote: > > Use atomic_try_cmpxchg() instead of atomic_cmpxchg(*ptr, old, new) == old > > in native_stop_other_cpus(). On x86 the CMPXCHG instruction returns success > > in the ZF flag, so this change saves a compare after CMPXCHG. Together > > with a small code reorder, the generated asm code improves from: > > > > 74: 8b 05 00 00 00 00 mov 0x0(%rip),%eax > > 7a: 41 54 push %r12 > > 7c: 55 push %rbp > > 7d: 65 8b 2d 00 00 00 00 mov %gs:0x0(%rip),%ebp > > 84: 53 push %rbx > > 85: 85 c0 test %eax,%eax > > 87: 75 71 jne fa <native_stop_other_cpus+0x8a> > > 89: b8 ff ff ff ff mov $0xffffffff,%eax > > 8e: f0 0f b1 2d 00 00 00 lock cmpxchg %ebp,0x0(%rip) > > 95: 00 > > 96: 83 f8 ff cmp $0xffffffff,%eax > > 99: 75 5f jne fa <native_stop_other_cpus+0x8a> > > > > to: > > > > 74: 8b 05 00 00 00 00 mov 0x0(%rip),%eax > > 7a: 85 c0 test %eax,%eax > > 7c: 0f 85 84 00 00 00 jne 106 <native_stop_other_cpus+0x96> > > 82: 41 54 push %r12 > > 84: b8 ff ff ff ff mov $0xffffffff,%eax > > 89: 55 push %rbp > > 8a: 53 push %rbx > > 8b: 65 8b 1d 00 00 00 00 mov %gs:0x0(%rip),%ebx > > 92: f0 0f b1 1d 00 00 00 lock cmpxchg %ebx,0x0(%rip) > > 99: 00 > > 9a: 75 5e jne fa <native_stop_other_cpus+0x8a> > > > > Please note early exit and lack of CMP after CMPXCHG. > > Uros, I really do appreciate that you are trying to optimize these > paths. But the thing we have to balance is the _need_ for optimization > with the chance that this will break something. > > This is about as much of a slow path as we have in the kernel. It's > only used at shutdown, right? That means this is one of the places in > the kernel that least needs optimization. It can only possibly get run > once per boot. > > So, the benefit is that it might make this code a few cycles faster. In > practice, it might not even be measurably faster. > > On the other hand, this is relatively untested and also makes the C code > more complicated. > > Is there some other side benefit that I'm missing here? Applying this > patch doesn't seem to have a great risk/reward ratio. Yes, in addition to better asm code, I think that the use of magic constant (-1) is not descriptive at all. I tried to make this code look like nmi_panic() from kernel/panic.c, which has similar functionality, and describe that this constant belongs to old_cpu (same as in nmi_panic() ). Also, from converting many cmpxchg to try_cmpxchg, it becomes evident that in cases like this (usage in "if" clauses) the correct locking primitive is try_cmpxchg. Additionally, in this particular case, it is not the speed, but a little code save that can be achieved with the same functionality. Thanks, Uros.
On 11/17/23 08:37, Uros Bizjak wrote: > On Fri, Nov 17, 2023 at 5:18 PM Dave Hansen <dave.hansen@intel.com> wrote: >> Is there some other side benefit that I'm missing here? Applying this >> patch doesn't seem to have a great risk/reward ratio. > > Yes, in addition to better asm code, I think that the use of magic > constant (-1) is not descriptive at all. I tried to make this code > look like nmi_panic() from kernel/panic.c, which has similar > functionality, and describe that this constant belongs to old_cpu > (same as in nmi_panic() ). I guess it's a step in the direction of nmi_panic(). But honestly, it doesn't go far enough for me at least. The nice part about nmi_panic() is that it gives -1 nice symbolic name and uses that name in the definition of the atomic_t. > Also, from converting many cmpxchg to try_cmpxchg, it becomes evident > that in cases like this (usage in "if" clauses) the correct locking > primitive is try_cmpxchg. Additionally, in this particular case, it > is not the speed, but a little code save that can be achieved with > the same functionality. I think I understand what you're trying to say: using try_cmpxchg can result in better code generation in general than plain cmpxchg. Thus, it's more "correct" to use try_cmpxchg in any case where plain cmpxchg is in use. Right? I honestly don't think cmpxchg is more or less "correct" than try_cmpxchg. If you're going to be sending patches like these, I'd remove this kind of language from your changelogs and discussions because it holds zero weight. Here's what I'm afraid of: this patch is not tested enough. We apply it and then start getting reports of reboot breaking on some server. Someone spends two hours bisecting to this patch. We'll wonder after the fact: Was this patch worth it? I don't think what you have here is more descriptive than what was there before. It still has a -1 and still doesn't logically connect it to the 'stopping_cpu' definition. I have no idea how much this was tested. It doesn't _clearly_ move the needle enough on making the code better to apply it. We shouldn't be blindly converting cmpxchg=>try_cmpxchg, and then trying to justify it after the fact. I _do_ agree that try_cmpxchg() leads to better looking C code *AND* generated code. So, for new x86 code, it seems like the best thing to do. But for old (working) code, I think it should mostly be left alone. Maybe you could keep an eye on: > https://lore.kernel.org/lkml/?q=dfb%3Aatomic_cmpxchg+-dfb%3Aatomic_try_cmpxchg and remind folks what they should be using, rather than expending efforts on trying to move existing code over.
On Fri, Nov 17, 2023 at 6:23 PM Dave Hansen <dave.hansen@intel.com> wrote: > > On 11/17/23 08:37, Uros Bizjak wrote: > > On Fri, Nov 17, 2023 at 5:18 PM Dave Hansen <dave.hansen@intel.com> wrote: > >> Is there some other side benefit that I'm missing here? Applying this > >> patch doesn't seem to have a great risk/reward ratio. > > > > Yes, in addition to better asm code, I think that the use of magic > > constant (-1) is not descriptive at all. I tried to make this code > > look like nmi_panic() from kernel/panic.c, which has similar > > functionality, and describe that this constant belongs to old_cpu > > (same as in nmi_panic() ). > > I guess it's a step in the direction of nmi_panic(). But honestly, it > doesn't go far enough for me at least. The nice part about nmi_panic() > is that it gives -1 nice symbolic name and uses that name in the > definition of the atomic_t. > > > Also, from converting many cmpxchg to try_cmpxchg, it becomes evident > > that in cases like this (usage in "if" clauses) the correct locking > > primitive is try_cmpxchg. Additionally, in this particular case, it > > is not the speed, but a little code save that can be achieved with > > the same functionality. > > I think I understand what you're trying to say: using try_cmpxchg can > result in better code generation in general than plain cmpxchg. Thus, > it's more "correct" to use try_cmpxchg in any case where plain cmpxchg > is in use. Right? Yes, when we have the condition "cmpxchg(*ptr, old, new) == old", then using try_cmpxchg not only generates better code (note also how the compiler creates fast path through the code due to likely/unlikely annotations), but is also *less* error prone. E.g., there were a couple of instances where the result of cmpxchg was compared to the wrong variable. > I honestly don't think cmpxchg is more or less "correct" than > try_cmpxchg. If you're going to be sending patches like these, I'd > remove this kind of language from your changelogs and discussions > because it holds zero weight. > > Here's what I'm afraid of: this patch is not tested enough. We apply it > and then start getting reports of reboot breaking on some server. > Someone spends two hours bisecting to this patch. We'll wonder after > the fact: Was this patch worth it? Let me just say that after some 50 conversions of code of various complexity to try_cmpxchg, fixing inconsistencies and plain logic bugs on the way, removing superfluous memory reads form the loops, eyeballing generated asm code and persuading relevant maintainers about the benefit of the conversion, I can confidently say that this particular conversion won't make troubles. Even without the proposed conversion, the call to smp_processor_id() should be put after early exit where reboot_force is checked. > I don't think what you have here is more descriptive than what was there > before. It still has a -1 and still doesn't logically connect it to the > 'stopping_cpu' definition. I have no idea how much this was tested. It > doesn't _clearly_ move the needle enough on making the code better to > apply it. I should state that I test my patches by bootstrapping the image in qemu, and from time to time put together a bunch of patches and build a real Fedora kernel rpm and run it for some time as my main kernel. This last part gives me much confidence when the patch is discussed with the maintainer. In this particular case, I didn't put much attention on reboot functionality, but the patched kernel definitely reboots without problems. Regarding "-1", I was thinking of introducing a CPU_INVALID #define instead of -1, but at the end, this #define should be eventually introduced as a target-independent patch that puts the new define into the generic part of the kernel source, since it looks like other targets could use this define as well. > We shouldn't be blindly converting cmpxchg=>try_cmpxchg, and then trying > to justify it after the fact. I _do_ agree that try_cmpxchg() leads to > better looking C code *AND* generated code. So, for new x86 code, it > seems like the best thing to do. But for old (working) code, I think it > should mostly be left alone. I'm not here to discuss policies, but the "don't fix it if it ain't broke" policy is a slippery slope, as it can lead to stagnation in the long term. Please read what happened here [1]. [1] https://lwn.net/Articles/488847/ > Maybe you could keep an eye on: > > > https://lore.kernel.org/lkml/?q=dfb%3Aatomic_cmpxchg+-dfb%3Aatomic_try_cmpxchg > > and remind folks what they should be using, rather than expending > efforts on trying to move existing code over. Yes, I'm doing my best to point out optimization opportunities involving try_cmpxchg when I come across one. But this one slipped below my radar... Thanks and BR, Uros.
On 11/22/23 12:18, Uros Bizjak wrote: > I'm not here to discuss policies, but the "don't fix it if it ain't > broke" policy is a slippery slope, as it can lead to stagnation in the > long term. Please read what happened here [1]. > > [1] https://lwn.net/Articles/488847/ Even though I'm not dying to apply this patch, I will take a look at it again if it improves in how its changelog is phrased, actually makes the code more readable and comes with some kind of statement about how thoroughly it was tested.
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c index 96a771f9f930..2908e063d7d8 100644 --- a/arch/x86/kernel/smp.c +++ b/arch/x86/kernel/smp.c @@ -148,14 +148,16 @@ static int register_stop_handler(void) static void native_stop_other_cpus(int wait) { - unsigned int cpu = smp_processor_id(); + unsigned int old_cpu, this_cpu; unsigned long flags, timeout; if (reboot_force) return; /* Only proceed if this is the first CPU to reach this code */ - if (atomic_cmpxchg(&stopping_cpu, -1, cpu) != -1) + old_cpu = -1; + this_cpu = smp_processor_id(); + if (!atomic_try_cmpxchg(&stopping_cpu, &old_cpu, this_cpu)) return; /* For kexec, ensure that offline CPUs are out of MWAIT and in HLT */ @@ -186,7 +188,7 @@ static void native_stop_other_cpus(int wait) * NMIs. */ cpumask_copy(&cpus_stop_mask, cpu_online_mask); - cpumask_clear_cpu(cpu, &cpus_stop_mask); + cpumask_clear_cpu(this_cpu, &cpus_stop_mask); if (!cpumask_empty(&cpus_stop_mask)) { apic_send_IPI_allbutself(REBOOT_VECTOR); @@ -210,6 +212,8 @@ static void native_stop_other_cpus(int wait) * CPUs to stop. */ if (!smp_no_nmi_ipi && !register_stop_handler()) { + unsigned int cpu; + pr_emerg("Shutting down cpus with NMI\n"); for_each_cpu(cpu, &cpus_stop_mask)