Messages in this thread Patch in this message | | | Date | Sat, 28 Nov 1998 18:12:58 -0800 (PST) | From | Stephane Belmon <> | Subject | [tiny patch] Timeout overflow in select() |
| |
Andrea, as you've seen, 2.1.130 fixes the select() timeout overflow. It's done directly in sys_select() - a simple fix (even though not exactly the one I proposed).
So I guess the original problem is solved. The only remaining point that I see is that currently, an overflow means "eternity". It might not be the best thing to do. Strictly speaking it would probably be better to do "as long as possible, but not forever" (see rationale).
But that's really a detail.
(Linus, if you're reading this, and are into details, here's what I mean:
--- linux-2.1.130/fs/select.c Sun Nov 22 10:08:50 1998 +++ linux/fs/select.c Sat Nov 28 17:35:03 1998 @@ -224,6 +224,9 @@ if ((unsigned long) sec < MAX_SELECT_SECONDS) { timeout = ROUND_UP(usec, 1000000/HZ); timeout += sec * (unsigned long) HZ; + } else { + /* long, but not forever */ + timeout = (MAX_SELECT_SECONDS-1)*HZ; } }
) Rationale: consider the portability of some code using long timeouts for select(). With 2.1.130, on some archs, the select() will wake up after (say) a day. On some others, never. The user code can't even know what's the limit (well, without testing :-) ). If it was woken up after a couple hours, it could easily recover: just check the current time - in most cases it will do that anyway. The "-1" in my patch makes _really_ sure we have no collision with "eternity" (in case someone changes the definition of MAX_SELECT_SECOND). Linus?
Andrea, here are some comments on your original patch, for the record:
On Thu, 26 Nov 1998, Andrea Arcangeli wrote:
> >+ nsec_tmp = nsec + (1000000000UL+HZ-1)/HZ - 1; > >+ if (nsec_tmp > nsec) > >+ nsec = nsec_tmp; > >"A moment of consideration" shows that (nsec_tmp > nsec) is false only > >when you have an overflow in the addition in the previous line. I'm sorry, > >but if that's what you meant, that's clear as mud :-) > Yes it' s what I mean. What does it mean "clear as mud"? What I should > change to fix the problem you are reporting?
I guess it's a matter of style. Usually, an overflow triggers something. Here it's the "fall-through" case: when you have an overflow, your code leaves nsec with some unspecified "too large to add" value, but not any specific one. You could have a commented "else" like this to make the intent clear:
nsec_tmp = nsec + (1000000000UL+HZ-1)/HZ - 1; if (nsec_tmp > nsec) nsec = nsec_tmp; else nsec = SOME_SUITABLE_VALUE; /* overflow */ Ooops, I forgot: you're into cycle-shaving ;-)
> >will return something that means "forever", which is not the same as "as > >long as you can do, but not forever". That might not be what some people > > This is true but it' s the right thing to do according to me... Really we > could do two helper function, one that return MAX_JIFFY-1; and one that > returns MAX_JIFFY;.
To me, what's questionable is the MAX_TIMEOUT_VALUE == MAX_LONG, and the name of the macro itself. It should be "ETERNITY_TIMEOUT", or something like that. It's not the upper bound on the timeout. "Eternity" is a special case. It's probably better to leave it clearly separated from the range of valid values. The same way open() returns -1 on error: -1 is not a valid FD (which is, in realistic settings, an unsigned short). Same trick for getchar(). The rest depends on the exact semantics you want for select(), as I explained above.
One more comment on your patch: Generally speaking, this style of checking overflows _after_ they happen makes me a little nervous. It works in many cases, like what you do: "a+b". If you're doing "a+b+c", testing things like the sign of the result doesn't catch all overflows; nor if you have multiplications. So a seemingly innocent change made later on could break the test: a case of bad "bug locality".
And I still like the idea of using functions to deal with timevals. Well, kinda. It makes sense not to do it as well, since the special cases are hard to handle in a generic manner.
Anyway, I'll stop commenting on this, it's a dead horse (and has been for some time). And I probably can't teach you anything!
-- Stephane Belmon <sbelmon@cse.ucsd.edu> University of California, San Diego Computer Science and Engineering Department
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |