Re: [RFC PATCH 1/1] smi_detector: A System Management Interrupt detector

[Jon Masters <jonathan@jonmasters.org>]

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.