From: cuigaosheng <cuigaosheng1@huawei.com>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
<dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
<puwen@hygon.cn>, <TonyWWang-oc@zhaoxin.com>,
<peterz@infradead.org>, <gregkh@linuxfoundation.org>,
<andrew.cooper3@citrix.com>, <tony.luck@intel.com>,
<mario.limonciello@amd.com>, <pawan.kumar.gupta@linux.intel.com>,
<chenyi.qiang@intel.com>, <rdunlap@infradead.org>,
<jithu.joseph@intel.com>, <rafael.j.wysocki@intel.com>,
<paulmck@kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] x86/cpu: replacing the open-coded shift with BIT(x)
Date: Tue, 1 Nov 2022 21:37:02 +0800 [thread overview]
Message-ID: <eed4512b-ac6e-bcec-2f66-7ab45aef9be9@huawei.com> (raw)
In-Reply-To: <Y2EEY63DnDodJFoz@zx2c4.com>
> Others might feel differently and that's fine, but I always found the
> BIT() thing so much less clear than doing 1<<n, which is not only a
> pattern that I recognize as builtin to my brain, but also provides a
> direct description of what's happening, "shift a 1 over n times",
> leaving no off-by-one ambiguity about it. If anything I'd like to see
> the BIT() macro expanded throughout and then removed entirely.
>
> Probably just me though. You can safely ignore my opinion.
Thanks for taking time to review the patch, I submit the patch to remove
the UBSAN warning, even it's not a bug, for example, when I am testing the
kernel, I get some logs as follows, maybe it's better to avoid this?
> [ 0.951719][ T0]
> ================================================================================
> 215 [ 0.953146][ T0] UBSAN: shift-out-of-bounds in mm/shmem.c:3749:18
> 216 [ 0.953863][ T0] left shift of 1 by 31 places cannot be
> represented in type 'int' 217 [ 0.955067][ T0] CPU: 0 PID: 0 Comm:
> swapper/0 Not tainted 6.1.0-rc2-00062-ga970174d7a10 #5 218 [
> 0.956400][ T0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014 219 [
> 0.958278][ T0] Call Trace: 220 [ 0.958777][ T0] <TASK> 221 [
> 0.959224][ T0] dump_stack_lvl+0x8d/0xcf 222 [ 0.959922][ T0]
> ubsan_epilogue+0xa/0x44 223 [ 0.960599][ T0]
> __ubsan_handle_shift_out_of_bounds+0x1e7/0x208 224 [ 0.961575][ T0] ?
> __kmem_cache_alloc_node+0x167/0x290 225 [ 0.962434][ T0] ?
> shmem_fill_super+0x2e/0x2e0 226 [ 0.963187][ T0] ?
> rcu_read_lock_held_common+0x9/0x40 227 [ 0.963857][ T0] ?
> shmem_alloc_hugefolio+0x110/0x110 228 [ 0.963857][ T0] ?
> shmem_fill_super+0x2cc/0x2e0 229 [ 0.963857][ T0]
> shmem_fill_super+0x2cc/0x2e0 230 [ 0.963857][ T0]
> vfs_get_super+0x78/0x160 231 [ 0.963857][ T0] vfs_get_tree+0x28/0x100
> 232 [ 0.963857][ T0] fc_mount+0x12/0x60 233 [ 0.963857][ T0]
> vfs_kern_mount.part.38+0xa5/0xc0 234 [ 0.963857][ T0]
> kern_mount+0x2e/0x60 235 [ 0.963857][ T0] shmem_init+0x63/0xef 236 [
> 0.963857][ T0] mnt_init+0x159/0x2e0 237 [ 0.963857][ T0] ?
> trace_init_perf_perm_irq_work_exit+0xe/0xe 238 [ 0.963857][ T0]
> vfs_caches_init+0xd4/0xde 239 [ 0.963857][ T0]
> start_kernel+0x837/0x8a4 240 [ 0.963857][ T0]
> secondary_startup_64_no_verify+0xce/0xdb 241 [ 0.963857][ T0] </TASK>
> 242 [ 0.963860][ T0]
> ================================================================================
> 243 [ 0.965288][ T0] Kernel panic - not syncing: panic_on_warn set ...
> 244 [ 0.966299][ T0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> 6.1.0-rc2-00062-ga970174d7a10 #5 245 [ 0.967645][ T0] Hardware name:
> QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> rel-1.13.0-48-gd9c812dda519-prebuilt.qemu.org 04/01/2014 246 [
> 0.969548][ T0] Call Trace: 247 [ 0.970050][ T0] <TASK> 248 [
> 0.970499][ T0] dump_stack_lvl+0x8d/0xcf 249 [ 0.971195][ T0]
> panic+0x182/0x387 250 [ 0.971797][ T0] ? ubsan_epilogue+0x33/0x44 251
> [ 0.972539][ T0] ubsan_epilogue+0x3f/0x44 252 [ 0.973237][ T0]
> __ubsan_handle_shift_out_of_bounds+0x1e7/0x208 253 [ 0.973857][ T0] ?
> __kmem_cache_alloc_node+0x167/0x290 254 [ 0.973857][ T0] ?
> shmem_fill_super+0x2e/0x2e0 255 [ 0.973857][ T0] ?
> rcu_read_lock_held_common+0x9/0x40 256 [ 0.973857][ T0] ?
> shmem_alloc_hugefolio+0x110/0x110 257 [ 0.973857][ T0] ?
> shmem_fill_super+0x2cc/0x2e0 258 [ 0.973857][ T0]
> shmem_fill_super+0x2cc/0x2e0 259 [ 0.973857][ T0]
> vfs_get_super+0x78/0x160 260 [ 0.973857][ T0] vfs_get_tree+0x28/0x100
> 261 [ 0.973857][ T0] fc_mount+0x12/0x60 262 [ 0.973857][ T0]
> vfs_kern_mount.part.38+0xa5/0xc0 263 [ 0.973857][ T0]
> kern_mount+0x2e/0x60 264 [ 0.973857][ T0] shmem_init+0x63/0xef 265 [
> 0.973857][ T0] mnt_init+0x159/0x2e0 266 [ 0.973857][ T0] ?
> trace_init_perf_perm_irq_work_exit+0xe/0xe 267 [ 0.973857][ T0]
> vfs_caches_init+0xd4/0xde 268 [ 0.973857][ T0]
> start_kernel+0x837/0x8a4 269 [ 0.973857][ T0]
> secondary_startup_64_no_verify+0xce/0xdb 270 [ 0.973857][ T0] </TASK>
> 271 [ 0.973857][ T0] Rebooting in 86400 seconds..
On 2022/11/1 19:34, Jason A. Donenfeld wrote:
> Others might feel differently and that's fine, but I always found the
> BIT() thing so much less clear than doing 1<<n, which is not only a
> pattern that I recognize as builtin to my brain, but also provides a
> direct description of what's happening, "shift a 1 over n times",
> leaving no off-by-one ambiguity about it. If anything I'd like to see
> the BIT() macro expanded throughout and then removed entirely.
>
> Probably just me though. You can safely ignore my opinion:).
next prev parent reply other threads:[~2022-11-01 13:37 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-01 11:14 Gaosheng Cui
2022-11-01 11:34 ` Jason A. Donenfeld
2022-11-01 13:37 ` cuigaosheng [this message]
2022-11-01 13:43 ` Jason A. Donenfeld
2022-11-01 14:05 ` cuigaosheng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=eed4512b-ac6e-bcec-2f66-7ab45aef9be9@huawei.com \
--to=cuigaosheng1@huawei.com \
--cc=Jason@zx2c4.com \
--cc=TonyWWang-oc@zhaoxin.com \
--cc=andrew.cooper3@citrix.com \
--cc=bp@alien8.de \
--cc=chenyi.qiang@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=hpa@zytor.com \
--cc=jithu.joseph@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mario.limonciello@amd.com \
--cc=mingo@redhat.com \
--cc=paulmck@kernel.org \
--cc=pawan.kumar.gupta@linux.intel.com \
--cc=peterz@infradead.org \
--cc=puwen@hygon.cn \
--cc=rafael.j.wysocki@intel.com \
--cc=rdunlap@infradead.org \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®