Re: posix timers patch comments.

[Randy.Dunlap]

On Tue, 21 Jan 2003, Jim Houston wrote:

| "Randy.Dunlap" wrote:
| >
| > Hi Jim,
| >
| > I'm reviewing both George's and your POSIX/high-res-timers patches.
| > I might not send you comments every time that I come across something,
| > and I might not cc lkml on every comment (unless you want me to).
| >
| > Anyway, here's a beginning.
| >
| > In functions get_eip(), check_expiry(), and run_posix_timers(),
| > the <regs> parameter is a void *.  Linus generally doesn't like
| > void * parameters, so they should be avoided as much as possible,
| > and I don't see any reason that <regs> in all of these can't be
| > declared as <struct pt_regs *> instead of <void *>.  Is there a
| > good reason?
| >
| > BTW, when do expect to have any updated patches?
|
| Hi Randy,
|
| It makes sense to include lkml in the discussion.  It's an
| interesting balancing act.  If you openly criticize code that you
| hope will be included in the kernel, you risk having your arguments
| used as a reason for its rejection.  If there is no visible discussion,
| the patch will be perceived as un-reviewed.

Yes, I'm aware of the tradeoffs and I expect to discuss most of it
openly, but I don't know that all nitpicking needs to be.
It could have negative effects, as you imply.

And some of my comments are just questions about "how does so-and-so
work?".  But I guess they won't be any more noise than we've had
lately.

| The code that uses get_eip() is just debug.  It may go away soon.
| I was feeling lazy and didn't want to chase down the header files needed
| to for "struct pt_regs".  I just made the change and it was easy.
| It turns out that pt_regs was already defined. I also found the
| instruction_pointer() macro so I can get rid of the get_eip() function I
| added.  The timer code has to deal with timer overruns so it always
| calculates the amount of time that a time interrupt is delayed.  I'm logging
| the EIP values which correspond to these long interrupt lockouts.

OK, thanks.

| I'm not sure when I will do the next patch.  I only have a few small
| changes in the queue.  I'm tempted to put it off a few days.

OK.

Here are some more comments and questions.  I still don't understand
all of the code, so I'm not commenting on the guts of it just yet.
This is mostly about style.  I do have some code comments, but
I don't want to mix them in here.

1.  In include/linux/posix-timers.h:

Keeping with current style, these should be all caps (for "linux"):
+#ifndef _linux_POSIX_TIMERS_H
+#define _linux_POSIX_TIMERS_H

No other files in include/linux/*.h use this "linux" convention.

Same header file:  I think that you can lose this comment.
+/* This should be in posix-timers.h - but this is easier now. */

Same header file:
Why is sys_timer_delete() the only (new) syscall that has a prototype?
+asmlinkage int sys_timer_delete(timer_t timer_id);

2.  In include/linux/sys.h:
Increases NR_syscalls by 6 more than needed?

3.  +#define NSEC_PER_SEC (1000000000L)

Would you use NSEC_PER_SEC instead of "1000000000" in the source code
in several places?  My eyes get tired of counting the zeros.

4.  From include/linux/id2ptr.h, ID_FULL is unused.

5.  From include/linux/time.h, MAX_CLOCKS is unused.

6.  (general:) Use of "return(val);" : lose the parens.  No, it's not
mentioned in Documentation/CodingStyle, but it is the preferred style
by more than 10 to 1 in my quick research.

More tomorrow...

-- 
~Randy