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: <CAJuCfpGH4UKmuaVMAE-CCPo-J-Qzufs55_DDR0bPbzyOC8-deg@mail.gmail.com>
Date: Tue, 26 Aug 2025 17:56:48 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Xuewen Yan <xuewen.yan94@...il.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

On Fri, Aug 22, 2025 at 6:51 PM Xuewen Yan <xuewen.yan94@...il.com> wrote:
>
> 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.

You can report what you think makes sense today for this tracepoint.
Tracepoints can be enhanced later if needed since their format is
exported to the userspace and well-designed userspace parsers should
be able to parse new fields.

>
> >
> > > +       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.

Huh? Are you modifying the t->event inside your handler?! If so, that
is unacceptable and I will NAK this change. A driver should not
interfere with core kernel mechanisms.

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