lkml.org 
[lkml]   [2009]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 1/1] smi_detector: A System Management Interrupt detector
On Sun, 31 May 2009 12:31:18 -0400 Jon Masters <jonathan@jonmasters.org> wrote:

> This patch introduces a new SMI (System Management Interrupt) detector module
> that can be used to detect high latencies within the system. It was originally
> written for use in the RT kernel, but has wider applications.
>

Neat-looking code.

AFACIT it can be used on any platform. Suppose that powerpcs or ia64s
also mysteriously go to lunch like PC's do - I think the code will work
for them too? In which case the "smi" name is excessively specific.
Not a big deal though.

>
> ...
>
> +static int smi_kthread_fn(void *unused)
> +{
> + int err = 0;
> + u64 interval = 0;
> +
> + while (!kthread_should_stop()) {
> +
> + mutex_lock(&smi_data.lock);
> +
> + err = stop_machine(smi_get_sample, unused, 0);
> + if (err) {
> + /* Houston, we have a problem */
> + mutex_unlock(&smi_data.lock);
> + goto err_out;
> + }
> +
> + interval = smi_data.sample_window - smi_data.sample_width;
> + do_div(interval, USEC_PER_MSEC); /* modifies interval value */
> +
> + mutex_unlock(&smi_data.lock);
> +
> + if (msleep_interruptible(interval))
> + goto out;
> +
> + }

whitespace went a bit nutty there.

> +
> + goto out;

and there.

> +err_out:
> + printk(KERN_ERR SMI_BANNER "could not call stop_machine, disabling\n");
> + enabled = 0;
> +out:
> + return err;
> +
> +}
> +
> +/**
> + * smi_start_kthread - Kick off the SMI sampling/detection kernel thread
> + *
> + * This starts a kernel thread that will sit and sample the CPU timestamp
> + * counter (TSC or similar) and look for potential SMIs.
> + */
> +static int smi_start_kthread(void)
> +{
> + smi_kthread = kthread_run(smi_kthread_fn, NULL,
> + "smi_detector");
> + if (!smi_kthread) {

You'll need an IS_ERR() test here.

> + printk(KERN_ERR SMI_BANNER "could not start sampling thread\n");
> + enabled = 0;
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * smi_stop_kthread - Inform the SMI detect/sampling kernel thread to stop
> + *
> + * This kicks the running SMI detect/sampling kernel thread and tells it to
> + * stop sampling now. Use this on unload and at system shutdown.
> + */
> +static int smi_stop_kthread(void)
> +{
> + int ret = -ENOMEM;

Unneeded initialisation.

> + ret = kthread_stop(smi_kthread);
> +
> + return ret;
> +}
> +
>
> ...
>
> +/**
> + * smi_init_stats - Setup the global state and statistics for the SMI detector
> + *
> + * We use smi_data to store various statistics and global state. We also use
> + * a global ring buffer (smi_ring_buffer) to keep raw samples of detected SMIs.
> + * This function initializes these structures and allocates the ring buffer.
> + */
> +static int smi_init_stats(void)
> +{
> + int ret = -ENOMEM;
> +
> + mutex_init(&smi_data.lock);
> + init_waitqueue_head(&smi_data.wq);
> + atomic_set(&smi_data.sample_open, 0);
> +
> + smi_ring_buffer = ring_buffer_alloc(smi_buf_size, SMI_BUF_FLAGS);
> +
> + if (!smi_ring_buffer) {
> + printk(KERN_ERR SMI_BANNER "failed to allocate ring buffer!\n");
> + WARN_ON(1);
> + goto out;
> + }

if (WARN(!smi_ring_buffer, KERN_ERR SMI_BANNER
"failed to allocate ring buffer!\n")
goto out;
neat, hm?

> +
> + __smi_reset_stats();
> + smi_data.threshold = SMI_DEFAULT_SMI_THRESHOLD; /* threshold us */
> + smi_data.sample_window = SMI_DEFAULT_SAMPLE_WINDOW; /* window us */
> + smi_data.sample_width = SMI_DEFAULT_SAMPLE_WIDTH; /* width us */
> +
> + ret = 0;
> +
> +out:
> + return ret;
> +
> +}
> +
>
> ...
>
> +static ssize_t smi_debug_count_fread(struct file *filp, char __user *ubuf,
> + size_t cnt, loff_t *ppos)
> +{
> + char buf[SMI_U64STR_SIZE];
> + u64 val = 0;
> + int len = 0;
> +
> + memset(buf, 0, sizeof(buf));
> +
> + if ((cnt < sizeof(buf)) || (*ppos))
> + return 0;
> +
> + mutex_lock(&smi_data.lock);
> + val = smi_data.count;
> + mutex_unlock(&smi_data.lock);
> +
> + len = snprintf(buf, SMI_U64STR_SIZE, "%llu\n", val);

You'll need to cast the u64 to unsigned long long to avoid warnings on
some architectures.

> + if (copy_to_user(ubuf, buf, len))
> + return -EFAULT;

I thought we had an sprintf_to_user() but I can't find it.

> + return *ppos = len;
> +}
> +
> +/**
> + * smi_debug_count_fwrite - Write function for "count" debugfs entry
> + * @filp: The active open file structure for the debugfs "file"
> + * @ubuf: The user buffer that contains the value to write
> + * @cnt: The maximum number of bytes to write to "file"
> + * @ppos: The current position in the debugfs "file"
> + */
> +static ssize_t smi_debug_count_fwrite(struct file *filp,
> + const char __user *ubuf,
> + size_t cnt,
> + loff_t *ppos)
> +{
> + char buf[SMI_U64STR_SIZE];
> + int csize = min(cnt, sizeof(buf));
> + u64 val = 0;
> + int err = 0;
> +
> + memset(buf, '\0', sizeof(buf));
> + if (copy_from_user(buf, ubuf, csize))
> + return -EFAULT;
> +
> + buf[SMI_U64STR_SIZE-1] = '\0'; /* just in case */
> + err = strict_strtoull(buf, 10, &val);
> + if (0 != err)

if (err != 0)
or

if (err)
would be more typical.

> + return -EINVAL;
> +
> + mutex_lock(&smi_data.lock);
> + smi_data.count = val;
> + mutex_unlock(&smi_data.lock);
> +
> + return csize;
> +}
> +
>
> ...
>
> +static ssize_t smi_debug_max_fwrite(struct file *filp,
> + const char __user *ubuf,
> + size_t cnt,
> + loff_t *ppos)
> +{
> + char buf[SMI_U64STR_SIZE];
> + int csize = min(cnt, sizeof(buf));
> + u64 val = 0;
> + int err = 0;
> +
> + memset(buf, '\0', sizeof(buf));
> + if (copy_from_user(buf, ubuf, csize))
> + return -EFAULT;
> +
> + buf[SMI_U64STR_SIZE-1] = '\0'; /* just in case */
> + err = strict_strtoull(buf, 10, &val);
> + if (0 != err)
> + return -EINVAL;
> +
> + mutex_lock(&smi_data.lock);
> + smi_data.max_sample = val;
> + mutex_unlock(&smi_data.lock);
> +
> + return csize;
> +}

There's a lot of code duplication amongst all these debugfs write()
handlers. Can a common helper be used?

> +
> +/**
> + * smi_debug_sample_fopen - An open function for "sample" debugfs interface
> + * @inode: The in-kernel inode representation of this debugfs "file"
> + * @filp: The active open file structure for the debugfs "file"
> + *
> + * This function handles opening the "sample" file within the SMI detector
> + * debugfs directory interface. This file is used to read raw samples from
> + * the SMI ring_buffer and allows the user to see a running SMI history.
> + */
> +static int smi_debug_sample_fopen(struct inode *inode, struct file *filp)
> +{
> + int ret = 0;
> +
> + mutex_lock(&smi_data.lock);
> + if (atomic_read(&smi_data.sample_open))
> + ret = -EBUSY;
> + else
> + atomic_inc(&smi_data.sample_open);
> + mutex_unlock(&smi_data.lock);
> +
> + return ret;
> +}

It's strange to use a lock to protect an atomic_t. A simple
atomic_add_unless() might suffice.

> +/**
> + * smi_debug_sample_fread - A read function for "sample" debugfs interface
> + * @filp: The active open file structure for the debugfs "file"
> + * @ubuf: The user buffer that will contain the samples read
> + * @cnt: The maximum bytes to read from the debugfs "file"
> + * @ppos: The current position in the debugfs "file"
> + *
> + * This function handles reading from the "sample" file within the SMI
> + * detector debugfs directory interface. This file is used to read raw samples
> + * from the SMI ring_buffer and allows the user to see a running SMI history.
> + */
> +static ssize_t smi_debug_sample_fread(struct file *filp, char __user *ubuf,
> + size_t cnt, loff_t *ppos)
> +{
> + int len = 0;
> + char buf[64];
> + struct smi_sample *sample = NULL;
> +
> + if (!enabled)
> + return 0;
> +
> + sample = kzalloc(sizeof(struct smi_sample), GFP_KERNEL);
> + if (!sample)
> + return -EFAULT;

-ENOMEM?

> + while (!smi_buffer_get_sample(sample)) {
> +
> + DEFINE_WAIT(wait);
> +
> + if (filp->f_flags & O_NONBLOCK) {
> + len = -EAGAIN;
> + goto out;
> + }
> +
> + prepare_to_wait(&smi_data.wq, &wait, TASK_INTERRUPTIBLE);
> + schedule();
> + finish_wait(&smi_data.wq, &wait);
> +
> + if (signal_pending(current)) {
> + len = -EINTR;
> + goto out;
> + }
> +
> + if (!enabled) { /* enable was toggled */
> + len = 0;
> + goto out;
> + }
> + }
> +
> + len = snprintf(buf, sizeof(buf), "%010lu.%010lu\t%llu\n",
> + sample->timestamp.tv_sec,
> + sample->timestamp.tv_nsec,
> + sample->duration);
> +
> +
> + /* handling partial reads is more trouble than it's worth */
> + if (len > cnt)
> + goto out;
> +
> + if (copy_to_user(ubuf, buf, len))
> + len = -EFAULT;
> +
> +out:
> + kfree(sample);
> + return len;
> +}
> +
>
> ...
>
> +static ssize_t smi_debug_threshold_fread(struct file *filp, char __user *ubuf,
> + size_t cnt, loff_t *ppos)
> +{
> + char buf[SMI_U64STR_SIZE];
> + u64 val = 0;
> + int len = 0;
> +
> + memset(buf, 0, sizeof(buf));
> +
> + if ((cnt < sizeof(buf)) || (*ppos))
> + return 0;
> +
> + mutex_lock(&smi_data.lock);
> + val = smi_data.threshold;
> + mutex_unlock(&smi_data.lock);
> +
> + len = snprintf(buf, SMI_U64STR_SIZE, "%llu\n", val);

printk warnings here.

> + if (copy_to_user(ubuf, buf, len))
> + return -EFAULT;
> + return *ppos = len;
> +}

More code duplication?

>
> ...
>
> +static ssize_t smi_debug_width_fread(struct file *filp, char __user *ubuf,
> + size_t cnt, loff_t *ppos)
> +{
> + char buf[SMI_U64STR_SIZE];
> + u64 val = 0;
> + int len = 0;
> +
> + memset(buf, 0, sizeof(buf));
> +
> + if ((cnt < sizeof(buf)) || (*ppos))
> + return 0;
> +
> + mutex_lock(&smi_data.lock);
> + val = smi_data.sample_width;
> + mutex_unlock(&smi_data.lock);
> +
> + len = snprintf(buf, SMI_U64STR_SIZE, "%llu\n", val);
> +
> + if (copy_to_user(ubuf, buf, len))
> + return -EFAULT;
> + return *ppos = len;
> +}

dittoes...

> +/**
> + * smi_debug_width_fwrite - Write function for "width" debugfs entry
> + * @filp: The active open file structure for the debugfs "file"
> + * @ubuf: The user buffer that contains the value to write
> + * @cnt: The maximum number of bytes to write to "file"
> + * @ppos: The current position in the debugfs "file"
> + */
> +static ssize_t smi_debug_width_fwrite(struct file *filp,
> + const char __user *ubuf,
> + size_t cnt,
> + loff_t *ppos)
> +{
> + char buf[SMI_U64STR_SIZE];
> + int csize = min(cnt, sizeof(buf));
> + u64 val = 0;
> + int err = 0;
> +
> + memset(buf, '\0', sizeof(buf));
> + if (copy_from_user(buf, ubuf, csize))
> + return -EFAULT;
> +
> + buf[SMI_U64STR_SIZE-1] = '\0'; /* just in case */
> + err = strict_strtoull(buf, 10, &val);
> + if (0 != err)
> + return -EINVAL;
> +
> + mutex_lock(&smi_data.lock);
> + if (val < smi_data.sample_window)
> + smi_data.sample_width = val;
> + else {
> + mutex_unlock(&smi_data.lock);
> + return -EINVAL;
> + }
> + mutex_unlock(&smi_data.lock);
> +
> + if (enabled)
> + wake_up_process(smi_kthread);
> +
> + return csize;
> +}

More duplication.

>
> ...
>



\
 
 \ /
  Last update: 2009-06-02 06:01    [from the cache]
©2003-2011 Jasper Spaans