On Wed, Aug 27, 2025 at 8:57 AM Suren Baghdasaryan <sur...@google.com> wrote:
>
> On Fri, Aug 22, 2025 at 6:51 PM Xuewen Yan <xuewen.ya...@gmail.com> wrote:
> >
> > Hi Suren,
> >
> > Thanks for your review:)
> >
> > On Thu, Aug 21, 2025 at 3:51 AM Suren Baghdasaryan <sur...@google.com>
> > wrote:
> > >
> > > On Wed, Aug 20, 2025 at 4:28 AM Xuewen Yan <xuewen....@unisoc.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....@unisoc.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.
Okay, I would change this to be tracepoint.
>
> >
> > >
> > > > + 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.
Sorry for missing this, the next patch, I would export some variables
instead of the pointer of t...
Thanks!
>
> >
> > Thanks!
> >
> > >
> > > > +
> > > > /* Generate an event */
> > > > if (cmpxchg(&t->event, 0, 1) == 0) {
> > > > if (t->of)
> > > > --
> > > > 2.25.1
> > > >
