linux-kernel - Re: [RFC PATCH V2] sched: psi: Add psi events trace point

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAB8ipk8ywWv8j8OGugXjhwcZXj567LKULhLz7HS8Btx-_27yBQ@mail.gmail.com>
Date: Sat, 23 Aug 2025 09:51:05 +0800
From: Xuewen Yan <xuewen.yan94@...il.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: Xuewen Yan <xuewen.yan@...soc.com>, hannes@...xchg.org, peterz@...radead.org, 
	mathieu.desnoyers@...icios.com, mhiramat@...nel.org, rostedt@...dmis.org, 
	mingo@...hat.com, juri.lelli@...hat.com, vincent.guittot@...aro.org, 
	dietmar.eggemann@....com, bsegall@...gle.com, mgorman@...e.de, 
	vschneid@...hat.com, linux-kernel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, ke.wang@...soc.com, yuming.han@...soc.com
Subject: Re: [RFC PATCH V2] sched: psi: Add psi events trace point

Hi Suren,

Thanks for your review:)

On Thu, Aug 21, 2025 at 3:51 AM Suren Baghdasaryan <surenb@...gle.com> wrote:
>
> On Wed, Aug 20, 2025 at 4:28 AM Xuewen Yan <xuewen.yan@...soc.com> wrote:
> >
> > Add trace point to psi triggers. This is useful to
> > observe the psi events in the kernel space.
> >
> > One use of this is to monitor memory pressure.
> > When the pressure is too high, we can kill the process
> > in the kernel space to prevent OOM.
> >
> > Signed-off-by: Xuewen Yan <xuewen.yan@...soc.com>
> > ---
> > v2:
> > -fix compilation error;
> > -export the tp;
> > -add more commit message;
> > ---
> >  include/trace/events/sched.h | 5 +++++
> >  kernel/sched/psi.c           | 4 ++++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
> > index 7b2645b50e78..d54db5fcbca2 100644
> > --- a/include/trace/events/sched.h
> > +++ b/include/trace/events/sched.h
> > @@ -896,6 +896,11 @@ DECLARE_TRACE(sched_set_need_resched,
> >         TP_PROTO(struct task_struct *tsk, int cpu, int tif),
> >         TP_ARGS(tsk, cpu, tif));
> >
> > +struct psi_trigger;
> > +DECLARE_TRACE(psi_event,
>
> DECLARE_TRACE will create a tracepoint but will not export it in the
> tracefs. Why should we not have it in the tracefs?

I haven't figured out what content should be displayed in the trace yet.
Until this is fully determined, I think it might be a better option to
just export the tracepoint and let users add their own hooks to print
the content they need.

>
> > +       TP_PROTO(struct psi_trigger *t),
> > +       TP_ARGS(t));
> > +
> >  #endif /* _TRACE_SCHED_H */
> >
> >  /* This part must be outside protection */
> > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> > index 59fdb7ebbf22..f06eb91a1250 100644
> > --- a/kernel/sched/psi.c
> > +++ b/kernel/sched/psi.c
> > @@ -141,6 +141,8 @@
> >  #include <linux/psi.h>
> >  #include "sched.h"
> >
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(psi_event_tp);
>
> So, are you planning to attach some handler to this trace event in your driver?

Yes, our modules would attach a handler to observe the memory pressure.

>
> > +
> >  static int psi_bug __read_mostly;
> >
> >  DEFINE_STATIC_KEY_FALSE(psi_disabled);
> > @@ -509,6 +511,8 @@ static void update_triggers(struct psi_group *group, u64 now,
> >                 if (now < t->last_event_time + t->win.size)
> >                         continue;
> >
> > +               trace_psi_event_tp(t);
>
> This should only be done if the below cmpxchg() check is true, right?
> Otherwise it will not match with what userspace is receiving.

If we put it below cmpxchg() check, we may lose some event before the
user space repose the signal.
Because the t->event needs to be set to 0 again.
In order to ensure that all events are displayed, it is better to put
it before the cmpxchg.

Thanks!

>
> > +
> >                 /* Generate an event */
> >                 if (cmpxchg(&t->event, 0, 1) == 0) {
> >                         if (t->of)
> > --
> > 2.25.1
> >