Re: Are jump labels broken on 6.11-rc1?

[Peter Zijlstra <peterz@infradead.org>]

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 :/