lkml.org 
[lkml]   [2008]   [Feb]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86: add the debugfs interface for the sysprof tool
On Tue, 19 Feb 2008 12:37:56 -0800 Arjan van de Ven <arjan@infradead.org> wrote:

> From: Soren Sandmann <sandmann@redhat.com>
> Subject: [PATCH] x86: add the debugfs interface for the sysprof tool
>
> The sysprof tool is a very easy to use GUI tool to find out where
> userspace is spending CPU time. See
> http://www.daimi.au.dk/~sandmann/sysprof/
> for more information and screenshots on this tool.
>
> Sysprof needs a 200 line kernel module to do it's work, this
> module puts some simple profiling data into debugfs.
>
> ...
>

Seems a poor idea to me. Sure, oprofile is "hard to set up", but not if
your distributor already did it for you.

Sidebar: the code uses the utterly crappy register_timer_hook() which

a) is woefully misnamed and

b) is racy and

c) will disrupt oprofile if it is being used. And vice versa.



This code adds a new kernel->userspace interface which is not even
documented in code comments. It appears to use a pollable debugfs file in
/proc somewhere, carrying an unspecified payload.


What happens when multiple processes are consuming data from the same
debugfs file?


> arch/x86/Kconfig.debug | 10 ++
> arch/x86/kernel/Makefile | 2 +
> arch/x86/kernel/sysprof.c | 200 +++++++++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/sysprof.h | 34 ++++++++
> 4 files changed, 246 insertions(+), 0 deletions(-)
> create mode 100644 arch/x86/kernel/sysprof.c
> create mode 100644 arch/x86/kernel/sysprof.h
>
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 12c98ea..8eb06c0 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -206,6 +206,16 @@ config MMIOTRACE_TEST
>
> Say N, unless you absolutely know what you are doing.
>
> +config SYSPROF
> + tristate "Enable sysprof userland performance sampler"
> + depends on PROFILING

Missing dependency on DEBUG_FS

> + help
> + This option enables the sysprof debugfs file that is used by the
> + sysprof tool. sysprof is a tool to show where in userspace CPU
> + time is spent.
> +
> + When in doubt, say N
> +

And it's x86-specific.

> #
> # IO delay types:
> #
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 4a4260c..1e8fb66 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -80,6 +80,8 @@ obj-$(CONFIG_DEBUG_NX_TEST) += test_nx.o
> obj-$(CONFIG_VMI) += vmi_32.o vmiclock_32.o
> obj-$(CONFIG_PARAVIRT) += paravirt.o paravirt_patch_$(BITS).o
>
> +obj-$(CONFIG_SYSPROF) += sysprof.o
> +
> ifdef CONFIG_INPUT_PCSPKR
> obj-y += pcspeaker.o
> endif
> diff --git a/arch/x86/kernel/sysprof.c b/arch/x86/kernel/sysprof.c
> new file mode 100644
> index 0000000..6220b9f
> --- /dev/null
> +++ b/arch/x86/kernel/sysprof.c
> @@ -0,0 +1,200 @@
> +/* -*- c-basic-offset: 8 -*- */
> +
> +/* Sysprof -- Sampling, systemwide CPU profiler
> + * Copyright 2004, Red Hat, Inc.
> + * Copyright 2004, 2005, Soeren Sandmann
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/proc_fs.h>
> +#include <linux/poll.h>
> +#include <linux/highmem.h>
> +#include <linux/pagemap.h>
> +#include <linux/profile.h>
> +#include <linux/debugfs.h>
> +#include <asm/uaccess.h>

checkpatch used to warn that linux/uaccess.h is preferred over asm/uaccess.h
but this is another of those checkpatch features which seems to have
mysteriously disappeared, or it broke?

> +#include <asm/atomic.h>
> +
> +#include "sysprof.h"
> +
> +#define SAMPLES_PER_SECOND (200)
> +#define INTERVAL ((HZ <= SAMPLES_PER_SECOND)? 1 : (HZ / SAMPLES_PER_SECOND))
> +#define N_TRACES 256
> +
> +static struct sysprof_stacktrace stack_traces[N_TRACES];
> +static struct sysprof_stacktrace *head = &stack_traces[0];
> +static struct sysprof_stacktrace *tail = &stack_traces[0];

Access to head and tail appear to be racy. See below.

> +static DECLARE_WAIT_QUEUE_HEAD(wait_for_trace);
> +static DECLARE_WAIT_QUEUE_HEAD(wait_for_exit);
> +
> +struct userspace_reader {
> + struct task_struct *task;
> + unsigned long cache_address;
> + unsigned long *cache;
> +};
> +
> +struct stack_frame;
> +
> +struct stack_frame {
> + struct stack_frame __user *next;
> + unsigned long return_address;
> +};
> +
> +static int read_frame(struct stack_frame __user *frame_pointer,
> + struct stack_frame *frame)
> +{
> + if (__copy_from_user_inatomic(frame, frame_pointer,
> + sizeof(struct stack_frame)))
> + return 1;
> + return 0;
> +}
> +
> +static DEFINE_PER_CPU(int, n_samples);
> +
> +static int timer_notify(struct pt_regs *regs)
> +{
> + struct sysprof_stacktrace *trace = head;

We read `head' before taking the "lock". Another CPU could modify it after
we took a local copy.

> + int i;
> + int is_user;
> + static atomic_t in_timer_notify = ATOMIC_INIT(1);
> + int n;
> +
> + n = ++get_cpu_var(n_samples);
> + put_cpu_var(n_samples);

Needlessly disables preemption. Use __get_cpu_var().

> + if (n % INTERVAL != 0)
> + return 0;

It'd be nice to make INTERVAL a power of 2.

> + /* 0: locked, 1: unlocked */
> +
> + if (!atomic_dec_and_test(&in_timer_notify))
> + goto out;

Why not use spin_trylock()? Then you get lockdep support too.

And why not use spin_lock()? At least a comment should be added explaining
and justifying this peculiar home-made-not-really-locking design.

> + is_user = user_mode(regs);
> +
> + if (!current || current->pid == 0)
> + goto out;

And the changelog should explain and justify why we cannot profile root's
code. I cannot begin to imagine why it was done and I cannot fathom why it
passed uncommented in documentation, code, changelog and "review" comments.
It greatly reduces the usefulness of an already dubious feature.

If this limitation _was_ documented then I'd be in a position to ask what is
special about root, as opposed to some non-root user who has <unspecified>
capabilities. And why we penalise a root who has dropped <unspecified>
capabilities. etcetera.

Is this open-coded test of ->pid correct in a containerised environment?

> + if (is_user && current->state != TASK_RUNNING)
> + goto out;

Needs a comment (although this one is fairly obvious)

> + if (!is_user) {
> + /* kernel */
> + trace->pid = current->pid;
> + trace->truncated = 0;
> + trace->n_addresses = 1;
> +
> + /* 0x1 is taken by sysprof to mean "in kernel" */
> + trace->addresses[0] = 0x1;
> + } else {
> + struct stack_frame __user *frame_pointer;
> + struct stack_frame frame;
> + memset(trace, 0, sizeof(struct sysprof_stacktrace));
> +
> + trace->pid = current->pid;

This is ambiguous in a containerised environment.

Ingo, please be alert for anything which exposes raw pids to userspace.

I don't know what a correct and suitable interface might be - perhaps Pavel
or Eric can suggest something.

> + trace->truncated = 0;
> +
> + i = 0;
> +
> + trace->addresses[i++] = regs->ip;
> +
> + frame_pointer = (struct stack_frame __user *)regs->bp;
> +
> + while (read_frame(frame_pointer, &frame) == 0 &&
> + i < SYSPROF_MAX_ADDRESSES &&
> + (unsigned long)frame_pointer >= regs->sp) {
> + trace->addresses[i++] = frame.return_address;
> + frame_pointer = frame.next;
> + }

The (absent) interface documentation should explain what happens when a
fault causes this information to be truncated.

> + trace->n_addresses = i;
> +
> + if (i == SYSPROF_MAX_ADDRESSES)
> + trace->truncated = 1;
> + else
> + trace->truncated = 0;
> + }
> +
> + if (head++ == &stack_traces[N_TRACES - 1])
> + head = &stack_traces[0];

`head' can just merrily advance over `tail' and there is no notification to
userspace of the lost data.

> + wake_up(&wait_for_trace);
> +
> +out:
> + atomic_inc(&in_timer_notify);
> + return 0;
> +}
> +
> +static ssize_t procfile_read(struct file *filp, char __user *buffer,
> + size_t count, loff_t *ppos)
> +{
> + ssize_t bcount;
> + if (head == tail)
> + return -EWOULDBLOCK;

Please put a blank line between end-of-variables and start-of-code.

This seems to be a wrong return value? Shouldn't it just return zero if
there was nothing there? As can happen if some other process is reading the
same debugfs file?

> + BUG_ON(tail->pid == 0);

whee. There was no locking above to prevent the tasks's pid from
transitioning from non-zero to zero after it was tested. Which means this
is triggerable. Perhaps the implicit locking due to cpu-pinnedness and
interruption will prevent that race. If so, such a subtlety should be
commented, no?

> + *ppos = 0;
> + bcount = simple_read_from_buffer(buffer, count, ppos,
> + tail, sizeof(struct sysprof_stacktrace));
> +
> + if (tail++ == &stack_traces[N_TRACES - 1])
> + tail = &stack_traces[0];

There is no locking for `tail', and afaict we support multiple simultaneous
readers.

> + return bcount;
> +}

This reads a single item even if there were 100 queued, which is quite
inefficient.

We already have infrastructure for bulk kernel->user transfer in
kernel/relay.c?

> +static unsigned int procfile_poll(struct file *filp, poll_table * poll_table)

checkpatch missed this coding-style error.

> +{
> + if (head != tail)
> + return POLLIN | POLLRDNORM;
> +
> + poll_wait(filp, &wait_for_trace, poll_table);
> +
> + if (head != tail)
> + return POLLIN | POLLRDNORM;
> +
> + return 0;
> +}
> +
> +static const struct file_operations sysprof_fops = {
> + .owner = THIS_MODULE,
> + .read = procfile_read,
> + .poll = procfile_poll,
> +};
> +
> +static struct dentry *debugfs_pe;
> +int init_module(void)
> +{
> + debugfs_pe = debugfs_create_file("sysprof-trace", 0600, NULL, NULL,
> + &sysprof_fops);
> + if (!debugfs_pe)
> + return -ENODEV;
> + register_timer_hook(timer_notify);
> + return 0;
> +}

This function will enable sysprof-trace even if prof_on==0,
prof_on==SLEEP_PROFILING, etc which is pointless.

> +void cleanup_module(void)
> +{
> + unregister_timer_hook(timer_notify);
> + debugfs_remove(debugfs_pe);
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Soeren Sandmann (sandmann@daimi.au.dk)");
> +MODULE_DESCRIPTION("Kernel driver for the sysprof performance analysis tool");
> diff --git a/arch/x86/kernel/sysprof.h b/arch/x86/kernel/sysprof.h
> new file mode 100644
> index 0000000..6e16d6f
> --- /dev/null
> +++ b/arch/x86/kernel/sysprof.h
> @@ -0,0 +1,34 @@
> +/* Sysprof -- Sampling, systemwide CPU profiler
> + * Copyright 2004, Red Hat, Inc.
> + * Copyright 2004, 2005, Soeren Sandmann
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + */
> +
> +#ifndef SYSPROF_MODULE_H
> +#define SYSPROF_MODULE_H
> +
> +#define SYSPROF_MAX_ADDRESSES 512
> +
> +struct sysprof_stacktrace {
> + int pid; /* -1 if in kernel */
> + int truncated;
> + int n_addresses; /* note: this can be 1 if the process was compiled
> + * with -fomit-frame-pointer or is otherwise weird
> + */
> + unsigned long addresses[SYSPROF_MAX_ADDRESSES];
> +};

This is broken for 32-bit userspace running on a 64-bit kernel. Unless
said userspace jumps through hoops and works out that it's running under a
64-bit kernel.

There might be alignment issues for addresses[], being at offset 12.


On Wed, 20 Feb 2008 10:10:22 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>
> thanks, looks good to me - applied.

This is pretty distressing, frankly. The threshold for getting code into
Linux should be much higher than this.

I do not have the time to review everything which goes into all the git
trees. Better review, please.



\
 
 \ /
  Last update: 2008-02-23 09:31    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site