From: Axel Rasmussen <axelrasmussen@google.com>
To: Gang Li <ligang.bdlg@bytedance.com>
Cc: Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
Andrew Morton <akpm@linux-foundation.org>,
Vlastimil Babka <vbabka@suse.cz>,
LKML <linux-kernel@vger.kernel.org>,
Linux MM <linux-mm@kvack.org>
Subject: Re: [PATCH 3/3] mm: mmap_lock: add ip to mmap_lock tracepoints
Date: Thu, 29 Jul 2021 10:33:38 -0700 [thread overview]
Message-ID: <CAJHvVcjBH+Vry8v5T0FWyFWWDY6_AqSxZcVQxXRm=LR8v=ML-Q@mail.gmail.com> (raw)
In-Reply-To: <20210729092853.38242-1-ligang.bdlg@bytedance.com>
Not a strong objection, but I think this can be achieved already using either:
- The "stacktrace" feature which histogram triggers support
(https://www.kernel.org/doc/html/latest/trace/histogram.html)
- bpftrace's kstack/ustack feature
(https://github.com/iovisor/bpftrace/blob/master/docs/tutorial_one_liners.md#lesson-9-profile-on-cpu-kernel-stacks)
I haven't tried it out myself, but I suspect you could construct a
synthetic event
(https://www.kernel.org/doc/html/latest/trace/histogram.html#synthetic-events)
which adds in the stack trace, then it ought to function a lot like it
would with this patch.
Then again, it's not like this change is huge by any means. So, if you
find this more convenient than those alternatives, you can take:
Reviewed-by: Axel Rasmussen <axelrasmussen@google.com>
It's possible Steven or Tom have a more strong opinion on this though. ;)
On Thu, Jul 29, 2021 at 2:29 AM Gang Li <ligang.bdlg@bytedance.com> wrote:
>
> The mmap_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. Sometimes we want to know
> where the lock is hold. And it's hard to locate without collecting ip.
>
> Here's an example: TP_printk("ip=%pS",ip)
> Log looks like this: "ip=do_user_addr_fault+0x274/0x640"
>
> We can find out who cause the contention amd make some improvements
> for it.
>
> Signed-off-by: Gang Li <ligang.bdlg@bytedance.com>
> ---
> include/trace/events/mmap_lock.h | 27 +++++++++++++++++----------
> mm/mmap_lock.c | 6 +++---
> 2 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/include/trace/events/mmap_lock.h b/include/trace/events/mmap_lock.h
> index b9dd66f9c226..8913a9f85894 100644
> --- a/include/trace/events/mmap_lock.h
> +++ b/include/trace/events/mmap_lock.h
> @@ -15,35 +15,39 @@ extern void trace_mmap_lock_unreg(void);
>
> DECLARE_EVENT_CLASS(mmap_lock,
>
> - TP_PROTO(struct mm_struct *mm, const char *memcg_path, bool write),
> + TP_PROTO(struct mm_struct *mm, const char *memcg_path, bool write,
> + unsigned long ip),
>
> - TP_ARGS(mm, memcg_path, write),
> + TP_ARGS(mm, memcg_path, write, ip),
>
> TP_STRUCT__entry(
> __field(struct mm_struct *, mm)
> __string(memcg_path, memcg_path)
> __field(bool, write)
> + __field(void *, ip)
> ),
>
> TP_fast_assign(
> __entry->mm = mm;
> __assign_str(memcg_path, memcg_path);
> __entry->write = write;
> + __entry->ip = (void *)ip;
> ),
>
> TP_printk(
> - "mm=%p memcg_path=%s write=%s",
> + "mm=%p memcg_path=%s write=%s ip=%pS",
> __entry->mm,
> __get_str(memcg_path),
> - __entry->write ? "true" : "false"
> - )
> + __entry->write ? "true" : "false",
> + __entry->ip
> + )
> );
>
> #define DEFINE_MMAP_LOCK_EVENT(name) \
> DEFINE_EVENT_FN(mmap_lock, name, \
> TP_PROTO(struct mm_struct *mm, const char *memcg_path, \
> - bool write), \
> - TP_ARGS(mm, memcg_path, write), \
> + bool write, unsigned long ip), \
> + TP_ARGS(mm, memcg_path, write, ip), \
> trace_mmap_lock_reg, trace_mmap_lock_unreg)
>
> DEFINE_MMAP_LOCK_EVENT(mmap_lock_start_locking);
> @@ -52,14 +56,15 @@ DEFINE_MMAP_LOCK_EVENT(mmap_lock_released);
> TRACE_EVENT_FN(mmap_lock_acquire_returned,
>
> TP_PROTO(struct mm_struct *mm, const char *memcg_path, bool write,
> - bool success),
> + unsigned long ip, bool success),
>
> - TP_ARGS(mm, memcg_path, write, success),
> + TP_ARGS(mm, memcg_path, write, ip, success),
>
> TP_STRUCT__entry(
> __field(struct mm_struct *, mm)
> __string(memcg_path, memcg_path)
> __field(bool, write)
> + __field(void *, ip)
> __field(bool, success)
> ),
>
> @@ -67,14 +72,16 @@ TRACE_EVENT_FN(mmap_lock_acquire_returned,
> __entry->mm = mm;
> __assign_str(memcg_path, memcg_path);
> __entry->write = write;
> + __entry->ip = (void *)ip;
> __entry->success = success;
> ),
>
> TP_printk(
> - "mm=%p memcg_path=%s write=%s success=%s",
> + "mm=%p memcg_path=%s write=%s ip=%pS success=%s",
> __entry->mm,
> __get_str(memcg_path),
> __entry->write ? "true" : "false",
> + __entry->ip,
> __entry->success ? "true" : "false"
> ),
>
> diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> index 1854850b4b89..f1100eae6f2f 100644
> --- a/mm/mmap_lock.c
> +++ b/mm/mmap_lock.c
> @@ -227,20 +227,20 @@ static const char *get_mm_memcg_path(struct mm_struct *mm)
>
> void __mmap_lock_do_trace_start_locking(struct mm_struct *mm, bool write)
> {
> - TRACE_MMAP_LOCK_EVENT(start_locking, mm, write);
> + TRACE_MMAP_LOCK_EVENT(start_locking, mm, write, _RET_IP_);
> }
> EXPORT_SYMBOL(__mmap_lock_do_trace_start_locking);
>
> void __mmap_lock_do_trace_acquire_returned(struct mm_struct *mm, bool write,
> bool success)
> {
> - TRACE_MMAP_LOCK_EVENT(acquire_returned, mm, write, success);
> + TRACE_MMAP_LOCK_EVENT(acquire_returned, mm, write, _RET_IP_, success);
> }
> EXPORT_SYMBOL(__mmap_lock_do_trace_acquire_returned);
>
> void __mmap_lock_do_trace_released(struct mm_struct *mm, bool write)
> {
> - TRACE_MMAP_LOCK_EVENT(released, mm, write);
> + TRACE_MMAP_LOCK_EVENT(released, mm, write, _RET_IP_);
> }
> EXPORT_SYMBOL(__mmap_lock_do_trace_released);
> #endif /* CONFIG_TRACING */
> --
> 2.20.1
>
next prev parent reply other threads:[~2021-07-29 17:34 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-29 9:28 [PATCH 3/3] mm: mmap_lock: add ip to mmap_lock tracepoints Gang Li
2021-07-29 17:33 ` Axel Rasmussen [this message]
2021-07-30 5:32 ` Gang Li
2021-07-30 20:03 ` Steven Rostedt
2021-08-02 2:44 ` Gang Li
2021-08-19 18:18 ` Gang Li
2021-08-31 3:24 ` Gang Li
2021-07-30 8:12 ` Vlastimil Babka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJHvVcjBH+Vry8v5T0FWyFWWDY6_AqSxZcVQxXRm=LR8v=ML-Q@mail.gmail.com' \
--to=axelrasmussen@google.com \
--cc=akpm@linux-foundation.org \
--cc=ligang.bdlg@bytedance.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mingo@redhat.com \
--cc=rostedt@goodmis.org \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.