Re: [PATCH 1/7] cleanup: Introduce DEFINE_ACQUIRE() a CLASS() for conditional locking

[Peter Zijlstra <peterz@infradead.org>]

On Mon, May 12, 2025 at 08:25:59PM +0200, Peter Zijlstra wrote:
> On Mon, May 12, 2025 at 12:50:26PM +0200, Peter Zijlstra wrote:
> 
> > +#define __GUARD_ERR(_ptr) \
> > +	({ long _rc = (__force unsigned long)(_ptr); \
> > +	   if (!_rc) { _rc = -EBUSY; } if (!IS_ERR_VALUE(_rc)) { _rc = 0; } \
> > +	   _rc; })
> > +
> 
> >  #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
> > -	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
> > +	DEFINE_CLASS(_name, _type, if (!__GUARD_ERR(_T)) { _unlock; }, ({ _lock; _T; }), _type _T); \
> >  	DEFINE_CLASS_IS_GUARD(_name)
> 
> GCC is 'stupid' and this generates atrocious code. I'll play with it.

PRE:
    bf9e:       48 85 db                test   %rbx,%rbx
    bfa1:       74 1a                   je     bfbd <foo+0x5d>
    bfa3:       48 81 fb 00 f0 ff ff    cmp    $0xfffffffffffff000,%rbx
    bfaa:       77 11                   ja     bfbd <foo+0x5d>

POST:
    bf9e:       48 8d 43 ff             lea    -0x1(%rbx),%rax
    bfa2:       48 3d ff ef ff ff       cmp    $0xffffffffffffefff,%rax
    bfa8:       77 11                   ja     bfbb <foo+0x5b>

---
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -293,17 +293,18 @@ static inline class_##_name##_t class_##
 #define __DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\
 static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
 
-#define __GUARD_ERR(_ptr) \
-	({ long _rc = (__force unsigned long)(_ptr); \
-	   if (!_rc) { _rc = -EBUSY; } if (!IS_ERR_VALUE(_rc)) { _rc = 0; } \
-	   _rc; })
+#define __GUARD_IS_ERR(_ptr) \
+	({ unsigned long _rc = (__force unsigned long)(_ptr); \
+	   unlikely((_rc-1) >= -MAX_ERRNO-1); })
 
 #define __DEFINE_GUARD_LOCK_PTR(_name, _exp) \
 	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
 	{ void *_ptr = (void *)(__force unsigned long)*(_exp); \
 	  if (IS_ERR(_ptr)) { _ptr = NULL; } return _ptr; } \
 	static inline int class_##_name##_lock_err(class_##_name##_t *_T) \
-	{ return __GUARD_ERR(*(_exp)); }
+	{ long _rc = (__force unsigned long)*(_exp); \
+	  if (!_rc) { _rc = -EBUSY; } if (!IS_ERR_VALUE(_rc)) { _rc = 0; } \
+	  return _rc; }
 
 #define DEFINE_CLASS_IS_GUARD(_name) \
 	__DEFINE_CLASS_IS_CONDITIONAL(_name, false); \
@@ -314,7 +315,7 @@ static __maybe_unused const bool class_#
 	__DEFINE_GUARD_LOCK_PTR(_name, _T)
 
 #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
-	DEFINE_CLASS(_name, _type, if (!__GUARD_ERR(_T)) { _unlock; }, ({ _lock; _T; }), _type _T); \
+	DEFINE_CLASS(_name, _type, if (!__GUARD_IS_ERR(_T)) { _unlock; }, ({ _lock; _T; }), _type _T); \
 	DEFINE_CLASS_IS_GUARD(_name)
 
 #define DEFINE_GUARD_COND_4(_name, _ext, _lock, _cond) \
@@ -406,7 +407,7 @@ typedef struct {							\
 									\
 static inline void class_##_name##_destructor(class_##_name##_t *_T)	\
 {									\
-	if (!__GUARD_ERR(_T->lock)) { _unlock; }			\
+	if (!__GUARD_IS_ERR(_T->lock)) { _unlock; }			\
 }									\
 									\
 __DEFINE_GUARD_LOCK_PTR(_name, &_T->lock)