Are jump labels broken on 6.11-rc1?

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

Quoted 5 lines · 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.

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.

Quoted 11 lines · Chandan Babu R

Quoted 5 lines · 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.

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,

Quoted 6 lines

83ab38ef0a0b2407d43af9575bb32333fdd74fb2
695ef796467ed228b60f1915995e390aea3d85c6
9bc2ff871f00437ad2f10c1eceff51aaa72b478f

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

Durr, does this help?

kernel/jump_label.c +0 -0

index 4ad5ed8adf96..57f70dfa1f3d 100644

kernel/jump_label.c +0 -0

 		jump_label_update(key);
 	jump_label_unlock();
 }

Quoted 11 lines · Chandan Babu R

Quoted 5 lines · 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.

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,

Quoted 6 lines

83ab38ef0a0b2407d43af9575bb32333fdd74fb2
695ef796467ed228b60f1915995e390aea3d85c6
9bc2ff871f00437ad2f10c1eceff51aaa72b478f

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

Durr, does this help?

kernel/jump_label.c +1 -1

index 4ad5ed8adf96..57f70dfa1f3d 100644
@@ -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();
 }

Quoted 19 lines · Peter Zijlstra

Quoted 17 lines · Chandan Babu R

Quoted 5 lines · 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.

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.

Quoted 1 line

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 <[email protected]>

--D

Quoted 15 lines

kernel/jump_label.c +1 -1

index 4ad5ed8adf96..57f70dfa1f3d 100644
@@ -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();
 }

Quoted 24 lines · Darrick J. Wong

Quoted 12 lines · Peter Zijlstra

Quoted 11 lines · Chandan Babu R

Quoted 5 lines · 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.

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,

Quoted 8 lines

Quoted 6 lines

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.

Quoted 4 lines

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

Thanks for the fix; you can add

Tested-by Darrick J. Wong <[email protected]>

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.

Quoted 15 lines · Darrick J. Wong

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.

Quoted 2 lines

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.

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Darrick J. Wong

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.

Quoted 7 lines

Quoted 2 lines

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. :(

Quoted 39 lines · Darrick J. Wong

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Darrick J. Wong

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.

Quoted 7 lines

Quoted 2 lines

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));

Quoted 5 lines

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.

Quoted 68 lines · Peter Zijlstra

Quoted 39 lines · Darrick J. Wong

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Darrick J. Wong

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.

Quoted 7 lines

Quoted 2 lines

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));

Quoted 5 lines

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

Quoted 1 line

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

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

Thomas?

---

kernel/jump_label.c +29 -15

index 6dc76b590703..5fa2c9f094b1 100644
@@ -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);

On Tue, Aug 06, 2024 at 11:44:13AM +0200, Peter Zijlstra wrote:
I've ended up with this, not exactly pretty :/

(quoted diff) +1 -3

-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

Review

Quoted 1 line

(quoted diff) +1 -0

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

Eeew.

Something like the below?

Thanks,

tglx
---

kernel/jump_label.c +23 -1

@@ -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.
+ *

kernel/jump_label.c +4 -0

+}
+
+/*
+ * Slowpath: Decrement and test whether the refcount hit 0.

Quoted 3 lines · Thomas Gleixner

Review

Quoted 1 line

(quoted diff) +1 -0

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

Eeew.

:-) I knew you'd hate on that

Quoted 11 lines

+/*
+ * 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.
+ */

kernel/jump_label.c void static_key_disable(struct static_ke · line 292

Quoted 5 lines

kernel/jump_label.c +5 -0

+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?

Quoted 8 lines

kernel/jump_label.c +8 -0

+}
+
+/*
+ * 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)

kernel/jump_label.c void static_key_disable(struct static_ke · line 306

Quoted 6 lines

kernel/jump_label.c +5 -0

+{
+	int v = static_key_dec(key, false);
+
+	lockdep_assert_held(&jump_label_mutex);
+	return v == 1;
 }

But yeah, this is nicer!

Quoted 7 lines · Peter Zijlstra

Quoted 3 lines · Thomas Gleixner

Review

Quoted 1 line

(quoted diff) +1 -0

+	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?

Quoted 11 lines

Quoted 11 lines

+/*
+ * 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.
+ */

Quoted 8 lines

kernel/jump_label.c void static_key_disable(struct static_ke · line 292

Quoted 5 lines

kernel/jump_label.c +5 -0

+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;

Quoted 7 lines

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

Perhaps?

Sure.

Quoted 8 lines

Quoted 8 lines

kernel/jump_label.c +8 -0

+}
+
+/*
+ * 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)

Quoted 8 lines

kernel/jump_label.c void static_key_disable(struct static_ke · line 306

Quoted 6 lines

kernel/jump_label.c +5 -0

+{
+	int v = static_key_dec(key, false);
+
+	lockdep_assert_held(&jump_label_mutex);
+	return v == 1;
 }

But yeah, this is nicer!

:)

Thanks,

tglx

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines · Peter Zijlstra

Quoted 3 lines · Thomas Gleixner

Review

Quoted 1 line

(quoted diff) +1 -0

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

Eeew.

Quoted 3 lines

Quoted 1 line

:-) 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.

Quoted 59 lines

Quoted 11 lines

Quoted 11 lines

+/*
+ * 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.
+ */

Quoted 8 lines

kernel/jump_label.c void static_key_disable(struct static_ke · line 292

Quoted 5 lines

kernel/jump_label.c +5 -0

+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;

Quoted 7 lines

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

Perhaps?

Sure.

Quoted 8 lines

Quoted 8 lines

kernel/jump_label.c +8 -0

+}
+
+/*
+ * 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)

Quoted 8 lines

kernel/jump_label.c void static_key_disable(struct static_ke · line 306

Quoted 6 lines

kernel/jump_label.c +5 -0

+{
+	int v = static_key_dec(key, false);
+
+	lockdep_assert_held(&jump_label_mutex);
+	return v == 1;
 }

But yeah, this is nicer!

Quoted 1 line

:)

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

Quoted 5 lines

Thanks,

tglx

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines · Peter Zijlstra

Quoted 3 lines · Thomas Gleixner

Review

Quoted 1 line

(quoted diff) +1 -0

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

Eeew.

Quoted 3 lines

Quoted 1 line

:-) 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.

Quoted 59 lines

Quoted 11 lines

Quoted 11 lines

+/*
+ * 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.
+ */

Quoted 8 lines

kernel/jump_label.c void static_key_disable(struct static_ke · line 292

Quoted 5 lines

kernel/jump_label.c +5 -0

+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;

Quoted 7 lines

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

Perhaps?

Sure.

Quoted 8 lines

Quoted 8 lines

kernel/jump_label.c +8 -0

+}
+
+/*
+ * 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)

Quoted 8 lines

kernel/jump_label.c void static_key_disable(struct static_ke · line 306

Quoted 6 lines

kernel/jump_label.c +5 -0

+{
+	int v = static_key_dec(key, false);
+
+	lockdep_assert_held(&jump_label_mutex);
+	return v == 1;
 }

But yeah, this is nicer!

Quoted 1 line

:)

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

Quoted 5 lines

Thanks,

tglx

Quoted 8 lines · Darrick J. Wong

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines · Peter Zijlstra

Quoted 3 lines · Thomas Gleixner

Review

Quoted 1 line

(quoted diff) +1 -0

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

Eeew.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

:-) 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));

Quoted 6 lines · Darrick J. Wong

[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

Quoted 7 lines

[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.

Quoted 16 lines · Peter Zijlstra

Quoted 6 lines · Darrick J. Wong

[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

Quoted 7 lines

[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));

Quoted 8 lines

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.

Quoted 26 lines · Peter Zijlstra

Quoted 16 lines · Peter Zijlstra

Quoted 6 lines · Darrick J. Wong

[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

Quoted 7 lines

[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));

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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

The following commit has been merged into the locking/urgent branch of tip:

jump_label: Fix static_key_slow_dec() yet again

While commit 83ab38ef0a0b ("jump_label: Fix concurrency issues in static_key_slow_dec()") fixed one problem, it created yet another, notably the following is now possible:

Quoted 7 lines · On Fri, Sep 06, 2024 at 02:41:14PM -0000, tip-bot2 for Peter Zijlstra wrote:

The following commit has been merged into the locking/urgent branch of tip:

jump_label: Fix static_key_slow_dec() yet again

While commit 83ab38ef0a0b ("jump_label: Fix concurrency issues in static_key_slow_dec()") fixed one problem, it created yet another, notably the following is now possible:

This patch, which is now in -next appears to have caused the KVM unit tests to start exploding badly on some arm64 systems (at least N1SDP and Cavium TX2). I've bisected the issue, but not analyzed it at all beyond noting that the commit looks relevant to the failure. None of the other tests we run on these platforms seem to trigger the issue.

Before producing any output the tests trigger a warning:

<4>[   17.303495] ------------[ cut here ]------------
<4>[   17.308364] WARNING: CPU: 1 PID: 279 at kernel/jump_label.c:266 static_key_dec+0x68/0x74
<4>[   17.316706] Modules linked in: crct10dif_ce arm_spe_pmu coresight_replicator coresight_funnel coresight_tmc coresight_stm stm_core coresight_tpiu arm_cmn coresight cfg80211 rfkill fuse dm_mod ip_tables x_tables ipv6
<4>[   17.336080] CPU: 1 UID: 0 PID: 279 Comm: qemu-system-aar Not tainted 6.11.0-rc7-00006-g3a0c7230588b #10
<4>[   17.345719] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
<4>[   17.352927] pc : static_key_dec+0x68/0x74
<4>[   17.357183] lr : __static_key_slow_dec_cpuslocked+0x24/0x84
<4>[   17.363003] sp : ffff800083c8ba80

....

<4>[   17.440381] Call trace:
<4>[   17.443074]  static_key_dec+0x68/0x74
<4>[   17.446984]  static_key_slow_dec+0x2c/0x80
<4>[   17.451327]  preempt_notifier_dec+0x18/0x24
<4>[   17.455759]  kvm_destroy_vm+0x208/0x2b0
<4>[   17.459845]  kvm_vm_release+0x80/0xb0
<4>[   17.463754]  __fput+0xcc/0x2d4
<4>[   17.467057]  ____fput+0x10/0x1c
<4>[   17.470446]  task_work_run+0x78/0xd4
<4>[   17.474268]  do_exit+0x2c8/0x90c

then the test times out and all the remaining cores splat on:

4>[ 18.067930] registering preempt_notifier while notifiers disabled

<4>[   18.067935] WARNING: CPU: 2 PID: 470 at kernel/sched/core.c:4729 preempt_notifier_register+0x24/0x58

The bisect seems to converge fairly smoothly:

git bisect start
# status: waiting for both good and bad commits
# bad: [6708132e80a2ced620bde9b9c36e426183544a23] Add linux-next specific files for 20240910
git bisect bad 6708132e80a2ced620bde9b9c36e426183544a23
# status: waiting for good commit(s), bad commit known
# good: [028ac237a57e1bcb07c7130b11527c0e025e4bef] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
git bisect good 028ac237a57e1bcb07c7130b11527c0e025e4bef
# good: [b66d58fce82c825b3dbb57a46b9a74f081ef7ec7] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good b66d58fce82c825b3dbb57a46b9a74f081ef7ec7
# good: [a636a90415dbc59f005369e3053996f859f0af50] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/libata/linux
git bisect good a636a90415dbc59f005369e3053996f859f0af50
# bad: [8e5ac35ddecbeddce79e915c226baaf577a2be6e] Merge branch 'driver-core-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git
git bisect bad 8e5ac35ddecbeddce79e915c226baaf577a2be6e
# bad: [1bcadc80ec6a46fb7193999935aaa299b4916569] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
git bisect bad 1bcadc80ec6a46fb7193999935aaa299b4916569
# good: [c2d0e416bdd9c83db3c9bb1f19433d5ba34e18c2] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
git bisect good c2d0e416bdd9c83db3c9bb1f19433d5ba34e18c2
# bad: [723da3b00882e1d13038fc48ba2602af9de4dd2e] Merge branch into tip/master: 'locking/core'
git bisect bad 723da3b00882e1d13038fc48ba2602af9de4dd2e
# bad: [a70a5c33a65ee54048e4ae479e3479d765a1bbc2] Merge branch into tip/master: 'core/core'
git bisect bad a70a5c33a65ee54048e4ae479e3479d765a1bbc2
# good: [85e511df3cec46021024176672a748008ed135bf] sched/eevdf: Allow shorter slices to wakeup-preempt
git bisect good 85e511df3cec46021024176672a748008ed135bf
# good: [20f13385b5836d00d64698748565fc6d3ce9b419] posix-timers: Consolidate timer setup
git bisect good 20f13385b5836d00d64698748565fc6d3ce9b419
# good: [42db2c2cb5ac3572380a9489b8f8bbe0e534dfc7] timekeeping: Use time_after() in timekeeping_check_update()
git bisect good 42db2c2cb5ac3572380a9489b8f8bbe0e534dfc7
# bad: [3a0c7230588b40caf1d81270ceaa3aa5c0355bc0] Merge branch into tip/master: 'perf/urgent'
git bisect bad 3a0c7230588b40caf1d81270ceaa3aa5c0355bc0
# bad: [de752774f38bb766941ed1bf910ba5a9f6cc6bf7] jump_label: Fix static_key_slow_dec() yet again
git bisect bad de752774f38bb766941ed1bf910ba5a9f6cc6bf7
# good: [fe513c2ef0a172a58f158e2e70465c4317f0a9a2] static_call: Replace pointless WARN_ON() in static_call_module_notify()
git bisect good fe513c2ef0a172a58f158e2e70465c4317f0a9a2

# first bad commit: [de752774f38bb766941ed1bf910ba5a9f6cc6bf7] jump_label: Fix static_key_slow_dec() yet again

When booting linux-next-20240910 on my MSI alpha 15 Laptop running debian sid (amd64), I get dropped to a shell and get the folllowing error in dmesg. I bisected this to commit de752774f38bb ("jump_label: Fix static_key_slow_dec() yet again").

Quoted 3 lines

When booting linux-next-20240910 on my MSI alpha 15 Laptop running debian sid (amd64), I get dropped to a shell and get the folllowing error in dmesg. I bisected this to commit de752774f38bb ("jump_label: Fix static_key_slow_dec() yet again").

I've just replaced that commit with the below -- which should be in tomorrows tree:

---
commit 1d7f856c2ca449f04a22d876e36b464b7a9d28b6

Author: Peter Zijlstra <[email protected]>
Date:   Mon Sep 9 12:50:09 2024 +0200

jump_label: Fix static_key_slow_dec() yet again

While commit 83ab38ef0a0b ("jump_label: Fix concurrency issues in static_key_slow_dec()") fixed one problem, it created yet another, notably the following is now possible:

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

Use dec_and_test instead of cmpxchg(), like it was prior to 83ab38ef0a0b. Add a few WARNs for the paranoid.

Fixes 83ab38ef0a0b ("jump_label: Fix concurrency issues in static_key_slow_dec()")

Reported-by "Darrick J. Wong" <[email protected]>

Tested-by Klara Modin <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

kernel/jump_label.c +26 -6

index 6dc76b590703..93a822d3c468 100644
@@ -168,7 +168,7 @@ 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,
+		 * static_key_dec_not_one() observe the positive value,
 		 * they must also observe all the text changes.
 		 */
 		atomic_set_release(&key->enabled, 1);

@@ -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_not_one(struct static_key *key)
 {
 	int v;
 

@@ -274,6 +274,14 @@ static bool static_key_slow_try_dec(struct static_key *key)
 		 * enabled. This suggests an ordering problem on the user side.
 		 */
 		WARN_ON_ONCE(v < 0);
+
+		/*
+		 * Warn about underflow, and lie about success in an attempt to
+		 * not make things worse.
+		 */
+		if (WARN_ON_ONCE(v == 0))
+			return true;
+
 		if (v <= 1)
 			return false;
 	} while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v - 1)));

@@ -284,15 +292,27 @@ static bool static_key_slow_try_dec(struct static_key *key)
 static void __static_key_slow_dec_cpuslocked(struct static_key *key)
 {
 	lockdep_assert_cpus_held();
+	int val;
 
-	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)
+	val = atomic_read(&key->enabled);
+	/*
+	 * It should be impossible to observe -1 with jump_label_mutex held,
+	 * see static_key_slow_inc_cpuslocked().
+	 */
+	if (WARN_ON_ONCE(val == -1))
+		return;
+	/*
+	 * Cannot already be 0, something went sideways.
+	 */
+	if (WARN_ON_ONCE(val == 0))
+		return;
+
+	if (atomic_dec_and_test(&key->enabled))
 		jump_label_update(key);
-	else
-		WARN_ON_ONCE(!static_key_slow_try_dec(key));
 }
 
 static void __static_key_slow_dec(struct static_key *key)