Re: [peterz-queue:locking/core 10/10] kernel/futex/core.c:982:1: warning: spinlock 'atomic ? __u.__val : q->lock_ptr' is still held at the end of function

[Marco Elver <elver@google.com>]

On Fri, Jan 23, 2026 at 09:55AM +0100, Peter Zijlstra wrote:
> On Thu, Jan 22, 2026 at 07:31:08PM +0100, Marco Elver wrote:
> > +Cc Will
> > 
> > On Thu, Jan 22, 2026 at 10:16AM +0100, Peter Zijlstra wrote:
> > > On Thu, Jan 22, 2026 at 10:30:28AM +0800, kernel test robot wrote:
> > > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/core
> > > > head:   33d9e7fdf9c17417347d1f06ddebaf25f21ab62a
> > > > commit: 33d9e7fdf9c17417347d1f06ddebaf25f21ab62a [10/10] futex: Convert to compiler context analysis
> > > > config: arm64-randconfig-001-20260122 (https://download.01.org/0day-ci/archive/20260122/202601221040.TeM0ihff-lkp@intel.com/config)
> > > > compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 9b8addffa70cee5b2acc5454712d9cf78ce45710)
> > > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260122/202601221040.TeM0ihff-lkp@intel.com/reproduce)
> > > > 
> > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > > the same patch/commit), kindly add following tags
> > > > | Reported-by: kernel test robot <lkp@intel.com>
> > > > | Closes: https://lore.kernel.org/oe-kbuild-all/202601221040.TeM0ihff-lkp@intel.com/
> > > > 
> > > > All warnings (new ones prefixed by >>):
> > > > 
> > > > >> kernel/futex/core.c:982:1: warning: spinlock 'atomic ? __u.__val : q->lock_ptr' is still held at the end of function [-Wthread-safety-analysis]
> > > >      982 | }
> > > >          | ^
> > > >    kernel/futex/core.c:976:2: note: spinlock acquired here
> > > >      976 |         spin_lock(lock_ptr);
> > > >          |         ^
> > > > >> kernel/futex/core.c:982:1: warning: expecting spinlock 'q->lock_ptr' to be held at the end of function [-Wthread-safety-analysis]
> > > >      982 | }
> > > >          | ^
> > > >    kernel/futex/core.c:966:6: note: spinlock acquired here
> > > >      966 | void futex_q_lockptr_lock(struct futex_q *q)
> > > >          |      ^
> > > >    2 warnings generated.
> > > 
> > > Urgh, this is the arm64 READ_ONCE() confusing the thing. It can't see
> > > through that mess and realize: lockptr == q->lock_ptr.
> > > 
> > > Marco, any suggestion how to best fix that?
> > 
> > I have a tentative solution:
> > 
> > diff --git a/arch/arm64/include/asm/rwonce.h b/arch/arm64/include/asm/rwonce.h
> > index 78beceec10cd..75265012ff2d 100644
> > --- a/arch/arm64/include/asm/rwonce.h
> > +++ b/arch/arm64/include/asm/rwonce.h
> > @@ -31,9 +31,8 @@
> >   */
> >  #define __READ_ONCE(x)							\
> >  ({									\
> > -	typeof(&(x)) __x = &(x);					\
> > -	int atomic = 1;							\
> > -	union { __unqual_scalar_typeof(*__x) __val; char __c[1]; } __u;	\
> > +	typeof(&(x)) __x = &(x), *__xp = &__x;				\
> > +	union { TYPEOF_UNQUAL(*__x) __val; char __c[1]; } __u;		\
> >  	switch (sizeof(x)) {						\
> >  	case 1:								\
> >  		asm volatile(__LOAD_RCPC(b, %w0, %1)			\
> > @@ -56,9 +55,10 @@
> >  			: "Q" (*__x) : "memory");			\
> >  		break;							\
> >  	default:							\
> > -		atomic = 0;						\
> > +		__u.__val = (*(volatile typeof(__x))__x);		\
> >  	}								\
> > -	atomic ? (typeof(*__x))__u.__val : (*(volatile typeof(__x))__x);\
> > +	*__xp = &__u.__val;						\
> > +	*__x;								\
> >  })
> >  
> >  #endif	/* !BUILD_VDSO */
> > 
> > This works because the compiler doesn't analyze alias reassignment
> > through pointer-to-alias within the same function. An alternative is to
> > do the alias reassignment via an __always_inline function that takes the
> > alias as a const pointer, and then cast the const away and do the
> > assignment. But I think the above is cleaner. Only compile-tested.
> 
> Oh clever! I suppose it does help if you know how the compiler works :-)

Latest version:


diff --git a/arch/arm64/include/asm/rwonce.h b/arch/arm64/include/asm/rwonce.h
index 78beceec10cd..1927bfe87695 100644
--- a/arch/arm64/include/asm/rwonce.h
+++ b/arch/arm64/include/asm/rwonce.h
@@ -31,9 +31,8 @@
  */
 #define __READ_ONCE(x)							\
 ({									\
-	typeof(&(x)) __x = &(x);					\
-	int atomic = 1;							\
-	union { __unqual_scalar_typeof(*__x) __val; char __c[1]; } __u;	\
+	TYPEOF_UNQUAL(x) *__x = (typeof(__x))&(x), **__xp = &__x;	\
+	union { typeof(*__x) __val; char __c[1]; } __u;			\
 	switch (sizeof(x)) {						\
 	case 1:								\
 		asm volatile(__LOAD_RCPC(b, %w0, %1)			\
@@ -56,9 +55,10 @@
 			: "Q" (*__x) : "memory");			\
 		break;							\
 	default:							\
-		atomic = 0;						\
+		__u.__val = (*(volatile typeof(x) *)__x);		\
 	}								\
-	atomic ? (typeof(*__x))__u.__val : (*(volatile typeof(__x))__x);\
+	*__xp = &__u.__val;						\
+	(typeof(x))*__x;						\
 })
 
 #endif	/* !BUILD_VDSO */


Because the previous one was not stripping volatile and we'd end up
reading things twice. This is more annoying than I thought, maybe I'll
go back to a version that produces equivalent binaries. This version
seems to be strictly reducing instructions (across ~100 functions or
so), but not sure why or if that's correct.

> I was thinking that perhaps it makes sense to invest in a 'directive'
> where we can explicitly hand alias information to the compiler.
> 
> Consider:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/tree/kernel/locking/rtmutex.c?h=locking/core&id=a31a1351e9ec9df7a72ea4877a8e53a9c02e8641#n1259
> 
> 		rtm = container_of(lock, struct rt_mutex, rtmutex);
> 		__assume_ctx_lock(&rtm->rtmutex.wait_lock);
> 		res = __ww_mutex_add_waiter(waiter, rtm, ww_ctx, wake_q);
> 
> Here there is a similar issue, where container_of() wrecks things. It
> cannot tell that rtm->rtmutex == lock and hence its idea of
> lock->wait_lock gets 'confused'.
> 
> I hack around it with that __assume_ctx_lock() and I suppose that works,
> but perhaps it makes sense to be able to tell the compiler that yes,
> these things are the actual same thing. Perhaps it will even improve
> code-gen, like in your example.

There is __builtin_assume, but I'd refrain from using that to tell the
compiler things that may not be true anymore in future. But
ThreadSafetyAnalysis's alias analysis ignores all that anyway because
its alias analysis is detached from the rest of the compiler.

Unclear how feasible it is to add a TSA-only builtin to do that. I think
in many cases we can actually use __returns_ctx_lock to create aliases,
but in the container_of cases I haven't yet figured out a way to do
that.