On Thu, 11 Jan 2024 21:05:30 +0000
Carlos Galo <carlosgalo@xxxxxxxxxx> wrote:
> The current implementation of the mark_victim tracepoint provides only
> the process ID (pid) of the victim process. This limitation poses
> challenges for userspace tools that need additional information
> about the OOM victim. The association between pid and the additional
> data may be lost after the kill, making it difficult for userspace to
> correlate the OOM event with the specific process.
>
> In order to mitigate this limitation, add the following fields:
>
> - UID
> In Android each installed application has a unique UID. Including
> the `uid` assists in correlating OOM events with specific apps.
>
> - Process Name (comm)
> Enables identification of the affected process.
>
> - OOM Score
> Allows userspace to get additional insights of the relative kill
> priority of the OOM victim.
>
>From a tracing POV this looks fine to me, but it's up to the mm
maintainers to decide to take it.
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> Signed-off-by: Carlos Galo <carlosgalo@xxxxxxxxxx>
> ---
> v2: Fixed build error. Added missing comma when printing `__entry->uid`.
>
> include/trace/events/oom.h | 19 +++++++++++++++----
> mm/oom_kill.c | 6 +++++-
> 2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/include/trace/events/oom.h b/include/trace/events/oom.h
> index 26a11e4a2c36..fb8a5d1b8a0a 100644
> --- a/include/trace/events/oom.h
> +++ b/include/trace/events/oom.h
> @@ -72,19 +72,30 @@ TRACE_EVENT(reclaim_retry_zone,
> );
>
> TRACE_EVENT(mark_victim,
> - TP_PROTO(int pid),
> + TP_PROTO(struct task_struct *task, uid_t uid),
>
> - TP_ARGS(pid),
> + TP_ARGS(task, uid),
>
> TP_STRUCT__entry(
> __field(int, pid)
> + __field(uid_t, uid)
> + __string(comm, task->comm)
> + __field(short, oom_score_adj)
> ),
>
> TP_fast_assign(
> - __entry->pid = pid;
> + __entry->pid = task->pid;
> + __entry->uid = uid;
> + __assign_str(comm, task->comm);
> + __entry->oom_score_adj = task->signal->oom_score_adj;
> ),
>
> - TP_printk("pid=%d", __entry->pid)
> + TP_printk("pid=%d uid=%u comm=%s oom_score_adj=%hd",
> + __entry->pid,
> + __entry->uid,
> + __get_str(comm),
> + __entry->oom_score_adj
> + )
> );
>
> TRACE_EVENT(wake_reaper,
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 9e6071fde34a..0698c00c5da6 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -44,6 +44,7 @@
> #include <linux/kthread.h>
> #include <linux/init.h>
> #include <linux/mmu_notifier.h>
> +#include <linux/cred.h>
>
> #include <asm/tlb.h>
> #include "internal.h"
> @@ -753,6 +754,7 @@ static inline void queue_oom_reaper(struct task_struct *tsk)
> */
> static void mark_oom_victim(struct task_struct *tsk)
> {
> + const struct cred *cred;
> struct mm_struct *mm = tsk->mm;
>
> WARN_ON(oom_killer_disabled);
> @@ -772,7 +774,9 @@ static void mark_oom_victim(struct task_struct *tsk)
> */
> __thaw_task(tsk);
> atomic_inc(&oom_victims);
> - trace_mark_victim(tsk->pid);
> + cred = get_task_cred(tsk);
get_task_cred() isn't a trivial function and is used only for tracing.
But this isn't a fast path so I guess that is fine.
For tracing:
Reviewed-by: Steven Rostedt (Google) <rostedt@xxxxxxxxxxx>
-- Steve
> + trace_mark_victim(tsk, cred->uid.val);
> + put_cred(cred);
> }
>
> /**
>
> base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
