diff mbox series

[v2] mm/khugepaged: add tracepoint to collapse_file()

Message ID 20221024173559.332324-1-gautammenghani201@gmail.com
State New
Headers
Series [v2] mm/khugepaged: add tracepoint to collapse_file() |

Commit Message

Gautam Menghani Oct. 24, 2022, 5:35 p.m. UTC 
  In the file mm/khugepaged.c, a TODO in the function collapse_file() asks
to add tracepoints. Add the tracepoint named "mm_khugepaged_collapse_file".

Signed-off-by: Gautam Menghani <gautammenghani201@gmail.com>
---
changes in v2:
1. Move filename dereference out of calling path (Steven Rostedt).
2. Capture is_shmem and index (Zach O'Keefe)

 include/trace/events/huge_memory.h | 38 ++++++++++++++++++++++++++++++
 mm/khugepaged.c                    |  4 +++-
 2 files changed, 41 insertions(+), 1 deletion(-)

Comments

Andrew Morton Oct. 24, 2022, 8:17 p.m. UTC | #1 
On Mon, 24 Oct 2022 23:05:58 +0530 Gautam Menghani <gautammenghani201@gmail.com> wrote:

> In the file mm/khugepaged.c, a TODO in the function collapse_file() asks
> to add tracepoints. Add the tracepoint named "mm_khugepaged_collapse_file".

This isn't a very satisfying explanation for changing the kernel.  Maybe
the comment is stale are this tracepoint is unneeded.

Please explain afresh how this addition benefits kernel users?
Gautam Menghani Oct. 25, 2022, 5:05 a.m. UTC | #2 
On Mon, Oct 24, 2022 at 01:17:06PM -0700, Andrew Morton wrote:
> On Mon, 24 Oct 2022 23:05:58 +0530 Gautam Menghani <gautammenghani201@gmail.com> wrote:
> 
> > In the file mm/khugepaged.c, a TODO in the function collapse_file() asks
> > to add tracepoints. Add the tracepoint named "mm_khugepaged_collapse_file".
> 
> This isn't a very satisfying explanation for changing the kernel.  Maybe
> the comment is stale are this tracepoint is unneeded.
> 
> Please explain afresh how this addition benefits kernel users?
> 
The function collapse_file() is called by the function hpage_collapse_scan_file(). 
Without a tracepoint in collapse_file(), we won't know if it was called or not and as a result,
we also won't know if it returned successfully or not. Also, as Zach mentioned earlier [1]:

there are a few scan result codes that overlap between hpage_collapse_scan_file() and those
possibly returned in collapse_file() such that, if we only have the one tracepoint in 
hpage_collapse_scan_file(), it could be ambiguous what callsite the error path stemmed from.

[1]:https://lore.kernel.org/lkml/CAAa6QmSKtj6T2dW1tkg5_HVj2+rXj5inOLdEzr0MkJzQxxcPXQ@mail.gmail.com/

Please do let me know if a v3 is needed.

Thanks,
Gautam
Zach O'Keefe Oct. 25, 2022, 6:20 p.m. UTC | #3 
On Oct 25 10:35, Gautam Menghani wrote:
> On Mon, Oct 24, 2022 at 01:17:06PM -0700, Andrew Morton wrote:
> > On Mon, 24 Oct 2022 23:05:58 +0530 Gautam Menghani <gautammenghani201@gmail.com> wrote:
> > 
> > > In the file mm/khugepaged.c, a TODO in the function collapse_file() asks
> > > to add tracepoints. Add the tracepoint named "mm_khugepaged_collapse_file".
> > 
> > This isn't a very satisfying explanation for changing the kernel.  Maybe
> > the comment is stale are this tracepoint is unneeded.
> > 
> > Please explain afresh how this addition benefits kernel users?
> > 
> The function collapse_file() is called by the function hpage_collapse_scan_file(). 
> Without a tracepoint in collapse_file(), we won't know if it was called or not and as a result,
> we also won't know if it returned successfully or not. Also, as Zach mentioned earlier [1]:
> 
> there are a few scan result codes that overlap between hpage_collapse_scan_file() and those
> possibly returned in collapse_file() such that, if we only have the one tracepoint in 
> hpage_collapse_scan_file(), it could be ambiguous what callsite the error path stemmed from.
> 
> [1]:https://lore.kernel.org/lkml/CAAa6QmSKtj6T2dW1tkg5_HVj2+rXj5inOLdEzr0MkJzQxxcPXQ@mail.gmail.com/
> 
> Please do let me know if a v3 is needed.
> 
> Thanks,
> Gautam

Thanks Guatam,

The ambiguous codes in particular are:

SCAN_PTE_MAPPED_HUGEPAGE
SCAN_PAGE_COMPOUND
SCAN_PAGE_COUNT

; properties of a page that are checked before/after the page is
locked/isolated.  This personally hasn't been an issue for me, as someone who's
spent considerable time in these codepaths over the last little while.  There
are certainly other codes (like SCAN_FAIL) which are ambiguous within functions
(and likewise haven't been an issue) so this reason alone isn't particularly
motivating.

However, some of the extra information (is_shmem, index), at times, would have
been useful if was already available -- but it was never enough of a pain to
force my hand to put this tracepoint in.

If the tracepoint is staying, then 2 nits:

CHECK: Alignment should match open parenthesis
#74: FILE: mm/khugepaged.c:2064:
+       trace_mm_khugepaged_collapse_file(mm, hpage, index, is_shmem,
+               addr, file, nr, result);

and, apply Steven's advice to trace_mm_khugepaged_scan_file() for consistency.

Thanks,
Zach
Gautam Menghani Oct. 26, 2022, 4:53 a.m. UTC | #4 
On Tue, Oct 25, 2022 at 11:20:40AM -0700, Zach O'Keefe wrote:
> On Oct 25 10:35, Gautam Menghani wrote:
> > On Mon, Oct 24, 2022 at 01:17:06PM -0700, Andrew Morton wrote:
> > > On Mon, 24 Oct 2022 23:05:58 +0530 Gautam Menghani <gautammenghani201@gmail.com> wrote:
> > > 
> > > > In the file mm/khugepaged.c, a TODO in the function collapse_file() asks
> > > > to add tracepoints. Add the tracepoint named "mm_khugepaged_collapse_file".
> > > 
> > > This isn't a very satisfying explanation for changing the kernel.  Maybe
> > > the comment is stale are this tracepoint is unneeded.
> > > 
> > > Please explain afresh how this addition benefits kernel users?
> > > 
> > The function collapse_file() is called by the function hpage_collapse_scan_file(). 
> > Without a tracepoint in collapse_file(), we won't know if it was called or not and as a result,
> > we also won't know if it returned successfully or not. Also, as Zach mentioned earlier [1]:
> > 
> > there are a few scan result codes that overlap between hpage_collapse_scan_file() and those
> > possibly returned in collapse_file() such that, if we only have the one tracepoint in 
> > hpage_collapse_scan_file(), it could be ambiguous what callsite the error path stemmed from.
> > 
> > [1]:https://lore.kernel.org/lkml/CAAa6QmSKtj6T2dW1tkg5_HVj2+rXj5inOLdEzr0MkJzQxxcPXQ@mail.gmail.com/
> > 
> > Please do let me know if a v3 is needed.
> > 
> > Thanks,
> > Gautam
> 
> Thanks Guatam,
> 
> The ambiguous codes in particular are:
> 
> SCAN_PTE_MAPPED_HUGEPAGE
> SCAN_PAGE_COMPOUND
> SCAN_PAGE_COUNT
> 
> ; properties of a page that are checked before/after the page is
> locked/isolated.  This personally hasn't been an issue for me, as someone who's
> spent considerable time in these codepaths over the last little while.  There
> are certainly other codes (like SCAN_FAIL) which are ambiguous within functions
> (and likewise haven't been an issue) so this reason alone isn't particularly
> motivating.
> 
> However, some of the extra information (is_shmem, index), at times, would have
> been useful if was already available -- but it was never enough of a pain to
> force my hand to put this tracepoint in.
> 
> If the tracepoint is staying, then 2 nits:
> 
> CHECK: Alignment should match open parenthesis
> #74: FILE: mm/khugepaged.c:2064:
> +       trace_mm_khugepaged_collapse_file(mm, hpage, index, is_shmem,
> +               addr, file, nr, result);
> 
> and, apply Steven's advice to trace_mm_khugepaged_scan_file() for consistency.
Yes I've sent a patch to refactor the mm_khugepaged_scan_file() tracepoint - 
lore.kernel.org/linux-mm/20221026044524.54793-1-gautammenghani201@gmail.com/T/#u
diff mbox series

Patch

diff --git a/include/trace/events/huge_memory.h b/include/trace/events/huge_memory.h
index 935af4947917..472238d0e3a3 100644
--- a/include/trace/events/huge_memory.h
+++ b/include/trace/events/huge_memory.h
@@ -203,5 +203,43 @@  TRACE_EVENT(mm_khugepaged_scan_file,
 		__print_symbolic(__entry->result, SCAN_STATUS))
 );
 
+TRACE_EVENT(mm_khugepaged_collapse_file,
+	TP_PROTO(struct mm_struct *mm, struct page *hpage, pgoff_t index,
+			bool is_shmem, unsigned long addr, struct file *file,
+			int nr, int result),
+	TP_ARGS(mm, hpage, index, is_shmem, addr, file, nr, result),
+	TP_STRUCT__entry(
+		__field(struct mm_struct *, mm)
+		__field(unsigned long, hpfn)
+		__field(pgoff_t, index)
+		__field(bool, is_shmem)
+		__field(unsigned long, addr)
+		__string(filename, file->f_path.dentry->d_iname)
+		__field(int, nr)
+		__field(int, result)
+	),
+
+	TP_fast_assign(
+		__entry->mm = mm;
+		__entry->hpfn = hpage ? page_to_pfn(hpage) : -1;
+		__entry->index = index;
+		__entry->is_shmem = is_shmem;
+		__entry->addr = addr;
+		__assign_str(filename, file->f_path.dentry->d_iname);
+		__entry->nr = nr;
+		__entry->result = result;
+	),
+
+	TP_printk("mm=%p, hpage_pfn=0x%lx, index=%ld, is_shmem=%d, addr=%ld, filename=%s, nr=%d, result=%s",
+		__entry->mm,
+		__entry->hpfn,
+		__entry->index,
+		__entry->is_shmem,
+		__entry->addr,
+		__get_str(filename),
+		__entry->nr,
+		__print_symbolic(__entry->result, SCAN_STATUS))
+);
+
 #endif /* __HUGE_MEMORY_H */
 #include <trace/define_trace.h>
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 4734315f7940..977c0ff82c46 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -2059,7 +2059,9 @@  static int collapse_file(struct mm_struct *mm, unsigned long addr,
 		mem_cgroup_uncharge(page_folio(hpage));
 		put_page(hpage);
 	}
-	/* TODO: tracepoints */
+
+	trace_mm_khugepaged_collapse_file(mm, hpage, index, is_shmem,
+		addr, file, nr, result);
 	return result;
 }