From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-08 18:42:23
Hello,
There have been some requests for low-overhead kernel lock contention
monitoring. The kernel has CONFIG_LOCK_STAT to provide such an infra
either via /proc/lock_stat or tracepoints directly.
However it's not light-weight and hard to be used in production. So
I'm trying to separate out the tracepoints and using them as a base to
build a new monitoring system.
As the lockdep and lock_stat provide good hooks in the lock functions,
it'd be natural to reuse them. Actually I tried to use lockdep as is
but disables the functionality at runtime (initialize debug_locks = 0,
lock_stat = 0). But it still has unacceptable overhead and the
lockdep data structures also increase memory footprint unnecessarily.
So I'm proposing a separate tracepoint-only configuration and keeping
lockdep_map only with minimal information needed for tracepoints (for
now, it's just name). And then the existing lockdep hooks can be used
for the tracepoints.
The patch 01-06 are preparation for the work. In a few places in the
kernel, they calls lockdep annotation explicitly to deal with
limitations in the lockdep implementation. In my understanding, they
are not needed to analyze lock contention.
To make matters worse, they rely on the compiler optimization (or
macro magic) so that it can get rid of the annotations and their
arguments when lockdep is not configured.
But it's not true any more when lock tracepoints are added and it'd
cause build errors. So I added #ifdef guards for LOCKDEP in the code
to prevent such errors.
In the patch 07 I mechanically changed most of code that depend on
CONFIG_LOCKDEP or CONFIG_DEBUG_LOCK_ALLOC to CONFIG_LOCK_INFO. It
paves the way for the codes to be shared for lockdep and tracepoints.
Mostly, it makes sure that locks are initialized with a proper name,
like in the patch 08 and 09.
I didn't change some places intentionally - for example, timer and
workqueue depend on LOCKDEP explicitly since they use some lockdep
annotations to work around the "held lock freed" warnings. The ocfs2
directly accesses lockdep_map.key so I didn't touch the code for now.
And RCU was because it generates too much overhead thanks to the
rcu_read_lock(). Maybe I need to revisit some of them later.
I added CONFIG_LOCK_TRACEPOINTS in the patch 10 to make it optional.
I found that it adds 2~3% of overhead when I ran `perf bench sched
messaging` even when the tracepoints are disabled. The benchmark
creates a lot of processes and make them communicate by socket pairs
(or pipes). I measured that around 15% of lock acquisition creates
contentions and it's mostly for spin locks (alc->lock and u->lock).
I ran perf record + report with the workload and it showed 50% of the
cpu cycles are in the spin lock slow path. So it's highly affected by
the spin lock slow path. Actually LOCK_CONTENDED() macro transforms
the spin lock code (and others) to use trylock first and then fall
back to real lock function if failed. Thus it'd add more (atomic)
operations and cache line bouncing for the contended cases:
#define LOCK_CONTENDED(_lock, try, lock) \
do { \
if (!try(_lock)) { \
lock_contended(&(_lock)->dep_map, _RET_IP_); \
lock(_lock); \
} \
lock_acquired(&(_lock)->dep_map, _RET_IP_); \
} while (0)
If I modify the macro not to use trylock and to call the real lock
function directly (so the lock_contended tracepoint would be called
always, if enabled), the overhead goes down to 0.x% when the
tracepoints are disabled.
I don't have a good solution as long as we use LOCK_CONTENDED() macro
to separate the contended locking path. Maybe we can make it call
some (generic) slow path lock function directly after failing trylock.
Or move the lockdep annotations into the each lock function bodies and
get rid of the LOCK_CONTENDED() macro entirely. Ideas?
Actually the patch 11 handles the same issue on the mutex code. The
fast version of mutex trylock was attempted only if LOCKDEP is not
enabled and it affects the mutex lock performance in the uncontended
cases too. So I partially reverted the change in the patch 07 to use
the fast functions with lock tracepoints too. Maybe we can use it
with LOCKDEP as well?
The last patch 12 might be controversial and I'd like to move the
lock_acquired annotation into the if(!try) block in the LOCK_CONTEDED
macro so that it can only be called when there's a contention.
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
It'd change lock hold time calculation in lock_stat for the fast path,
but I assume time difference between lock_acquire and lock_acquired
would be small when the lock is not contended. So I think we can use
the timestamp in lock_acquire. If it's not acceptable, we might
consider adding a new tracepoint to track the timing of contended
locks.
This series base on the current tip/locking/core and you get it from
'locking/tracepoint-v1' branch in my tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
Thanks,
Namhyung
Cc: Thomas Gleixner <redacted>
Cc: Steven Rostedt <redacted>
Cc: Tejun Heo <redacted>
Cc: Byungchul Park <redacted>
Cc: rcu-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-btrfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: intel-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Namhyung Kim (12):
locking: Pass correct outer wait type info
cgroup: rstat: Make cgroup_rstat_cpu_lock name readable
timer: Protect lockdep functions with #ifdef
workqueue: Protect lockdep functions with #ifdef
drm/i915: Protect lockdep functions with #ifdef
btrfs: change lockdep class size check using ks->names
locking: Introduce CONFIG_LOCK_INFO
locking/mutex: Init name properly w/ CONFIG_LOCK_INFO
locking: Add more static lockdep init macros
locking: Add CONFIG_LOCK_TRACEPOINTS option
locking/mutex: Revive fast functions for LOCK_TRACEPOINTS
locking: Move lock_acquired() from the fast path
drivers/gpu/drm/drm_connector.c | 7 +-
drivers/gpu/drm/i915/i915_sw_fence.h | 2 +-
drivers/gpu/drm/i915/intel_wakeref.c | 3 +
drivers/gpu/drm/i915/selftests/lib_sw_fence.h | 2 +-
.../net/wireless/intel/iwlwifi/iwl-trans.c | 4 +-
.../net/wireless/intel/iwlwifi/iwl-trans.h | 2 +-
drivers/tty/tty_ldsem.c | 2 +-
fs/btrfs/disk-io.c | 4 +-
fs/btrfs/disk-io.h | 2 +-
fs/cifs/connect.c | 2 +-
fs/kernfs/file.c | 2 +-
include/linux/completion.h | 2 +-
include/linux/jbd2.h | 2 +-
include/linux/kernfs.h | 2 +-
include/linux/kthread.h | 2 +-
include/linux/local_lock_internal.h | 18 +-
include/linux/lockdep.h | 170 ++++++++++++++++--
include/linux/lockdep_types.h | 8 +-
include/linux/mmu_notifier.h | 2 +-
include/linux/mutex.h | 12 +-
include/linux/percpu-rwsem.h | 4 +-
include/linux/regmap.h | 4 +-
include/linux/rtmutex.h | 14 +-
include/linux/rwlock_api_smp.h | 4 +-
include/linux/rwlock_rt.h | 4 +-
include/linux/rwlock_types.h | 11 +-
include/linux/rwsem.h | 14 +-
include/linux/seqlock.h | 4 +-
include/linux/spinlock_api_smp.h | 4 +-
include/linux/spinlock_rt.h | 4 +-
include/linux/spinlock_types.h | 4 +-
include/linux/spinlock_types_raw.h | 28 ++-
include/linux/swait.h | 2 +-
include/linux/tty_ldisc.h | 2 +-
include/linux/wait.h | 2 +-
include/linux/ww_mutex.h | 6 +-
include/media/v4l2-ctrls.h | 2 +-
include/net/sock.h | 2 +-
include/trace/events/lock.h | 4 +-
kernel/cgroup/rstat.c | 7 +-
kernel/locking/Makefile | 1 +
kernel/locking/lockdep.c | 40 ++++-
kernel/locking/mutex-debug.c | 2 +-
kernel/locking/mutex.c | 22 ++-
kernel/locking/mutex.h | 7 +
kernel/locking/percpu-rwsem.c | 2 +-
kernel/locking/rtmutex_api.c | 10 +-
kernel/locking/rwsem.c | 4 +-
kernel/locking/spinlock.c | 2 +-
kernel/locking/spinlock_debug.c | 4 +-
kernel/locking/spinlock_rt.c | 8 +-
kernel/locking/ww_rt_mutex.c | 2 +-
kernel/printk/printk.c | 14 +-
kernel/rcu/update.c | 27 +--
kernel/time/timer.c | 8 +-
kernel/workqueue.c | 13 ++
lib/Kconfig.debug | 14 ++
mm/memcontrol.c | 7 +-
mm/mmu_notifier.c | 2 +-
net/core/dev.c | 2 +-
net/sunrpc/svcsock.c | 2 +-
net/sunrpc/xprtsock.c | 2 +-
62 files changed, 391 insertions(+), 180 deletions(-)
base-commit: 1dc01abad6544cb9d884071b626b706e37aa9601
--
2.35.0.263.gb82422642f-goog
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-08 18:42:38
The raw_spin_lock_init() uses the argument to name its lockdep map.
But passing per_cpu_ptr() macro directly makes it a very very long
name as it expanded like below:
({ do { const void *__vpp_verify = (typeof((&cgroup_rstat_cpu_lock) ...
Let's fix it by passing a local variable instead. With this change,
the name now looks like:
cgrp_rstat_cpu_lock
Cc: Tejun Heo <redacted>
Cc: Zefan Li <redacted>
Cc: Johannes Weiner <redacted>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Namhyung Kim <redacted>
---
kernel/cgroup/rstat.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
On Tue, Feb 08, 2022 at 10:41:58AM -0800, Namhyung Kim wrote:
The raw_spin_lock_init() uses the argument to name its lockdep map.
But passing per_cpu_ptr() macro directly makes it a very very long
name as it expanded like below:
({ do { const void *__vpp_verify = (typeof((&cgroup_rstat_cpu_lock) ...
Let's fix it by passing a local variable instead. With this change,
the name now looks like:
cgrp_rstat_cpu_lock
Cc: Tejun Heo <tj@kernel.org>
Cc: Zefan Li <redacted>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: cgroups@vger.kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Acked-by: Tejun Heo <tj@kernel.org>
but maybe add a comment explaining what's going on?
Thanks.
--
tejun
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-08 19:14:25
Oops, I used the wrong email address of Paul. Sorry about that!
I'll resend with a new address soon.
Thanks,
Namhyung
On Tue, Feb 8, 2022 at 10:42 AM Namhyung Kim [off-list ref] wrote:
Hello,
There have been some requests for low-overhead kernel lock contention
monitoring. The kernel has CONFIG_LOCK_STAT to provide such an infra
either via /proc/lock_stat or tracepoints directly.
However it's not light-weight and hard to be used in production. So
I'm trying to separate out the tracepoints and using them as a base to
build a new monitoring system.
As the lockdep and lock_stat provide good hooks in the lock functions,
it'd be natural to reuse them. Actually I tried to use lockdep as is
but disables the functionality at runtime (initialize debug_locks = 0,
lock_stat = 0). But it still has unacceptable overhead and the
lockdep data structures also increase memory footprint unnecessarily.
So I'm proposing a separate tracepoint-only configuration and keeping
lockdep_map only with minimal information needed for tracepoints (for
now, it's just name). And then the existing lockdep hooks can be used
for the tracepoints.
The patch 01-06 are preparation for the work. In a few places in the
kernel, they calls lockdep annotation explicitly to deal with
limitations in the lockdep implementation. In my understanding, they
are not needed to analyze lock contention.
To make matters worse, they rely on the compiler optimization (or
macro magic) so that it can get rid of the annotations and their
arguments when lockdep is not configured.
But it's not true any more when lock tracepoints are added and it'd
cause build errors. So I added #ifdef guards for LOCKDEP in the code
to prevent such errors.
In the patch 07 I mechanically changed most of code that depend on
CONFIG_LOCKDEP or CONFIG_DEBUG_LOCK_ALLOC to CONFIG_LOCK_INFO. It
paves the way for the codes to be shared for lockdep and tracepoints.
Mostly, it makes sure that locks are initialized with a proper name,
like in the patch 08 and 09.
I didn't change some places intentionally - for example, timer and
workqueue depend on LOCKDEP explicitly since they use some lockdep
annotations to work around the "held lock freed" warnings. The ocfs2
directly accesses lockdep_map.key so I didn't touch the code for now.
And RCU was because it generates too much overhead thanks to the
rcu_read_lock(). Maybe I need to revisit some of them later.
I added CONFIG_LOCK_TRACEPOINTS in the patch 10 to make it optional.
I found that it adds 2~3% of overhead when I ran `perf bench sched
messaging` even when the tracepoints are disabled. The benchmark
creates a lot of processes and make them communicate by socket pairs
(or pipes). I measured that around 15% of lock acquisition creates
contentions and it's mostly for spin locks (alc->lock and u->lock).
I ran perf record + report with the workload and it showed 50% of the
cpu cycles are in the spin lock slow path. So it's highly affected by
the spin lock slow path. Actually LOCK_CONTENDED() macro transforms
the spin lock code (and others) to use trylock first and then fall
back to real lock function if failed. Thus it'd add more (atomic)
operations and cache line bouncing for the contended cases:
#define LOCK_CONTENDED(_lock, try, lock) \
do { \
if (!try(_lock)) { \
lock_contended(&(_lock)->dep_map, _RET_IP_); \
lock(_lock); \
} \
lock_acquired(&(_lock)->dep_map, _RET_IP_); \
} while (0)
If I modify the macro not to use trylock and to call the real lock
function directly (so the lock_contended tracepoint would be called
always, if enabled), the overhead goes down to 0.x% when the
tracepoints are disabled.
I don't have a good solution as long as we use LOCK_CONTENDED() macro
to separate the contended locking path. Maybe we can make it call
some (generic) slow path lock function directly after failing trylock.
Or move the lockdep annotations into the each lock function bodies and
get rid of the LOCK_CONTENDED() macro entirely. Ideas?
Actually the patch 11 handles the same issue on the mutex code. The
fast version of mutex trylock was attempted only if LOCKDEP is not
enabled and it affects the mutex lock performance in the uncontended
cases too. So I partially reverted the change in the patch 07 to use
the fast functions with lock tracepoints too. Maybe we can use it
with LOCKDEP as well?
The last patch 12 might be controversial and I'd like to move the
lock_acquired annotation into the if(!try) block in the LOCK_CONTEDED
macro so that it can only be called when there's a contention.
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
It'd change lock hold time calculation in lock_stat for the fast path,
but I assume time difference between lock_acquire and lock_acquired
would be small when the lock is not contended. So I think we can use
the timestamp in lock_acquire. If it's not acceptable, we might
consider adding a new tracepoint to track the timing of contended
locks.
This series base on the current tip/locking/core and you get it from
'locking/tracepoint-v1' branch in my tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
Thanks,
Namhyung
Cc: Thomas Gleixner <redacted>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Byungchul Park <redacted>
Cc: rcu@vger.kernel.org
Cc: cgroups@vger.kernel.org
Cc: linux-btrfs@vger.kernel.org
Cc: intel-gfx@lists.freedesktop.org
Namhyung Kim (12):
locking: Pass correct outer wait type info
cgroup: rstat: Make cgroup_rstat_cpu_lock name readable
timer: Protect lockdep functions with #ifdef
workqueue: Protect lockdep functions with #ifdef
drm/i915: Protect lockdep functions with #ifdef
btrfs: change lockdep class size check using ks->names
locking: Introduce CONFIG_LOCK_INFO
locking/mutex: Init name properly w/ CONFIG_LOCK_INFO
locking: Add more static lockdep init macros
locking: Add CONFIG_LOCK_TRACEPOINTS option
locking/mutex: Revive fast functions for LOCK_TRACEPOINTS
locking: Move lock_acquired() from the fast path
drivers/gpu/drm/drm_connector.c | 7 +-
drivers/gpu/drm/i915/i915_sw_fence.h | 2 +-
drivers/gpu/drm/i915/intel_wakeref.c | 3 +
drivers/gpu/drm/i915/selftests/lib_sw_fence.h | 2 +-
.../net/wireless/intel/iwlwifi/iwl-trans.c | 4 +-
.../net/wireless/intel/iwlwifi/iwl-trans.h | 2 +-
drivers/tty/tty_ldsem.c | 2 +-
fs/btrfs/disk-io.c | 4 +-
fs/btrfs/disk-io.h | 2 +-
fs/cifs/connect.c | 2 +-
fs/kernfs/file.c | 2 +-
include/linux/completion.h | 2 +-
include/linux/jbd2.h | 2 +-
include/linux/kernfs.h | 2 +-
include/linux/kthread.h | 2 +-
include/linux/local_lock_internal.h | 18 +-
include/linux/lockdep.h | 170 ++++++++++++++++--
include/linux/lockdep_types.h | 8 +-
include/linux/mmu_notifier.h | 2 +-
include/linux/mutex.h | 12 +-
include/linux/percpu-rwsem.h | 4 +-
include/linux/regmap.h | 4 +-
include/linux/rtmutex.h | 14 +-
include/linux/rwlock_api_smp.h | 4 +-
include/linux/rwlock_rt.h | 4 +-
include/linux/rwlock_types.h | 11 +-
include/linux/rwsem.h | 14 +-
include/linux/seqlock.h | 4 +-
include/linux/spinlock_api_smp.h | 4 +-
include/linux/spinlock_rt.h | 4 +-
include/linux/spinlock_types.h | 4 +-
include/linux/spinlock_types_raw.h | 28 ++-
include/linux/swait.h | 2 +-
include/linux/tty_ldisc.h | 2 +-
include/linux/wait.h | 2 +-
include/linux/ww_mutex.h | 6 +-
include/media/v4l2-ctrls.h | 2 +-
include/net/sock.h | 2 +-
include/trace/events/lock.h | 4 +-
kernel/cgroup/rstat.c | 7 +-
kernel/locking/Makefile | 1 +
kernel/locking/lockdep.c | 40 ++++-
kernel/locking/mutex-debug.c | 2 +-
kernel/locking/mutex.c | 22 ++-
kernel/locking/mutex.h | 7 +
kernel/locking/percpu-rwsem.c | 2 +-
kernel/locking/rtmutex_api.c | 10 +-
kernel/locking/rwsem.c | 4 +-
kernel/locking/spinlock.c | 2 +-
kernel/locking/spinlock_debug.c | 4 +-
kernel/locking/spinlock_rt.c | 8 +-
kernel/locking/ww_rt_mutex.c | 2 +-
kernel/printk/printk.c | 14 +-
kernel/rcu/update.c | 27 +--
kernel/time/timer.c | 8 +-
kernel/workqueue.c | 13 ++
lib/Kconfig.debug | 14 ++
mm/memcontrol.c | 7 +-
mm/mmu_notifier.c | 2 +-
net/core/dev.c | 2 +-
net/sunrpc/svcsock.c | 2 +-
net/sunrpc/xprtsock.c | 2 +-
62 files changed, 391 insertions(+), 180 deletions(-)
base-commit: 1dc01abad6544cb9d884071b626b706e37aa9601
--
2.35.0.263.gb82422642f-goog
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-08 19:17:07
Hi Tejun,
On Tue, Feb 8, 2022 at 10:46 AM Tejun Heo [off-list ref] wrote:
On Tue, Feb 08, 2022 at 10:41:58AM -0800, Namhyung Kim wrote:
quoted
The raw_spin_lock_init() uses the argument to name its lockdep map.
But passing per_cpu_ptr() macro directly makes it a very very long
name as it expanded like below:
({ do { const void *__vpp_verify = (typeof((&cgroup_rstat_cpu_lock) ...
Let's fix it by passing a local variable instead. With this change,
the name now looks like:
cgrp_rstat_cpu_lock
Cc: Tejun Heo <redacted>
Cc: Zefan Li <redacted>
Cc: Johannes Weiner <redacted>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Namhyung Kim <redacted>
Acked-by: Tejun Heo <redacted>
Thanks!
but maybe add a comment explaining what's going on?
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-08 23:52:01
On Tue, Feb 8, 2022 at 11:16 AM Namhyung Kim [off-list ref] wrote:
Hi Tejun,
On Tue, Feb 8, 2022 at 10:46 AM Tejun Heo [off-list ref] wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:58AM -0800, Namhyung Kim wrote:
quoted
The raw_spin_lock_init() uses the argument to name its lockdep map.
But passing per_cpu_ptr() macro directly makes it a very very long
name as it expanded like below:
({ do { const void *__vpp_verify = (typeof((&cgroup_rstat_cpu_lock) ...
Let's fix it by passing a local variable instead. With this change,
the name now looks like:
cgrp_rstat_cpu_lock
Cc: Tejun Heo <redacted>
Cc: Zefan Li <redacted>
Cc: Johannes Weiner <redacted>
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Namhyung Kim <redacted>
Acked-by: Tejun Heo <redacted>
Thanks!
quoted
but maybe add a comment explaining what's going on?
Sure, I'll add the comment.
Actually it depends on CONFIG_DEBUG_SPINLOCK.
If it's enabled, the name looks fine. But if not, the macro
expansion would happen and generate the long name.
Thanks,
Namhyung
From: Peter Zijlstra <peterz@infradead.org> Date: 2022-02-09 09:10:23
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
Yes, the name is convenient, but it's just not worth it IMO. It makes
the whole proposition too much of a trade-off.
Would it not be possible to reconstruct enough useful information from
the lock callsite?
From: Waiman Long <longman@redhat.com> Date: 2022-02-09 18:20:22
On 2/9/22 04:09, Peter Zijlstra wrote:
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
Yes, the name is convenient, but it's just not worth it IMO. It makes
the whole proposition too much of a trade-off.
Would it not be possible to reconstruct enough useful information from
the lock callsite?
I second that as I don't want to see the size of a spinlock exceeds 4 bytes in a production system.
Instead of storing additional information (e.g. lock name) directly into the lock itself. Maybe we can store it elsewhere and use the lock address as the key to locate it in a hash table. We can certainly extend the various lock init functions to do that. It will be trickier for statically initialized locks, but we can probably find a way to do that too.
Cheers,
Longman
----- On Feb 9, 2022, at 1:19 PM, Waiman Long longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
On 2/9/22 04:09, Peter Zijlstra wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
Yes, the name is convenient, but it's just not worth it IMO. It makes
the whole proposition too much of a trade-off.
Would it not be possible to reconstruct enough useful information from
the lock callsite?
I second that as I don't want to see the size of a spinlock exceeds 4
bytes in a production system.
Instead of storing additional information (e.g. lock name) directly into
the lock itself. Maybe we can store it elsewhere and use the lock
address as the key to locate it in a hash table. We can certainly extend
the various lock init functions to do that. It will be trickier for
statically initialized locks, but we can probably find a way to do that too.
If we go down that route, it would be nice if we can support a few different
use-cases for various tracers out there.
One use-case (a) requires the ability to query the lock name based on its address as key.
For this a hash table is a good fit. This would allow tracers like ftrace to
output lock names in its human-readable output which is formatted within the kernel.
Another use-case (b) is to be able to "dump" the lock { name, address } tuples
into the trace stream (we call this statedump events in lttng), and do the
translation from address to name at post-processing. This simply requires
that this information is available for iteration for both the core kernel
and module locks, so the tracer can dump this information on trace start
and module load.
Use-case (b) is very similar to what is done for the kernel tracepoints. Based
on this, implementing the init code that iterates on those sections and populates
a hash table for use-case (a) should be easy enough.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
From: Waiman Long <longman@redhat.com> Date: 2022-02-09 19:13:45
On 2/9/22 13:29, Mathieu Desnoyers wrote:
----- On Feb 9, 2022, at 1:19 PM, Waiman Long longman@redhat.com wrote:
quoted
On 2/9/22 04:09, Peter Zijlstra wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
Yes, the name is convenient, but it's just not worth it IMO. It makes
the whole proposition too much of a trade-off.
Would it not be possible to reconstruct enough useful information from
the lock callsite?
I second that as I don't want to see the size of a spinlock exceeds 4
bytes in a production system.
Instead of storing additional information (e.g. lock name) directly into
the lock itself. Maybe we can store it elsewhere and use the lock
address as the key to locate it in a hash table. We can certainly extend
the various lock init functions to do that. It will be trickier for
statically initialized locks, but we can probably find a way to do that too.
If we go down that route, it would be nice if we can support a few different
use-cases for various tracers out there.
One use-case (a) requires the ability to query the lock name based on its address as key.
For this a hash table is a good fit. This would allow tracers like ftrace to
output lock names in its human-readable output which is formatted within the kernel.
Another use-case (b) is to be able to "dump" the lock { name, address } tuples
into the trace stream (we call this statedump events in lttng), and do the
translation from address to name at post-processing. This simply requires
that this information is available for iteration for both the core kernel
and module locks, so the tracer can dump this information on trace start
and module load.
Use-case (b) is very similar to what is done for the kernel tracepoints. Based
on this, implementing the init code that iterates on those sections and populates
a hash table for use-case (a) should be easy enough.
Yes, that are good use cases for this type of functionality. I do need to think about how to do it for statically initialized lock first.
Thanks,
Longman
----- On Feb 9, 2022, at 2:22 PM, Namhyung Kim namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
Hello,
On Wed, Feb 9, 2022 at 11:02 AM Waiman Long [off-list ref] wrote:
quoted
On 2/9/22 13:29, Mathieu Desnoyers wrote:
quoted
----- On Feb 9, 2022, at 1:19 PM, Waiman Long longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
quoted
On 2/9/22 04:09, Peter Zijlstra wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
Yes, the name is convenient, but it's just not worth it IMO. It makes
the whole proposition too much of a trade-off.
Would it not be possible to reconstruct enough useful information from
the lock callsite?
I second that as I don't want to see the size of a spinlock exceeds 4
bytes in a production system.
Instead of storing additional information (e.g. lock name) directly into
the lock itself. Maybe we can store it elsewhere and use the lock
address as the key to locate it in a hash table. We can certainly extend
the various lock init functions to do that. It will be trickier for
statically initialized locks, but we can probably find a way to do that too.
If we go down that route, it would be nice if we can support a few different
use-cases for various tracers out there.
One use-case (a) requires the ability to query the lock name based on its
address as key.
For this a hash table is a good fit. This would allow tracers like ftrace to
output lock names in its human-readable output which is formatted within the
kernel.
Another use-case (b) is to be able to "dump" the lock { name, address } tuples
into the trace stream (we call this statedump events in lttng), and do the
translation from address to name at post-processing. This simply requires
that this information is available for iteration for both the core kernel
and module locks, so the tracer can dump this information on trace start
and module load.
Use-case (b) is very similar to what is done for the kernel tracepoints. Based
on this, implementing the init code that iterates on those sections and
populates
a hash table for use-case (a) should be easy enough.
Yes, that are good use cases for this type of functionality. I do need
to think about how to do it for statically initialized lock first.
Thank you all for the review and good suggestions.
I'm also concerning dynamic allocated locks in a data structure.
If we keep the info in a hash table, we should delete it when the
lock is gone. I'm not sure we have a good place to hook it up all.
I was wondering about this use case as well. Can we make it mandatory to
declare the lock "class" (including the name) statically, even though the
lock per-se is allocated dynamically ? Then the initialization of the lock
embedded within the data structure would simply refer to the lock class
definition.
But perhaps I am missing something here.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-09 19:31:58
Hello,
On Wed, Feb 9, 2022 at 11:02 AM Waiman Long [off-list ref] wrote:
On 2/9/22 13:29, Mathieu Desnoyers wrote:
quoted
----- On Feb 9, 2022, at 1:19 PM, Waiman Long longman@redhat.com wrote:
quoted
On 2/9/22 04:09, Peter Zijlstra wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
Yes, the name is convenient, but it's just not worth it IMO. It makes
the whole proposition too much of a trade-off.
Would it not be possible to reconstruct enough useful information from
the lock callsite?
I second that as I don't want to see the size of a spinlock exceeds 4
bytes in a production system.
Instead of storing additional information (e.g. lock name) directly into
the lock itself. Maybe we can store it elsewhere and use the lock
address as the key to locate it in a hash table. We can certainly extend
the various lock init functions to do that. It will be trickier for
statically initialized locks, but we can probably find a way to do that too.
If we go down that route, it would be nice if we can support a few different
use-cases for various tracers out there.
One use-case (a) requires the ability to query the lock name based on its address as key.
For this a hash table is a good fit. This would allow tracers like ftrace to
output lock names in its human-readable output which is formatted within the kernel.
Another use-case (b) is to be able to "dump" the lock { name, address } tuples
into the trace stream (we call this statedump events in lttng), and do the
translation from address to name at post-processing. This simply requires
that this information is available for iteration for both the core kernel
and module locks, so the tracer can dump this information on trace start
and module load.
Use-case (b) is very similar to what is done for the kernel tracepoints. Based
on this, implementing the init code that iterates on those sections and populates
a hash table for use-case (a) should be easy enough.
Yes, that are good use cases for this type of functionality. I do need
to think about how to do it for statically initialized lock first.
Thank you all for the review and good suggestions.
I'm also concerning dynamic allocated locks in a data structure.
If we keep the info in a hash table, we should delete it when the
lock is gone. I'm not sure we have a good place to hook it up all.
Thanks,
Namhyung
----- On Feb 9, 2022, at 2:02 PM, Waiman Long longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
On 2/9/22 13:29, Mathieu Desnoyers wrote:
quoted
----- On Feb 9, 2022, at 1:19 PM, Waiman Long longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org wrote:
quoted
On 2/9/22 04:09, Peter Zijlstra wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
Yes, the name is convenient, but it's just not worth it IMO. It makes
the whole proposition too much of a trade-off.
Would it not be possible to reconstruct enough useful information from
the lock callsite?
I second that as I don't want to see the size of a spinlock exceeds 4
bytes in a production system.
Instead of storing additional information (e.g. lock name) directly into
the lock itself. Maybe we can store it elsewhere and use the lock
address as the key to locate it in a hash table. We can certainly extend
the various lock init functions to do that. It will be trickier for
statically initialized locks, but we can probably find a way to do that too.
If we go down that route, it would be nice if we can support a few different
use-cases for various tracers out there.
One use-case (a) requires the ability to query the lock name based on its
address as key.
For this a hash table is a good fit. This would allow tracers like ftrace to
output lock names in its human-readable output which is formatted within the
kernel.
Another use-case (b) is to be able to "dump" the lock { name, address } tuples
into the trace stream (we call this statedump events in lttng), and do the
translation from address to name at post-processing. This simply requires
that this information is available for iteration for both the core kernel
and module locks, so the tracer can dump this information on trace start
and module load.
Use-case (b) is very similar to what is done for the kernel tracepoints. Based
on this, implementing the init code that iterates on those sections and
populates
a hash table for use-case (a) should be easy enough.
Yes, that are good use cases for this type of functionality. I do need
to think about how to do it for statically initialized lock first.
Tracepoints already solved that problem.
Look at the macro DEFINE_TRACE_FN() in include/linux/tracepoint.h. You will notice that
it statically defines a struct tracepoint in a separate section and a tracepoint_ptr_t
in a __tracepoints_ptrs section.
Then the other parts of the picture are in kernel/tracepoint.c:
extern tracepoint_ptr_t __start___tracepoints_ptrs[];
extern tracepoint_ptr_t __stop___tracepoints_ptrs[];
and kernel/module.c:find_module_sections()
#ifdef CONFIG_TRACEPOINTS
mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
sizeof(*mod->tracepoints_ptrs),
&mod->num_tracepoints);
#endif
and the iteration code over kernel and modules in kernel/tracepoint.c.
All you need in addition is in include/asm-generic/vmlinux.lds.h, we add
to the DATA_DATA define an entry such as:
STRUCT_ALIGN(); \
*(__tracepoints) \
and in RO_DATA:
. = ALIGN(8); \
__start___tracepoints_ptrs = .; \
KEEP(*(__tracepoints_ptrs)) /* Tracepoints: pointer array */ \
__stop___tracepoints_ptrs = .;
AFAIU, if you do something similar for a structure that contains your relevant
lock information, it should be straightforward to handle statically initialized
locks.
Thanks,
Mathieu
From: Waiman Long <longman@redhat.com> Date: 2022-02-09 19:38:52
On 2/9/22 14:17, Mathieu Desnoyers wrote:
----- On Feb 9, 2022, at 2:02 PM, Waiman Long longman@redhat.com wrote:
quoted
On 2/9/22 13:29, Mathieu Desnoyers wrote:
quoted
----- On Feb 9, 2022, at 1:19 PM, Waiman Long longman@redhat.com wrote:
quoted
On 2/9/22 04:09, Peter Zijlstra wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
Yes, the name is convenient, but it's just not worth it IMO. It makes
the whole proposition too much of a trade-off.
Would it not be possible to reconstruct enough useful information from
the lock callsite?
I second that as I don't want to see the size of a spinlock exceeds 4
bytes in a production system.
Instead of storing additional information (e.g. lock name) directly into
the lock itself. Maybe we can store it elsewhere and use the lock
address as the key to locate it in a hash table. We can certainly extend
the various lock init functions to do that. It will be trickier for
statically initialized locks, but we can probably find a way to do that too.
If we go down that route, it would be nice if we can support a few different
use-cases for various tracers out there.
One use-case (a) requires the ability to query the lock name based on its
address as key.
For this a hash table is a good fit. This would allow tracers like ftrace to
output lock names in its human-readable output which is formatted within the
kernel.
Another use-case (b) is to be able to "dump" the lock { name, address } tuples
into the trace stream (we call this statedump events in lttng), and do the
translation from address to name at post-processing. This simply requires
that this information is available for iteration for both the core kernel
and module locks, so the tracer can dump this information on trace start
and module load.
Use-case (b) is very similar to what is done for the kernel tracepoints. Based
on this, implementing the init code that iterates on those sections and
populates
a hash table for use-case (a) should be easy enough.
Yes, that are good use cases for this type of functionality. I do need
to think about how to do it for statically initialized lock first.
Tracepoints already solved that problem.
Look at the macro DEFINE_TRACE_FN() in include/linux/tracepoint.h. You will notice that
it statically defines a struct tracepoint in a separate section and a tracepoint_ptr_t
in a __tracepoints_ptrs section.
Then the other parts of the picture are in kernel/tracepoint.c:
extern tracepoint_ptr_t __start___tracepoints_ptrs[];
extern tracepoint_ptr_t __stop___tracepoints_ptrs[];
and kernel/module.c:find_module_sections()
#ifdef CONFIG_TRACEPOINTS
mod->tracepoints_ptrs = section_objs(info, "__tracepoints_ptrs",
sizeof(*mod->tracepoints_ptrs),
&mod->num_tracepoints);
#endif
and the iteration code over kernel and modules in kernel/tracepoint.c.
All you need in addition is in include/asm-generic/vmlinux.lds.h, we add
to the DATA_DATA define an entry such as:
STRUCT_ALIGN(); \
*(__tracepoints) \
and in RO_DATA:
. = ALIGN(8); \
__start___tracepoints_ptrs = .; \
KEEP(*(__tracepoints_ptrs)) /* Tracepoints: pointer array */ \
__stop___tracepoints_ptrs = .;
AFAIU, if you do something similar for a structure that contains your relevant
lock information, it should be straightforward to handle statically initialized
locks.
Thanks,
Mathieu
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-09 19:46:22
On Wed, Feb 9, 2022 at 11:28 AM Mathieu Desnoyers
[off-list ref] wrote:
----- On Feb 9, 2022, at 2:22 PM, Namhyung Kim namhyung@kernel.org wrote:
quoted
I'm also concerning dynamic allocated locks in a data structure.
If we keep the info in a hash table, we should delete it when the
lock is gone. I'm not sure we have a good place to hook it up all.
I was wondering about this use case as well. Can we make it mandatory to
declare the lock "class" (including the name) statically, even though the
lock per-se is allocated dynamically ? Then the initialization of the lock
embedded within the data structure would simply refer to the lock class
definition.
Isn't it still the same if we have static lock classes that the entry needs
to be deleted from the hash table when it frees the data structure?
I'm more concerned about free than alloc as there seems to be no
API to track that in a place.
Thanks,
Namhyung
----- On Feb 9, 2022, at 2:45 PM, Namhyung Kim namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
On Wed, Feb 9, 2022 at 11:28 AM Mathieu Desnoyers
[off-list ref] wrote:
quoted
----- On Feb 9, 2022, at 2:22 PM, Namhyung Kim namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
quoted
I'm also concerning dynamic allocated locks in a data structure.
If we keep the info in a hash table, we should delete it when the
lock is gone. I'm not sure we have a good place to hook it up all.
I was wondering about this use case as well. Can we make it mandatory to
declare the lock "class" (including the name) statically, even though the
lock per-se is allocated dynamically ? Then the initialization of the lock
embedded within the data structure would simply refer to the lock class
definition.
Isn't it still the same if we have static lock classes that the entry needs
to be deleted from the hash table when it frees the data structure?
I'm more concerned about free than alloc as there seems to be no
API to track that in a place.
If the lock class is defined statically, even for dynamically initialized
locks, then its associated object sits either in the core kernel or within
a module.
So if it's in the core kernel, it could be added to the hash table at kernel
init and stay there forever.
If it's in a module, it would be added to the hash table on module load and
removed on module unload. We would have to be careful about how the ftrace
printout to human readable text deals with missing hash table data, because
that printout will happen after buffering, so an untimely module unload could
make this address to string mapping unavailable for a few events. If we care
about getting this corner case right there are a few things we could do as
well.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
From: Waiman Long <longman@redhat.com> Date: 2022-02-09 20:18:00
On 2/9/22 14:45, Namhyung Kim wrote:
On Wed, Feb 9, 2022 at 11:28 AM Mathieu Desnoyers
[off-list ref] wrote:
quoted
----- On Feb 9, 2022, at 2:22 PM, Namhyung Kim namhyung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote:
quoted
I'm also concerning dynamic allocated locks in a data structure.
If we keep the info in a hash table, we should delete it when the
lock is gone. I'm not sure we have a good place to hook it up all.
I was wondering about this use case as well. Can we make it mandatory to
declare the lock "class" (including the name) statically, even though the
lock per-se is allocated dynamically ? Then the initialization of the lock
embedded within the data structure would simply refer to the lock class
definition.
Isn't it still the same if we have static lock classes that the entry needs
to be deleted from the hash table when it frees the data structure?
I'm more concerned about free than alloc as there seems to be no
API to track that in a place.
We may have to invent some new APIs to do that. For example, spin_lock_exit() can be the counterpart of spin_lock_init() and so on. Of course, existing kernel code have to be modified to designate the point after which a lock is no longer being used or is freed.
Cheers,
Longman
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-10 02:00:34
On Wed, Feb 9, 2022 at 12:17 PM Waiman Long [off-list ref] wrote:
On 2/9/22 14:45, Namhyung Kim wrote:
quoted
On Wed, Feb 9, 2022 at 11:28 AM Mathieu Desnoyers
[off-list ref] wrote:
quoted
----- On Feb 9, 2022, at 2:22 PM, Namhyung Kim namhyung@kernel.org wrote:
quoted
I'm also concerning dynamic allocated locks in a data structure.
If we keep the info in a hash table, we should delete it when the
lock is gone. I'm not sure we have a good place to hook it up all.
I was wondering about this use case as well. Can we make it mandatory to
declare the lock "class" (including the name) statically, even though the
lock per-se is allocated dynamically ? Then the initialization of the lock
embedded within the data structure would simply refer to the lock class
definition.
Isn't it still the same if we have static lock classes that the entry needs
to be deleted from the hash table when it frees the data structure?
I'm more concerned about free than alloc as there seems to be no
API to track that in a place.
We may have to invent some new APIs to do that. For example,
spin_lock_exit() can be the counterpart of spin_lock_init() and so on.
Of course, existing kernel code have to be modified to designate the
point after which a lock is no longer being used or is freed.
Yeah, but I'm afraid that it could be easy to miss something.
Also it would add some runtime overhead due to maintaining
the hash table even if the tracepoints are not used.
Thanks,
Namhyung
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-10 02:02:42
On Wed, Feb 9, 2022 at 1:09 AM Peter Zijlstra [off-list ref] wrote:
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
If it's only lockref, is it possible to change it to use arch_spinlock_t
so that it can remain in 4 bytes? It'd be really nice if we can keep
spin lock size, but it'd be easier to carry the name with it for
analysis IMHO.
Thanks,
Namhyung
From: Waiman Long <longman@redhat.com> Date: 2022-02-10 02:12:39
On 2/9/22 19:27, Namhyung Kim wrote:
On Wed, Feb 9, 2022 at 12:17 PM Waiman Long [off-list ref] wrote:
quoted
On 2/9/22 14:45, Namhyung Kim wrote:
quoted
On Wed, Feb 9, 2022 at 11:28 AM Mathieu Desnoyers
[off-list ref] wrote:
quoted
----- On Feb 9, 2022, at 2:22 PM, Namhyung Kim namhyung@kernel.org wrote:
quoted
I'm also concerning dynamic allocated locks in a data structure.
If we keep the info in a hash table, we should delete it when the
lock is gone. I'm not sure we have a good place to hook it up all.
I was wondering about this use case as well. Can we make it mandatory to
declare the lock "class" (including the name) statically, even though the
lock per-se is allocated dynamically ? Then the initialization of the lock
embedded within the data structure would simply refer to the lock class
definition.
Isn't it still the same if we have static lock classes that the entry needs
to be deleted from the hash table when it frees the data structure?
I'm more concerned about free than alloc as there seems to be no
API to track that in a place.
We may have to invent some new APIs to do that. For example,
spin_lock_exit() can be the counterpart of spin_lock_init() and so on.
Of course, existing kernel code have to be modified to designate the
point after which a lock is no longer being used or is freed.
Yeah, but I'm afraid that it could be easy to miss something.
Also it would add some runtime overhead due to maintaining
the hash table even if the tracepoints are not used.
Yes, there is some overhead at lock init and exit time, but it is generally negligible if the lock is used multiple times. The hash table will consume some additional memory if configured, but it shouldn't be much.
Cheers,
Longman
From: Peter Zijlstra <peterz@infradead.org> Date: 2022-02-10 09:14:30
On Wed, Feb 09, 2022 at 04:32:58PM -0800, Namhyung Kim wrote:
On Wed, Feb 9, 2022 at 1:09 AM Peter Zijlstra [off-list ref] wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
If it's only lockref, is it possible to change it to use arch_spinlock_t
so that it can remain in 4 bytes? It'd be really nice if we can keep
spin lock size, but it'd be easier to carry the name with it for
analysis IMHO.
It's just vile and disgusting to blow up the lock size for convenience
like this.
And no, there's more of that around. A lot of effort has been spend to
make sure spinlocks are 32bit and we're not going to give that up for
something as daft as this.
Just think harder on the analysis side. Like said; I'm thinking the
caller IP should be good enough most of the time.
From: Peter Zijlstra <peterz@infradead.org> Date: 2022-02-10 09:33:47
On Wed, Feb 09, 2022 at 03:17:38PM -0500, Waiman Long wrote:
On 2/9/22 14:45, Namhyung Kim wrote:
quoted
On Wed, Feb 9, 2022 at 11:28 AM Mathieu Desnoyers
[off-list ref] wrote:
quoted
----- On Feb 9, 2022, at 2:22 PM, Namhyung Kim namhyung@kernel.org wrote:
quoted
I'm also concerning dynamic allocated locks in a data structure.
If we keep the info in a hash table, we should delete it when the
lock is gone. I'm not sure we have a good place to hook it up all.
I was wondering about this use case as well. Can we make it mandatory to
declare the lock "class" (including the name) statically, even though the
lock per-se is allocated dynamically ? Then the initialization of the lock
embedded within the data structure would simply refer to the lock class
definition.
Isn't it still the same if we have static lock classes that the entry needs
to be deleted from the hash table when it frees the data structure?
I'm more concerned about free than alloc as there seems to be no
API to track that in a place.
We may have to invent some new APIs to do that. For example,
spin_lock_exit() can be the counterpart of spin_lock_init() and so on. Of
course, existing kernel code have to be modified to designate the point
after which a lock is no longer being used or is freed.
The canonical name is _destroy(). We even have mutex_destroy() except
it's usage isn't mandatory.
The easy way out is doing as lockdep does and hook into the memory
allocators and check every free'd hunk of memory for a lock. It does
hoever mean your data structure of choice needs to be able to answer: do
I have an entry in @range. Which mostly disqualifies a hash-table.
Still, I really don't think you need any of this, it's just bloat. A
very limited stack unwind for one of the two tracepoints should allow
you to find the offending lock just fine.
From: "Paul E. McKenney" <paulmck@kernel.org> Date: 2022-02-10 19:14:11
On Thu, Feb 10, 2022 at 10:13:53AM +0100, Peter Zijlstra wrote:
On Wed, Feb 09, 2022 at 04:32:58PM -0800, Namhyung Kim wrote:
quoted
On Wed, Feb 9, 2022 at 1:09 AM Peter Zijlstra [off-list ref] wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
If it's only lockref, is it possible to change it to use arch_spinlock_t
so that it can remain in 4 bytes? It'd be really nice if we can keep
spin lock size, but it'd be easier to carry the name with it for
analysis IMHO.
It's just vile and disgusting to blow up the lock size for convenience
like this.
And no, there's more of that around. A lot of effort has been spend to
make sure spinlocks are 32bit and we're not going to give that up for
something as daft as this.
Just think harder on the analysis side. Like said; I'm thinking the
caller IP should be good enough most of the time.
Another option is to keep any additional storage in a separate data
structure keyed off of lock address, lockdep class, or whatever.
Whether or not this is a -good- option, well, who knows? ;-)
Thanx, Paul
From: Waiman Long <longman@redhat.com> Date: 2022-02-10 19:27:27
On 2/10/22 14:14, Paul E. McKenney wrote:
On Thu, Feb 10, 2022 at 10:13:53AM +0100, Peter Zijlstra wrote:
quoted
On Wed, Feb 09, 2022 at 04:32:58PM -0800, Namhyung Kim wrote:
quoted
On Wed, Feb 9, 2022 at 1:09 AM Peter Zijlstra [off-list ref] wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
If it's only lockref, is it possible to change it to use arch_spinlock_t
so that it can remain in 4 bytes? It'd be really nice if we can keep
spin lock size, but it'd be easier to carry the name with it for
analysis IMHO.
It's just vile and disgusting to blow up the lock size for convenience
like this.
And no, there's more of that around. A lot of effort has been spend to
make sure spinlocks are 32bit and we're not going to give that up for
something as daft as this.
Just think harder on the analysis side. Like said; I'm thinking the
caller IP should be good enough most of the time.
Another option is to keep any additional storage in a separate data
structure keyed off of lock address, lockdep class, or whatever.
Whether or not this is a -good- option, well, who knows? ;-)
I have suggested that too. Unfortunately, I was replying to an email with your wrong email address. So you might not have received it.
Cheers,
Longman
From: "Paul E. McKenney" <paulmck@kernel.org> Date: 2022-02-10 20:11:04
On Thu, Feb 10, 2022 at 02:27:11PM -0500, Waiman Long wrote:
On 2/10/22 14:14, Paul E. McKenney wrote:
quoted
On Thu, Feb 10, 2022 at 10:13:53AM +0100, Peter Zijlstra wrote:
quoted
On Wed, Feb 09, 2022 at 04:32:58PM -0800, Namhyung Kim wrote:
quoted
On Wed, Feb 9, 2022 at 1:09 AM Peter Zijlstra [off-list ref] wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
If it's only lockref, is it possible to change it to use arch_spinlock_t
so that it can remain in 4 bytes? It'd be really nice if we can keep
spin lock size, but it'd be easier to carry the name with it for
analysis IMHO.
It's just vile and disgusting to blow up the lock size for convenience
like this.
And no, there's more of that around. A lot of effort has been spend to
make sure spinlocks are 32bit and we're not going to give that up for
something as daft as this.
Just think harder on the analysis side. Like said; I'm thinking the
caller IP should be good enough most of the time.
Another option is to keep any additional storage in a separate data
structure keyed off of lock address, lockdep class, or whatever.
Whether or not this is a -good- option, well, who knows? ;-)
I have suggested that too. Unfortunately, I was replying to an email with
your wrong email address. So you might not have received it.
Plus I was too lazy to go look at lore. ;-)
For whatever it is worth, we did something similar in DYNIX/ptx, whose
spinlocks were limited to a single byte. But it does have its drawbacks.
Thanx, Paul
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-11 05:55:47
On Thu, Feb 10, 2022 at 1:14 AM Peter Zijlstra [off-list ref] wrote:
On Wed, Feb 09, 2022 at 04:32:58PM -0800, Namhyung Kim wrote:
quoted
On Wed, Feb 9, 2022 at 1:09 AM Peter Zijlstra [off-list ref] wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
If it's only lockref, is it possible to change it to use arch_spinlock_t
so that it can remain in 4 bytes? It'd be really nice if we can keep
spin lock size, but it'd be easier to carry the name with it for
analysis IMHO.
It's just vile and disgusting to blow up the lock size for convenience
like this.
And no, there's more of that around. A lot of effort has been spend to
make sure spinlocks are 32bit and we're not going to give that up for
something as daft as this.
Just think harder on the analysis side. Like said; I'm thinking the
caller IP should be good enough most of the time.
Ok, I'll go in this direction then.
So you are ok with adding two new tracepoints, even if they are
similar to what we already have in lockdep/lock_stat, right?
Thanks,
Namhyung
From: Namhyung Kim <namhyung@kernel.org> Date: 2022-02-11 05:58:01
Hi Paul,
On Thu, Feb 10, 2022 at 12:10 PM Paul E. McKenney [off-list ref] wrote:
On Thu, Feb 10, 2022 at 02:27:11PM -0500, Waiman Long wrote:
quoted
On 2/10/22 14:14, Paul E. McKenney wrote:
quoted
On Thu, Feb 10, 2022 at 10:13:53AM +0100, Peter Zijlstra wrote:
quoted
On Wed, Feb 09, 2022 at 04:32:58PM -0800, Namhyung Kim wrote:
quoted
On Wed, Feb 9, 2022 at 1:09 AM Peter Zijlstra [off-list ref] wrote:
quoted
On Tue, Feb 08, 2022 at 10:41:56AM -0800, Namhyung Kim wrote:
quoted
Eventually I'm mostly interested in the contended locks only and I
want to reduce the overhead in the fast path. By moving that, it'd be
easy to track contended locks with timing by using two tracepoints.
So why not put in two new tracepoints and call it a day?
Why muck about with all that lockdep stuff just to preserve the name
(and in the process continue to blow up data structures etc..). This
leaves distros in a bind, will they enable this config and provide
tracepoints while bloating the data structures and destroying things
like lockref (which relies on sizeof(spinlock_t)), or not provide this
at all.
If it's only lockref, is it possible to change it to use arch_spinlock_t
so that it can remain in 4 bytes? It'd be really nice if we can keep
spin lock size, but it'd be easier to carry the name with it for
analysis IMHO.
It's just vile and disgusting to blow up the lock size for convenience
like this.
And no, there's more of that around. A lot of effort has been spend to
make sure spinlocks are 32bit and we're not going to give that up for
something as daft as this.
Just think harder on the analysis side. Like said; I'm thinking the
caller IP should be good enough most of the time.
Another option is to keep any additional storage in a separate data
structure keyed off of lock address, lockdep class, or whatever.
Whether or not this is a -good- option, well, who knows? ;-)
I have suggested that too. Unfortunately, I was replying to an email with
your wrong email address. So you might not have received it.
Plus I was too lazy to go look at lore. ;-)
Sorry for the noise about the email address in the first place.
It has been so long since the last time I sent you a patch..
Thanks,
Namhyung