Are jump labels broken on 6.11-rc1?

Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

Hi everyone,

I got the following splat on 6.11-rc1 when I tried to QA xfs online
fsck.  Does this ring a bell for anyone?  I'll try bisecting in the
morning to see if I can find the culprit.

[  110.854792] run fstests xfs/286 at 2024-07-29 17:39:44
[  113.323388] XFS (sda4): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
[  113.326183] XFS (sda4): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
[  113.330249] XFS (sda4): Mounting V5 Filesystem 6ae91845-a649-4f4b-a05c-211c6570beb9
[  113.366071] XFS (sda4): Ending clean mount
[  113.378055] XFS (sda4): EXPERIMENTAL online scrub feature in use. Use at your own risk!
[  117.051754] jump_label: Fatal kernel bug, unexpected op at xfs_dir_update_hook+0x14/0x60 [xfs] [ffffffffa05b8bd4] (eb 15 48 8b 44 != 66 90 0f 1f 00)) size:2 type:1
[  117.063445] ------------[ cut here ]------------
[  117.065515] kernel BUG at arch/x86/kernel/jump_label.c:73!
[  117.068132] Oops: invalid opcode: 0000 [#1] PREEMPT SMP
[  117.070593] CPU: 1 UID: 0 PID: 7088 Comm: xfs_scrub Not tainted 6.11.0-rc1-djwx #rc1 6a5cf31730abd495745b329a08aeb665e92a9b62
[  117.074096] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20171121_152543-x86-ol7-builder-01.us.oracle.com-4.el7.1 04/01/2014
[  117.077176] RIP: 0010:__jump_label_patch+0x10a/0x110
[  117.078427] Code: eb a0 0f 0b 0f 0b 48 c7 c3 a4 8a 7b 82 41 56 45 89 e1 49 89 d8 4c 89 e9 4c 89 ea 4c 89 ee 48 c7 c7 48 8a e7 81 e8 d6 db 0d 00 <0f> 0b 0f 1f 40 00 0f 1f 44 00 00 e9 56 66 93 00 66 0f 1f 44 00 00
[  117.082916] RSP: 0018:ffffc90006873bc8 EFLAGS: 00010246
[  117.084129] RAX: 0000000000000097 RBX: ffffffff81c08901 RCX: 0000000000000000
[  117.085841] RDX: 0000000000000000 RSI: ffffffff81eacf51 RDI: 00000000ffffffff
[  117.087517] RBP: ffffc90006873bf8 R08: 0000000000000000 R09: 205d343537313530
[  117.089177] R10: 61746146203a6c65 R11: 62616c5f706d756a R12: 0000000000000002
[  117.090976] R13: ffffffffa05b8bd4 R14: 0000000000000001 R15: 0000001b3f9aa79f
[  117.101135] FS:  00007ff50d200680(0000) GS:ffff88842d080000(0000) knlGS:0000000000000000
[  117.111084] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  117.112730] CR2: 0000563357158958 CR3: 000000010440c000 CR4: 00000000003506f0
[  117.114567] Call Trace:
[  117.115153]  <TASK>
[  117.115674]  ? die+0x32/0x80
[  117.116462]  ? do_trap+0xd4/0x100
[  117.117281]  ? do_error_trap+0x65/0x80
[  117.118387]  ? __jump_label_patch+0x10a/0x110
[  117.119483]  ? exc_invalid_op+0x4c/0x60
[  117.120497]  ? __jump_label_patch+0x10a/0x110
[  117.121563]  ? asm_exc_invalid_op+0x16/0x20
[  117.122535]  ? xfs_dir_update_hook+0x14/0x60 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
[  117.124669]  ? __jump_label_patch+0x10a/0x110
[  117.125696]  ? __jump_label_patch+0x10a/0x110
[  117.126713]  arch_jump_label_transform_queue+0x33/0x70
[  117.128028]  __jump_label_update+0x6e/0x130
[  117.129034]  __static_key_slow_dec_cpuslocked.part.0+0x2c/0x50
[  117.130411]  static_key_slow_dec+0x3e/0x60
[  117.131398]  xchk_teardown+0x1a2/0x1d0 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
[  117.133649]  xfs_scrub_metadata+0x448/0x5c0 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
[  117.135869]  xfs_ioc_scrubv_metadata+0x389/0x550 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
[  117.138220]  xfs_file_ioctl+0x8f0/0xe80 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
[  117.140398]  ? inode_needs_update_time+0x4b/0xc0
[  117.141438]  ? preempt_count_add+0x4a/0xa0
[  117.142381]  ? up_write+0x64/0x180
[  117.143233]  ? shmem_file_write_iter+0x5a/0x90
[  117.144239]  ? preempt_count_add+0x4a/0xa0
[  117.145252]  ? vfs_write+0x3a2/0x4a0
[  117.146107]  __x64_sys_ioctl+0x8a/0xb0
[  117.146974]  do_syscall_64+0x47/0x100
[  117.147835]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
[  117.148796] RIP: 0033:0x7ff50f71ec5b
[  117.149753] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00
[  117.154106] RSP: 002b:00007ff50d1ff4c0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[  117.155900] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007ff50f71ec5b
[  117.157576] RDX: 00007ff50d1ff610 RSI: 00000000c0285840 RDI: 0000000000000004
[  117.159316] RBP: 00007ff50d1ff610 R08: 0000000000000000 R09: 0000000000000064
[  117.161058] R10: 00007ff50d1ff235 R11: 0000000000000246 R12: 00007ffc370a5ea0
[  117.162767] R13: 000000000000001d R14: 00007ffc370a6048 R15: 00007ff4fc005470
[  117.164490]  </TASK>
[  117.165083] Modules linked in: xfs rpcsec_gss_krb5 auth_rpcgss nft_chain_nat xt_REDIRECT nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_tcpudp ip_set_hash_ip ip_set_hash_net xt_set nft_compat ip_set_hash_mac ip_set nf_tables libcrc32c nfnetlink bfq sha512_ssse3 pvpanic_mmio sha512_generic pvpanic sha256_ssse3 sch_fq_codel fuse configfs ip_tables x_tables overlay nfsv4 af_packet
[  117.174091] Dumping ftrace buffer:
[  117.174888]    (ftrace buffer empty)
[  117.175904] ---[ end trace 0000000000000000 ]---
[  117.177523] RIP: 0010:__jump_label_patch+0x10a/0x110
[  117.178763] Code: eb a0 0f 0b 0f 0b 48 c7 c3 a4 8a 7b 82 41 56 45 89 e1 49 89 d8 4c 89 e9 4c 89 ea 4c 89 ee 48 c7 c7 48 8a e7 81 e8 d6 db 0d 00 <0f> 0b 0f 1f 40 00 0f 1f 44 00 00 e9 56 66 93 00 66 0f 1f 44 00 00
[  117.183181] RSP: 0018:ffffc90006873bc8 EFLAGS: 00010246
[  117.184298] RAX: 0000000000000097 RBX: ffffffff81c08901 RCX: 0000000000000000
[  117.185948] RDX: 0000000000000000 RSI: ffffffff81eacf51 RDI: 00000000ffffffff
[  117.187865] RBP: ffffc90006873bf8 R08: 0000000000000000 R09: 205d343537313530
[  117.195190] R10: 61746146203a6c65 R11: 62616c5f706d756a R12: 0000000000000002
[  117.201641] R13: ffffffffa05b8bd4 R14: 0000000000000001 R15: 0000001b3f9aa79f
[  117.205179] FS:  00007ff50d200680(0000) GS:ffff88842d000000(0000) knlGS:0000000000000000
[  117.216723] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  117.220417] CR2: 00007f7d9bb54010 CR3: 000000010440c000 CR4: 00000000003506f0

--Darrick

Re: Are jump labels broken on 6.11-rc1?

[Chandan Babu R]

On Mon, Jul 29, 2024 at 08:38:49 PM -0700, Darrick J. Wong wrote:
> Hi everyone,
>
> I got the following splat on 6.11-rc1 when I tried to QA xfs online
> fsck.  Does this ring a bell for anyone?  I'll try bisecting in the
> morning to see if I can find the culprit.

xfs/566 on v6.11-rc1 would consistently cause the oops mentioned below.
However, I was able to get xfs/566 to successfully execute for five times on a
v6.11-rc1 kernel with the following commits reverted,

83ab38ef0a0b2407d43af9575bb32333fdd74fb2
695ef796467ed228b60f1915995e390aea3d85c6
9bc2ff871f00437ad2f10c1eceff51aaa72b478f

Reinstating commit 83ab38ef0a0b2407d43af9575bb32333fdd74fb2 causes the kernel
to oops once again.

>
> [  110.854792] run fstests xfs/286 at 2024-07-29 17:39:44
> [  113.323388] XFS (sda4): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
> [  113.326183] XFS (sda4): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
> [  113.330249] XFS (sda4): Mounting V5 Filesystem 6ae91845-a649-4f4b-a05c-211c6570beb9
> [  113.366071] XFS (sda4): Ending clean mount
> [  113.378055] XFS (sda4): EXPERIMENTAL online scrub feature in use. Use at your own risk!
> [  117.051754] jump_label: Fatal kernel bug, unexpected op at xfs_dir_update_hook+0x14/0x60 [xfs] [ffffffffa05b8bd4] (eb 15 48 8b 44 != 66 90 0f 1f 00)) size:2 type:1
> [  117.063445] ------------[ cut here ]------------
> [  117.065515] kernel BUG at arch/x86/kernel/jump_label.c:73!
> [  117.068132] Oops: invalid opcode: 0000 [#1] PREEMPT SMP
> [  117.070593] CPU: 1 UID: 0 PID: 7088 Comm: xfs_scrub Not tainted 6.11.0-rc1-djwx #rc1 6a5cf31730abd495745b329a08aeb665e92a9b62
> [  117.074096] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20171121_152543-x86-ol7-builder-01.us.oracle.com-4.el7.1 04/01/2014
> [  117.077176] RIP: 0010:__jump_label_patch+0x10a/0x110
> [ 117.078427] Code: eb a0 0f 0b 0f 0b 48 c7 c3 a4 8a 7b 82 41 56 45 89
> e1 49 89 d8 4c 89 e9 4c 89 ea 4c 89 ee 48 c7 c7 48 8a e7 81 e8 d6 db
> 0d 00 <0f> 0b 0f 1f 40 00 0f 1f 44 00 00 e9 56 66 93 00 66 0f 1f 44 00
> 00
> [  117.082916] RSP: 0018:ffffc90006873bc8 EFLAGS: 00010246
> [  117.084129] RAX: 0000000000000097 RBX: ffffffff81c08901 RCX: 0000000000000000
> [  117.085841] RDX: 0000000000000000 RSI: ffffffff81eacf51 RDI: 00000000ffffffff
> [  117.087517] RBP: ffffc90006873bf8 R08: 0000000000000000 R09: 205d343537313530
> [  117.089177] R10: 61746146203a6c65 R11: 62616c5f706d756a R12: 0000000000000002
> [  117.090976] R13: ffffffffa05b8bd4 R14: 0000000000000001 R15: 0000001b3f9aa79f
> [  117.101135] FS:  00007ff50d200680(0000) GS:ffff88842d080000(0000) knlGS:0000000000000000
> [  117.111084] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  117.112730] CR2: 0000563357158958 CR3: 000000010440c000 CR4: 00000000003506f0
> [  117.114567] Call Trace:
> [  117.115153]  <TASK>
> [  117.115674]  ? die+0x32/0x80
> [  117.116462]  ? do_trap+0xd4/0x100
> [  117.117281]  ? do_error_trap+0x65/0x80
> [  117.118387]  ? __jump_label_patch+0x10a/0x110
> [  117.119483]  ? exc_invalid_op+0x4c/0x60
> [  117.120497]  ? __jump_label_patch+0x10a/0x110
> [  117.121563]  ? asm_exc_invalid_op+0x16/0x20
> [  117.122535]  ? xfs_dir_update_hook+0x14/0x60 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
> [  117.124669]  ? __jump_label_patch+0x10a/0x110
> [  117.125696]  ? __jump_label_patch+0x10a/0x110
> [  117.126713]  arch_jump_label_transform_queue+0x33/0x70
> [  117.128028]  __jump_label_update+0x6e/0x130
> [  117.129034]  __static_key_slow_dec_cpuslocked.part.0+0x2c/0x50
> [  117.130411]  static_key_slow_dec+0x3e/0x60
> [  117.131398]  xchk_teardown+0x1a2/0x1d0 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
> [  117.133649]  xfs_scrub_metadata+0x448/0x5c0 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
> [  117.135869]  xfs_ioc_scrubv_metadata+0x389/0x550 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
> [  117.138220]  xfs_file_ioctl+0x8f0/0xe80 [xfs 0af47fdcb6d4e3620a66423ae12a83496a207bf6]
> [  117.140398]  ? inode_needs_update_time+0x4b/0xc0
> [  117.141438]  ? preempt_count_add+0x4a/0xa0
> [  117.142381]  ? up_write+0x64/0x180
> [  117.143233]  ? shmem_file_write_iter+0x5a/0x90
> [  117.144239]  ? preempt_count_add+0x4a/0xa0
> [  117.145252]  ? vfs_write+0x3a2/0x4a0
> [  117.146107]  __x64_sys_ioctl+0x8a/0xb0
> [  117.146974]  do_syscall_64+0x47/0x100
> [  117.147835]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
> [  117.148796] RIP: 0033:0x7ff50f71ec5b
> [ 117.149753] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10
> 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00
> 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00
> 00
> [  117.154106] RSP: 002b:00007ff50d1ff4c0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [  117.155900] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007ff50f71ec5b
> [  117.157576] RDX: 00007ff50d1ff610 RSI: 00000000c0285840 RDI: 0000000000000004
> [  117.159316] RBP: 00007ff50d1ff610 R08: 0000000000000000 R09: 0000000000000064
> [  117.161058] R10: 00007ff50d1ff235 R11: 0000000000000246 R12: 00007ffc370a5ea0
> [  117.162767] R13: 000000000000001d R14: 00007ffc370a6048 R15: 00007ff4fc005470
> [  117.164490]  </TASK>
> [ 117.165083] Modules linked in: xfs rpcsec_gss_krb5 auth_rpcgss
> nft_chain_nat xt_REDIRECT nf_nat nf_conntrack nf_defrag_ipv6
> nf_defrag_ipv4 ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4
> xt_tcpudp ip_set_hash_ip ip_set_hash_net xt_set nft_compat
> ip_set_hash_mac ip_set nf_tables libcrc32c nfnetlink bfq sha512_ssse3
> pvpanic_mmio sha512_generic pvpanic sha256_ssse3 sch_fq_codel fuse
> configfs ip_tables x_tables overlay nfsv4 af_packet
> [  117.174091] Dumping ftrace buffer:
> [  117.174888]    (ftrace buffer empty)
> [  117.175904] ---[ end trace 0000000000000000 ]---
> [  117.177523] RIP: 0010:__jump_label_patch+0x10a/0x110
> [ 117.178763] Code: eb a0 0f 0b 0f 0b 48 c7 c3 a4 8a 7b 82 41 56 45 89
> e1 49 89 d8 4c 89 e9 4c 89 ea 4c 89 ee 48 c7 c7 48 8a e7 81 e8 d6 db
> 0d 00 <0f> 0b 0f 1f 40 00 0f 1f 44 00 00 e9 56 66 93 00 66 0f 1f 44 00
> 00
> [  117.183181] RSP: 0018:ffffc90006873bc8 EFLAGS: 00010246
> [  117.184298] RAX: 0000000000000097 RBX: ffffffff81c08901 RCX: 0000000000000000
> [  117.185948] RDX: 0000000000000000 RSI: ffffffff81eacf51 RDI: 00000000ffffffff
> [  117.187865] RBP: ffffc90006873bf8 R08: 0000000000000000 R09: 205d343537313530
> [  117.195190] R10: 61746146203a6c65 R11: 62616c5f706d756a R12: 0000000000000002
> [  117.201641] R13: ffffffffa05b8bd4 R14: 0000000000000001 R15: 0000001b3f9aa79f
> [  117.205179] FS:  00007ff50d200680(0000) GS:ffff88842d000000(0000) knlGS:0000000000000000
> [  117.216723] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  117.220417] CR2: 00007f7d9bb54010 CR3: 000000010440c000 CR4: 00000000003506f0

-- 
Chandan

Re: Are jump labels broken on 6.11-rc1?

[Peter Zijlstra]

On Tue, Jul 30, 2024 at 01:00:02PM +0530, Chandan Babu R wrote:
> On Mon, Jul 29, 2024 at 08:38:49 PM -0700, Darrick J. Wong wrote:
> > Hi everyone,
> >
> > I got the following splat on 6.11-rc1 when I tried to QA xfs online
> > fsck.  Does this ring a bell for anyone?  I'll try bisecting in the
> > morning to see if I can find the culprit.
> 
> xfs/566 on v6.11-rc1 would consistently cause the oops mentioned below.
> However, I was able to get xfs/566 to successfully execute for five times on a
> v6.11-rc1 kernel with the following commits reverted,
> 
> 83ab38ef0a0b2407d43af9575bb32333fdd74fb2
> 695ef796467ed228b60f1915995e390aea3d85c6
> 9bc2ff871f00437ad2f10c1eceff51aaa72b478f
> 
> Reinstating commit 83ab38ef0a0b2407d43af9575bb32333fdd74fb2 causes the kernel
> to oops once again.

Durr, does this help?


diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 4ad5ed8adf96..57f70dfa1f3d 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -236,7 +236,7 @@ void static_key_disable_cpuslocked(struct static_key *key)
 	}
 
 	jump_label_lock();
-	if (atomic_cmpxchg(&key->enabled, 1, 0))
+	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
 		jump_label_update(key);
 	jump_label_unlock();
 }

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Tue, Jul 30, 2024 at 03:26:26PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 30, 2024 at 01:00:02PM +0530, Chandan Babu R wrote:
> > On Mon, Jul 29, 2024 at 08:38:49 PM -0700, Darrick J. Wong wrote:
> > > Hi everyone,
> > >
> > > I got the following splat on 6.11-rc1 when I tried to QA xfs online
> > > fsck.  Does this ring a bell for anyone?  I'll try bisecting in the
> > > morning to see if I can find the culprit.
> > 
> > xfs/566 on v6.11-rc1 would consistently cause the oops mentioned below.
> > However, I was able to get xfs/566 to successfully execute for five times on a
> > v6.11-rc1 kernel with the following commits reverted,
> > 
> > 83ab38ef0a0b2407d43af9575bb32333fdd74fb2
> > 695ef796467ed228b60f1915995e390aea3d85c6
> > 9bc2ff871f00437ad2f10c1eceff51aaa72b478f
> > 
> > Reinstating commit 83ab38ef0a0b2407d43af9575bb32333fdd74fb2 causes the kernel
> > to oops once again.
> 
> Durr, does this help?
> 
> 
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 4ad5ed8adf96..57f70dfa1f3d 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -236,7 +236,7 @@ void static_key_disable_cpuslocked(struct static_key *key)
>  	}
>  
>  	jump_label_lock();
> -	if (atomic_cmpxchg(&key->enabled, 1, 0))
> +	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)

Ah, because key->enabled can be -1 sometimes?  I'll throw that at the
fstests cloud for a few hours and report back.

--D

>  		jump_label_update(key);
>  	jump_label_unlock();
>  }
> 

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Tue, Jul 30, 2024 at 03:26:26PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 30, 2024 at 01:00:02PM +0530, Chandan Babu R wrote:
> > On Mon, Jul 29, 2024 at 08:38:49 PM -0700, Darrick J. Wong wrote:
> > > Hi everyone,
> > >
> > > I got the following splat on 6.11-rc1 when I tried to QA xfs online
> > > fsck.  Does this ring a bell for anyone?  I'll try bisecting in the
> > > morning to see if I can find the culprit.
> > 
> > xfs/566 on v6.11-rc1 would consistently cause the oops mentioned below.
> > However, I was able to get xfs/566 to successfully execute for five times on a
> > v6.11-rc1 kernel with the following commits reverted,
> > 
> > 83ab38ef0a0b2407d43af9575bb32333fdd74fb2
> > 695ef796467ed228b60f1915995e390aea3d85c6
> > 9bc2ff871f00437ad2f10c1eceff51aaa72b478f
> > 
> > Reinstating commit 83ab38ef0a0b2407d43af9575bb32333fdd74fb2 causes the kernel
> > to oops once again.
> 
> Durr, does this help?

Yes, it does!  After ~8, a full fstests run completes without incident.

(vs. before where it would blow up within 2 minutes)

Thanks for the fix; you can add
Tested-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 4ad5ed8adf96..57f70dfa1f3d 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -236,7 +236,7 @@ void static_key_disable_cpuslocked(struct static_key *key)
>  	}
>  
>  	jump_label_lock();
> -	if (atomic_cmpxchg(&key->enabled, 1, 0))
> +	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
>  		jump_label_update(key);
>  	jump_label_unlock();
>  }
> 

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Tue, Jul 30, 2024 at 05:19:50PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 30, 2024 at 03:26:26PM +0200, Peter Zijlstra wrote:
> > On Tue, Jul 30, 2024 at 01:00:02PM +0530, Chandan Babu R wrote:
> > > On Mon, Jul 29, 2024 at 08:38:49 PM -0700, Darrick J. Wong wrote:
> > > > Hi everyone,
> > > >
> > > > I got the following splat on 6.11-rc1 when I tried to QA xfs online
> > > > fsck.  Does this ring a bell for anyone?  I'll try bisecting in the
> > > > morning to see if I can find the culprit.
> > > 
> > > xfs/566 on v6.11-rc1 would consistently cause the oops mentioned below.
> > > However, I was able to get xfs/566 to successfully execute for five times on a
> > > v6.11-rc1 kernel with the following commits reverted,
> > > 
> > > 83ab38ef0a0b2407d43af9575bb32333fdd74fb2
> > > 695ef796467ed228b60f1915995e390aea3d85c6
> > > 9bc2ff871f00437ad2f10c1eceff51aaa72b478f
> > > 
> > > Reinstating commit 83ab38ef0a0b2407d43af9575bb32333fdd74fb2 causes the kernel
> > > to oops once again.
> > 
> > Durr, does this help?
> 
> Yes, it does!  After ~8, a full fstests run completes without incident.
> 
> (vs. before where it would blow up within 2 minutes)
> 
> Thanks for the fix; you can add
> Tested-by: Darrick J. Wong <djwong@kernel.org>

Ofc as soon as this I push it to the whole fleet then things start
failing again. :(

> --D
> 
> > 
> > diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> > index 4ad5ed8adf96..57f70dfa1f3d 100644
> > --- a/kernel/jump_label.c
> > +++ b/kernel/jump_label.c
> > @@ -236,7 +236,7 @@ void static_key_disable_cpuslocked(struct static_key *key)
> >  	}
> >  
> >  	jump_label_lock();
> > -	if (atomic_cmpxchg(&key->enabled, 1, 0))
> > +	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
> >  		jump_label_update(key);
> >  	jump_label_unlock();
> >  }
> > 
> 

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Tue, Jul 30, 2024 at 08:10:33PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 30, 2024 at 05:19:50PM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 30, 2024 at 03:26:26PM +0200, Peter Zijlstra wrote:
> > > On Tue, Jul 30, 2024 at 01:00:02PM +0530, Chandan Babu R wrote:
> > > > On Mon, Jul 29, 2024 at 08:38:49 PM -0700, Darrick J. Wong wrote:
> > > > > Hi everyone,
> > > > >
> > > > > I got the following splat on 6.11-rc1 when I tried to QA xfs online
> > > > > fsck.  Does this ring a bell for anyone?  I'll try bisecting in the
> > > > > morning to see if I can find the culprit.
> > > > 
> > > > xfs/566 on v6.11-rc1 would consistently cause the oops mentioned below.
> > > > However, I was able to get xfs/566 to successfully execute for five times on a
> > > > v6.11-rc1 kernel with the following commits reverted,
> > > > 
> > > > 83ab38ef0a0b2407d43af9575bb32333fdd74fb2
> > > > 695ef796467ed228b60f1915995e390aea3d85c6
> > > > 9bc2ff871f00437ad2f10c1eceff51aaa72b478f
> > > > 
> > > > Reinstating commit 83ab38ef0a0b2407d43af9575bb32333fdd74fb2 causes the kernel
> > > > to oops once again.
> > > 
> > > Durr, does this help?
> > 
> > Yes, it does!  After ~8, a full fstests run completes without incident.
> > 
> > (vs. before where it would blow up within 2 minutes)
> > 
> > Thanks for the fix; you can add
> > Tested-by: Darrick J. Wong <djwong@kernel.org>
> 
> Ofc as soon as this I push it to the whole fleet then things start
> failing again. :(

Sooooo... it turns out that somehow your patch got mismerged on the
first go-round, and that worked.  The second time, there was no
mismerge, which mean that the wrong atomic_cmpxchg() callsite was
tested.

Looking back at the mismerge, it actually changed
__static_key_slow_dec_cpuslocked, which had in 6.10:

	if (atomic_dec_and_test(&key->enabled))
		jump_label_update(key);

Decrement, then return true if the value was set to zero.  With the 6.11
code, it looks like we want to exchange a 1 with a 0, and act only if
the previous value had been 1.

So perhaps we really want this change?  I'll send it out to the fleet
and we'll see what it reports tomorrow morning.

--D

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 4ad5ed8adf96..5f80c128e90e 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -289,7 +289,7 @@ static void __static_key_slow_dec_cpuslocked(struct static_key *key)
 		return;
 
 	guard(mutex)(&jump_label_mutex);
-	if (atomic_cmpxchg(&key->enabled, 1, 0))
+	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
 		jump_label_update(key);
 	else
 		WARN_ON_ONCE(!static_key_slow_try_dec(key));

Re: Are jump labels broken on 6.11-rc1?

[Peter Zijlstra]

On Tue, Jul 30, 2024 at 10:33:41PM -0700, Darrick J. Wong wrote:

> Sooooo... it turns out that somehow your patch got mismerged on the
> first go-round, and that worked.  The second time, there was no
> mismerge, which mean that the wrong atomic_cmpxchg() callsite was
> tested.
> 
> Looking back at the mismerge, it actually changed
> __static_key_slow_dec_cpuslocked, which had in 6.10:
> 
> 	if (atomic_dec_and_test(&key->enabled))
> 		jump_label_update(key);
> 
> Decrement, then return true if the value was set to zero.  With the 6.11
> code, it looks like we want to exchange a 1 with a 0, and act only if
> the previous value had been 1.
> 
> So perhaps we really want this change?  I'll send it out to the fleet
> and we'll see what it reports tomorrow morning.

Bah yes, I missed we had it twice. Definitely both sites want this.

I'll tentatively merge the below patch in tip/locking/urgent. I can
rebase if there is need.

---
Subject: jump_label: Fix the fix, brown paper bags galore
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Jul 31 12:43:21 CEST 2024

Per the example of:

  !atomic_cmpxchg(&key->enabled, 0, 1)

the inverse was written as:

  atomic_cmpxchg(&key->enabled, 1, 0)

except of course, that while !old is only true for old == 0, old is
true for everything except old == 0.

Fix it to read:

  atomic_cmpxchg(&key->enabled, 1, 0) == 1

such that only the 1->0 transition returns true and goes on to disable
the keys.

Fixes: 83ab38ef0a0b ("jump_label: Fix concurrency issues in static_key_slow_dec()")
Reported-by: Darrick J. Wong <djwong@kernel.org>
Tested-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/jump_label.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -236,7 +236,7 @@ void static_key_disable_cpuslocked(struc
 	}
 
 	jump_label_lock();
-	if (atomic_cmpxchg(&key->enabled, 1, 0))
+	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
 		jump_label_update(key);
 	jump_label_unlock();
 }
@@ -289,7 +289,7 @@ static void __static_key_slow_dec_cpuslo
 		return;
 
 	guard(mutex)(&jump_label_mutex);
-	if (atomic_cmpxchg(&key->enabled, 1, 0))
+	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
 		jump_label_update(key);
 	else
 		WARN_ON_ONCE(!static_key_slow_try_dec(key));

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Wed, Jul 31, 2024 at 12:55:57PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 30, 2024 at 10:33:41PM -0700, Darrick J. Wong wrote:
> 
> > Sooooo... it turns out that somehow your patch got mismerged on the
> > first go-round, and that worked.  The second time, there was no
> > mismerge, which mean that the wrong atomic_cmpxchg() callsite was
> > tested.
> > 
> > Looking back at the mismerge, it actually changed
> > __static_key_slow_dec_cpuslocked, which had in 6.10:
> > 
> > 	if (atomic_dec_and_test(&key->enabled))
> > 		jump_label_update(key);
> > 
> > Decrement, then return true if the value was set to zero.  With the 6.11
> > code, it looks like we want to exchange a 1 with a 0, and act only if
> > the previous value had been 1.
> > 
> > So perhaps we really want this change?  I'll send it out to the fleet
> > and we'll see what it reports tomorrow morning.
> 
> Bah yes, I missed we had it twice. Definitely both sites want this.
> 
> I'll tentatively merge the below patch in tip/locking/urgent. I can
> rebase if there is need.

Hi Peter,

This morning, I noticed the splat below with -rc2.

WARNING: CPU: 0 PID: 8578 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0x50/0x60

Line 295 is the else branch of this code:

	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
		jump_label_update(key);
	else
		WARN_ON_ONCE(!static_key_slow_try_dec(key));

Apparently static_key_slow_try_dec returned false?  Looking at that
function, I suppose the atomic_read of key->enabled returned 0, since it
didn't trigger the "WARN_ON_ONCE(v < 0)" code.  Does that mean the value
must have dropped from positive N to 0 without anyone ever taking the
jump_label_mutex?

Unfortunately I'm a little too covfid-brained to figure this out today.
:(

--D

[  104.985250] run fstests xfs/1877 at 2024-08-04 15:46:00
[  107.669818] XFS (sda4): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
[  107.672983] XFS (sda4): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
[  107.676466] XFS (sda4): Mounting V5 Filesystem e1e9e96c-0d94-4a7e-8405-86e4d16d331d
[  107.732210] XFS (sda4): Ending clean mount
[  107.744657] XFS (sda4): EXPERIMENTAL online scrub feature in use. Use at your own risk!
[  120.562636] Direct I/O collision with buffered writes! File: d2d4/d50c/d4a2/fe7 Comm: fsstress
[  132.078284] Direct I/O collision with buffered writes! File: d820/d6b9/d6f5/fbcc Comm: fsstress
[  134.261151] Direct I/O collision with buffered writes! File: d13e/d261/d896/fc9e Comm: fsstress
[  172.025695] Direct I/O collision with buffered writes! File: rt/p2/d1/f194c Comm: fsstress
[  238.690588] Direct I/O collision with buffered writes! File: dca1/dd3f/d2346/f26e3 Comm: fsstress
[  374.677179] Direct I/O collision with buffered writes! File: de70/dd75/de74/f2f38 Comm: fsstress
[  399.084085] Direct I/O collision with buffered writes! File: dd50/d1584/d1f9/f1e65 Comm: fsstress
[  547.895266] ------------[ cut here ]------------
[  547.905274] WARNING: CPU: 0 PID: 8578 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0x50/0x60
[  547.914707] Modules linked in: xfs rpcsec_gss_krb5 auth_rpcgss nft_chain_nat xt_REDIRECT nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_tcpudp ip_set_hash_ip ip_set_hash_net xt_set nft_compat ip_set_hash_mac ip_set nf_tables libcrc32c nfnetlink bfq sha512_ssse3 sha512_generic pvpanic_mmio sha256_ssse3 pvpanic sch_fq_codel configfs fuse ip_tables x_tables overlay nfsv4 af_packet
[  547.934623] CPU: 0 UID: 0 PID: 8578 Comm: xfs_scrub Not tainted 6.11.0-rc2-djwx #rc2 d9817e54d8a35981d261570492175bf5e1b3bc11
[  547.941392] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20171121_152543-x86-ol7-builder-01.us.oracle.com-4.el7.1 04/01/2014
[  547.948411] RIP: 0010:__static_key_slow_dec_cpuslocked.part.0+0x50/0x60
[  547.958769] Code: 74 16 e8 a3 f7 ff ff 84 c0 74 1f 5b 48 c7 c7 60 96 13 82 e9 82 15 6f 00 e8 dd fb ff ff 5b 48 c7 c7 60 96 13 82 e9 70 15 6f 00 <0f> 0b eb dd 66 66 2e 0f 1f 84 00 00 00 00 00 90 0f 1f 44 00 00 55
[  547.990804] RSP: 0018:ffffc9000a033c58 EFLAGS: 00010246
[  547.996006] RAX: 0000000000000000 RBX: ffffffffa0755a40 RCX: 0000000000000000
[  548.005476] RDX: 0000000000000000 RSI: 000000000000011b RDI: ffffffffa0755a40
[  548.020780] RBP: 0000000000000001 R08: ffffe8ffffc1d808 R09: ffffe8ffffc1d820
[  548.022169] R10: ffffc9000a033b28 R11: 0000000092a11c8d R12: 00000000ffffffa1
[  548.023686] R13: ffff888101deca00 R14: ffff888102b5d000 R15: 0000007f8ffa83f7
[  548.031184] FS:  00007f41b1000680(0000) GS:ffff88842d000000(0000) knlGS:0000000000000000
[  548.039927] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  548.044941] CR2: 00007f41842b43a8 CR3: 00000002d0745000 CR4: 00000000003506f0
[  548.049473] Call Trace:
[  548.050026]  <TASK>
[  548.050543]  ? __warn+0x7c/0x120
[  548.051235]  ? __static_key_slow_dec_cpuslocked.part.0+0x50/0x60
[  548.053966]  ? report_bug+0x1a7/0x210
[  548.059439]  ? handle_bug+0x3c/0x60
[  548.063914]  ? exc_invalid_op+0x13/0x60
[  548.070776]  ? asm_exc_invalid_op+0x16/0x20
[  548.074709]  ? __static_key_slow_dec_cpuslocked.part.0+0x50/0x60
[  548.078116]  ? __static_key_slow_dec_cpuslocked.part.0+0x2d/0x60
[  548.081773]  static_key_slow_dec+0x3e/0x60
[  548.084107]  xchk_teardown+0x1a2/0x1d0 [xfs 6c824b8f28c8b3bc861384b1edb6577088bd4dda]
[  548.089682]  xfs_scrub_metadata+0x448/0x5c0 [xfs 6c824b8f28c8b3bc861384b1edb6577088bd4dda]
[  548.098202]  xfs_ioc_scrubv_metadata+0x389/0x550 [xfs 6c824b8f28c8b3bc861384b1edb6577088bd4dda]
[  548.101209]  xfs_file_ioctl+0x8f0/0xe80 [xfs 6c824b8f28c8b3bc861384b1edb6577088bd4dda]
[  548.110146]  ? preempt_count_add+0x4a/0xa0
[  548.116282]  ? up_write+0x64/0x180
[  548.117009]  ? shmem_file_write_iter+0x5a/0x90
[  548.117987]  ? preempt_count_add+0x4a/0xa0
[  548.122761]  ? vfs_write+0x3a2/0x4a0
[  548.125244]  __x64_sys_ioctl+0x8a/0xb0
[  548.125987]  do_syscall_64+0x47/0x100
[  548.126895]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
[  548.128056] RIP: 0033:0x7f41b491ec5b
[  548.128874] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00
[  548.133010] RSP: 002b:00007f41b0fff4c0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[  548.134754] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f41b491ec5b
[  548.136731] RDX: 00007f41b0fff610 RSI: 00000000c0285840 RDI: 0000000000000004
[  548.138545] RBP: 00007f41b0fff610 R08: 0000000000000000 R09: 0000000000000064
[  548.140068] R10: 00007f41b0fff235 R11: 0000000000000246 R12: 00007ffe4738a3b0
[  548.141605] R13: 000000000000001d R14: 00007ffe4738a558 R15: 00007f418824e4e0
[  548.143161]  </TASK>
[  548.143659] ---[ end trace 0000000000000000 ]---
[  564.628369] Direct I/O collision with buffered writes! File: d23/d2b9/db54/f14fc Comm: fsstress
[  677.002836] u16:0 (11) used greatest stack depth: 11408 bytes left
[  706.851715] Direct I/O collision with buffered writes! File: d2ee3/da3b/d3258/f255e Comm: fsstress
[  853.054101] Direct I/O collision with buffered writes! File: d1b4b/d2ac/d8b5/f13f8 Comm: fsstress
[  895.426556] Direct I/O collision with buffered writes! File: d747b/d577c/d5893/f6014 Comm: fsstress
[ 1037.454921] u16:5 (67) used greatest stack depth: 11016 bytes left
[ 1064.482959] Direct I/O collision with buffered writes! File: d60b4/d6e91/d6852/f6a87 Comm: fsstress
[ 1159.694683] XFS (sda3): Unmounting Filesystem eef3561a-2a86-49c8-9aa1-5024529cd3e7
[ 1170.139417] XFS (sda4): Unmounting Filesystem e1e9e96c-0d94-4a7e-8405-86e4d16d331d
[ 1179.548273] XFS (sda4): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
[ 1179.553727] XFS (sda4): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
[ 1179.558023] XFS (sda4): Mounting V5 Filesystem e1e9e96c-0d94-4a7e-8405-86e4d16d331d
[ 1179.590448] XFS (sda4): Ending clean mount
[ 1179.600826] XFS (sda4): Unmounting Filesystem e1e9e96c-0d94-4a7e-8405-86e4d16d331d


[  104.985250] run fstests xfs/1877 at 2024-08-04 15:46:00
[  107.669818] XFS (sda4): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
[  107.672983] XFS (sda4): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
[  107.676466] XFS (sda4): Mounting V5 Filesystem e1e9e96c-0d94-4a7e-8405-86e4d16d331d
[  107.732210] XFS (sda4): Ending clean mount
[  107.744657] XFS (sda4): EXPERIMENTAL online scrub feature in use. Use at your own risk!
[  120.562636] Direct I/O collision with buffered writes! File: d2d4/d50c/d4a2/fe7 Comm: fsstress
[  132.078284] Direct I/O collision with buffered writes! File: d820/d6b9/d6f5/fbcc Comm: fsstress
[  134.261151] Direct I/O collision with buffered writes! File: d13e/d261/d896/fc9e Comm: fsstress
[  172.025695] Direct I/O collision with buffered writes! File: rt/p2/d1/f194c Comm: fsstress
[  238.690588] Direct I/O collision with buffered writes! File: dca1/dd3f/d2346/f26e3 Comm: fsstress
[  374.677179] Direct I/O collision with buffered writes! File: de70/dd75/de74/f2f38 Comm: fsstress
[  399.084085] Direct I/O collision with buffered writes! File: dd50/d1584/d1f9/f1e65 Comm: fsstress
[  547.895266] ------------[ cut here ]------------
[  547.905274] WARNING: CPU: 0 PID: 8578 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0x50/0x60
[  547.914707] Modules linked in: xfs rpcsec_gss_krb5 auth_rpcgss nft_chain_nat xt_REDIRECT nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_tcpudp ip_set_hash_ip ip_set_hash_net xt_set nft_compat ip_set_hash_mac ip_set nf_tables libcrc32c nfnetlink bfq sha512_ssse3 sha512_generic pvpanic_mmio sha256_ssse3 pvpanic sch_fq_codel configfs fuse ip_tables x_tables overlay nfsv4 af_packet
[  547.934623] CPU: 0 UID: 0 PID: 8578 Comm: xfs_scrub Not tainted 6.11.0-rc2-djwx #rc2 d9817e54d8a35981d261570492175bf5e1b3bc11
[  547.941392] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20171121_152543-x86-ol7-builder-01.us.oracle.com-4.el7.1 04/01/2014
[  547.948411] RIP: 0010:__static_key_slow_dec_cpuslocked.part.0+0x50/0x60
[  547.958769] Code: 74 16 e8 a3 f7 ff ff 84 c0 74 1f 5b 48 c7 c7 60 96 13 82 e9 82 15 6f 00 e8 dd fb ff ff 5b 48 c7 c7 60 96 13 82 e9 70 15 6f 00 <0f> 0b eb dd 66 66 2e 0f 1f 84 00 00 00 00 00 90 0f 1f 44 00 00 55
[  547.990804] RSP: 0018:ffffc9000a033c58 EFLAGS: 00010246
[  547.996006] RAX: 0000000000000000 RBX: ffffffffa0755a40 RCX: 0000000000000000
[  548.005476] RDX: 0000000000000000 RSI: 000000000000011b RDI: ffffffffa0755a40
[  548.020780] RBP: 0000000000000001 R08: ffffe8ffffc1d808 R09: ffffe8ffffc1d820
[  548.022169] R10: ffffc9000a033b28 R11: 0000000092a11c8d R12: 00000000ffffffa1
[  548.023686] R13: ffff888101deca00 R14: ffff888102b5d000 R15: 0000007f8ffa83f7
[  548.031184] FS:  00007f41b1000680(0000) GS:ffff88842d000000(0000) knlGS:0000000000000000
[  548.039927] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  548.044941] CR2: 00007f41842b43a8 CR3: 00000002d0745000 CR4: 00000000003506f0
[  548.049473] Call Trace:
[  548.050026]  <TASK>
[  548.050543]  ? __warn+0x7c/0x120
[  548.051235]  ? __static_key_slow_dec_cpuslocked.part.0+0x50/0x60
[  548.053966]  ? report_bug+0x1a7/0x210
[  548.059439]  ? handle_bug+0x3c/0x60
[  548.063914]  ? exc_invalid_op+0x13/0x60
[  548.070776]  ? asm_exc_invalid_op+0x16/0x20
[  548.074709]  ? __static_key_slow_dec_cpuslocked.part.0+0x50/0x60
[  548.078116]  ? __static_key_slow_dec_cpuslocked.part.0+0x2d/0x60
[  548.081773]  static_key_slow_dec+0x3e/0x60
[  548.084107]  xchk_teardown+0x1a2/0x1d0 [xfs 6c824b8f28c8b3bc861384b1edb6577088bd4dda]
[  548.089682]  xfs_scrub_metadata+0x448/0x5c0 [xfs 6c824b8f28c8b3bc861384b1edb6577088bd4dda]
[  548.098202]  xfs_ioc_scrubv_metadata+0x389/0x550 [xfs 6c824b8f28c8b3bc861384b1edb6577088bd4dda]
[  548.101209]  xfs_file_ioctl+0x8f0/0xe80 [xfs 6c824b8f28c8b3bc861384b1edb6577088bd4dda]
[  548.110146]  ? preempt_count_add+0x4a/0xa0
[  548.116282]  ? up_write+0x64/0x180
[  548.117009]  ? shmem_file_write_iter+0x5a/0x90
[  548.117987]  ? preempt_count_add+0x4a/0xa0
[  548.122761]  ? vfs_write+0x3a2/0x4a0
[  548.125244]  __x64_sys_ioctl+0x8a/0xb0
[  548.125987]  do_syscall_64+0x47/0x100
[  548.126895]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
[  548.128056] RIP: 0033:0x7f41b491ec5b
[  548.128874] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00
[  548.133010] RSP: 002b:00007f41b0fff4c0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[  548.134754] RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f41b491ec5b
[  548.136731] RDX: 00007f41b0fff610 RSI: 00000000c0285840 RDI: 0000000000000004
[  548.138545] RBP: 00007f41b0fff610 R08: 0000000000000000 R09: 0000000000000064
[  548.140068] R10: 00007f41b0fff235 R11: 0000000000000246 R12: 00007ffe4738a3b0
[  548.141605] R13: 000000000000001d R14: 00007ffe4738a558 R15: 00007f418824e4e0
[  548.143161]  </TASK>
[  548.143659] ---[ end trace 0000000000000000 ]---
[  564.628369] Direct I/O collision with buffered writes! File: d23/d2b9/db54/f14fc Comm: fsstress
[  677.002836] u16:0 (11) used greatest stack depth: 11408 bytes left
[  706.851715] Direct I/O collision with buffered writes! File: d2ee3/da3b/d3258/f255e Comm: fsstress
[  853.054101] Direct I/O collision with buffered writes! File: d1b4b/d2ac/d8b5/f13f8 Comm: fsstress
[  895.426556] Direct I/O collision with buffered writes! File: d747b/d577c/d5893/f6014 Comm: fsstress
[ 1037.454921] u16:5 (67) used greatest stack depth: 11016 bytes left
[ 1064.482959] Direct I/O collision with buffered writes! File: d60b4/d6e91/d6852/f6a87 Comm: fsstress
[ 1159.694683] XFS (sda3): Unmounting Filesystem eef3561a-2a86-49c8-9aa1-5024529cd3e7
[ 1170.139417] XFS (sda4): Unmounting Filesystem e1e9e96c-0d94-4a7e-8405-86e4d16d331d
[ 1179.548273] XFS (sda4): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
[ 1179.553727] XFS (sda4): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
[ 1179.558023] XFS (sda4): Mounting V5 Filesystem e1e9e96c-0d94-4a7e-8405-86e4d16d331d
[ 1179.590448] XFS (sda4): Ending clean mount
[ 1179.600826] XFS (sda4): Unmounting Filesystem e1e9e96c-0d94-4a7e-8405-86e4d16d331d

Re: Are jump labels broken on 6.11-rc1?

[Peter Zijlstra]

On Mon, Aug 05, 2024 at 07:35:22AM -0700, Darrick J. Wong wrote:
> On Wed, Jul 31, 2024 at 12:55:57PM +0200, Peter Zijlstra wrote:
> > On Tue, Jul 30, 2024 at 10:33:41PM -0700, Darrick J. Wong wrote:
> > 
> > > Sooooo... it turns out that somehow your patch got mismerged on the
> > > first go-round, and that worked.  The second time, there was no
> > > mismerge, which mean that the wrong atomic_cmpxchg() callsite was
> > > tested.
> > > 
> > > Looking back at the mismerge, it actually changed
> > > __static_key_slow_dec_cpuslocked, which had in 6.10:
> > > 
> > > 	if (atomic_dec_and_test(&key->enabled))
> > > 		jump_label_update(key);
> > > 
> > > Decrement, then return true if the value was set to zero.  With the 6.11
> > > code, it looks like we want to exchange a 1 with a 0, and act only if
> > > the previous value had been 1.
> > > 
> > > So perhaps we really want this change?  I'll send it out to the fleet
> > > and we'll see what it reports tomorrow morning.
> > 
> > Bah yes, I missed we had it twice. Definitely both sites want this.
> > 
> > I'll tentatively merge the below patch in tip/locking/urgent. I can
> > rebase if there is need.
> 
> Hi Peter,
> 
> This morning, I noticed the splat below with -rc2.
> 
> WARNING: CPU: 0 PID: 8578 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0x50/0x60
> 
> Line 295 is the else branch of this code:
> 
> 	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
> 		jump_label_update(key);
> 	else
> 		WARN_ON_ONCE(!static_key_slow_try_dec(key));
> 
> Apparently static_key_slow_try_dec returned false?  Looking at that
> function, I suppose the atomic_read of key->enabled returned 0, since it
> didn't trigger the "WARN_ON_ONCE(v < 0)" code.  Does that mean the value
> must have dropped from positive N to 0 without anyone ever taking the
> jump_label_mutex?

One possible scenario I see:

  slow_dec
    if (try_dec) // dec_not_one-ish, false
    // enabled == 1
				slow_inc
				  if (inc_not_disabled) // inc_not_zero-ish
				  // enabled == 2
				    return

    guard((mutex)(&jump_label_mutex);
    if (atomic_cmpxchg(1,0)==1) // false, we're 2
    
				slow_dec
				  if (try-dec) // dec_not_one, true
				  // enabled == 1
				    return
    else
      try_dec() // dec_not_one, false
      WARN


Let me go play to see how best to cure this.

> Unfortunately I'm a little too covfid-brained to figure this out today.
> :(

Urgh, brain-fog is the worst :/

Re: Are jump labels broken on 6.11-rc1?

[Peter Zijlstra]

On Tue, Aug 06, 2024 at 11:44:13AM +0200, Peter Zijlstra wrote:
> On Mon, Aug 05, 2024 at 07:35:22AM -0700, Darrick J. Wong wrote:
> > On Wed, Jul 31, 2024 at 12:55:57PM +0200, Peter Zijlstra wrote:
> > > On Tue, Jul 30, 2024 at 10:33:41PM -0700, Darrick J. Wong wrote:
> > > 
> > > > Sooooo... it turns out that somehow your patch got mismerged on the
> > > > first go-round, and that worked.  The second time, there was no
> > > > mismerge, which mean that the wrong atomic_cmpxchg() callsite was
> > > > tested.
> > > > 
> > > > Looking back at the mismerge, it actually changed
> > > > __static_key_slow_dec_cpuslocked, which had in 6.10:
> > > > 
> > > > 	if (atomic_dec_and_test(&key->enabled))
> > > > 		jump_label_update(key);
> > > > 
> > > > Decrement, then return true if the value was set to zero.  With the 6.11
> > > > code, it looks like we want to exchange a 1 with a 0, and act only if
> > > > the previous value had been 1.
> > > > 
> > > > So perhaps we really want this change?  I'll send it out to the fleet
> > > > and we'll see what it reports tomorrow morning.
> > > 
> > > Bah yes, I missed we had it twice. Definitely both sites want this.
> > > 
> > > I'll tentatively merge the below patch in tip/locking/urgent. I can
> > > rebase if there is need.
> > 
> > Hi Peter,
> > 
> > This morning, I noticed the splat below with -rc2.
> > 
> > WARNING: CPU: 0 PID: 8578 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0x50/0x60
> > 
> > Line 295 is the else branch of this code:
> > 
> > 	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
> > 		jump_label_update(key);
> > 	else
> > 		WARN_ON_ONCE(!static_key_slow_try_dec(key));
> > 
> > Apparently static_key_slow_try_dec returned false?  Looking at that
> > function, I suppose the atomic_read of key->enabled returned 0, since it
> > didn't trigger the "WARN_ON_ONCE(v < 0)" code.  Does that mean the value
> > must have dropped from positive N to 0 without anyone ever taking the
> > jump_label_mutex?
> 
> One possible scenario I see:
> 
>   slow_dec
>     if (try_dec) // dec_not_one-ish, false
>     // enabled == 1
> 				slow_inc
> 				  if (inc_not_disabled) // inc_not_zero-ish
> 				  // enabled == 2
> 				    return
> 
>     guard((mutex)(&jump_label_mutex);
>     if (atomic_cmpxchg(1,0)==1) // false, we're 2
>     
> 				slow_dec
> 				  if (try-dec) // dec_not_one, true
> 				  // enabled == 1
> 				    return
>     else
>       try_dec() // dec_not_one, false
>       WARN
> 
> 
> Let me go play to see how best to cure this.

I've ended up with this, not exactly pretty :/

Thomas?

---
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 6dc76b590703..5fa2c9f094b1 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -168,8 +168,8 @@ bool static_key_slow_inc_cpuslocked(struct static_key *key)
 		jump_label_update(key);
 		/*
 		 * Ensure that when static_key_fast_inc_not_disabled() or
-		 * static_key_slow_try_dec() observe the positive value,
-		 * they must also observe all the text changes.
+		 * static_key_dec() observe the positive value, they must also
+		 * observe all the text changes.
 		 */
 		atomic_set_release(&key->enabled, 1);
 	} else {
@@ -250,7 +250,7 @@ void static_key_disable(struct static_key *key)
 }
 EXPORT_SYMBOL_GPL(static_key_disable);
 
-static bool static_key_slow_try_dec(struct static_key *key)
+static bool static_key_dec(struct static_key *key, bool fast)
 {
 	int v;
 
@@ -268,31 +268,45 @@ static bool static_key_slow_try_dec(struct static_key *key)
 	v = atomic_read(&key->enabled);
 	do {
 		/*
-		 * Warn about the '-1' case though; since that means a
-		 * decrement is concurrent with a first (0->1) increment. IOW
-		 * people are trying to disable something that wasn't yet fully
-		 * enabled. This suggests an ordering problem on the user side.
+		 * Warn about the '-1' case; since that means a decrement is
+		 * concurrent with a first (0->1) increment. IOW people are
+		 * trying to disable something that wasn't yet fully enabled.
+		 * This suggests an ordering problem on the user side.
+		 *
+		 * Warn about the '0' case; simple underflow.
+		 *
+		 * Neither case should succeed and change things.
+		 */
+		if (WARN_ON_ONCE(v <= 0))
+			return false;
+
+		/*
+		 * Lockless fast-path, dec-not-one like behaviour.
 		 */
-		WARN_ON_ONCE(v < 0);
-		if (v <= 1)
+		if (fast && v <= 1)
 			return false;
 	} while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v - 1)));
 
-	return true;
+	if (fast)
+		return true;
+
+	/*
+	 * Locked slow path, dec-and-test like behaviour.
+	 */
+	lockdep_assert_held(&jump_label_mutex);
+	return v == 1;
 }
 
 static void __static_key_slow_dec_cpuslocked(struct static_key *key)
 {
 	lockdep_assert_cpus_held();
 
-	if (static_key_slow_try_dec(key))
+	if (static_key_dec(key, true)) // dec-not-one
 		return;
 
 	guard(mutex)(&jump_label_mutex);
-	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
+	if (static_key_dec(key, false)) // dec-and-test
 		jump_label_update(key);
-	else
-		WARN_ON_ONCE(!static_key_slow_try_dec(key));
 }
 
 static void __static_key_slow_dec(struct static_key *key)
@@ -329,7 +343,7 @@ void __static_key_slow_dec_deferred(struct static_key *key,
 {
 	STATIC_KEY_CHECK_USE(key);
 
-	if (static_key_slow_try_dec(key))
+	if (static_key_dec(key, true)) // dec-not-one
 		return;
 
 	schedule_delayed_work(work, timeout);

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Tue, Aug 06, 2024 at 12:38:08PM +0200, Peter Zijlstra wrote:
> On Tue, Aug 06, 2024 at 11:44:13AM +0200, Peter Zijlstra wrote:
> > On Mon, Aug 05, 2024 at 07:35:22AM -0700, Darrick J. Wong wrote:
> > > On Wed, Jul 31, 2024 at 12:55:57PM +0200, Peter Zijlstra wrote:
> > > > On Tue, Jul 30, 2024 at 10:33:41PM -0700, Darrick J. Wong wrote:
> > > > 
> > > > > Sooooo... it turns out that somehow your patch got mismerged on the
> > > > > first go-round, and that worked.  The second time, there was no
> > > > > mismerge, which mean that the wrong atomic_cmpxchg() callsite was
> > > > > tested.
> > > > > 
> > > > > Looking back at the mismerge, it actually changed
> > > > > __static_key_slow_dec_cpuslocked, which had in 6.10:
> > > > > 
> > > > > 	if (atomic_dec_and_test(&key->enabled))
> > > > > 		jump_label_update(key);
> > > > > 
> > > > > Decrement, then return true if the value was set to zero.  With the 6.11
> > > > > code, it looks like we want to exchange a 1 with a 0, and act only if
> > > > > the previous value had been 1.
> > > > > 
> > > > > So perhaps we really want this change?  I'll send it out to the fleet
> > > > > and we'll see what it reports tomorrow morning.
> > > > 
> > > > Bah yes, I missed we had it twice. Definitely both sites want this.
> > > > 
> > > > I'll tentatively merge the below patch in tip/locking/urgent. I can
> > > > rebase if there is need.
> > > 
> > > Hi Peter,
> > > 
> > > This morning, I noticed the splat below with -rc2.
> > > 
> > > WARNING: CPU: 0 PID: 8578 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0x50/0x60
> > > 
> > > Line 295 is the else branch of this code:
> > > 
> > > 	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
> > > 		jump_label_update(key);
> > > 	else
> > > 		WARN_ON_ONCE(!static_key_slow_try_dec(key));
> > > 
> > > Apparently static_key_slow_try_dec returned false?  Looking at that
> > > function, I suppose the atomic_read of key->enabled returned 0, since it
> > > didn't trigger the "WARN_ON_ONCE(v < 0)" code.  Does that mean the value
> > > must have dropped from positive N to 0 without anyone ever taking the
> > > jump_label_mutex?
> > 
> > One possible scenario I see:
> > 
> >   slow_dec
> >     if (try_dec) // dec_not_one-ish, false
> >     // enabled == 1
> > 				slow_inc
> > 				  if (inc_not_disabled) // inc_not_zero-ish
> > 				  // enabled == 2
> > 				    return
> > 
> >     guard((mutex)(&jump_label_mutex);
> >     if (atomic_cmpxchg(1,0)==1) // false, we're 2
> >     
> > 				slow_dec
> > 				  if (try-dec) // dec_not_one, true
> > 				  // enabled == 1
> > 				    return
> >     else
> >       try_dec() // dec_not_one, false
> >       WARN
> > 
> > 
> > Let me go play to see how best to cure this.
> 
> I've ended up with this, not exactly pretty :/
> 
> Thomas?

It seems to survive a short test, will send it out for overnight testing
on the full fleet, thanks.

--D

> ---
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 6dc76b590703..5fa2c9f094b1 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -168,8 +168,8 @@ bool static_key_slow_inc_cpuslocked(struct static_key *key)
>  		jump_label_update(key);
>  		/*
>  		 * Ensure that when static_key_fast_inc_not_disabled() or
> -		 * static_key_slow_try_dec() observe the positive value,
> -		 * they must also observe all the text changes.
> +		 * static_key_dec() observe the positive value, they must also
> +		 * observe all the text changes.
>  		 */
>  		atomic_set_release(&key->enabled, 1);
>  	} else {
> @@ -250,7 +250,7 @@ void static_key_disable(struct static_key *key)
>  }
>  EXPORT_SYMBOL_GPL(static_key_disable);
>  
> -static bool static_key_slow_try_dec(struct static_key *key)
> +static bool static_key_dec(struct static_key *key, bool fast)
>  {
>  	int v;
>  
> @@ -268,31 +268,45 @@ static bool static_key_slow_try_dec(struct static_key *key)
>  	v = atomic_read(&key->enabled);
>  	do {
>  		/*
> -		 * Warn about the '-1' case though; since that means a
> -		 * decrement is concurrent with a first (0->1) increment. IOW
> -		 * people are trying to disable something that wasn't yet fully
> -		 * enabled. This suggests an ordering problem on the user side.
> +		 * Warn about the '-1' case; since that means a decrement is
> +		 * concurrent with a first (0->1) increment. IOW people are
> +		 * trying to disable something that wasn't yet fully enabled.
> +		 * This suggests an ordering problem on the user side.
> +		 *
> +		 * Warn about the '0' case; simple underflow.
> +		 *
> +		 * Neither case should succeed and change things.
> +		 */
> +		if (WARN_ON_ONCE(v <= 0))
> +			return false;
> +
> +		/*
> +		 * Lockless fast-path, dec-not-one like behaviour.
>  		 */
> -		WARN_ON_ONCE(v < 0);
> -		if (v <= 1)
> +		if (fast && v <= 1)
>  			return false;
>  	} while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v - 1)));
>  
> -	return true;
> +	if (fast)
> +		return true;
> +
> +	/*
> +	 * Locked slow path, dec-and-test like behaviour.
> +	 */
> +	lockdep_assert_held(&jump_label_mutex);
> +	return v == 1;
>  }
>  
>  static void __static_key_slow_dec_cpuslocked(struct static_key *key)
>  {
>  	lockdep_assert_cpus_held();
>  
> -	if (static_key_slow_try_dec(key))
> +	if (static_key_dec(key, true)) // dec-not-one
>  		return;
>  
>  	guard(mutex)(&jump_label_mutex);
> -	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
> +	if (static_key_dec(key, false)) // dec-and-test
>  		jump_label_update(key);
> -	else
> -		WARN_ON_ONCE(!static_key_slow_try_dec(key));
>  }
>  
>  static void __static_key_slow_dec(struct static_key *key)
> @@ -329,7 +343,7 @@ void __static_key_slow_dec_deferred(struct static_key *key,
>  {
>  	STATIC_KEY_CHECK_USE(key);
>  
> -	if (static_key_slow_try_dec(key))
> +	if (static_key_dec(key, true)) // dec-not-one
>  		return;
>  
>  	schedule_delayed_work(work, timeout);

Re: Are jump labels broken on 6.11-rc1?

[Thomas Gleixner]

On Tue, Aug 06 2024 at 12:38, Peter Zijlstra wrote:
> On Tue, Aug 06, 2024 at 11:44:13AM +0200, Peter Zijlstra wrote:
> I've ended up with this, not exactly pretty :/
>
> -static bool static_key_slow_try_dec(struct static_key *key)
> +static bool static_key_dec(struct static_key *key, bool fast)
>  {
>  	int v;
>  
> @@ -268,31 +268,45 @@ static bool static_key_slow_try_dec(struct static_key *key)
>  	v = atomic_read(&key->enabled);
>  	do {
>  		/*
> -		 * Warn about the '-1' case though; since that means a
> -		 * decrement is concurrent with a first (0->1) increment. IOW
> -		 * people are trying to disable something that wasn't yet fully
> -		 * enabled. This suggests an ordering problem on the user side.
> +		 * Warn about the '-1' case; since that means a decrement is
> +		 * concurrent with a first (0->1) increment. IOW people are
> +		 * trying to disable something that wasn't yet fully enabled.
> +		 * This suggests an ordering problem on the user side.
> +		 *
> +		 * Warn about the '0' case; simple underflow.
> +		 *
> +		 * Neither case should succeed and change things.

Which is confusing because the fastpath will drop down into the slowpath
due to this.

> +		 */
> +		if (WARN_ON_ONCE(v <= 0))
> +			return false;

This forces the fastpath into the slowpath. I assume this on purpose to
handle the concurrent 'first enable (enabled == -1)'. But hell this is
not comprehensible without a comment.

>  static void __static_key_slow_dec_cpuslocked(struct static_key *key)
>  {
>  	lockdep_assert_cpus_held();
>  
> -	if (static_key_slow_try_dec(key))
> +	if (static_key_dec(key, true)) // dec-not-one

Eeew.

Something like the below?

Thanks,

        tglx
---
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -168,8 +168,8 @@ bool static_key_slow_inc_cpuslocked(stru
 		jump_label_update(key);
 		/*
 		 * Ensure that when static_key_fast_inc_not_disabled() or
-		 * static_key_slow_try_dec() observe the positive value,
-		 * they must also observe all the text changes.
+		 * static_key_dec() observe the positive value, they must also
+		 * observe all the text changes.
 		 */
 		atomic_set_release(&key->enabled, 1);
 	} else {
@@ -250,49 +250,71 @@ void static_key_disable(struct static_ke
 }
 EXPORT_SYMBOL_GPL(static_key_disable);
 
-static bool static_key_slow_try_dec(struct static_key *key)
+static bool static_key_dec(struct static_key *key, bool dec_not_one)
 {
-	int v;
+	int v = atomic_read(&key->enabled);
 
-	/*
-	 * Go into the slow path if key::enabled is less than or equal than
-	 * one. One is valid to shut down the key, anything less than one
-	 * is an imbalance, which is handled at the call site.
-	 *
-	 * That includes the special case of '-1' which is set in
-	 * static_key_slow_inc_cpuslocked(), but that's harmless as it is
-	 * fully serialized in the slow path below. By the time this task
-	 * acquires the jump label lock the value is back to one and the
-	 * retry under the lock must succeed.
-	 */
-	v = atomic_read(&key->enabled);
 	do {
 		/*
-		 * Warn about the '-1' case though; since that means a
-		 * decrement is concurrent with a first (0->1) increment. IOW
-		 * people are trying to disable something that wasn't yet fully
-		 * enabled. This suggests an ordering problem on the user side.
+		 * Warn about the '-1' case; since that means a decrement is
+		 * concurrent with a first (0->1) increment. IOW people are
+		 * trying to disable something that wasn't yet fully enabled.
+		 * This suggests an ordering problem on the user side.
+		 *
+		 * Warn about the '0' case; simple underflow.
 		 */
-		WARN_ON_ONCE(v < 0);
-		if (v <= 1)
-			return false;
+		if (WARN_ON_ONCE(v <= 0))
+			return v;
+
+		if (dec_not_one && v == 1)
+			return v;
+
 	} while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v - 1)));
 
-	return true;
+	return v;
+}
+
+/*
+ * Fastpath: Decrement if the reference count is greater than one
+ *
+ * Returns false, if the reference count is 1 or -1 to force the caller
+ * into the slowpath.
+ *
+ * The -1 case is to handle a decrement during a concurrent first enable,
+ * which sets the count to -1 in static_key_slow_inc_cpuslocked(). As the
+ * slow path is serialized the caller will observe 1 once it acquired the
+ * jump_label_mutex, so the slow path can succeed.
+ */
+static bool static_key_dec_not_one(struct static_key *key)
+{
+	int v = static_key_dec(key, true);
+
+	return v != 1 && v != -1;
+}
+
+/*
+ * Slowpath: Decrement and test whether the refcount hit 0.
+ *
+ * Returns true if the refcount hit zero, i.e. the previous value was one.
+ */
+static bool static_key_dec_and_test(struct static_key *key)
+{
+	int v = static_key_dec(key, false);
+
+	lockdep_assert_held(&jump_label_mutex);
+	return v == 1;
 }
 
 static void __static_key_slow_dec_cpuslocked(struct static_key *key)
 {
 	lockdep_assert_cpus_held();
 
-	if (static_key_slow_try_dec(key))
+	if (static_key_dec_not_one(key))
 		return;
 
 	guard(mutex)(&jump_label_mutex);
-	if (atomic_cmpxchg(&key->enabled, 1, 0) == 1)
+	if (static_key_dec_and_test(key))
 		jump_label_update(key);
-	else
-		WARN_ON_ONCE(!static_key_slow_try_dec(key));
 }
 
 static void __static_key_slow_dec(struct static_key *key)
@@ -329,7 +351,7 @@ void __static_key_slow_dec_deferred(stru
 {
 	STATIC_KEY_CHECK_USE(key);
 
-	if (static_key_slow_try_dec(key))
+	if (static_key_dec_not_one(key))
 		return;
 
 	schedule_delayed_work(work, timeout);

Re: Are jump labels broken on 6.11-rc1?

[Peter Zijlstra]

On Wed, Aug 07, 2024 at 04:03:12PM +0200, Thomas Gleixner wrote:

> > +	if (static_key_dec(key, true)) // dec-not-one
> 
> Eeew.

:-) I knew you'd hate on that

> Something like the below?
> 
> Thanks,
> 
>         tglx
> ---
> @@ -250,49 +250,71 @@ void static_key_disable(struct static_ke
>  }
>  EXPORT_SYMBOL_GPL(static_key_disable);
>  
> -static bool static_key_slow_try_dec(struct static_key *key)
> +static bool static_key_dec(struct static_key *key, bool dec_not_one)
>  {
> +	int v = atomic_read(&key->enabled);
>  
>  	do {
>  		/*
> +		 * Warn about the '-1' case; since that means a decrement is
> +		 * concurrent with a first (0->1) increment. IOW people are
> +		 * trying to disable something that wasn't yet fully enabled.
> +		 * This suggests an ordering problem on the user side.
> +		 *
> +		 * Warn about the '0' case; simple underflow.
>  		 */
> +		if (WARN_ON_ONCE(v <= 0))
> +			return v;
> +
> +		if (dec_not_one && v == 1)
> +			return v;
> +
>  	} while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v - 1)));
>  
> +	return v;
> +}
> +
> +/*
> + * Fastpath: Decrement if the reference count is greater than one
> + *
> + * Returns false, if the reference count is 1 or -1 to force the caller
> + * into the slowpath.
> + *
> + * The -1 case is to handle a decrement during a concurrent first enable,
> + * which sets the count to -1 in static_key_slow_inc_cpuslocked(). As the
> + * slow path is serialized the caller will observe 1 once it acquired the
> + * jump_label_mutex, so the slow path can succeed.
> + */
> +static bool static_key_dec_not_one(struct static_key *key)
> +{
> +	int v = static_key_dec(key, true);
> +
> +	return v != 1 && v != -1;

	if (v < 0)
		return false;

	/*
	 * Notably, 0 (underflow) returns true such that it bails out
	 * without doing anything.
	 */
	return v != 1;

Perhaps?

> +}
> +
> +/*
> + * Slowpath: Decrement and test whether the refcount hit 0.
> + *
> + * Returns true if the refcount hit zero, i.e. the previous value was one.
> + */
> +static bool static_key_dec_and_test(struct static_key *key)
> +{
> +	int v = static_key_dec(key, false);
> +
> +	lockdep_assert_held(&jump_label_mutex);
> +	return v == 1;
>  }

But yeah, this is nicer!

Re: Are jump labels broken on 6.11-rc1?

[Thomas Gleixner]

On Wed, Aug 07 2024 at 16:34, Peter Zijlstra wrote:
> On Wed, Aug 07, 2024 at 04:03:12PM +0200, Thomas Gleixner wrote:
>
>> > +	if (static_key_dec(key, true)) // dec-not-one
>> 
>> Eeew.
>
> :-) I knew you'd hate on that

So you added it just to make me grumpy enough to fix it for you, right?

>> +/*
>> + * Fastpath: Decrement if the reference count is greater than one
>> + *
>> + * Returns false, if the reference count is 1 or -1 to force the caller
>> + * into the slowpath.
>> + *
>> + * The -1 case is to handle a decrement during a concurrent first enable,
>> + * which sets the count to -1 in static_key_slow_inc_cpuslocked(). As the
>> + * slow path is serialized the caller will observe 1 once it acquired the
>> + * jump_label_mutex, so the slow path can succeed.
>> + */
>> +static bool static_key_dec_not_one(struct static_key *key)
>> +{
>> +	int v = static_key_dec(key, true);
>> +
>> +	return v != 1 && v != -1;
>
> 	if (v < 0)
> 		return false;

Hmm. I think we should do:

#define KEY_ENABLE_IN_PROGRESS		-1

or even a more distinct value like (INT_MIN / 2)

and replace all the magic -1 numbers with it. Then the check becomes
explicit:

        if (v == KEY_ENABLE_IN_PROGRESS)
        	return false;

> 	/*
> 	 * Notably, 0 (underflow) returns true such that it bails out
> 	 * without doing anything.
> 	 */
> 	return v != 1;
>
> Perhaps?

Sure.

>> +}
>> +
>> +/*
>> + * Slowpath: Decrement and test whether the refcount hit 0.
>> + *
>> + * Returns true if the refcount hit zero, i.e. the previous value was one.
>> + */
>> +static bool static_key_dec_and_test(struct static_key *key)
>> +{
>> +	int v = static_key_dec(key, false);
>> +
>> +	lockdep_assert_held(&jump_label_mutex);
>> +	return v == 1;
>>  }
>
> But yeah, this is nicer!

:)

Thanks,

        tglx

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Wed, Aug 07, 2024 at 04:55:53PM +0200, Thomas Gleixner wrote:
> On Wed, Aug 07 2024 at 16:34, Peter Zijlstra wrote:
> > On Wed, Aug 07, 2024 at 04:03:12PM +0200, Thomas Gleixner wrote:
> >
> >> > +	if (static_key_dec(key, true)) // dec-not-one
> >> 
> >> Eeew.
> >
> > :-) I knew you'd hate on that
> 
> So you added it just to make me grumpy enough to fix it for you, right?

FWIW with peter's 'ugly' patch applied, fstests didn't cough up any
static key complaints overnight.

> >> +/*
> >> + * Fastpath: Decrement if the reference count is greater than one
> >> + *
> >> + * Returns false, if the reference count is 1 or -1 to force the caller
> >> + * into the slowpath.
> >> + *
> >> + * The -1 case is to handle a decrement during a concurrent first enable,
> >> + * which sets the count to -1 in static_key_slow_inc_cpuslocked(). As the
> >> + * slow path is serialized the caller will observe 1 once it acquired the
> >> + * jump_label_mutex, so the slow path can succeed.
> >> + */
> >> +static bool static_key_dec_not_one(struct static_key *key)
> >> +{
> >> +	int v = static_key_dec(key, true);
> >> +
> >> +	return v != 1 && v != -1;
> >
> > 	if (v < 0)
> > 		return false;
> 
> Hmm. I think we should do:
> 
> #define KEY_ENABLE_IN_PROGRESS		-1
> 
> or even a more distinct value like (INT_MIN / 2)
> 
> and replace all the magic -1 numbers with it. Then the check becomes
> explicit:
> 
>         if (v == KEY_ENABLE_IN_PROGRESS)
>         	return false;
> 
> > 	/*
> > 	 * Notably, 0 (underflow) returns true such that it bails out
> > 	 * without doing anything.
> > 	 */
> > 	return v != 1;
> >
> > Perhaps?
> 
> Sure.
> 
> >> +}
> >> +
> >> +/*
> >> + * Slowpath: Decrement and test whether the refcount hit 0.
> >> + *
> >> + * Returns true if the refcount hit zero, i.e. the previous value was one.
> >> + */
> >> +static bool static_key_dec_and_test(struct static_key *key)
> >> +{
> >> +	int v = static_key_dec(key, false);
> >> +
> >> +	lockdep_assert_held(&jump_label_mutex);
> >> +	return v == 1;
> >>  }
> >
> > But yeah, this is nicer!
> 
> :)

It probably goes without saying that if either of you send a cleaned up
patch with all these changes baked in, I will test it for you all. :)

--D

> 
> Thanks,
> 
>         tglx
> 

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Wed, Aug 07, 2024 at 08:05:03AM -0700, Darrick J. Wong wrote:
> On Wed, Aug 07, 2024 at 04:55:53PM +0200, Thomas Gleixner wrote:
> > On Wed, Aug 07 2024 at 16:34, Peter Zijlstra wrote:
> > > On Wed, Aug 07, 2024 at 04:03:12PM +0200, Thomas Gleixner wrote:
> > >
> > >> > +	if (static_key_dec(key, true)) // dec-not-one
> > >> 
> > >> Eeew.
> > >
> > > :-) I knew you'd hate on that
> > 
> > So you added it just to make me grumpy enough to fix it for you, right?
> 
> FWIW with peter's 'ugly' patch applied, fstests didn't cough up any
> static key complaints overnight.

But with Thomas' patch and the "if (v < 0) return false;" change
applied, the kernel crashes on boot:

[   11.563329] jump_label: Fatal kernel bug, unexpected op at mem_cgroup_sk_alloc+0x5/0xc0 [ffffffff81377af5] (eb 01 c3 53 48 != 66 90 0f 1f 00)) size:2 type:1
[   11.566166] ------------[ cut here ]------------
[   11.567150] kernel BUG at arch/x86/kernel/jump_label.c:73!
[   11.568416] Oops: invalid opcode: 0000 [#1] PREEMPT SMP
[   11.569586] CPU: 1 UID: 0 PID: 58 Comm: 1:1 Not tainted 6.11.0-rc2-djwx #rc2 d917e89fa198c1bdec418be517dc3e49f564823f
[   11.571790] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[   11.573738] Workqueue: cgroup_destroy css_free_rwork_fn
[   11.574898] RIP: 0010:__jump_label_patch+0x10a/0x110
[   11.576122] Code: eb a0 0f 0b 0f 0b 48 c7 c3 a4 7a 7b 82 41 56 45 89 e1 49 89 d8 4c 89 e9 4c 89 ea 4c 89 ee 48 c7 c7 60 8a e7 81 e8 66 dd 0d 00 <0f> 0b 0f 1f 40 00 0f 1f 44 00 00 e9 36 0
[   11.579843] RSP: 0018:ffffc90000527d70 EFLAGS: 00010246
[   11.580986] RAX: 0000000000000090 RBX: ffffffff81c088c1 RCX: 0000000000000000
[   11.582470] RDX: 0000000000000000 RSI: ffffffff81eacf61 RDI: 00000000ffffffff
[   11.583962] RBP: ffffc90000527da0 R08: 0000000000000000 R09: 205d393233333635
[   11.585449] R10: 0000000000000731 R11: 62616c5f706d756a R12: 0000000000000002
[   11.589526] R13: ffffffff81377af5 R14: 0000000000000001 R15: 0000000000000000
[   11.591030] FS:  0000000000000000(0000) GS:ffff88803ed00000(0000) knlGS:0000000000000000
[   11.592776] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   11.594018] CR2: 00007fec0a3f5d90 CR3: 0000000002033004 CR4: 00000000001706f0
[   11.595506] Call Trace:
[   11.596174]  <TASK>
[   11.605028]  arch_jump_label_transform_queue+0x33/0x70
[   11.606170]  __jump_label_update+0x6e/0x130
[   11.607131]  __static_key_slow_dec_cpuslocked+0x50/0x60
[   11.608280]  static_key_slow_dec+0x2d/0x50
[   11.609230]  mem_cgroup_css_free+0xc2/0xd0
[   11.610183]  css_free_rwork_fn+0x40/0x3f0
[   11.612094]  process_one_work+0x17a/0x3b0
[   11.613045]  worker_thread+0x252/0x360
[   11.615974]  kthread+0xe5/0x120

--D

> > >> +/*
> > >> + * Fastpath: Decrement if the reference count is greater than one
> > >> + *
> > >> + * Returns false, if the reference count is 1 or -1 to force the caller
> > >> + * into the slowpath.
> > >> + *
> > >> + * The -1 case is to handle a decrement during a concurrent first enable,
> > >> + * which sets the count to -1 in static_key_slow_inc_cpuslocked(). As the
> > >> + * slow path is serialized the caller will observe 1 once it acquired the
> > >> + * jump_label_mutex, so the slow path can succeed.
> > >> + */
> > >> +static bool static_key_dec_not_one(struct static_key *key)
> > >> +{
> > >> +	int v = static_key_dec(key, true);
> > >> +
> > >> +	return v != 1 && v != -1;
> > >
> > > 	if (v < 0)
> > > 		return false;
> > 
> > Hmm. I think we should do:
> > 
> > #define KEY_ENABLE_IN_PROGRESS		-1
> > 
> > or even a more distinct value like (INT_MIN / 2)
> > 
> > and replace all the magic -1 numbers with it. Then the check becomes
> > explicit:
> > 
> >         if (v == KEY_ENABLE_IN_PROGRESS)
> >         	return false;
> > 
> > > 	/*
> > > 	 * Notably, 0 (underflow) returns true such that it bails out
> > > 	 * without doing anything.
> > > 	 */
> > > 	return v != 1;
> > >
> > > Perhaps?
> > 
> > Sure.
> > 
> > >> +}
> > >> +
> > >> +/*
> > >> + * Slowpath: Decrement and test whether the refcount hit 0.
> > >> + *
> > >> + * Returns true if the refcount hit zero, i.e. the previous value was one.
> > >> + */
> > >> +static bool static_key_dec_and_test(struct static_key *key)
> > >> +{
> > >> +	int v = static_key_dec(key, false);
> > >> +
> > >> +	lockdep_assert_held(&jump_label_mutex);
> > >> +	return v == 1;
> > >>  }
> > >
> > > But yeah, this is nicer!
> > 
> > :)
> 
> It probably goes without saying that if either of you send a cleaned up
> patch with all these changes baked in, I will test it for you all. :)
> 
> --D
> 
> > 
> > Thanks,
> > 
> >         tglx
> > 
> 

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Wed, Aug 07, 2024 at 08:05:03AM -0700, Darrick J. Wong wrote:
> On Wed, Aug 07, 2024 at 04:55:53PM +0200, Thomas Gleixner wrote:
> > On Wed, Aug 07 2024 at 16:34, Peter Zijlstra wrote:
> > > On Wed, Aug 07, 2024 at 04:03:12PM +0200, Thomas Gleixner wrote:
> > >
> > >> > +	if (static_key_dec(key, true)) // dec-not-one
> > >> 
> > >> Eeew.
> > >
> > > :-) I knew you'd hate on that
> > 
> > So you added it just to make me grumpy enough to fix it for you, right?
> 
> FWIW with peter's 'ugly' patch applied, fstests didn't cough up any
> static key complaints overnight.

Unfortunately, I must take back these words -- after starting up a
nastier stress test, I can still reproduce this, but this time on arm64:

[   49.571229] run fstests xfs/286 at 2024-08-18 15:22:51
[   49.763554] spectre-v4 mitigation disabled by command-line option
[   50.004771] XFS (sda2): EXPERIMENTAL online scrub feature in use. Use at your own risk!
[   50.968906] XFS (sda3): EXPERIMENTAL metadata directory feature in use. Use at your own risk!
[   50.972647] XFS (sda3): EXPERIMENTAL realtime allocation group and superblock feature in use. Use at your own risk!
[   50.982134] XFS (sda3): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
[   50.986169] XFS (sda3): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
[   50.992801] XFS (sda3): Mounting V5 Filesystem 035cf5e0-d5e2-4739-8dab-15a52dddf130
[   51.025796] XFS (sda3): Ending clean mount
[   51.028980] XFS (sda3): EXPERIMENTAL realtime quota feature in use. Use at your own risk!
[   51.036655] XFS (sda3): Quotacheck needed: Please wait.
[   51.060215] XFS (sda3): Quotacheck: Done.
[   51.072704] XFS (sda3): EXPERIMENTAL online scrub feature in use. Use at your own risk!
[  100.296395] Direct I/O collision with buffered writes! File: d2eb/dbbb/d6ed/f1168 Comm: fsstress
[  109.211668] Direct I/O collision with buffered writes! File: de2a/dd5e/dcdd/f9f9 Comm: fsstress
[  137.196279] Direct I/O collision with buffered writes! File: da23/d11dc/de52/f1826 Comm: fsstress
[  175.740403] Direct I/O collision with buffered writes! File: d1163/d1085/d1225/f174e Comm: fsstress
[  280.081330] Direct I/O collision with buffered writes! File: dd74/d1184/d12b8/f26b8 Comm: fsstress
[  314.030300] Direct I/O collision with buffered writes! File: d31a1/d348a/d1a8d/f2de1 Comm: fsstress
[  421.526543] Direct I/O collision with buffered writes! File: d762/d765/d1931/f1859 Comm: fsstress
[  526.158683] Direct I/O collision with buffered writes! File: d4b6/d48f9/d5151/f12d7 Comm: fsstress
[  699.273894] Direct I/O collision with buffered writes! File: d68b/d10a7/d991/fcd4 Comm: fsstress
[  866.299077] Direct I/O collision with buffered writes! File: d7022/d4553/d5d89/f6925 Comm: fsstress
[  885.699829] Direct I/O collision with buffered writes! File: d3a72/d4ce4/d4c6e/f58ed Comm: fsstress
[ 1340.999610] Direct I/O collision with buffered writes! File: da8c/d26fe/d36ff/f6a94 Comm: fsstress
[ 1591.402638] Direct I/O collision with buffered writes! File: d1d18/d321a/d7d0/f3666 Comm: fsstress
[ 1595.377018] Direct I/O collision with buffered writes! File: d2fad/d15ef/d78da/f95e1 Comm: fsstress
[ 1618.061948] Direct I/O collision with buffered writes! File: d7b12/d51b7/d2337/f4ed5 Comm: fsstress
[ 1717.713414] Direct I/O collision with buffered writes! File: d5eb5/d3598/d71d/f42d0 Comm: fsstress
[ 1851.153819] Direct I/O collision with buffered writes! File: d5bc9/d3aad/d1892/faafc Comm: fsstress
[ 2080.574935] Direct I/O collision with buffered writes! File: d391f/d3e3a/d5246/fe17 Comm: fsstress
[ 2598.295098] Direct I/O collision with buffered writes! File: d923d/d4d30/da5d0/faf5b Comm: fsstress
[ 3549.070989] Direct I/O collision with buffered writes! File: da817/d87b0/dbc17/f7aeb Comm: fsstress
[22298.378392] XFS (sda3): page discard on page ffffffff407e3c80, inode 0x6a0c8fe, pos 9170944.
[22298.380577] sda3: writeback error on inode 111200510, offset 9166848, sector 24924992
[32769.915951] XFS (sda3): page discard on page ffffffff4073ef00, inode 0x41c8aa1, pos 593920.
[33965.988873] ------------[ cut here ]------------
[33966.013870] WARNING: CPU: 1 PID: 8992 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
[33966.017596] Modules linked in: xfs time_stats mean_and_variance nft_chain_nat xt_REDIRECT nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_tcpudp ip_set_hash_ip ip_set_hash_net xt_set nft_compat ip_set_hash_mac nf_tables libcrc32c crct10dif_ce sha2_ce sha256_arm64 bfq evbug sch_fq_codel fuse configfs efivarfs ip_tables x_tables overlay nfsv4
[33966.031983] CPU: 1 UID: 0 PID: 8992 Comm: xfs_scrub Not tainted 6.11.0-rc4-xfsa #rc4 eee7712a56abc3d2e1a397d28a5166a26e38d1d6
[33966.035837] Hardware name: QEMU KVM Virtual Machine, BIOS 1.6.6 08/22/2023
[33966.037739] pstate: 40401005 (nZcv daif +PAN -UAO -TCO -DIT +SSBS BTYPE=--)
[33966.040184] pc : __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
[33966.042845] lr : __static_key_slow_dec_cpuslocked.part.0+0x48/0xc0
[33966.045128] sp : fffffe008708f9f0
[33966.046504] x29: fffffe008708f9f0 x28: fffffc0031a1a500 x27: 000003fd77c6e100
[33966.048942] x26: fffffc00e82de000 x25: 0000000000000000 x24: fffffe007a96ac88
[33966.050713] x23: fffffc00e82de000 x22: fffffc00c71f0cd0 x21: 00000000ffffffe4
[33966.053423] x20: fffffe00812f25c8 x19: fffffe007a890940 x18: 0000000000000000
[33966.056225] x17: 0000000000000000 x16: 0000000000000000 x15: 000003ffc8e2ef28
[33966.059311] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
[33966.062103] x11: 0000000000000040 x10: 0000000000001b30 x9 : fffffe0080239558
[33966.064880] x8 : fffffe008708f9b8 x7 : 2222222222222222 x6 : 00000c99773e47d7
[33966.067463] x5 : 0000000000000002 x4 : 0000000000000000 x3 : 0000000000000001
[33966.070363] x2 : 0000000000000001 x1 : 0000000000000001 x0 : 0000000000000000
[33966.072840] Call trace:
[33966.073838]  __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
[33966.076105]  static_key_slow_dec+0x48/0x88
[33966.077739]  xfs_dir_hook_disable+0x20/0x38 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
[33966.080947]  xchk_teardown+0x1d4/0x220 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
[33966.084101]  xfs_scrub_metadata+0x52c/0x820 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
[33966.087429]  xfs_ioc_scrubv_metadata+0x3ec/0x5b0 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
[33966.090615]  xfs_file_ioctl+0xa58/0x1168 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
[33966.093672]  __arm64_sys_ioctl+0x4f8/0xd00
[33966.095016]  do_el0_svc+0x74/0x110
[33966.095983]  el0_svc+0x48/0x1f0
[33966.097124]  el0t_64_sync_handler+0x100/0x130
[33966.098843]  el0t_64_sync+0x190/0x198
[33966.100330] ---[ end trace 0000000000000000 ]---
[561654.524297] XFS (sda2): Unmounting Filesystem 4b02ad48-7ae9-4d54-a76c-32266d3a2e41
[563105.524971] XFS (sda3): Unmounting Filesystem 035cf5e0-d5e2-4739-8dab-15a52dddf130
[563245.534266] XFS (sda3): EXPERIMENTAL metadata directory feature in use. Use at your own risk!
[563245.575660] XFS (sda3): EXPERIMENTAL realtime allocation group and superblock feature in use. Use at your own risk!
[563245.576454] XFS (sda3): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
[563245.578363] XFS (sda3): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
[563245.592134] XFS (sda3): Mounting V5 Filesystem 035cf5e0-d5e2-4739-8dab-15a52dddf130
[563245.632709] XFS (sda3): EXPERIMENTAL realtime quota feature in use. Use at your own risk!
[563245.644275] XFS (sda3): Ending clean mount
[563245.843692] XFS (sda3): Unmounting Filesystem 035cf5e0-d5e2-4739-8dab-15a52dddf130

This corresponds to the:

	WARN_ON_ONCE(!static_key_slow_try_dec(key));

at the end of __static_key_slow_dec_cpuslocked.  Perhaps we're seeing a
-1 value from the atomic_cmpxchg(&key->enabled, 1, 0)?

If I surround the static_branch_{inc,dec} calls with a mutex the
complaints go away, though I gather that's not an acceptable hackaround.

Though as you can observe, the system ran stress testing for another 147
hours without any xfs problems reported.

--D

> > >> +/*
> > >> + * Fastpath: Decrement if the reference count is greater than one
> > >> + *
> > >> + * Returns false, if the reference count is 1 or -1 to force the caller
> > >> + * into the slowpath.
> > >> + *
> > >> + * The -1 case is to handle a decrement during a concurrent first enable,
> > >> + * which sets the count to -1 in static_key_slow_inc_cpuslocked(). As the
> > >> + * slow path is serialized the caller will observe 1 once it acquired the
> > >> + * jump_label_mutex, so the slow path can succeed.
> > >> + */
> > >> +static bool static_key_dec_not_one(struct static_key *key)
> > >> +{
> > >> +	int v = static_key_dec(key, true);
> > >> +
> > >> +	return v != 1 && v != -1;
> > >
> > > 	if (v < 0)
> > > 		return false;
> > 
> > Hmm. I think we should do:
> > 
> > #define KEY_ENABLE_IN_PROGRESS		-1
> > 
> > or even a more distinct value like (INT_MIN / 2)
> > 
> > and replace all the magic -1 numbers with it. Then the check becomes
> > explicit:
> > 
> >         if (v == KEY_ENABLE_IN_PROGRESS)
> >         	return false;
> > 
> > > 	/*
> > > 	 * Notably, 0 (underflow) returns true such that it bails out
> > > 	 * without doing anything.
> > > 	 */
> > > 	return v != 1;
> > >
> > > Perhaps?
> > 
> > Sure.
> > 
> > >> +}
> > >> +
> > >> +/*
> > >> + * Slowpath: Decrement and test whether the refcount hit 0.
> > >> + *
> > >> + * Returns true if the refcount hit zero, i.e. the previous value was one.
> > >> + */
> > >> +static bool static_key_dec_and_test(struct static_key *key)
> > >> +{
> > >> +	int v = static_key_dec(key, false);
> > >> +
> > >> +	lockdep_assert_held(&jump_label_mutex);
> > >> +	return v == 1;
> > >>  }
> > >
> > > But yeah, this is nicer!
> > 
> > :)
> 
> It probably goes without saying that if either of you send a cleaned up
> patch with all these changes baked in, I will test it for you all. :)
> 
> --D
> 
> > 
> > Thanks,
> > 
> >         tglx
> > 
> 

Re: Are jump labels broken on 6.11-rc1?

[Peter Zijlstra]

On Mon, Aug 26, 2024 at 08:35:06PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 07, 2024 at 08:05:03AM -0700, Darrick J. Wong wrote:
> > On Wed, Aug 07, 2024 at 04:55:53PM +0200, Thomas Gleixner wrote:
> > > On Wed, Aug 07 2024 at 16:34, Peter Zijlstra wrote:
> > > > On Wed, Aug 07, 2024 at 04:03:12PM +0200, Thomas Gleixner wrote:
> > > >
> > > >> > +	if (static_key_dec(key, true)) // dec-not-one
> > > >> 
> > > >> Eeew.
> > > >
> > > > :-) I knew you'd hate on that
> > > 
> > > So you added it just to make me grumpy enough to fix it for you, right?
> > 
> > FWIW with peter's 'ugly' patch applied, fstests didn't cough up any
> > static key complaints overnight.
> 
> Unfortunately, I must take back these words -- after starting up a
> nastier stress test, I can still reproduce this, but this time on arm64:
> 
> [   49.571229] run fstests xfs/286 at 2024-08-18 15:22:51
> [   49.763554] spectre-v4 mitigation disabled by command-line option
> [   50.004771] XFS (sda2): EXPERIMENTAL online scrub feature in use. Use at your own risk!
> [   50.968906] XFS (sda3): EXPERIMENTAL metadata directory feature in use. Use at your own risk!
> [   50.972647] XFS (sda3): EXPERIMENTAL realtime allocation group and superblock feature in use. Use at your own risk!
> [   50.982134] XFS (sda3): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
> [   50.986169] XFS (sda3): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
> [   50.992801] XFS (sda3): Mounting V5 Filesystem 035cf5e0-d5e2-4739-8dab-15a52dddf130
> [   51.025796] XFS (sda3): Ending clean mount
> [   51.028980] XFS (sda3): EXPERIMENTAL realtime quota feature in use. Use at your own risk!
> [   51.036655] XFS (sda3): Quotacheck needed: Please wait.
> [   51.060215] XFS (sda3): Quotacheck: Done.
> [   51.072704] XFS (sda3): EXPERIMENTAL online scrub feature in use. Use at your own risk!
> [  100.296395] Direct I/O collision with buffered writes! File: d2eb/dbbb/d6ed/f1168 Comm: fsstress
> [  109.211668] Direct I/O collision with buffered writes! File: de2a/dd5e/dcdd/f9f9 Comm: fsstress
> [  137.196279] Direct I/O collision with buffered writes! File: da23/d11dc/de52/f1826 Comm: fsstress
> [  175.740403] Direct I/O collision with buffered writes! File: d1163/d1085/d1225/f174e Comm: fsstress
> [  280.081330] Direct I/O collision with buffered writes! File: dd74/d1184/d12b8/f26b8 Comm: fsstress
> [  314.030300] Direct I/O collision with buffered writes! File: d31a1/d348a/d1a8d/f2de1 Comm: fsstress
> [  421.526543] Direct I/O collision with buffered writes! File: d762/d765/d1931/f1859 Comm: fsstress
> [  526.158683] Direct I/O collision with buffered writes! File: d4b6/d48f9/d5151/f12d7 Comm: fsstress
> [  699.273894] Direct I/O collision with buffered writes! File: d68b/d10a7/d991/fcd4 Comm: fsstress
> [  866.299077] Direct I/O collision with buffered writes! File: d7022/d4553/d5d89/f6925 Comm: fsstress
> [  885.699829] Direct I/O collision with buffered writes! File: d3a72/d4ce4/d4c6e/f58ed Comm: fsstress
> [ 1340.999610] Direct I/O collision with buffered writes! File: da8c/d26fe/d36ff/f6a94 Comm: fsstress
> [ 1591.402638] Direct I/O collision with buffered writes! File: d1d18/d321a/d7d0/f3666 Comm: fsstress
> [ 1595.377018] Direct I/O collision with buffered writes! File: d2fad/d15ef/d78da/f95e1 Comm: fsstress
> [ 1618.061948] Direct I/O collision with buffered writes! File: d7b12/d51b7/d2337/f4ed5 Comm: fsstress
> [ 1717.713414] Direct I/O collision with buffered writes! File: d5eb5/d3598/d71d/f42d0 Comm: fsstress
> [ 1851.153819] Direct I/O collision with buffered writes! File: d5bc9/d3aad/d1892/faafc Comm: fsstress
> [ 2080.574935] Direct I/O collision with buffered writes! File: d391f/d3e3a/d5246/fe17 Comm: fsstress
> [ 2598.295098] Direct I/O collision with buffered writes! File: d923d/d4d30/da5d0/faf5b Comm: fsstress
> [ 3549.070989] Direct I/O collision with buffered writes! File: da817/d87b0/dbc17/f7aeb Comm: fsstress
> [22298.378392] XFS (sda3): page discard on page ffffffff407e3c80, inode 0x6a0c8fe, pos 9170944.
> [22298.380577] sda3: writeback error on inode 111200510, offset 9166848, sector 24924992
> [32769.915951] XFS (sda3): page discard on page ffffffff4073ef00, inode 0x41c8aa1, pos 593920.
> [33965.988873] ------------[ cut here ]------------
> [33966.013870] WARNING: CPU: 1 PID: 8992 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> [33966.017596] Modules linked in: xfs time_stats mean_and_variance nft_chain_nat xt_REDIRECT nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_tcpudp ip_set_hash_ip ip_set_hash_net xt_set nft_compat ip_set_hash_mac nf_tables libcrc32c crct10dif_ce sha2_ce sha256_arm64 bfq evbug sch_fq_codel fuse configfs efivarfs ip_tables x_tables overlay nfsv4
> [33966.031983] CPU: 1 UID: 0 PID: 8992 Comm: xfs_scrub Not tainted 6.11.0-rc4-xfsa #rc4 eee7712a56abc3d2e1a397d28a5166a26e38d1d6
> [33966.035837] Hardware name: QEMU KVM Virtual Machine, BIOS 1.6.6 08/22/2023
> [33966.037739] pstate: 40401005 (nZcv daif +PAN -UAO -TCO -DIT +SSBS BTYPE=--)
> [33966.040184] pc : __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> [33966.042845] lr : __static_key_slow_dec_cpuslocked.part.0+0x48/0xc0
> [33966.045128] sp : fffffe008708f9f0
> [33966.046504] x29: fffffe008708f9f0 x28: fffffc0031a1a500 x27: 000003fd77c6e100
> [33966.048942] x26: fffffc00e82de000 x25: 0000000000000000 x24: fffffe007a96ac88
> [33966.050713] x23: fffffc00e82de000 x22: fffffc00c71f0cd0 x21: 00000000ffffffe4
> [33966.053423] x20: fffffe00812f25c8 x19: fffffe007a890940 x18: 0000000000000000
> [33966.056225] x17: 0000000000000000 x16: 0000000000000000 x15: 000003ffc8e2ef28
> [33966.059311] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
> [33966.062103] x11: 0000000000000040 x10: 0000000000001b30 x9 : fffffe0080239558
> [33966.064880] x8 : fffffe008708f9b8 x7 : 2222222222222222 x6 : 00000c99773e47d7
> [33966.067463] x5 : 0000000000000002 x4 : 0000000000000000 x3 : 0000000000000001
> [33966.070363] x2 : 0000000000000001 x1 : 0000000000000001 x0 : 0000000000000000
> [33966.072840] Call trace:
> [33966.073838]  __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> [33966.076105]  static_key_slow_dec+0x48/0x88
> [33966.077739]  xfs_dir_hook_disable+0x20/0x38 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
> [33966.080947]  xchk_teardown+0x1d4/0x220 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
> [33966.084101]  xfs_scrub_metadata+0x52c/0x820 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
> [33966.087429]  xfs_ioc_scrubv_metadata+0x3ec/0x5b0 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
> [33966.090615]  xfs_file_ioctl+0xa58/0x1168 [xfs 81b6cc501f608332f7590e39811e5ddd66afb315]
> [33966.093672]  __arm64_sys_ioctl+0x4f8/0xd00
> [33966.095016]  do_el0_svc+0x74/0x110
> [33966.095983]  el0_svc+0x48/0x1f0
> [33966.097124]  el0t_64_sync_handler+0x100/0x130
> [33966.098843]  el0t_64_sync+0x190/0x198
> [33966.100330] ---[ end trace 0000000000000000 ]---
> [561654.524297] XFS (sda2): Unmounting Filesystem 4b02ad48-7ae9-4d54-a76c-32266d3a2e41
> [563105.524971] XFS (sda3): Unmounting Filesystem 035cf5e0-d5e2-4739-8dab-15a52dddf130
> [563245.534266] XFS (sda3): EXPERIMENTAL metadata directory feature in use. Use at your own risk!
> [563245.575660] XFS (sda3): EXPERIMENTAL realtime allocation group and superblock feature in use. Use at your own risk!
> [563245.576454] XFS (sda3): EXPERIMENTAL exchange-range feature enabled. Use at your own risk!
> [563245.578363] XFS (sda3): EXPERIMENTAL parent pointer feature enabled. Use at your own risk!
> [563245.592134] XFS (sda3): Mounting V5 Filesystem 035cf5e0-d5e2-4739-8dab-15a52dddf130
> [563245.632709] XFS (sda3): EXPERIMENTAL realtime quota feature in use. Use at your own risk!
> [563245.644275] XFS (sda3): Ending clean mount
> [563245.843692] XFS (sda3): Unmounting Filesystem 035cf5e0-d5e2-4739-8dab-15a52dddf130
> 
> This corresponds to the:
> 
> 	WARN_ON_ONCE(!static_key_slow_try_dec(key));

But but but,... my patch killed that function. So are you sure it is
applied ?!

Because this sounds like exactly that issue again.

Anyway, it appears I had totally forgotten about this issue again due to
holidays, sorry. Let me stare hard at Thomas' patch and make a 'pretty'
one that does boot.

Re: Are jump labels broken on 6.11-rc1?

[Peter Zijlstra]

On Thu, Sep 05, 2024 at 10:12:41AM +0200, Peter Zijlstra wrote:
> On Mon, Aug 26, 2024 at 08:35:06PM -0700, Darrick J. Wong wrote:

> > [33965.988873] ------------[ cut here ]------------
> > [33966.013870] WARNING: CPU: 1 PID: 8992 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0

> > [33966.040184] pc : __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> > [33966.042845] lr : __static_key_slow_dec_cpuslocked.part.0+0x48/0xc0

> > [33966.072840] Call trace:
> > [33966.073838]  __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> > [33966.076105]  static_key_slow_dec+0x48/0x88

> > This corresponds to the:
> > 
> > 	WARN_ON_ONCE(!static_key_slow_try_dec(key));
> 
> But but but,... my patch killed that function. So are you sure it is
> applied ?!
> 
> Because this sounds like exactly that issue again.
> 
> Anyway, it appears I had totally forgotten about this issue again due to
> holidays, sorry. Let me stare hard at Thomas' patch and make a 'pretty'
> one that does boot.

I've taken tglx's version with a small change (added comment) and boot
tested it and queued it here:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/urgent

Could you please double check on both x86_64 and arm64?

If green by with the build robots and your own testing I'll push this
into tip/locking/urgent to be sent to Linus on Sunday. Hopefully finally
resolving this issue.

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Thu, Sep 05, 2024 at 11:16:05AM +0200, Peter Zijlstra wrote:
> On Thu, Sep 05, 2024 at 10:12:41AM +0200, Peter Zijlstra wrote:
> > On Mon, Aug 26, 2024 at 08:35:06PM -0700, Darrick J. Wong wrote:
> 
> > > [33965.988873] ------------[ cut here ]------------
> > > [33966.013870] WARNING: CPU: 1 PID: 8992 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> 
> > > [33966.040184] pc : __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> > > [33966.042845] lr : __static_key_slow_dec_cpuslocked.part.0+0x48/0xc0
> 
> > > [33966.072840] Call trace:
> > > [33966.073838]  __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> > > [33966.076105]  static_key_slow_dec+0x48/0x88
> 
> > > This corresponds to the:
> > > 
> > > 	WARN_ON_ONCE(!static_key_slow_try_dec(key));
> > 
> > But but but,... my patch killed that function. So are you sure it is
> > applied ?!
> > 
> > Because this sounds like exactly that issue again.
> > 
> > Anyway, it appears I had totally forgotten about this issue again due to
> > holidays, sorry. Let me stare hard at Thomas' patch and make a 'pretty'
> > one that does boot.
> 
> I've taken tglx's version with a small change (added comment) and boot
> tested it and queued it here:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/urgent
> 
> Could you please double check on both x86_64 and arm64?

Will send this out on the test farm tonight, thanks for the patch.

> If green by with the build robots and your own testing I'll push this
> into tip/locking/urgent to be sent to Linus on Sunday. Hopefully finally
> resolving this issue.

Sorry I didn't get to this earlier; I've been on vacation since the end
of August.  Now to get to the ~1300 fsdevel emails... ;)

--D

Re: Are jump labels broken on 6.11-rc1?

[Darrick J. Wong]

On Mon, Sep 16, 2024 at 09:08:01AM -0700, Darrick J. Wong wrote:
> On Thu, Sep 05, 2024 at 11:16:05AM +0200, Peter Zijlstra wrote:
> > On Thu, Sep 05, 2024 at 10:12:41AM +0200, Peter Zijlstra wrote:
> > > On Mon, Aug 26, 2024 at 08:35:06PM -0700, Darrick J. Wong wrote:
> > 
> > > > [33965.988873] ------------[ cut here ]------------
> > > > [33966.013870] WARNING: CPU: 1 PID: 8992 at kernel/jump_label.c:295 __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> > 
> > > > [33966.040184] pc : __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> > > > [33966.042845] lr : __static_key_slow_dec_cpuslocked.part.0+0x48/0xc0
> > 
> > > > [33966.072840] Call trace:
> > > > [33966.073838]  __static_key_slow_dec_cpuslocked.part.0+0xb0/0xc0
> > > > [33966.076105]  static_key_slow_dec+0x48/0x88
> > 
> > > > This corresponds to the:
> > > > 
> > > > 	WARN_ON_ONCE(!static_key_slow_try_dec(key));
> > > 
> > > But but but,... my patch killed that function. So are you sure it is
> > > applied ?!
> > > 
> > > Because this sounds like exactly that issue again.
> > > 
> > > Anyway, it appears I had totally forgotten about this issue again due to
> > > holidays, sorry. Let me stare hard at Thomas' patch and make a 'pretty'
> > > one that does boot.
> > 
> > I've taken tglx's version with a small change (added comment) and boot
> > tested it and queued it here:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/urgent
> > 
> > Could you please double check on both x86_64 and arm64?
> 
> Will send this out on the test farm tonight, thanks for the patch.
> 
> > If green by with the build robots and your own testing I'll push this
> > into tip/locking/urgent to be sent to Linus on Sunday. Hopefully finally
> > resolving this issue.
> 
> Sorry I didn't get to this earlier; I've been on vacation since the end
> of August.  Now to get to the ~1300 fsdevel emails... ;)

After 3.5 days of continuous pounding on the jump labels I haven't seen
any complaints from the kernel, so consider commit 6b01e5a8c11611
("jump_label: Fix static_key_slow_dec() yet again")

Tested-by: Darrick J. Wong <djwong@kernel.org>

Thanks for your help!

--D

> --D
>