Re: [RFC PATCH V0 09/10] trace/kmmscand: Add tracing of scanning and migration

[Raghavendra K T]

On 12/5/2024 11:16 PM, Steven Rostedt wrote:
> On Sun, 1 Dec 2024 15:38:17 +0000
> Raghavendra K T <raghavendra.kt@xxxxxxx> wrote:
>
> > Add tracing support to track
> >   - start and end of scanning.
> >   - migration.
> >
> > CC: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > CC: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > CC: linux-trace-kernel@xxxxxxxxxxxxxxx

[...]

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

Thank you for the review Steve. The motivation was to filter benchmark
in the trace to understand the behavior.
I will explore regarding other ways of retrieving comm.
(or may be even PID is enough..)

[...]

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

Sure. will do.


> > +
> > +	TP_STRUCT__entry(
> > +		__array(	char, comm, TASK_COMM_LEN	)
>
> Again, why comm?

Will do same change here too.

[...]

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

Very helpful suggestion.

Thanks again.. I will keep these points in mind for next version.

- Raghu