Re: [PATCH] mm/migration: Add trace events for THP migrations

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 1/10/22 10:33 PM, Steven Rostedt wrote:
> On Fri,  7 Jan 2022 10:29:35 +0530
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
>> This adds two trace events for PMD based THP migration without split. These
>> events closely follow the implementation details like setting and removing
>> of PMD migration entries, which are essential operations for THP migration.
>>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Zi Yan <ziy@nvidia.com>
>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>> Cc: John Hubbard <jhubbard@nvidia.com>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> This applies on v5.16-rc8
>>
>> Changes in V1:
>>
>> - Dropped mm, pmdp, page arguments from trace
>> - Updated trace argument names and output format
>>
>> Changes in RFC:
>>
>> https://lore.kernel.org/all/1640328398-20698-1-git-send-email-anshuman.khandual@arm.com/
>>
>>  include/trace/events/thp.h | 37 +++++++++++++++++++++++++++++++++++++
>>  mm/huge_memory.c           |  5 +++++
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/include/trace/events/thp.h b/include/trace/events/thp.h
>> index d7fbbe551841..193a555aa2ea 100644
>> --- a/include/trace/events/thp.h
>> +++ b/include/trace/events/thp.h
>> @@ -83,6 +83,43 @@ TRACE_EVENT(hugepage_splitting,
>>  		      __entry->addr, __entry->pte)
>>  );
>>  
>> +TRACE_EVENT(set_migration_pmd,
>> +
>> +	TP_PROTO(unsigned long addr, unsigned long pmd),
>> +
>> +	TP_ARGS(addr, pmd),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(unsigned long, addr)
>> +		__field(unsigned long, pmd)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->addr = addr;
>> +		__entry->pmd = pmd;
>> +	),
>> +
>> +	TP_printk("create pmd migration entry addr=%lx, pmd=%lx", __entry->addr, __entry->pmd)
>> +);
>> +
>> +TRACE_EVENT(remove_migration_pmd,
>> +
>> +	TP_PROTO(unsigned long addr, unsigned long pmd),
>> +
>> +	TP_ARGS(addr, pmd),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(unsigned long, addr)
>> +		__field(unsigned long, pmd)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->addr = addr;
>> +		__entry->pmd = pmd;
>> +	),
>> +
>> +	TP_printk("remove pmd migration entry addr=%lx, val=%lx", __entry->addr, __entry->pmd)
> 
> The two above are pretty much identical, except the first one has "pmd=%lx"
> for the pmd, and the second has "val=%lx" for the pmd. I'd suggest they
> both be the same, and then you could save memory by combining the two into

Right, both can be the same 'pmd=%lx'.

> DECLARE_EVENT_CLASS() / DEFINE_EVENT() macros:
> 
> DECLARE_EVENT_CLASS(migration_pmd,
> 
> 	TP_PROTO(unsigned long addr, unsigned long pmd),
> 
> 	TP_ARGS(addr, pmd),
> 
> 	TP_STRUCT__entry(
> 		__field(unsigned long, addr)
> 		__field(unsigned long, pmd)
> 	),
> 
> 	TP_fast_assign(
> 		__entry->addr = addr;
> 		__entry->pmd = pmd;
> 	),
> 
> 	TP_printk("create pmd migration entry addr=%lx, pmd=%lx", __entry->addr, __entry->pmd)
> );
> 
> DEFINE_EVENT(migration_pmd, set_migration_pmd,
> 	TP_PROTO(unsigned long addr, unsigned long pmd),
> 	TP_ARGS(addr, pmd)
> );
> 
> DEFINE_EVENT(migration_pmd, remove_migration_pmd,
> 	TP_PROTO(unsigned long addr, unsigned long pmd),
> 	TP_ARGS(addr, pmd)
> );
> 
> And then you have the same thing, but it combines the code which saves both
> data and text.

But both need to print different messages. Hence I am wondering whether
a (const char *) based message can be passed down here as well.

create pmd migration entry addr=%lx, pmd=%lx
remove pmd migration entry addr=%lx, pmd=%lx

> 
> -- Steve
> 
>> +);
>>  #endif /* _TRACE_THP_H */
>>  
>>  /* This part must be outside protection */
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index e5483347291c..d0adc019afe0 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -39,6 +39,9 @@
>>  #include <asm/pgalloc.h>
>>  #include "internal.h"
>>  
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/thp.h>
>> +
>>  /*
>>   * By default, transparent hugepage support is disabled in order to avoid
>>   * risking an increased memory footprint for applications that are not
>> @@ -3173,6 +3176,7 @@ void set_pmd_migration_entry(struct page_vma_mapped_walk *pvmw,
>>  	set_pmd_at(mm, address, pvmw->pmd, pmdswp);
>>  	page_remove_rmap(page, true);
>>  	put_page(page);
>> +	trace_set_migration_pmd(address, pmd_val(pmdswp));
>>  }
>>  
>>  void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
>> @@ -3206,5 +3210,6 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
>>  	if ((vma->vm_flags & VM_LOCKED) && !PageDoubleMap(new))
>>  		mlock_vma_page(new);
>>  	update_mmu_cache_pmd(vma, address, pvmw->pmd);
>> +	trace_remove_migration_pmd(address, pmd_val(pmde));
>>  }
>>  #endif
>