Re: [bigeasy-staging:futex_local_v6] [futex] 865221325b: WARNING:at_lib/rcuref.c:#rcuref_put_slowpath

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, Jan 17 2025 at 09:49, Thomas Gleixner wrote:
> On Fri, Jan 17 2025 at 00:31, Sebastian Andrzej Siewior wrote:
>>
>>     -> cnt = atomic_read(&ref->refcnt);                                          # cnt -> 0xffffffff / RCUREF_NOREF
>>     -> atomic_try_cmpxchg_release(&ref->refcnt, &cnt, RCUREF_DEAD))              # ref -> 0xe0000000 / RCUREF_DEAD
>>        -> return true
>>                                            -> cnt = atomic_read(&ref->refcnt);   # cnt -> 0xe0000000 / RCUREF_DEAD
>>                                            -> if (cnt > RCUREF_RELEASED)         # 0xe0000000 > 0xc0000000
>>                                              -> WARN_ONCE(cnt >= RCUREF_RELEASED, "rcuref - imbalanced put()")
>
> Duh. Indeed.
>
> That's a nasty one. Let me think about it.

The problem is the disconnect between the decrement and the read in the
slow path. The untested below should plug that hole. It results in two
extra instructions:

 412:	f0 83 43 40 ff       	lock addl $0xffffffff,0x40(%rbx)
 417:	78 69                	js     482 <dst_release+0x92>

vs.

 412:	be ff ff ff ff       	mov    $0xffffffff,%esi
 417:	f0 0f c1 73 40       	lock xadd %esi,0x40(%rbx)
 41c:	83 ee 01             	sub    $0x1,%esi
 41f:	78 69                	js     48a <dst_release+0x9a>

That's not the end of the world and could be further optimized, but
that's a minor detail.

Thanks,

        tglx
---
--- a/include/linux/rcuref.h
+++ b/include/linux/rcuref.h
@@ -71,27 +71,30 @@ static inline __must_check bool rcuref_g
 	return rcuref_get_slowpath(ref);
 }
 
-extern __must_check bool rcuref_put_slowpath(rcuref_t *ref);
+extern __must_check bool rcuref_put_slowpath(rcuref_t *ref, unsigned int cnt);
 
 /*
  * Internal helper. Do not invoke directly.
  */
 static __always_inline __must_check bool __rcuref_put(rcuref_t *ref)
 {
+	int cnt;
+
 	RCU_LOCKDEP_WARN(!rcu_read_lock_held() && preemptible(),
 			 "suspicious rcuref_put_rcusafe() usage");
 	/*
 	 * Unconditionally decrease the reference count. The saturation and
 	 * dead zones provide enough tolerance for this.
 	 */
-	if (likely(!atomic_add_negative_release(-1, &ref->refcnt)))
+	cnt = atomic_sub_return_release(1, &ref->refcnt);
+	if (likely(cnt >= 0))
 		return false;
 
 	/*
 	 * Handle the last reference drop and cases inside the saturation
 	 * and dead zones.
 	 */
-	return rcuref_put_slowpath(ref);
+	return rcuref_put_slowpath(ref, cnt);
 }
 
 /**
--- a/lib/rcuref.c
+++ b/lib/rcuref.c
@@ -220,6 +220,7 @@ EXPORT_SYMBOL_GPL(rcuref_get_slowpath);
 /**
  * rcuref_put_slowpath - Slowpath of __rcuref_put()
  * @ref:	Pointer to the reference count
+ * @cnt:	The resulting value of the fastpath decrement
  *
  * Invoked when the reference count is outside of the valid zone.
  *
@@ -233,10 +234,8 @@ EXPORT_SYMBOL_GPL(rcuref_get_slowpath);
  *	with a concurrent get()/put() pair. Caller is not allowed to
  *	deconstruct the protected object.
  */
-bool rcuref_put_slowpath(rcuref_t *ref)
+bool rcuref_put_slowpath(rcuref_t *ref, unsigned int cnt)
 {
-	unsigned int cnt = atomic_read(&ref->refcnt);
-
 	/* Did this drop the last reference? */
 	if (likely(cnt == RCUREF_NOREF)) {
 		/*