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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86d39d52-fcc0-4215-8b68-ea6e526c0a01@amd.com>
Date: Fri, 6 Dec 2024 12:03:29 +0530
From: Raghavendra K T <raghavendra.kt@....com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org, gourry@...rry.net,
 nehagholkar@...a.com, abhishekd@...a.com, david@...hat.com,
 ying.huang@...el.com, nphamcs@...il.com, akpm@...ux-foundation.org,
 hannes@...xchg.org, feng.tang@...el.com, kbusch@...a.com, bharata@....com,
 Hasan.Maruf@....com, sj@...nel.org, willy@...radead.org,
 kirill.shutemov@...ux.intel.com, mgorman@...hsingularity.net,
 vbabka@...e.cz, hughd@...gle.com, rientjes@...gle.com, shy828301@...il.com,
 Liam.Howlett@...cle.com, peterz@...radead.org, mingo@...hat.com,
 Masami Hiramatsu <mhiramat@...nel.org>, linux-trace-kernel@...r.kernel.org
Subject: Re: [RFC PATCH V0 09/10] trace/kmmscand: Add tracing of scanning and
 migration



On 12/5/2024 11:16 PM, Steven Rostedt wrote:
> On Sun, 1 Dec 2024 15:38:17 +0000
> Raghavendra K T <raghavendra.kt@....com> wrote:
> 
>> Add tracing support to track
>>   - start and end of scanning.
>>   - migration.
>>
>> CC: Steven Rostedt <rostedt@...dmis.org>
>> CC: Masami Hiramatsu <mhiramat@...nel.org>
>> CC: linux-trace-kernel@...r.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