[v4,0/3] page_owner: print stacks and their counter

  Changes v3 -> v4:
     - Rebase (long time has passed)
     - Use boolean instead of enum for action by Alexander Potapenko
     - (I left some feedback untouched because it's been long and
        would like to discuss it here now instead of re-vamping
        and old thread)

Changes v2 -> v3:
     - Replace interface in favor of seq operations (suggested by Vlastimil)
     - Use debugfs interface to store/read valued (suggested by Ammar)

Hi,

page_owner is a great debug functionality tool that gets us to know
about all pages that have been allocated/freed and their stacktrace.
This comes very handy when e.g: debugging leaks, as with some scripting
we might be able to see those stacktraces that are allocating pages
but not freeing theme.

In my experience, that is one of the most useful cases, but it can get
really tedious to screen through all pages aand try to reconstruct the
stack <-> allocated/freed relationship. There is a lot of noise
to cancel off.

This patch aims to fix that by adding a new functionality into page_owner.
What this does is to create a new read-only file "page_owner_stacks",
which prints only the allocating stacktraces and their counting, being that
the times the stacktrace has allocated - the times it has freed.

So we have a clear overview of stacks <-> allocated/freed relationship
without the need to fiddle with pages and trying to match free stacktraces
with allocated stacktraces.

This is achieved by adding a new refcount_t field in the stack_record struct,
incrementing that refcount_t everytime the same stacktrace allocates,
and decrementing it when it frees a page. Details can be seen in the
respective patches.

We also create another file called "page_owner_threshold", which let us
specify a threshold, so when when reading from "page_owner_stacks",
we will only see those stacktraces which counting goes beyond the
threshold we specified.

One thing I am not completely happy about is to polute lib/stackdepot.c file
with the stack_* functions.
We could sort that out if the stack_record struct definitions were in a header
file instead of stackdepot.c.
But I am not sure about that trade-off, so suggestions are accepted.

A PoC can be found below:

# cat /sys/kernel/debug/page_owner_threshold
 0
# cat /sys/kernel/debug/page_owner_stacks > stacks_full.txt
# head -32 stacks_full.txt
 prep_new_page+0x10d/0x180
 get_page_from_freelist+0x1bd6/0x1e10
 __alloc_pages+0x194/0x360
 alloc_page_interleave+0x13/0x90
 new_slab+0x31d/0x530
 ___slab_alloc+0x5d7/0x720
 __slab_alloc.isra.85+0x4a/0x90
 kmem_cache_alloc+0x455/0x4a0
 acpi_ps_alloc_op+0x57/0x8f
 acpi_ps_create_scope_op+0x12/0x23
 acpi_ps_execute_method+0x102/0x2c1
 acpi_ns_evaluate+0x343/0x4da
 acpi_evaluate_object+0x1cb/0x392
 acpi_run_osc+0x135/0x260
 acpi_init+0x165/0x4ed
 do_one_initcall+0x3e/0x200
stack count: 2

 free_pcp_prepare+0x287/0x5c0
 free_unref_page+0x1c/0xd0
 __mmdrop+0x50/0x160
 finish_task_switch+0x249/0x2b0
 __schedule+0x2c3/0x960
 schedule+0x44/0xb0
 futex_wait_queue+0x70/0xd0
 futex_wait+0x160/0x250
 do_futex+0x11c/0x1b0
 __x64_sys_futex+0x5e/0x1d0
 do_syscall_64+0x37/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
stack count: 1

 

# echo 10000 > /sys/kernel/debug/page_owner_threshold
# cat /sys/kernel/debug/page_owner_stacks > stacks_10000.txt
# cat stacks_10000.txt 
 prep_new_page+0x10d/0x180
 get_page_from_freelist+0x1bd6/0x1e10
 __alloc_pages+0x194/0x360
 folio_alloc+0x17/0x40
 page_cache_ra_unbounded+0x96/0x170
 filemap_get_pages+0x23d/0x5e0
 filemap_read+0xbf/0x3a0
 __kernel_read+0x136/0x2f0
 kernel_read_file+0x197/0x2d0
 kernel_read_file_from_fd+0x54/0x90
 __do_sys_finit_module+0x89/0x120
 do_syscall_64+0x37/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
stack count: 36195

 prep_new_page+0x10d/0x180
 get_page_from_freelist+0x1bd6/0x1e10
 __alloc_pages+0x194/0x360
 folio_alloc+0x17/0x40
 page_cache_ra_unbounded+0x96/0x170
 filemap_get_pages+0x23d/0x5e0
 filemap_read+0xbf/0x3a0
 new_sync_read+0x106/0x180
 vfs_read+0x16f/0x190
 ksys_read+0xa5/0xe0
 do_syscall_64+0x37/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
stack count: 44484

 prep_new_page+0x10d/0x180
 get_page_from_freelist+0x1bd6/0x1e10
 __alloc_pages+0x194/0x360
 folio_alloc+0x17/0x40
 page_cache_ra_unbounded+0x96/0x170
 filemap_get_pages+0xdd/0x5e0
 filemap_read+0xbf/0x3a0
 new_sync_read+0x106/0x180
 vfs_read+0x16f/0x190
 ksys_read+0xa5/0xe0
 do_syscall_64+0x37/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
stack count: 17874

Oscar Salvador (3):
  lib/stackdepot: Add a refcount field in stack_record
  mm, page_owner: Add page_owner_stacks file to print out only stacks
    and their counte
  mm,page_owner: Filter out stacks by a threshold counter

 include/linux/stackdepot.h |  17 +++-
 lib/stackdepot.c           | 160 ++++++++++++++++++++++++++++++++++---
 mm/kasan/common.c          |   3 +-
 mm/page_owner.c            |  46 ++++++++++-
 4 files changed, 209 insertions(+), 17 deletions(-)
  

On Fri, Apr 21, 2023 at 12:14 PM Oscar Salvador <osalvador@suse.de> wrote:
>
> Changes v3 -> v4:
>      - Rebase (long time has passed)
>      - Use boolean instead of enum for action by Alexander Potapenko
>      - (I left some feedback untouched because it's been long and
>         would like to discuss it here now instead of re-vamping
>         and old thread)
>
> Changes v2 -> v3:
>      - Replace interface in favor of seq operations (suggested by Vlastimil)
>      - Use debugfs interface to store/read valued (suggested by Ammar)
>
> Hi,
>
> page_owner is a great debug functionality tool that gets us to know
> about all pages that have been allocated/freed and their stacktrace.
> This comes very handy when e.g: debugging leaks, as with some scripting
> we might be able to see those stacktraces that are allocating pages
> but not freeing theme.
>
> In my experience, that is one of the most useful cases, but it can get
> really tedious to screen through all pages aand try to reconstruct the
> stack <-> allocated/freed relationship. There is a lot of noise
> to cancel off.
>
> This patch aims to fix that by adding a new functionality into page_owner.
> What this does is to create a new read-only file "page_owner_stacks",
> which prints only the allocating stacktraces and their counting, being that
> the times the stacktrace has allocated - the times it has freed.
>
> So we have a clear overview of stacks <-> allocated/freed relationship
> without the need to fiddle with pages and trying to match free stacktraces
> with allocated stacktraces.
>
> This is achieved by adding a new refcount_t field in the stack_record struct,
> incrementing that refcount_t everytime the same stacktrace allocates,
> and decrementing it when it frees a page. Details can be seen in the
> respective patches.

I think the implementation of these counters is too specific to
page_owner and is hard to use for any other purpose.
If we decide to have them, there should be no page_owner-specific
logic in the way we initialize/increment/decrement these counters.
The thresholds in "mm,page_owner: Filter out stacks by a threshold
counter" should also belong elsewhere.

Given that no other stackdepot user needs these counters, maybe it
should be cleaner to store an opaque struct along with the stack,
passing its size to stack_depot_save(), and letting users access it
directly using the stackdepot handler.

I am also wondering if a separate hashtable mapping handlers to
counters would solve the problem for you?
  

On 2023-04-21 13:19, Alexander Potapenko wrote:
> I think the implementation of these counters is too specific to
> page_owner and is hard to use for any other purpose.
> If we decide to have them, there should be no page_owner-specific
> logic in the way we initialize/increment/decrement these counters.

Another solution would be to always increment the refcount in 
__stack_depot_save,
in this case the "page-owner" specific changes are gone, and
it is more of a generic thing.
e.g: Andrey Konovalov mentioned that in a future KASAN remodelation,
he would be using a stack refcount as well.

> The thresholds in "mm,page_owner: Filter out stacks by a threshold
> counter" should also belong elsewhere.

That can certainly be cleaned up I guess to not polute non-page_owner 
code.

> Given that no other stackdepot user needs these counters, maybe it
> should be cleaner to store an opaque struct along with the stack,
> passing its size to stack_depot_save(), and letting users access it
> directly using the stackdepot handler.
> 
> I am also wondering if a separate hashtable mapping handlers to
> counters would solve the problem for you?

Let us see first if with the changes from above the code gets to a more
generic and clean stage, if not we can explore further options.

Thanks for your feedback Alexander!
  

On Mon, 24 Apr 2023 05:54:59 +0200 Oscar Salvador <osalvador@suse.de> wrote:

> > Given that no other stackdepot user needs these counters, maybe it
> > should be cleaner to store an opaque struct along with the stack,
> > passing its size to stack_depot_save(), and letting users access it
> > directly using the stackdepot handler.
> > 
> > I am also wondering if a separate hashtable mapping handlers to
> > counters would solve the problem for you?
> 
> Let us see first if with the changes from above the code gets to a more
> generic and clean stage, if not we can explore further options.

Alexander, does this approach sound reasonable to you?

The overall feature seems useful, although I'm not seeing any positive
reviewer feedback.
  

On 6/9/23 23:55, Andrew Morton wrote:
> On Mon, 24 Apr 2023 05:54:59 +0200 Oscar Salvador <osalvador@suse.de> wrote:
> 
>> > Given that no other stackdepot user needs these counters, maybe it
>> > should be cleaner to store an opaque struct along with the stack,
>> > passing its size to stack_depot_save(), and letting users access it
>> > directly using the stackdepot handler.
>> > 
>> > I am also wondering if a separate hashtable mapping handlers to
>> > counters would solve the problem for you?
>> 
>> Let us see first if with the changes from above the code gets to a more
>> generic and clean stage, if not we can explore further options.
> 
> Alexander, does this approach sound reasonable to you?

Note this is a v4 thread; there was (and the version in mm-unstable is) v5,
where the latest was Alexander requesting further changes:

https://lore.kernel.org/all/CAG_fn%3DUJgnLFGgpkXbMeD6axZN_ifEPHvWpy2_oiPyG1a6PXng@mail.gmail.com/

> The overall feature seems useful, although I'm not seeing any positive
> reviewer feedback.