Re: [PATCH 0/5] locking/qspinlock: Add contended_release tracepoint

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4614 bytes --]

On 04.08.26 09:15, Dmitry Ilvokhin wrote:
> The contended_release tracepoint landed in v7.2-rc2 for sleeping locks
> (4f070ccb4dc4 "locking: Add contended_release tracepoint to sleepable
> locks"). Spinlock support was dropped from that series. This one adds it
> for queued spinlocks.
> 
> The existing contention_begin/contention_end tracepoints fire on the
> waiter side. The holder's identity and stack can be captured at
> contention_begin time (e.g. perf lock contention --lock-owner), but only
> for locks with an owner field to read: mutex and rwsem. qspinlock has
> none, so a contended spinlock cannot be attributed to its holder at all.
> Even where the owner can be read, it reflects the holder's state when a
> waiter arrives, not when the lock is released.
> 
> This series adds a contended_release tracepoint to qspinlock that fires
> on the holder side when a lock with waiters is released. This provides:
> 
> - Hold time estimation: when the holder's own acquisition was
>    contended, its contention_end (acquisition) and contended_release
>    can be correlated to measure how long the lock was held under
>    contention.
> 
> - The holder's stack at release time, which for spinlocks is not
>    available by any other means.
> 
> The unlock path might be quite hot, so the tracepoint is made as cheap
> as possible, to keep it usable in production:
> 
> - x86 with PARAVIRT_SPINLOCKS=y, which is what distributions ship, swaps
>    the unlock implementation via static_call() when the tracepoint is
>    enabled. The disabled path is byte-identical to today's: the same
>    inline movb, no NOP and no call.
> 
> - Everywhere else a static-branch check is compiled into
>    queued_spin_unlock(). On x86_64 that is a single NOP on the executed
>    path, with the call to the traced helper emitted out of line and
>    unreachable while the tracepoint is off. On other architectures a few
>    more instructions to manage a stack frame land on the executed path
>    too, so the generic path sits behind
>    CONFIG_QUEUED_SPINLOCKS_TRACE_CONTENDED_RELEASE (default n).
> 
> Costs and measurements are in the individual changelogs. Briefly, no
> throughput or latency change is measurable on either x86_64 or arm64
> with QUEUED_SPINLOCKS_TRACE_CONTENDED_RELEASE=y.
> 
> Tested: x86_64 with PARAVIRT_SPINLOCKS=y and =n, arm64, tracepoint on
> and off, disassembly checked in both states, locktorture with tracepoint
> on and off.
> 
> Not covered: qrwlock, and architectures with fully custom qspinlock
> implementations (e.g. PowerPC). The stack frame managing instructions on
> arm64 should be avoidable, but that is not done in this patchset.
> 
> Patch 1 is Peter's draft from [1] and is missing his Signed-off-by.
> Peter, please add it if you are happy with the patch.
> 
> [1]: https://lore.kernel.org/all/20260603120811.GW3493090@noisy.programming.kicks-ass.net/
> 
> Dmitry Ilvokhin (4):
>    locking: Factor out queued_spin_release()
>    locking/qspinlock: Add contended_release tracepoint
>    tracing/lock: Use TRACE_EVENT_FN() for contended_release
>    x86/paravirt: Trace contended_release on unlock
> 
> Peter Zijlstra (1):
>    x86/paravirt: Use static_call() for the paravirt spinlock ops
> 
>   arch/mips/include/asm/spinlock.h         |  6 +--
>   arch/x86/hyperv/hv_spinlock.c            |  4 +-
>   arch/x86/include/asm/cpufeatures.h       |  1 -
>   arch/x86/include/asm/paravirt-spinlock.h | 21 +++++---
>   arch/x86/kernel/kvm.c                    |  5 +-
>   arch/x86/kernel/paravirt-spinlocks.c     | 63 +++++++++++++++++++++---
>   arch/x86/kernel/static_call.c            | 27 ++++++++++
>   arch/x86/xen/spinlock.c                  |  5 +-
>   include/asm-generic/qspinlock.h          | 38 ++++++++++++--
>   include/trace/events/lock.h              | 10 +++-
>   kernel/Kconfig.locks                     | 20 ++++++++
>   kernel/locking/mutex.c                   |  4 ++
>   kernel/locking/qspinlock.c               | 22 +++++++++
>   tools/arch/x86/include/asm/cpufeatures.h |  1 -
>   14 files changed, 195 insertions(+), 32 deletions(-)
> 
> 
> base-commit: 5e601ab3615c86be7c4068ce992f94654693a032

For the whole series:

Acked-by: Juergen Gross <jgross@suse.com>

I'm considering some followup patches replacing the remaining paravirt
cases not covered by CONFIG_PARAVIRT_XXL with static_call(), too.

This will allow to drop the 32-bit paravirt patching completely. :-)

The queued_spin_unlock() hook was the main reason I didn't do that yet.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]