Messages in this thread | | | Subject | Re: [RFC PATCH 1/1] smi_detector: A System Management Interrupt detector | From | Jon Masters <> | Date | Tue, 09 Jun 2009 17:50:01 -0400 |
| |
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.
| |