lkml.org 
[lkml]   [2009]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: + page-owner-tracking.patch added to -mm tree

    * akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:

    >
    > The patch titled
    > page-owner tracking
    > has been added to the -mm tree. Its filename is
    > page-owner-tracking.patch
    >
    > Before you just go and hit "reply", please:
    > a) Consider who else should be cc'ed
    > b) Prefer to cc a suitable mailing list as well
    > c) Ideally: find the original patch on the mailing list and do a
    > reply-to-all to that, adding suitable additional cc's
    >
    > *** Remember to use Documentation/SubmitChecklist when testing your code ***
    >
    > See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
    > out what to do about this
    >
    > The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
    >
    > ------------------------------------------------------
    > Subject: page-owner tracking
    > From: Alexander Nyberg <alexn@dsv.su.se>
    >
    > > On Fri, 20 Mar 2009 15:27:00 +0000 Mel Gorman <mel@csn.ul.ie> wrote:
    >
    > Here is a rebased reversion. Appears to work as advertised based on a
    > quick test with qemu and builds without CONFIG_PROC_PAGEOWNER
    >
    >
    >
    > Introduces CONFIG_PROC_PAGEOWNER that keeps track of the call chain
    > under which a page was allocated. Includes a user-space helper in
    > Documentation/page_owner.c to sort the enormous amount of output that this
    > may give (thanks tridge).
    >
    > Information available through /proc/page_owner
    >
    > x86_64 introduces some stack noise in certain call chains so for exact
    > output use of x86 && CONFIG_FRAME_POINTER is suggested. Tested on x86,
    > x86 && CONFIG_FRAME_POINTER, x86_64
    >
    > Output looks like:
    >
    > 4819 times:
    > Page allocated via order 0, mask 0x50
    > [0xc012b7b9] find_lock_page+25
    > [0xc012b8c8] find_or_create_page+152
    > [0xc0147d74] grow_dev_page+36
    > [0xc0148164] __find_get_block+84
    > [0xc0147ebc] __getblk_slow+124
    > [0xc0148164] __find_get_block+84
    > [0xc01481e7] __getblk+55
    > [0xc0185d14] do_readahead+100
    >
    > We use a custom stack unwinder because using __builtin_return_address([0-7])
    > causes gcc to generate code that might try to unwind the stack looking for
    > function return addresses and "fall off" causing early panics if the call
    > chain is not deep enough. So in that case we could have had a depth of
    > around 3 functions in all traces (I experimented a bit with this).
    >
    >
    > make page_owner handle non-contiguous page ranges
    >
    >
    > I've cleaned up the __alloc_pages() part to a simple set_page_owner() call.
    >
    > Signed-off-by: Alexander Nyberg <alexn@dsv.su.se>
    > Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
    > Signed-off-by: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitu.com>
    > DESC
    > Update page->order at an appropriate time when tracking PAGE_OWNER
    > EDESC
    >
    > PAGE_OWNER tracks free pages by setting page->order to -1. However, it is
    > set during __free_pages() which is not the only free path as
    > __pagevec_free() and free_compound_page() do not go through __free_pages().
    > This leads to a situation where free pages are visible in /proc/page_owner
    > which is confusing and might be interpreted as a memory leak.
    >
    > This patch sets page->owner when PageBuddy is set. It also prints a
    > warning to the kernel log if a free page is found that does not appear free
    > to PAGE_OWNER. This should be considered a fix to
    > page-owner-tracking-leak-detector.patch.
    >
    > This only applies to -mm as PAGE_OWNER is not in mainline.
    >
    > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
    > Acked-by: Andy Whitcroft <apw@shadowen.org>
    > DESC
    > Print out PAGE_OWNER statistics in relation to fragmentation avoidance
    > EDESC
    >
    > When PAGE_OWNER is set, more information is available of relevance to
    > fragmentation avoidance. A second line is added to /proc/page_owner showing
    > the PFN, the pageblock number, the mobility type of the page based on its
    > allocation flags, whether the allocation is improperly placed and the flags.
    > A sample entry looks like
    >
    > Page allocated via order 0, mask 0x1280d2
    > PFN 7355 Block 7 type 3 Fallback Flags LA
    > [0xc01528c6] __handle_mm_fault+598
    > [0xc0320427] do_page_fault+279
    > [0xc031ed9a] error_code+114
    >
    > This information can be used to identify pages that are improperly placed.
    > As the format of PAGE_OWNER data is now different, the comment at the top
    > of Documentation/page_owner.c is updated with new instructions.
    >
    > As PAGE_OWNER tracks the GFP flags used to allocate the pages,
    > /proc/pagetypeinfo is enhanced to contain how many mixed blocks exist.
    > The additional output looks like
    >
    > Number of mixed blocks Unmovable Reclaimable Movable Reserve
    > Node 0, zone DMA 0 1 2 1
    > Node 0, zone Normal 2 11 33 0
    >
    > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
    > Acked-by: Andy Whitcroft <apw@shadowen.org>
    > Acked-by: Christoph Lameter <cl@linux-foundation.org>
    > DESC
    > Allow PAGE_OWNER to be set on any architecture
    > EDESC
    >
    > Currently PAGE_OWNER depends on CONFIG_X86. This appears to be due to
    > pfn_to_page() being called in an inappropriate for many memory models
    > and the presense of memory holes. This patch ensures that pfn_valid()
    > and pfn_valid_within() is called at the appropriate places and the offsets
    > correctly updated so that PAGE_OWNER is safe on any architecture.
    >
    > In situations where CONFIG_HOLES_IN_ZONES is set (IA64 with VIRTUAL_MEM_MAP),
    > there may be cases where pages allocated within a MAX_ORDER_NR_PAGES block
    > of pages may not be displayed in /proc/page_owner if the hole is at the
    > start of the block. Addressing this would be quite complex, perform slowly
    > and is of no clear benefit.
    >
    > Once PAGE_OWNER is allowed on all architectures, the statistics for grouping
    > pages by mobility that declare how many pageblocks contain mixed page types
    > becomes optionally available on all arches.
    >
    > This patch was tested successfully on x86, x86_64, ppc64 and IA64 machines.
    >
    > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
    > Acked-by: Andy Whitcroft <apw@shadowen.org>
    > DESC
    > allow-page_owner-to-be-set-on-any-architecture-fix
    > EDESC
    >
    > Cc: Andy Whitcroft <apw@shadowen.org>
    > Cc: Mel Gorman <mel@csn.ul.ie>
    > DESC
    > allow-page_owner-to-be-set-on-any-architecture-fix fix
    > EDESC
    >
    > Page-owner-tracking stores the a backtrace of an allocation in the
    > struct page. How the stack trace is generated depends on whether
    > CONFIG_FRAME_POINTER is set or not. If CONFIG_FRAME_POINTER is set,
    > the frame pointer must be read using some inline assembler which is not
    > available for all architectures.
    >
    > This patch uses the frame pointer where it is available but has a fallback
    > where it is not.
    >
    > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
    > Cc: Andy Whitcroft <apw@shadowen.org>
    > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    >
    >
    > Rebase on top of procfs changes
    >
    > Signed-off-by: Mel Gorman <mel@csn.ul.ie>
    >
    > new file mode 100644
    > index 0000000..9081bd6
    > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    > ---
    >
    > Documentation/page_owner.c | 144 +++++++++++++++++++++++++++++++++++
    > fs/proc/Makefile | 1
    > include/linux/mm_types.h | 6 +
    > lib/Kconfig.debug | 10 ++
    > mm/page_alloc.c | 66 ++++++++++++++++
    > mm/vmstat.c | 93 ++++++++++++++++++++++
    > 6 files changed, 320 insertions(+)
    >
    > diff -puN /dev/null Documentation/page_owner.c
    > --- /dev/null
    > +++ a/Documentation/page_owner.c
    > @@ -0,0 +1,144 @@
    > +/*
    > + * User-space helper to sort the output of /proc/page_owner
    > + *
    > + * Example use:
    > + * cat /proc/page_owner > page_owner_full.txt
    > + * grep -v ^PFN page_owner_full.txt > page_owner.txt
    > + * ./sort page_owner.txt sorted_page_owner.txt
    > +*/
    > +
    > +#include <stdio.h>
    > +#include <stdlib.h>
    > +#include <sys/types.h>
    > +#include <sys/stat.h>
    > +#include <fcntl.h>
    > +#include <unistd.h>
    > +#include <string.h>
    > +
    > +struct block_list {
    > + char *txt;
    > + int len;
    > + int num;
    > +};
    > +
    > +
    > +static struct block_list *list;
    > +static int list_size;
    > +static int max_size;
    > +
    > +struct block_list *block_head;
    > +
    > +int read_block(char *buf, FILE *fin)
    > +{
    > + int ret = 0;
    > + int hit = 0;
    > + char *curr = buf;
    > +
    > + for (;;) {
    > + *curr = getc(fin);
    > + if (*curr == EOF) return -1;
    > +
    > + ret++;
    > + if (*curr == '\n' && hit == 1)
    > + return ret - 1;
    > + else if (*curr == '\n')
    > + hit = 1;
    > + else
    > + hit = 0;
    > + curr++;
    > + }
    > +}
    > +
    > +static int compare_txt(const void *d1, const void *d2)
    > +{
    > + struct block_list *l1 = (struct block_list *)d1;
    > + struct block_list *l2 = (struct block_list *)d2;
    > + return strcmp(l1->txt, l2->txt);
    > +}
    > +
    > +static int compare_num(const void *d1, const void *d2)
    > +{
    > + struct block_list *l1 = (struct block_list *)d1;
    > + struct block_list *l2 = (struct block_list *)d2;
    > + return l2->num - l1->num;
    > +}
    > +
    > +static void add_list(char *buf, int len)
    > +{
    > + if (list_size != 0 &&
    > + len == list[list_size-1].len &&
    > + memcmp(buf, list[list_size-1].txt, len) == 0) {
    > + list[list_size-1].num++;
    > + return;
    > + }
    > + if (list_size == max_size) {
    > + printf("max_size too small??\n");
    > + exit(1);
    > + }
    > + list[list_size].txt = malloc(len+1);
    > + list[list_size].len = len;
    > + list[list_size].num = 1;
    > + memcpy(list[list_size].txt, buf, len);
    > + list[list_size].txt[len] = 0;
    > + list_size++;
    > + if (list_size % 1000 == 0) {
    > + printf("loaded %d\r", list_size);
    > + fflush(stdout);
    > + }
    > +}
    > +
    > +int main(int argc, char **argv)
    > +{
    > + FILE *fin, *fout;
    > + char buf[1024];
    > + int ret, i, count;
    > + struct block_list *list2;
    > + struct stat st;
    > +
    > + fin = fopen(argv[1], "r");
    > + fout = fopen(argv[2], "w");
    > + if (!fin || !fout) {
    > + printf("Usage: ./program <input> <output>\n");
    > + perror("open: ");
    > + exit(2);
    > + }
    > +
    > + fstat(fileno(fin), &st);
    > + max_size = st.st_size / 100; /* hack ... */
    > +
    > + list = malloc(max_size * sizeof(*list));
    > +
    > + for(;;) {
    > + ret = read_block(buf, fin);
    > + if (ret < 0)
    > + break;
    > +
    > + buf[ret] = '\0';
    > + add_list(buf, ret);
    > + }
    > +
    > + printf("loaded %d\n", list_size);
    > +
    > + printf("sorting ....\n");
    > +
    > + qsort(list, list_size, sizeof(list[0]), compare_txt);
    > +
    > + list2 = malloc(sizeof(*list) * list_size);
    > +
    > + printf("culling\n");
    > +
    > + for (i=count=0;i<list_size;i++) {
    > + if (count == 0 ||
    > + strcmp(list2[count-1].txt, list[i].txt) != 0)
    > + list2[count++] = list[i];
    > + else
    > + list2[count-1].num += list[i].num;
    > + }
    > +
    > + qsort(list2, count, sizeof(list[0]), compare_num);
    > +
    > + for (i=0;i<count;i++) {
    > + fprintf(fout, "%d times:\n%s\n", list2[i].num, list2[i].txt);
    > + }
    > + return 0;
    > +}
    > diff -puN fs/proc/Makefile~page-owner-tracking fs/proc/Makefile
    > --- a/fs/proc/Makefile~page-owner-tracking
    > +++ a/fs/proc/Makefile
    > @@ -24,6 +24,7 @@ proc-$(CONFIG_PROC_SYSCTL) += proc_sysct
    > proc-$(CONFIG_NET) += proc_net.o
    > proc-$(CONFIG_PROC_KCORE) += kcore.o
    > proc-$(CONFIG_PROC_VMCORE) += vmcore.o
    > +proc-$(CONFIG_PROC_PAGEOWNER) += pageowner.o
    > proc-$(CONFIG_PROC_DEVICETREE) += proc_devtree.o
    > proc-$(CONFIG_PRINTK) += kmsg.o
    > proc-$(CONFIG_PROC_PAGE_MONITOR) += page.o
    > diff -puN include/linux/mm_types.h~page-owner-tracking include/linux/mm_types.h
    > --- a/include/linux/mm_types.h~page-owner-tracking
    > +++ a/include/linux/mm_types.h
    > @@ -95,6 +95,12 @@ struct page {
    > void *virtual; /* Kernel virtual address (NULL if
    > not kmapped, ie. highmem) */
    > #endif /* WANT_PAGE_VIRTUAL */
    > +
    > +#ifdef CONFIG_PROC_PAGEOWNER
    > + int order;
    > + unsigned int gfp_mask;
    > + unsigned long trace[8];
    > +#endif
    > };
    >
    > /*
    > diff -puN lib/Kconfig.debug~page-owner-tracking lib/Kconfig.debug
    > --- a/lib/Kconfig.debug~page-owner-tracking
    > +++ a/lib/Kconfig.debug
    > @@ -66,6 +66,16 @@ config UNUSED_SYMBOLS
    > you really need it, and what the merge plan to the mainline kernel for
    > your module is.
    >
    > +config PROC_PAGEOWNER
    > + bool "Track page owner"
    > + depends on DEBUG_KERNEL
    > + help
    > + This keeps track of what call chain is the owner of a page, may
    > + help to find bare alloc_page(s) leaks. Eats a fair amount of memory.
    > + See Documentation/page_owner.c for user-space helper.
    > +
    > + If unsure, say N.
    > +
    > config DEBUG_FS
    > bool "Debug Filesystem"
    > depends on SYSFS
    > diff -puN mm/page_alloc.c~page-owner-tracking mm/page_alloc.c
    > --- a/mm/page_alloc.c~page-owner-tracking
    > +++ a/mm/page_alloc.c
    > @@ -359,6 +359,9 @@ static inline void set_page_order(struct
    > {
    > set_page_private(page, order);
    > __SetPageBuddy(page);
    > +#ifdef CONFIG_PROC_PAGEOWNER
    > + page->order = -1;
    > +#endif
    > }
    >
    > static inline void rmv_page_order(struct page *page)
    > @@ -1458,6 +1461,62 @@ try_next_zone:
    > return page;
    > }
    >
    > +#ifdef CONFIG_PROC_PAGEOWNER
    > +static inline int valid_stack_ptr(struct thread_info *tinfo, void *p)
    > +{
    > + return p > (void *)tinfo &&
    > + p < (void *)tinfo + THREAD_SIZE - 3;
    > +}
    > +
    > +static inline void __stack_trace(struct page *page, unsigned long *stack,
    > + unsigned long bp)
    > +{
    > + int i = 0;
    > + unsigned long addr;
    > + struct thread_info *tinfo = (struct thread_info *)
    > + ((unsigned long)stack & (~(THREAD_SIZE - 1)));
    > +
    > + memset(page->trace, 0, sizeof(long) * 8);
    > +
    > +#ifdef CONFIG_FRAME_POINTER
    > + if (bp) {
    > + while (valid_stack_ptr(tinfo, (void *)bp)) {
    > + addr = *(unsigned long *)(bp + sizeof(long));
    > + page->trace[i] = addr;
    > + if (++i >= 8)
    > + break;
    > + bp = *(unsigned long *)bp;
    > + }
    > + return;
    > + }
    > +#endif /* CONFIG_FRAME_POINTER */
    > + while (valid_stack_ptr(tinfo, stack)) {
    > + addr = *stack++;
    > + if (__kernel_text_address(addr)) {
    > + page->trace[i] = addr;
    > + if (++i >= 8)
    > + break;
    > + }
    > + }
    > +}

    Uhm, this is not acceptable and broken, we have generic stacktrace
    saving facilities for this precise purpose. It has other problems
    too.

    The purpose of the patch seems genuinely useful and i support the
    concept, but the current design is limiting (we could do much
    better) and the implementation is awful. Please.

    Has this patch been reviewed by or Cc:-ed to anyone versed in kernel
    instrumentation details, before it was applied to -mm? Those folks
    hang around the tracing tree usually so they are easy to find. :)

    Ingo


    \
     
     \ /
      Last update: 2009-04-01 13:21    [W:0.062 / U:61.624 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site