lkml.org 
[lkml]   [2009]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 1/1] smi_detector: A System Management Interrupt detector
From
Date
On Mon, 2009-06-01 at 20:57 -0700, Andrew Morton wrote:
> 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.

Thanks. Finally gotten around to cleaning it up, and renamed it. I think
I should have hwlat_detector out in a few minutes.

> AFACIT it can be used on any platform.

Agreed. I've added a description that is generic in terms of system
hardware latencies - nothing specific to SMIs except in a comment.

> > + smi_kthread = kthread_run(smi_kthread_fn, NULL,
> > + "smi_detector");
> > + if (!smi_kthread) {
>
> You'll need an IS_ERR() test here.

Thanks. I realized later that I did, because there's no reason that the
value returned couldn't, in theory, change someday (recent zero page
discussions notwithstanding).

> > + if (0 != err)
>
> if (err != 0)
>
> or
>
> if (err)
>
> would be more typical.

The former runs the risk of assignment, whereas <value> != <variable>
will generate a compiler error if it goes wrong, so I trained myself to
always do that. The desired value is zero, so I prefer to show that in
the test, but I have changed it following your advice anyway - it's like
how I have to force myself not to use '{' '}' on single line
if-statements despite generally doing so, again for safety :)

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

I originally used the generic debugfs _u|s<blah> functions to just
read/write from the variables directly but then needed some side effects
- but in any case, the generic functions don't offer any locking AFAIK.
I'm adding a little helper function instead.

> > +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.

You're right. I was just being pedantic to use the lock every time. I'll
take that out and wrap it with an _unless, I think.

Jon.




\
 
 \ /
  Last update: 2009-06-09 23:53    [W:0.455 / U:0.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site