On Sun, 1 Dec 2024 15:38:17 +0000 Raghavendra K T <[email protected]> wrote:
> Add tracing support to track > - start and end of scanning. > - migration. > > CC: Steven Rostedt <[email protected]> > CC: Masami Hiramatsu <[email protected]> > CC: [email protected] > > Signed-off-by: Raghavendra K T <[email protected]> > --- > include/trace/events/kmem.h | 99 +++++++++++++++++++++++++++++++++++++ > mm/kmmscand.c | 12 ++++- > 2 files changed, 110 insertions(+), 1 deletion(-) > > diff --git a/include/trace/events/kmem.h b/include/trace/events/kmem.h > index b37eb0a7060f..80978d85300d 100644 > --- a/include/trace/events/kmem.h > +++ b/include/trace/events/kmem.h > @@ -9,6 +9,105 @@ > #include <linux/tracepoint.h> > #include <trace/events/mmflags.h> > > +TRACE_EVENT(kmem_mm_enter, > + > + TP_PROTO(struct task_struct *t, > + struct mm_struct *mm), > + > + TP_ARGS(t, mm), > + > + TP_STRUCT__entry( > + __array( char, comm, TASK_COMM_LEN ) Is there a reason to record "comm"? There's other ways to retrieve it than to always write it to the ring buffer. > + __field( struct mm_struct *, mm ) > + ), > + > + TP_fast_assign( > + memcpy(__entry->comm, t->comm, TASK_COMM_LEN); > + __entry->mm = mm; > + ), > + > + TP_printk("kmmscand: mm_enter comm =%s mm=%p", __entry->comm, > __entry->mm) > +); > + > +TRACE_EVENT(kmem_scan_mm_start, > + > + TP_PROTO(struct task_struct *t, > + struct mm_struct *mm), > + > + TP_ARGS(t, mm), > + > + TP_STRUCT__entry( > + __array( char, comm, TASK_COMM_LEN ) > + __field( struct mm_struct *, mm ) > + ), > + > + TP_fast_assign( > + memcpy(__entry->comm, t->comm, TASK_COMM_LEN); > + __entry->mm = mm; > + ), > + > + TP_printk("kmmscand: scan_mm_start comm =%s mm=%p", __entry->comm, > __entry->mm) No need to write the event name into the TP_printk(). That's redundant. Also, the above two events are pretty much identical. Please use DECLARE_EVENT_CLASS(). > +); > + > +TRACE_EVENT(kmem_scan_mm_end, > + > + TP_PROTO(struct task_struct *t, > + struct mm_struct *mm, > + unsigned long start, > + unsigned long total, > + unsigned long scan_size, > + unsigned long scan_period), > + > + TP_ARGS(t, mm, start, total, scan_period, scan_size), > + > + TP_STRUCT__entry( > + __array( char, comm, TASK_COMM_LEN ) Again, why comm? > + __field( struct mm_struct *, mm ) > + __field( unsigned long, start ) > + __field( unsigned long, total ) > + __field( unsigned long, scan_period ) > + __field( unsigned long, scan_size ) > + ), > + > + TP_fast_assign( > + memcpy(__entry->comm, t->comm, TASK_COMM_LEN); > + __entry->mm = mm; > + __entry->start = start; > + __entry->total = total; > + __entry->scan_period = scan_period; > + __entry->scan_size = scan_size; > + ), > + > + TP_printk("kmmscand: scan_mm_end comm =%s mm=%p, start = %ld, total = > %ld, scan_period = %ld, scan_size = %ld", > + __entry->comm, __entry->mm, __entry->start, > + __entry->total, __entry->scan_period, > __entry->scan_size) > +); > + > +TRACE_EVENT(kmem_scan_mm_migrate, > + > + TP_PROTO(struct task_struct *t, > + struct mm_struct *mm, > + int rc), > + > + TP_ARGS(t, mm, rc), > + > + TP_STRUCT__entry( > + __array( char, comm, TASK_COMM_LEN ) > + __field( struct mm_struct *, mm ) > + __field( int, rc ) > + ), > + > + TP_fast_assign( > + memcpy(__entry->comm, t->comm, TASK_COMM_LEN); > + __entry->mm = mm; > + __entry->rc = rc; > + ), > + > + TP_printk("kmmscand: scan_mm_migrate comm =%s mm=%p rc=%d", > __entry->comm, > + __entry->mm, __entry->rc) > +); > + > + > TRACE_EVENT(kmem_cache_alloc, > > TP_PROTO(unsigned long call_site, > diff --git a/mm/kmmscand.c b/mm/kmmscand.c > index 682c0523c0b4..70f588a210dd 100644 > --- a/mm/kmmscand.c > +++ b/mm/kmmscand.c > @@ -668,8 +668,10 @@ static void kmmscand_migrate_folio(void) > WRITE_ONCE(kmmscand_cur_migrate_mm, info->mm); > spin_unlock(&kmmscand_migrate_lock); > > - if (info->mm) > + if (info->mm) { > ret = kmmscand_promote_folio(info); > + trace_kmem_scan_mm_migrate(info->mm->owner, > info->mm, ret); > + } > > /* TBD: encode migrated count here, currently assume > folio_nr_pages */ > if (!ret) > @@ -828,6 +830,9 @@ static unsigned long kmmscand_scan_mm_slot(void) > goto outerloop; > } > > + if (mm->owner) > + trace_kmem_scan_mm_start(mm->owner, mm); > + > now = jiffies; > /* > * Dont scan if : > @@ -868,6 +873,10 @@ static unsigned long kmmscand_scan_mm_slot(void) > > update_mmslot_info = true; > > + if (mm->owner) > + trace_kmem_scan_mm_end(mm->owner, mm, address, total, > + mm_slot_scan_period, mm_slot_scan_size); Please do not add conditions that is used just for calling a tracepoint. That takes away the "nop" of the function. You can either use TRACE_EVENT_CONDITION() or DEFINE_EVENT_CONDITION(), or you can hard code it here: if (trace_kmem_scan_mm_end_enabled()) { if (mm->owner) trace_kmem_scan_mm_end(mm->owner, mm, address, total, mm_slot_scan_period, mm_slot_scan_size); } But since it is a single condition, I would prefer the *_CONDITION() macros above. -- Steve > + > count_kmmscand_mm_scans(); > > outerloop: > @@ -1020,6 +1029,7 @@ void __kmmscand_enter(struct mm_struct *mm) > spin_unlock(&kmmscand_mm_lock); > > mmgrab(mm); > + trace_kmem_mm_enter(mm->owner, mm); > if (wakeup) > wake_up_interruptible(&kmmscand_wait); > }
