Re: [PATCH v1] mm: add tracepoints to ksm

[Stefan Roesch <shr@devkernel.io>]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Fri, 10 Feb 2023 13:46:45 -0800
> Stefan Roesch <shr@devkernel.io> wrote:
>
> Sorry for the late reply, I just noticed this (I had the flu when this was
> originally sent).
>
>> +/**
>> + * ksm_remove_ksm_page - called after a ksm page has been removed
>> + *
>> + * @pfn:		page frame number of ksm page
>> + *
>> + * Allows to trace the removing of stable ksm pages.
>> + */
>> +TRACE_EVENT(ksm_remove_ksm_page,
>> +
>> +	TP_PROTO(unsigned long pfn),
>> +
>> +	TP_ARGS(pfn),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(unsigned long, pfn)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->pfn = pfn;
>> +	),
>> +
>> +	TP_printk("pfn %lu", __entry->pfn)
>> +);
>> +
>> +/**
>> + * ksm_remove_rmap_item - called after a rmap_item has been removed from the
>> + *                        stable tree
>> + *
>> + * @pfn:		page frame number of ksm page
>> + * @rmap_item:		address of rmap_item  object
>> + * @mm:			address of the process mm struct
>> + *
>> + * Allows to trace the removal of pages from the stable tree list.
>> + */
>> +TRACE_EVENT(ksm_remove_rmap_item,
>> +
>> +	TP_PROTO(unsigned long pfn, void *rmap_item, void *mm),
>> +
>> +	TP_ARGS(pfn, rmap_item, mm),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(unsigned long,	pfn)
>> +		__field(void *,		rmap_item)
>> +		__field(void *,		mm)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->pfn		= pfn;
>> +		__entry->rmap_item	= rmap_item;
>> +		__entry->mm		= mm;
>> +	),
>> +
>> +	TP_printk("pfn %lu rmap_item %p mm %p",
>> +			__entry->pfn, __entry->rmap_item, __entry->mm)
>> +);
>> +
>> +#endif /* _TRACE_KSM_H */
>> +
>> +/* This part must be outside protection */
>> +#include <trace/define_trace.h>
>> diff --git a/mm/ksm.c b/mm/ksm.c
>> index 56808e3bfd19..4356af760735 100644
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -45,6 +45,9 @@
>>  #include "internal.h"
>>  #include "mm_slot.h"
>>
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/ksm.h>
>> +
>>  #ifdef CONFIG_NUMA
>>  #define NUMA(x)		(x)
>>  #define DO_NUMA(x)	do { (x); } while (0)
>> @@ -655,10 +658,12 @@ static void remove_node_from_stable_tree(struct ksm_stable_node *stable_node)
>>  	BUG_ON(stable_node->rmap_hlist_len < 0);
>>
>>  	hlist_for_each_entry(rmap_item, &stable_node->hlist, hlist) {
>> -		if (rmap_item->hlist.next)
>> +		if (rmap_item->hlist.next) {
>>  			ksm_pages_sharing--;
>> -		else
>> +			trace_ksm_remove_rmap_item(stable_node->kpfn, rmap_item, rmap_item->mm);
>
> Instead of dereferencing the stable_node here, where the work could
> possibly happen outside the trace event and in the hot path, could you pass
> in the stable_node instead, and then in the TP_fast_assign() do:
>
> 		__entry->pfn = stable_node->kpfn;
>
>

I'll make the change in the next version.

>> +		} else {
>>  			ksm_pages_shared--;
>> +		}
>>
>>  		rmap_item->mm->ksm_merging_pages--;
>>
>> @@ -679,6 +684,7 @@ static void remove_node_from_stable_tree(struct ksm_stable_node *stable_node)
>>  	BUILD_BUG_ON(STABLE_NODE_DUP_HEAD <= &migrate_nodes);
>>  	BUILD_BUG_ON(STABLE_NODE_DUP_HEAD >= &migrate_nodes + 1);
>>
>> +	trace_ksm_remove_ksm_page(stable_node->kpfn);
>
> Here too?
>
> -- Steve
>

I'll make the change in the next version.

>>  	if (stable_node->head == &migrate_nodes)
>>  		list_del(&stable_node->list);
>>  	else
>> @@ -1367,6 +1373,8 @@ static int try_to_merge_with_ksm_page(struct ksm_rmap_item *rmap_item,
>>  	get_anon_vma(vma->anon_vma);
>>  out:
>>  	mmap_read_unlock(mm);
>> +	trace_ksm_merge_with_ksm_page(kpage, page_to_pfn(kpage ? kpage : page),
>> +				rmap_item, mm, err);
>>  	return err;
>>  }
>>
>> @@ -2114,6 +2122,9 @@ static int try_to_merge_with_kernel_zero_page(struct ksm_rmap_item *rmap_item,
>>  		if (vma) {
>>  			err = try_to_merge_one_page(vma, page,
>>  						ZERO_PAGE(rmap_item->address));
>> +			trace_ksm_merge_one_page(
>> +				page_to_pfn(ZERO_PAGE(rmap_item->address)),
>> +				rmap_item, mm, err);
>>  			if (!err) {
>>  				rmap_item->address |= ZERO_PAGE_FLAG;
>>  				ksm_zero_pages_sharing++;
>> @@ -2344,6 +2355,8 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>
>>  	mm_slot = ksm_scan.mm_slot;
>>  	if (mm_slot == &ksm_mm_head) {
>> +		trace_ksm_start_scan(ksm_scan.seqnr, ksm_rmap_items);
>> +
>>  		/*
>>  		 * A number of pages can hang around indefinitely on per-cpu
>>  		 * pagevecs, raised page count preventing write_protect_page
>> @@ -2510,6 +2523,7 @@ static struct ksm_rmap_item *scan_get_next_rmap_item(struct page **page)
>>  	if (mm_slot != &ksm_mm_head)
>>  		goto next_mm;
>>
>> +	trace_ksm_stop_scan(ksm_scan.seqnr, ksm_rmap_items);
>>  	ksm_scan.seqnr++;
>>  	return NULL;
>>  }
>> @@ -2661,6 +2675,7 @@ int __ksm_enter(struct mm_struct *mm)
>>  	if (needs_wakeup)
>>  		wake_up_interruptible(&ksm_thread_wait);
>>
>> +	trace_ksm_enter(mm);
>>  	return 0;
>>  }
>>
>> @@ -2702,6 +2717,8 @@ void __ksm_exit(struct mm_struct *mm)
>>  		mmap_write_lock(mm);
>>  		mmap_write_unlock(mm);
>>  	}
>> +
>> +	trace_ksm_exit(mm);
>>  }
>>
>>  struct page *ksm_might_need_to_copy(struct page *page,
>>
>> base-commit: 234a68e24b120b98875a8b6e17a9dead277be16a