On 12/5/2024 11:16 PM, Steven Rostedt wrote:

On Sun, 1 Dec 2024 15:38:17 +0000
Raghavendra K T [off-list ref] wrote:

quoted

Add tracing support to track
  - start and end of scanning.
  - migration.

CC: Steven Rostedt <rostedt@goodmis.org>
CC: Masami Hiramatsu <mhiramat@kernel.org>
CC: linux-trace-kernel@vger.kernel.org

[...]

quoted

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

[...]

quoted

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

quoted

+
+	TP_STRUCT__entry(
+		__array(	char, comm, TASK_COMM_LEN	)

Again, why comm?

Will do same change here too.

[...]

quoted

+	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