Add tracepoints around mmap_lock acquisition

PATCH Superseded Axel Rasmussen 2 participants 6 messages Oct 7, 2020 – Oct 8, 2020

Subsystems (3)

Memory management Tracing Memory mapping - locking

lore

Share & tools

lore

Axel Rasmussen

author

2020-10-07 18:44 UTC #0 lore raw

v2 cover Add tracepoints around mmap_lock acquisition

This patchset adds tracepoints around mmap_lock acquisition. This is useful so we can measure the latency of lock acquisition, in order to detect contention.

This version is based upon linux-next (since it depends on some recently-merged patches [1] [2]), and incorporates feedback from the review of the previous version:

- Functions renamed to reserve the "trace_" prefix for actual tracepoints.

- We no longer measure the duration directly. Instead, users are expected to construct a synthetic event which computes the interval between "start locking" and "acquire returned".

- The new helper for checking if tracepoints are enabled in a header is used to avoid un-inlining any of the lock wrappers. This yields ~zero overhead if the tracepoints aren't enabled, and therefore obviates the need for a Kconfig for this change.

[1] https://lore.kernel.org/patchwork/patch/1316922/
[2] https://lore.kernel.org/patchwork/patch/1311996/

Axel Rasmussen (2):
tracing: support "bool" type in synthetic trace events
mmap_lock: add tracepoints around lock acquisition

 include/linux/mmap_lock.h         | 112 +++++++++++++++++++++++++++---
 include/trace/events/mmap_lock.h  |  70 +++++++++++++++++++
 kernel/trace/trace_events_synth.c |   4 ++
 mm/Makefile                       |   2 +-
 mm/mmap_lock.c                    |  87 +++++++++++++++++++++++
 5 files changed, 265 insertions(+), 10 deletions(-)
 create mode 100644 include/trace/events/mmap_lock.h
 create mode 100644 mm/mmap_lock.c

--
2.28.0.806.g8561365e88-goog

Axel Rasmussen

author

2020-10-07 18:44 UTC #1 lore raw

PATCH v2 1/2 tracing: support "bool" type in synthetic trace events

It's common [1] to define tracepoint fields as "bool" when they contain a true / false value. Currently, defining a synthetic event with a "bool" field yields EINVAL. It's possible to work around this by using e.g. u8 (assuming sizeof(bool) is 1, and bool is unsigned; if either of these properties don't match, you get EINVAL [2]).

Supporting "bool" explicitly makes hooking this up easier and more portable for userspace.

[1]: grep -r "bool" include/trace/events/
[2]: check_synth_field() in kernel/trace/trace_events_hist.c

Signed-off-by Axel Rasmussen <[email protected]>

---
 kernel/trace/trace_events_synth.c | 4 ++++
 1 file changed, 4 insertions(+)

kernel/trace/trace_events_synth.c +4 -0

index 742ce5f62d6d..92eb530ec287 100644
@@ -187,6 +187,8 @@ static int synth_field_size(char *type)
 		size = sizeof(long);
 	else if (strcmp(type, "unsigned long") == 0)
 		size = sizeof(unsigned long);
+	else if (strcmp(type, "bool") == 0)
+		size = sizeof(bool);
 	else if (strcmp(type, "pid_t") == 0)
 		size = sizeof(pid_t);
 	else if (strcmp(type, "gfp_t") == 0)

@@ -229,6 +231,8 @@ static const char *synth_field_fmt(char *type)
 		fmt = "%ld";
 	else if (strcmp(type, "unsigned long") == 0)
 		fmt = "%lu";
+	else if (strcmp(type, "bool") == 0)
+		fmt = "%d";
 	else if (strcmp(type, "pid_t") == 0)
 		fmt = "%d";
 	else if (strcmp(type, "gfp_t") == 0)

Signature · 1 line

2.28.0.806.g8561365e88-goog

Michel Lespinasse

Acked-by on v2 1/2

2020-10-08 07:22 UTC #2 lore raw

Re: [PATCH v2 1/2] tracing: support "bool" type in synthetic trace events

Quoted 8 lines · Axel Rasmussen

It's common [1] to define tracepoint fields as "bool" when they contain a true / false value. Currently, defining a synthetic event with a "bool" field yields EINVAL. It's possible to work around this by using e.g. u8 (assuming sizeof(bool) is 1, and bool is unsigned; if either of these properties don't match, you get EINVAL [2]).
Supporting "bool" explicitly makes hooking this up easier and more portable for userspace.

Acked-by Michel Lespinasse <[email protected]>

Looks fine to me, but you really want to get Steven's opinion over mine here :)

Axel Rasmussen

author

2020-10-07 18:44 UTC #3 lore raw

PATCH v2 2/2 mmap_lock: add tracepoints around lock acquisition

In reply to Axel Rasmussen

The goal of these tracepoints is to be able to debug lock contention issues. This lock is acquired on most (all?) mmap / munmap / page fault operations, so a multi-threaded process which does a lot of these can experience significant contention.

We trace just before we start acquisition, when the acquisition returns (whether it succeeded or not), and when the lock is released (or downgraded). The events are broken out by lock type (read / write).

The events are also broken out by memcg path. For container-based workloads, users often think of several processes in a memcg as a single logical "task", so collecting statistics at this level is useful.

The end goal is to get latency information. This isn't directly included in the trace events. Instead, users are expected to compute the time between "start locking" and "acquire returned", using e.g. synthetic events or BPF. The benefit we get from this is simpler code.

Because we use tracepoint_enabled() to decide whether or not to trace, this patch has effectively no overhead unless tracepoints are enabled at runtime. If tracepoints are enabled, there is a performance impact, but how much depends on exactly what e.g. the BPF program does.

Signed-off-by Axel Rasmussen <[email protected]>

---
 include/linux/mmap_lock.h        | 112 ++++++++++++++++++++++++++++---
 include/trace/events/mmap_lock.h |  70 +++++++++++++++++++
 mm/Makefile                      |   2 +-
 mm/mmap_lock.c                   |  87 ++++++++++++++++++++++++
 4 files changed, 261 insertions(+), 10 deletions(-)
 create mode 100644 include/trace/events/mmap_lock.h
 create mode 100644 mm/mmap_lock.c

include/linux/mmap_lock.h +103 -9

index e1afb420fc94..aa3d64f99d17 100644
@@ -1,11 +1,95 @@
 #ifndef _LINUX_MMAP_LOCK_H
 #define _LINUX_MMAP_LOCK_H
 
+#include <linux/lockdep.h>
+#include <linux/mm_types.h>
 #include <linux/mmdebug.h>
+#include <linux/rwsem.h>
+#include <linux/tracepoint-defs.h>
+#include <linux/types.h>
 
 #define MMAP_LOCK_INITIALIZER(name) \
 	.mmap_lock = __RWSEM_INITIALIZER((name).mmap_lock),
 
+DECLARE_TRACEPOINT(mmap_lock_start_locking);
+DECLARE_TRACEPOINT(mmap_lock_acquire_returned);
+DECLARE_TRACEPOINT(mmap_lock_released);
+
+#ifdef CONFIG_TRACING
+
+void __mmap_lock_do_trace_start_locking(struct mm_struct *mm, bool write);
+void __mmap_lock_do_trace_acquire_returned(struct mm_struct *mm, bool write,
+					   bool success);
+void __mmap_lock_do_trace_released(struct mm_struct *mm, bool write);
+
+#else /* !COFNIG_TRACING */
+
+static inline void __mmap_lock_do_trace_start_locking(struct mm_struct *mm,
+						      bool write)
+{
+}
+
+static inline void __mmap_lock_do_trace_acquire_returned(struct mm_struct *mm,
+							 bool write,
+							 bool success)
+{
+}
+
+static inline void __mmap_lock_do_trace_released(struct mm_struct *mm,
+						 bool write)
+{
+}
+
+#endif /* CONFIG_TRACING */
+
+static inline bool
+__mmap_lock_traced_trylock(struct mm_struct *mm,
+			   int (*trylock)(struct rw_semaphore *), bool write)
+{
+	bool ret;
+
+	if (tracepoint_enabled(mmap_lock_start_locking))
+		__mmap_lock_do_trace_start_locking(mm, write);
+	ret = trylock(&mm->mmap_lock) != 0;
+	if (tracepoint_enabled(mmap_lock_acquire_returned))
+		__mmap_lock_do_trace_acquire_returned(mm, write, ret);
+	return ret;
+}
+
+static inline void __mmap_lock_traced_lock(struct mm_struct *mm,
+					   void (*lock)(struct rw_semaphore *),
+					   bool write)
+{
+	if (tracepoint_enabled(mmap_lock_start_locking))
+		__mmap_lock_do_trace_start_locking(mm, write);
+	lock(&mm->mmap_lock);
+	if (tracepoint_enabled(mmap_lock_acquire_returned))
+		__mmap_lock_do_trace_acquire_returned(mm, write, true);
+}
+
+static inline int
+__mmap_lock_traced_lock_return(struct mm_struct *mm,
+			       int (*lock)(struct rw_semaphore *), bool write)
+{
+	int ret;
+
+	if (tracepoint_enabled(mmap_lock_start_locking))
+		__mmap_lock_do_trace_start_locking(mm, write);
+	ret = lock(&mm->mmap_lock);
+	if (tracepoint_enabled(mmap_lock_acquire_returned))
+		__mmap_lock_do_trace_acquire_returned(mm, write, ret == 0);
+	return ret;
+}
+
+static inline void
+__mmap_lock_traced_unlock(struct mm_struct *mm,
+			  void (*unlock)(struct rw_semaphore *), bool write)
+{
+	unlock(&mm->mmap_lock);
+	if (tracepoint_enabled(mmap_lock_released))
+		__mmap_lock_do_trace_released(mm, write);
+}
+
 static inline void mmap_init_lock(struct mm_struct *mm)
 {
 	init_rwsem(&mm->mmap_lock);

@@ -13,58 +97,66 @@ static inline void mmap_init_lock(struct mm_struct *mm)
 
 static inline void mmap_write_lock(struct mm_struct *mm)
 {
-	down_write(&mm->mmap_lock);
+	__mmap_lock_traced_lock(mm, down_write, true);
 }
 
 static inline void mmap_write_lock_nested(struct mm_struct *mm, int subclass)
 {
+	if (tracepoint_enabled(mmap_lock_start_locking))
+		__mmap_lock_do_trace_start_locking(mm, true);
 	down_write_nested(&mm->mmap_lock, subclass);
+	if (tracepoint_enabled(mmap_lock_acquire_returned))
+		__mmap_lock_do_trace_acquire_returned(mm, true, true);
 }
 
 static inline int mmap_write_lock_killable(struct mm_struct *mm)
 {
-	return down_write_killable(&mm->mmap_lock);
+	return __mmap_lock_traced_lock_return(mm, down_write_killable, true);
 }
 
 static inline bool mmap_write_trylock(struct mm_struct *mm)
 {
-	return down_write_trylock(&mm->mmap_lock) != 0;
+	return __mmap_lock_traced_trylock(mm, down_write_trylock, true);
 }
 
 static inline void mmap_write_unlock(struct mm_struct *mm)
 {
-	up_write(&mm->mmap_lock);
+	__mmap_lock_traced_unlock(mm, up_write, true);
 }
 
 static inline void mmap_write_downgrade(struct mm_struct *mm)
 {
 	downgrade_write(&mm->mmap_lock);
+	if (tracepoint_enabled(mmap_lock_acquire_returned))
+		__mmap_lock_do_trace_acquire_returned(mm, false, true);
 }
 
 static inline void mmap_read_lock(struct mm_struct *mm)
 {
-	down_read(&mm->mmap_lock);
+	__mmap_lock_traced_lock(mm, down_read, false);
 }
 
 static inline int mmap_read_lock_killable(struct mm_struct *mm)
 {
-	return down_read_killable(&mm->mmap_lock);
+	return __mmap_lock_traced_lock_return(mm, down_read_killable, false);
 }
 
 static inline bool mmap_read_trylock(struct mm_struct *mm)
 {
-	return down_read_trylock(&mm->mmap_lock) != 0;
+	return __mmap_lock_traced_trylock(mm, down_read_trylock, false);
 }
 
 static inline void mmap_read_unlock(struct mm_struct *mm)
 {
-	up_read(&mm->mmap_lock);
+	__mmap_lock_traced_unlock(mm, up_read, false);
 }
 
 static inline bool mmap_read_trylock_non_owner(struct mm_struct *mm)
 {
-	if (down_read_trylock(&mm->mmap_lock)) {
+	if (mmap_read_trylock(mm)) {
 		rwsem_release(&mm->mmap_lock.dep_map, _RET_IP_);
+		if (tracepoint_enabled(mmap_lock_released))
+			__mmap_lock_do_trace_released(mm, false);
 		return true;
 	}
 	return false;

@@ -73,6 +165,8 @@ static inline bool mmap_read_trylock_non_owner(struct mm_struct *mm)
 static inline void mmap_read_unlock_non_owner(struct mm_struct *mm)
 {
 	up_read_non_owner(&mm->mmap_lock);
+	if (tracepoint_enabled(mmap_lock_released))
+		__mmap_lock_do_trace_released(mm, false);
 }
 
 static inline void mmap_assert_locked(struct mm_struct *mm)

include/trace/events/mmap_lock.h new file +70 -0

70 lines not on this page · raw message

mm/Makefile +1 -1

index 069f216e109e..b6cd2fffa492 100644
@@ -52,7 +52,7 @@ obj-y			:= filemap.o mempool.o oom_kill.o fadvise.o \
 			   mm_init.o percpu.o slab_common.o \
 			   compaction.o vmacache.o \
 			   interval_tree.o list_lru.o workingset.o \
-			   debug.o gup.o $(mmu-y)
+			   debug.o gup.o mmap_lock.o $(mmu-y)
 
 # Give 'page_alloc' its own module-parameter namespace
 page-alloc-y := page_alloc.o

mm/mmap_lock.c new file +87 -0

87 lines not on this page · raw message

Signature · 1 line

2.28.0.806.g8561365e88-goog

Michel Lespinasse

2020-10-08 07:40 UTC #4 lore raw

Re: [PATCH v2 2/2] mmap_lock: add tracepoints around lock acquisition

Quoted 18 lines · Axel Rasmussen

The goal of these tracepoints is to be able to debug lock contention issues. This lock is acquired on most (all?) mmap / munmap / page fault operations, so a multi-threaded process which does a lot of these can experience significant contention.
We trace just before we start acquisition, when the acquisition returns (whether it succeeded or not), and when the lock is released (or downgraded). The events are broken out by lock type (read / write).
The events are also broken out by memcg path. For container-based workloads, users often think of several processes in a memcg as a single logical "task", so collecting statistics at this level is useful.
The end goal is to get latency information. This isn't directly included in the trace events. Instead, users are expected to compute the time between "start locking" and "acquire returned", using e.g. synthetic events or BPF. The benefit we get from this is simpler code.

Quoted 6 lines

Because we use tracepoint_enabled() to decide whether or not to trace, this patch has effectively no overhead unless tracepoints are enabled at runtime. If tracepoints are enabled, there is a performance impact, but how much depends on exactly what e.g. the BPF program does.
Signed-off-by Axel Rasmussen <[email protected]>

Thanks for working on this.

I like that there is no overhead unless CONFIG_TRACING is set. However, I think the __mmap_lock_traced_lock and similar functions are the wrong level of abstraction, especially considering that we are considering to switch away from using rwsem as the underlying lock implementation. Would you consider something along the following lines instead for include/linux/mmap_lock.h ?

#ifdef CONFIG_TRACING

DECLARE_TRACEPOINT(...);

void __mmap_lock_do_trace_start_locking(struct mm_struct *mm, bool write);

static inline void mmap_lock_trace_start_locking(struct mm_struct *mm, bool write)
{
if (tracepoint_enabled(mmap_lock_start_locking))

    __mmap_lock_do_trace_start_locking(mm, write);
}

#else

static inline void mmap_lock_trace_start_locking(struct mm_struct *mm, bool write) {}

#endif

static inline void mmap_write_lock(struct mm_struct *mm) {
mmap_lock_trace_start_locking(mm, true); down_write(&mm->mmap_lock); mmap_lock_trace_acquire_returned(mm, true, true);
}

I think this is more straightforward, and also the mmap_lock_trace_start_locking and similar functions don't depend on the underlying lock implementation.

The changes to the other files look fine to me.

Signature · 2 lines

Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Axel Rasmussen

author

2020-10-08 18:04 UTC #5 lore raw

Re: [PATCH v2 2/2] mmap_lock: add tracepoints around lock acquisition

Quoted 25 lines · Michel Lespinasse

Quoted 23 lines · Axel Rasmussen
The goal of these tracepoints is to be able to debug lock contention issues. This lock is acquired on most (all?) mmap / munmap / page fault operations, so a multi-threaded process which does a lot of these can experience significant contention.
We trace just before we start acquisition, when the acquisition returns (whether it succeeded or not), and when the lock is released (or downgraded). The events are broken out by lock type (read / write).
The events are also broken out by memcg path. For container-based workloads, users often think of several processes in a memcg as a single logical "task", so collecting statistics at this level is useful.
The end goal is to get latency information. This isn't directly included in the trace events. Instead, users are expected to compute the time between "start locking" and "acquire returned", using e.g. synthetic events or BPF. The benefit we get from this is simpler code.
Because we use tracepoint_enabled() to decide whether or not to trace, this patch has effectively no overhead unless tracepoints are enabled at runtime. If tracepoints are enabled, there is a performance impact, but how much depends on exactly what e.g. the BPF program does.

Quoted 5 lines

Quoted 1 line
Signed-off-by Axel Rasmussen <[email protected]>
Thanks for working on this.
I like that there is no overhead unless CONFIG_TRACING is set.

Actually, it's even a bit better. Even if CONFIG_TRACING is set, the only overhead we add is a single static_key branch (much cheaper than a normal branch). The overhead is only introduced when the tracepoints are enabled at runtime, e.g. by writing to /sys/kernel/trace/events/(event)/enabled.

Quoted 5 lines

However, I think the __mmap_lock_traced_lock and similar functions are the wrong level of abstraction, especially considering that we are considering to switch away from using rwsem as the underlying lock implementation. Would you consider something along the following lines instead for include/linux/mmap_lock.h ?

Sure, there's no reason that can't work. It's just a tradeoff:

Breaking up these helpers the way you describe makes the locking functions more verbose (more repeated code; overall this file would be longer), but there are a few less lines to change when we switch from rwsem to something else (because we'd only need to change the lock functions vs. as-is we'd have to change a couple of lines in each of the helpers too).

I prefer the way it is, but I don't feel that strongly about it. I'll refactor and send a v3 that looks as you describe.

Quoted 37 lines

#ifdef CONFIG_TRACING
DECLARE_TRACEPOINT(...);
void __mmap_lock_do_trace_start_locking(struct mm_struct *mm, bool write);
static inline void mmap_lock_trace_start_locking(struct mm_struct *mm, bool write)
{
if (tracepoint_enabled(mmap_lock_start_locking))
    __mmap_lock_do_trace_start_locking(mm, write);
}
#else
static inline void mmap_lock_trace_start_locking(struct mm_struct *mm, bool write) {}
#endif
static inline void mmap_write_lock(struct mm_struct *mm) {
mmap_lock_trace_start_locking(mm, true); down_write(&mm->mmap_lock); mmap_lock_trace_acquire_returned(mm, true, true);
}
I think this is more straightforward, and also the mmap_lock_trace_start_locking and similar functions don't depend on the underlying lock implementation.
The changes to the other files look fine to me.
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.