| Message ID | 20240111210539.636607-1-carlosgalo@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] mm: Update mark_victim tracepoints fields | expand |
On Thu, 11 Jan 2024 21:05:30 +0000 Carlos Galo <carlosgalo@google.com> 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@goodmis.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Suren Baghdasaryan <surenb@google.com> > Signed-off-by: Carlos Galo <carlosgalo@google.com> > --- > 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@goodmis.org> -- Steve > + trace_mark_victim(tsk, cred->uid.val); > + put_cred(cred); > } > > /** > > base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
Hi, sorry I have missed this before. On Thu 11-01-24 21:05:30, Carlos Galo 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. You are correct that post OOM all per-process information is lost. On the other hand we do dump all this information to the kernel log. Could you explain why that is not suitable for your purpose? > 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. What is the oom score useful for? Is there any reason to provide a different information from the one reported to the kernel log? __oom_kill_process: pr_err("%s: Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB, UID:%u pgtables:%lukB oom_score_adj:%hd\n", message, task_pid_nr(victim), victim->comm, K(mm->total_vm), K(get_mm_counter(mm, MM_ANONPAGES)), K(get_mm_counter(mm, MM_FILEPAGES)), K(get_mm_counter(mm, MM_SHMEMPAGES)), from_kuid(&init_user_ns, task_uid(victim)), mm_pgtables_bytes(mm) >> 10, victim->signal->oom_score_adj);
On Tue, Feb 20, 2024 at 11:55 PM Michal Hocko <mhocko@suse.com> wrote: > > Hi, > sorry I have missed this before. > > On Thu 11-01-24 21:05:30, Carlos Galo 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. > > You are correct that post OOM all per-process information is lost. On > the other hand we do dump all this information to the kernel log. Could > you explain why that is not suitable for your purpose? Userspace tools often need real-time visibility into OOM situations for userspace intervention. Our use case involves utilizing BPF programs, along with BPF ring buffers, to provide OOM notification to userspace. Parsing kernel logs would be significant overhead as opposed to the event based BPF approach. > > 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. > > What is the oom score useful for? > The OOM score provides us a measure of the victim's importance. On the android side, it allows us to identify if top or foreground apps are killed, which have user perceptible impact. > Is there any reason to provide a different information from the one > reported to the kernel log? > __oom_kill_process: > pr_err("%s: Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB, UID:%u pgtables:%lukB oom_score_adj:%hd\n", > message, task_pid_nr(victim), victim->comm, K(mm->total_vm), > K(get_mm_counter(mm, MM_ANONPAGES)), > K(get_mm_counter(mm, MM_FILEPAGES)), > K(get_mm_counter(mm, MM_SHMEMPAGES)), > from_kuid(&init_user_ns, task_uid(victim)), > mm_pgtables_bytes(mm) >> 10, victim->signal->oom_score_adj); > We added these fields we need (UID, process name, and OOM score), but we're open to adding the others if you prefer that for consistency with the kernel log. Thanks, Carlos > -- > Michal Hocko > SUSE Labs
On Wed 21-02-24 13:30:51, Carlos Galo wrote: > On Tue, Feb 20, 2024 at 11:55 PM Michal Hocko <mhocko@suse.com> wrote: > > > > Hi, > > sorry I have missed this before. > > > > On Thu 11-01-24 21:05:30, Carlos Galo 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. > > > > You are correct that post OOM all per-process information is lost. On > > the other hand we do dump all this information to the kernel log. Could > > you explain why that is not suitable for your purpose? > > Userspace tools often need real-time visibility into OOM situations > for userspace intervention. Our use case involves utilizing BPF > programs, along with BPF ring buffers, to provide OOM notification to > userspace. Parsing kernel logs would be significant overhead as > opposed to the event based BPF approach. Please put that into the changelog. > > > 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. > > > > What is the oom score useful for? > > > The OOM score provides us a measure of the victim's importance. On the > android side, it allows us to identify if top or foreground apps are > killed, which have user perceptible impact. But the value on its own (wihtout knowing scores of other tasks) doesn't really tell you anything, does it? > > Is there any reason to provide a different information from the one > > reported to the kernel log? > > __oom_kill_process: > > pr_err("%s: Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB, UID:%u pgtables:%lukB oom_score_adj:%hd\n", > > message, task_pid_nr(victim), victim->comm, K(mm->total_vm), > > K(get_mm_counter(mm, MM_ANONPAGES)), > > K(get_mm_counter(mm, MM_FILEPAGES)), > > K(get_mm_counter(mm, MM_SHMEMPAGES)), > > from_kuid(&init_user_ns, task_uid(victim)), > > mm_pgtables_bytes(mm) >> 10, victim->signal->oom_score_adj); > > > > We added these fields we need (UID, process name, and OOM score), but > we're open to adding the others if you prefer that for consistency > with the kernel log. yes, I think the consistency would be better here. For one it reports numbers which can tell quite a lot about the killed victim. It is a superset of what you already asking for. With a notable exception of the oom_score which is really dubious without a wider context.
On Thu, Feb 22, 2024 at 6:16 AM Michal Hocko <mhocko@suse.com> wrote: > > On Wed 21-02-24 13:30:51, Carlos Galo wrote: > > On Tue, Feb 20, 2024 at 11:55 PM Michal Hocko <mhocko@suse.com> wrote: > > > > > > Hi, > > > sorry I have missed this before. > > > > > > On Thu 11-01-24 21:05:30, Carlos Galo 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. > > > > > > You are correct that post OOM all per-process information is lost. On > > > the other hand we do dump all this information to the kernel log. Could > > > you explain why that is not suitable for your purpose? > > > > Userspace tools often need real-time visibility into OOM situations > > for userspace intervention. Our use case involves utilizing BPF > > programs, along with BPF ring buffers, to provide OOM notification to > > userspace. Parsing kernel logs would be significant overhead as > > opposed to the event based BPF approach. > > Please put that into the changelog. Will do. > > > > 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. > > > > > > What is the oom score useful for? > > > > > The OOM score provides us a measure of the victim's importance. On the > > android side, it allows us to identify if top or foreground apps are > > killed, which have user perceptible impact. > > But the value on its own (wihtout knowing scores of other tasks) doesn't > really tell you anything, does it? Android uses the OOM adj_score ranges to categorize app state (foreground, background, ...). I'll resend a v3 with the commit text updated to include details about this. > > > Is there any reason to provide a different information from the one > > > reported to the kernel log? > > > __oom_kill_process: > > > pr_err("%s: Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB, UID:%u pgtables:%lukB oom_score_adj:%hd\n", > > > message, task_pid_nr(victim), victim->comm, K(mm->total_vm), > > > K(get_mm_counter(mm, MM_ANONPAGES)), > > > K(get_mm_counter(mm, MM_FILEPAGES)), > > > K(get_mm_counter(mm, MM_SHMEMPAGES)), > > > from_kuid(&init_user_ns, task_uid(victim)), > > > mm_pgtables_bytes(mm) >> 10, victim->signal->oom_score_adj); > > > > > > > We added these fields we need (UID, process name, and OOM score), but > > we're open to adding the others if you prefer that for consistency > > with the kernel log. > > yes, I think the consistency would be better here. For one it reports > numbers which can tell quite a lot about the killed victim. It is a > superset of what you already asking for. With a notable exception of the > oom_score which is really dubious without a wider context. Sounds good, I'll resend a v3 that includes these fields as well. Thanks, Carlos > -- > Michal Hocko > SUSE Labs
On Thu, Feb 22, 2024 at 9:59 AM Carlos Galo <carlosgalo@google.com> wrote: > > On Thu, Feb 22, 2024 at 6:16 AM Michal Hocko <mhocko@suse.com> wrote: > > > > On Wed 21-02-24 13:30:51, Carlos Galo wrote: > > > On Tue, Feb 20, 2024 at 11:55 PM Michal Hocko <mhocko@suse.com> wrote: > > > > > > > > Hi, > > > > sorry I have missed this before. > > > > > > > > On Thu 11-01-24 21:05:30, Carlos Galo 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. > > > > > > > > You are correct that post OOM all per-process information is lost. On > > > > the other hand we do dump all this information to the kernel log. Could > > > > you explain why that is not suitable for your purpose? > > > > > > Userspace tools often need real-time visibility into OOM situations > > > for userspace intervention. Our use case involves utilizing BPF > > > programs, along with BPF ring buffers, to provide OOM notification to > > > userspace. Parsing kernel logs would be significant overhead as > > > opposed to the event based BPF approach. > > > > Please put that into the changelog. > > Will do. > > > > > > 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. > > > > > > > > What is the oom score useful for? > > > > > > > The OOM score provides us a measure of the victim's importance. On the > > > android side, it allows us to identify if top or foreground apps are > > > killed, which have user perceptible impact. > > > > But the value on its own (wihtout knowing scores of other tasks) doesn't > > really tell you anything, does it? > > Android uses the OOM adj_score ranges to categorize app state > (foreground, background, ...). I'll resend a v3 with the commit text > updated to include details about this. > > > > > Is there any reason to provide a different information from the one > > > > reported to the kernel log? > > > > __oom_kill_process: > > > > pr_err("%s: Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB, UID:%u pgtables:%lukB oom_score_adj:%hd\n", > > > > message, task_pid_nr(victim), victim->comm, K(mm->total_vm), > > > > K(get_mm_counter(mm, MM_ANONPAGES)), > > > > K(get_mm_counter(mm, MM_FILEPAGES)), > > > > K(get_mm_counter(mm, MM_SHMEMPAGES)), > > > > from_kuid(&init_user_ns, task_uid(victim)), > > > > mm_pgtables_bytes(mm) >> 10, victim->signal->oom_score_adj); > > > > > > > > > > We added these fields we need (UID, process name, and OOM score), but > > > we're open to adding the others if you prefer that for consistency > > > with the kernel log. > > > > yes, I think the consistency would be better here. For one it reports > > numbers which can tell quite a lot about the killed victim. It is a > > superset of what you already asking for. With a notable exception of the > > oom_score which is really dubious without a wider context. > > Sounds good, I'll resend a v3 that includes these fields as well. > > Thanks, > Carlos > I posted V3 here: https://lore.kernel.org/all/20240223173258.174828-1-carlosgalo@google.com/ Thanks, Carlos > > -- > > Michal Hocko > > SUSE Labs
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); + trace_mark_victim(tsk, cred->uid.val); + put_cred(cred); } /**
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. Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Suren Baghdasaryan <surenb@google.com> Signed-off-by: Carlos Galo <carlosgalo@google.com> --- 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(-) base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a