Re: [PATCH v6 5/7] locking: Add contended_release tracepoint to qspinlock

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 14 May 2026 14:13:35 +0000
Dmitry Ilvokhin <d@ilvokhin.com> wrote:

> > > +void __lockfunc queued_spin_release_traced(struct qspinlock *lock)
> > > +{
> > > +	if (queued_spin_is_contended(lock))
> > > +		trace_call__contended_release(lock);
> > > +	queued_spin_release(lock);  
> > 
> > And then remove the duplicate call of "queued_spin_release()" here.  
> 
> This is the scenario the comment above the static branch describes.
> Here's what it looks like in practice on x86_64 (defconfig, compiled
> with GCC 11).
> 
> Current design (trace + unlock combined, with return):
>   
>     endbr64
>     xchg %ax,%ax                     ; NOP (static branch)
>     movb $0x0,(%rdi)                 ; unlock
>     decl %gs:__preempt_count
>     je   preempt
>     jmp  __x86_return_thunk
>     call queued_spin_release_traced  ; cold
>     jmp  preempt_handling            ; cold
>     call __SCT__preempt_schedule
>     jmp  __x86_return_thunk
> 
> With the trace-only function (no return, unlock after the call):
>   
>     endbr64
>     push %rbx                        ; saves callee-saved rbx (!)
>     mov  %rdi,%rbx                   ; preserve lock across call (!)
>     xchg %ax,%ax                     ; NOP (static branch)
>     movb $0x0,(%rbx)                 ; unlock
>     decl %gs:__preempt_count
>     je   preempt
>     pop  %rbx                        ; callee-saved restore (!)
>     jmp  __x86_return_thunk
>     call queued_spin_release_traced  ; cold
>     jmp  unlock                      ; cold
>     call __SCT__preempt_schedule
>     pop  %rbx
>     jmp  __x86_return_thunk
> 
> Three extra instructions marked by "!" on the hot path (push, mov, pop),
> all wasted when the tracepoint is off. That's the main reason for
> combining trace and unlock in the same out-of-line function.

Ah, because the return makes it into two tail calls.

I still don't like the duplication, perhaps add some more comments about
needing to update the other location if anything changes here? And perhaps
comment that this duplicate code helps the assembly.

-- Steve