Re: [PATCH v2] mm: Update mark_victim tracepoints fields

[Steven Rostedt]

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