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

[Raghavendra K T <raghavendra.kt@amd.com>]

On 12/5/2024 11:16 PM, Steven Rostedt wrote:
> On Sun, 1 Dec 2024 15:38:17 +0000
> Raghavendra K T <raghavendra.kt@amd.com> wrote:
> 
>> 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
>>

[...]

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