lkml.org 
[lkml]   [2010]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv2 2/2] tracing,mm - add kernel pagefault tracepoint for x86 & x86_64
    On Thu, Nov 11, 2010 at 10:09:09AM +0100, Jiri Olsa wrote:
    > This provides a tracepoint to trace kernel pagefault event.
    >
    > When analyzing a vmcore resulting from a kernel failure, we
    > often hypothesize that "there should have a pagefault event
    > just before this instruction" or similar. Sometimes it means
    > that there should have a small delay between instructions that
    > extends a critical session and exposed a missing lock. Since
    > there have been no evidence of kernel pagefault, it is quite
    > difficult to adopt the hypothesis.
    >
    > If we can trace the kernel pagefault event, it will help narrow
    > the possible cause of failure and will accelerate the
    > investigation a lot.
    >
    >
    > Signed-off-by: Larry Woodman <lwoodman@redhat.com>
    > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
    > ---
    > arch/x86/mm/fault.c | 33 ++++++++++++++++++++++-----------
    > include/trace/events/kmem.h | 23 +++++++++++++++++++++++
    > 2 files changed, 45 insertions(+), 11 deletions(-)
    >
    > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
    > index 7d90ceb..171dcc9 100644
    > --- a/arch/x86/mm/fault.c
    > +++ b/arch/x86/mm/fault.c
    > @@ -12,6 +12,7 @@
    > #include <linux/mmiotrace.h> /* kmmio_handler, ... */
    > #include <linux/perf_event.h> /* perf_sw_event */
    > #include <linux/hugetlb.h> /* hstate_index_to_shift */
    > +#include <trace/events/kmem.h>
    >
    > #include <asm/traps.h> /* dotraplinkage, ... */
    > #include <asm/pgalloc.h> /* pgd_*(), ... */
    > @@ -944,17 +945,11 @@ static int fault_in_kernel_space(unsigned long address)
    > return address >= TASK_SIZE_MAX;
    > }
    >
    > -/*
    > - * This routine handles page faults. It determines the address,
    > - * and the problem, and then passes it off to one of the appropriate
    > - * routines.
    > - */
    > -dotraplinkage void __kprobes
    > -do_page_fault(struct pt_regs *regs, unsigned long error_code)
    > +static inline void __do_page_fault(struct pt_regs *regs, unsigned long address,
    > + unsigned long error_code)
    > {
    > struct vm_area_struct *vma;
    > struct task_struct *tsk;
    > - unsigned long address;
    > struct mm_struct *mm;
    > int fault;
    > int write = error_code & PF_WRITE;
    > @@ -964,9 +959,6 @@ do_page_fault(struct pt_regs *regs, unsigned long error_code)
    > tsk = current;
    > mm = tsk->mm;
    >
    > - /* Get the faulting address: */
    > - address = read_cr2();
    > -
    > /*
    > * Detect and handle instructions that would cause a page fault for
    > * both a tracked kernel page and a userspace page.
    > @@ -1158,3 +1150,22 @@ good_area:
    >
    > up_read(&mm->mmap_sem);
    > }
    > +
    > +/*
    > + * This routine handles page faults. It determines the address,
    > + * and the problem, and then passes it off to one of the appropriate
    > + * routines.
    > + */
    > +dotraplinkage void __kprobes
    > +do_page_fault(struct pt_regs *regs, unsigned long error_code)
    > +{
    > + unsigned long address;
    > +
    > + /* Get the faulting address: */
    > + address = read_cr2();
    > +
    > + __do_page_fault(regs, address, error_code);
    > +
    > + if (!user_mode(regs))
    > + trace_mm_kernel_pagefault(current, address, error_code);
    > +}



    I (and others) have been testing your patch to measure the latencies of page
    faults.

    So I have several comments about it.

    First, we don't want a pointer to current, we can already retrieve the pid
    from a trace.

    Second, it would be definetly interesting to also have the instruction address
    that faulted (regs->ip).

    Three, I wonder why this tracepoint only traces kernel faults. And in fact
    kernel faults is a confusing name. Users will be confused whether this is
    about tracing only faults happening in kernel or also faults happening in
    kernel data.
    Actually I don't see any reason right now to trace only kernel faults. Do you?
    If that's needed, one can still check on post-processing that the address
    was in the kernel.

    And four, measuring page fault handling duration can be desired, it would be
    nice to have a page_fault_start, page_fault_end.


    So in the end we can get:


    dotraplinkage void __kprobes
    do_page_fault(struct pt_regs *regs, unsigned long error_code)
    {
    unsigned long address;

    /* Get the faulting address: */
    address = read_cr2();

    trace_mm_pagefault_start(address, error_code);
    __do_page_fault(regs, address, error_code);
    trace_mm_pagefault_end(address);
    }


    Would you be ok with that?

    Last thing I worry about is that error_code that is very arch dependent.
    If someone writes a script that depends on the x86 code, it won't work
    elsewhere while it's fairly possible to have a generic tracepoint there.

    So perhaps we rather need a generic enum field instead of the error_code,
    to express the most interesting specific fault attributes. Than can
    probably be added later though, once someone really needs it.

    Hm?



    \
     
     \ /
      Last update: 2010-11-15 14:47    [W:0.031 / U:1.112 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site