Re: Are jump labels broken on 6.11-rc1?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux