Re: Are jump labels broken on 6.11-rc1?

["Darrick J. Wong" <djwong@kernel.org>]

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